rubberduck-vba / Rubberduck

Every programmer needs a rubberduck. COM add-in for the VBA & VB6 IDE (VBE).
https://rubberduckvba.com
GNU General Public License v3.0
1.92k stars 302 forks source link

UiDispatcher shutdown/restart problem causing Addin unload/reload to fail #3645

Open WaynePhillipsEA opened 6 years ago

WaynePhillipsEA commented 6 years ago

As per title, there is a problem in the UiDispatcher that causes the weird crash when re-loading the RD addin from the VBE after unloading.

To demonstrate, we can remove the InvokeShutdown method call, and the unload/reload works without crashing (but with some RCW disconnection errors in the log which will need looking at)

bclothier commented 6 years ago

I analyzed this a bit using the branch for the PR #4456 I can observe that:

1) the numbers of Add entries and Removed entries from the final trace of COM Safe do not always balance. At minimum, 3 will not have their Removed entries; the IVBE, the IAddIn, and the IHostApplication. 2) We will get RaceOnRCWCleanup warnings in process of disposing the COM safe. 3) Even if we exempt those objects from disposal, we still get the warning on say, a ICodePane. It doesn't even have to be an active code pane. 4) Forcing the disposal in the UI thread does not help.

I'm inclined to suspect that to correctly dispose all the items, we need to ensure that there are no other threads running, and that they all have released their use of COM before we may proceed to dispose of COM Safe.

I have not seen any RCW disconnection errors, however.

MDoerner commented 6 years ago

For context, please note that the call to InvokeShutdown is currently commented out in the UiDispatcher. This does not seem to be ideal but symptomatically fixes the original issue.

MDoerner commented 6 years ago

Regarding, the RaceOnRCWCleanup, this cannot be caused by the IVBE or IAddIn because we still hold a reference to them when we dispose the COM safe in Extension.ShutdownAddIn. That these are still in the COM safe is to be expected because we use it call dispose. Excempting them from addition to the COM safe means that we will not dispose them.

bclothier commented 6 years ago

FWIW, I just checked the InvokeShutdown. First, because the UiDispatcher has been since refactored, it no longer works as-is. That said, I made a static instance and attempted InvokeShutdown again to see if anything had changed since. It has not.

I'm not as confident that we will ever have InvokeShutdown re-implemented.

bclothier commented 5 years ago

I still cannot reproduce any RCW disconnection errors as originally reported. As noted in the last comment, I do not see any point in restoring the InvokeShutdown. The only reason we even call it originally is because we do access the CurrentDispatcher in 3 places between Extension.cs and Settings.cs, and the documentation says that we are supposed to shut it down when we're done with it. However, careful reading suggests that it would be only necessary for background threads. Since the code called on Settings.cs and Extension.cs would most likely be UI thread, that most likely won't apply.

The only uncertainty is whether the Settings.cs could be run on a background thread, which may necessitate the cleanup. We can guard against this with UiDispatcher.Inovke() but unfortunately the LoadLanguage method is static which complicates things.

If others think that is a non-concern, we should close this issue.