Prevent LegacyGlobalOptionService from GC rooting PreviewWorkspace

[sharwell]

Fixes AB#1756080

[jasonmalinowski]

This seems to be because we're not disposing the PreviewWorkspace which would have done the unsubscribe. Although this may be a good defense in depth, can we fix that too?

[jasonmalinowski]

Or alternatively: if we're making this weak, can we just get rid of the Unregister method entirely?

[sharwell]

This seems to be because we're not disposing the PreviewWorkspace which would have done the unsubscribe. Although this may be a good defense in depth, can we fix that too?

➡️ Ideally yes. In practice, the cancellation paths involved in ensuring that happens are extra non-trivial. I have no objection to trying to fix that, but it's lower priority and if we decide to service 17.5 with a fix for this issue we aren't going to want to bring both.

Or alternatively: if we're making this weak, can we just get rid of the Unregister method entirely?

➡️ It probably wouldn't end up building up too large a collection of workspaces in the list to notify, but there are other concerns. Currently, notifications sent to a forgotten workspace will not be in the disposed state, but if we omit Unregister they might get callbacks to set the current solution after they are disposed (this may be possible today, but it would be more likely after removing Unregister). It would also be a code smell where someone would come along eventually and want to add it back just because it seems wrong.

[jasonmalinowski]

@sharwell: my concern isn't the unregister list specifically, but there's a lot of other stuff also happening in Workspace.Dispose() that we're also skipping, and I have no idea if any of that is safe to skip or whether that's also causing additional rooting. Basically, we're violating the contract of the Workspace, we got caught, and this isn't fixing that.

If we're going to take this approach due to us servicing this for 17.5, then I'd request a few additional things:

1. A unit test verifying this actually works and there isn't something else rooting it. Given we already have helpers for writing tests like this I don't believe this should be difficult to write. The chance of this being regressed by accident is very high.
2. A comment in the Dispose() method explaining that people really shouldn't put any new event unsubscriptions in there.
3. A tracking bug to either completely empty the Dispose() method or fix the fact PreviewWorkspace is not being disposed -- either is fine with me.

I actually don't really like the idea that Workspace is Disposable so I don't mind the direction this PR takes, but this PR as written leaves us in a place that's likely to have regressions without a test, and we should fix that.

[sharwell]

A comment in the Dispose() method explaining that people really shouldn't put any new event unsubscriptions in there.

I'm not sure this matters in practice. If someone is thinking about adding an unsubscribe to that method, it seems unlikely that a comment would have any impact on the outcome. We also have numerous other cases where we can accidentally keep memory alive due to event subscriptions.

[jasonmalinowski]

If someone is thinking about adding an unsubscribe to that method, it seems unlikely that a comment would have any impact on the outcome.

Even if the tests would catch them I can still imagine people asking "so are workspaces supposed to be disposed or not?" and there's not a clear answer either way.

[sharwell]

I'm going to get this merged since it fixes the one known issue. Hopefully in the future we can fix other cases we find where solutions are not disposed, but at least we won't be leaking memory from them.