Managed memory leak on disconnect

[MDoerner]

RD seems to have a managed memory leak on disconnect.

This can be observed as follows:

1) Comment out the shutdown invokation in UIDispatcher.Shutdown(). (See #3645 for why you should do this.) 2) Add GC.Collect(); GC.WaitForPendingFinalizers() to the finally block of ShutdownAddIn in Extension. (This is nessesary since no finalizers will ever run until shutdown, provided you have enough memory.) 3) Open RD in the debugger. 4) Unload RD via the add-ins dialog. 5) Reload RD via the add-in dialog. 6) Close the host application.

In the debug output, you should be able to observe the following:

• There are messages that the finalizers for the DockableToolwindowPresenters have run.
• All but the one for SC get finalized right on disconnect (unload and shutdown).
• Both SC presenters get finalized only on the final shutdown, and much later than anything else.
• On reload, there are exceptions for the three commands OpenProjectPropertiesCommand, ImportCommand and ExportAllCommand stating that an RCW has already been disconnected from its target.

I investigated a bit further by adding addig finalizers for logging to some suspects that could hold the SC presenter alive. It seems that the presenter is kept alive by two commands, the CommitCommand and the UndoCommand, which are both kept alive by the CodeExplorerViewModel, I could find out yet what is keeping this alive.

That the CE is kept alive means that everything it references, beit directly or indirectly, is kept alive as well; that is half of RD.

The exceptions on the three commands seem to be connected. All three are referenced by the CE view model and their EvaluateCanExecute methods contain the accesses to the stale RCWs. Somehow, they still seem to react to some change notifications.

[bclothier]

Purely by happenstance, I noticed there were actually two code explorer windows:

The useless code explorer window is initially invisible and must be programmatically made visible via the Immediate windows. Note that it's possibly easy to miss because a For Each will not iterate all VBE.Window in the VBE.Windows collection. A For must be used instead.

for i = 0 to application.VBE.Windows.Count -1: ?application.VBE.Windows(i).caption:next
Code Explorer
Code Explorer
Source Control
Code Metrics
Todo Explorer
Test Explorer
Code Inspections
Project - ItImpactTemplate

Properties
Object Browser
Watches
Locals

You can then make the useless Code Explorer visible:

?Application.VBE.Windows(0).Visible = True

And as a sanity check that they aren't same somehow:

?application.VBE.Windows(0) is application.VBE.Windows(1)
False

[bclothier]

Addendum - that may be useful to know that the first useless CE window is not a tool window:

?application.VBE.Windows(0).Type
 11 '(vbext_wt_LinkedWindowFrame)
?application.VBE.Windows(1).Type
 15  '(vbext_wt_ToolWindow)

[bclothier]

Update from the chat - the report about CE being duplicated appears to be a red herring - it is simply an artifact of how VBE behaves when docking/undocking windows. The act of undocking a window will create a LinkedWindowFrame at index 0 on Windows collection which is normally a one-based collection. This frame will inherit the same caption as the undocked window. Thus we should disregard the above for the bug report.

[bclothier]

I no longer can reproduce this issue. I do not get any RCW errors when loading or unloading or shutting down, even with the added GC.Collect() & GC.WaitForPendingFinalizers(). In fact, I get a clean exit.

Since the issue's original reporting we had the following changes:

• We no longer have SC toolwindows
• Commands that touches COM now derives from ComCommand, which helps makes the teardown of such commands deterministic.

Of note were numbers of C++ exceptions thrown, as I had both the managed and native debugging. However, Rubberduck's trace do not report any exceptions, so it is likely that the exceptions comes from other components outside of Rubberduck, so I'm inclined to discount those exceptions, considering that we do get a clean exit nonetheless.

Debug Output.txt

If others agree, we should close this issue as solved.

[MDoerner]

This issue is neither about the RCW nor about the SC commands. It is about the CE being kept alive by something.

To rule out this still is an issue we probably have to do something like memory profiling.

[bclothier]

I ran dotMemory and can confirm that the memory does not decrease when unloading the addins. Every time I reload the addin, the usage increase but only by a small amount; it is hard to tell if it's just incidental or an actually small memory leak. However, large swath of memory definitely isn't released and they are mainly unmanaged, likely held because of other managed objects that uses them. There was roughly ~30 MB of managed memory which persist even with multiple forced GCs.

[bclothier]

I'm thinking that the whole problem stems from how WPF should be shutdown. Even when calling InvokeShutdown, it is apparent that the WPF resources actually don't go away.

I found this article which may help explain and some suggestions:

Another thing to keep in mind is that the Application singleton will remain alive for the remainder of the process lifetime unless you take steps to release it. If your scenario only calls for WPF to be used for a finite period of time during application execution, you should call the Application’s Shutdown() method when you no longer need it. In a non-hosted WPF application, this would actually shut down the application by shutting down the dispatcher. In our hosted scenario, it will simply release the rooted reference to the singleton. After calling Shutdown(), the static Application.Current property will once again return null. At this point, the object should be available for garbage collection.

I will test this tomorrow.

[bclothier]

So I experimented with manually creating System.Windows.Application and explicitly shutting it down. The code seems to work but the memory does not deallocate once the add-in is unloaded.

But after the explicit shutdown, I find that I get an error when I re-load the add-in with an InvalidOperationException saying that an application is already loaded for this AppDomain. The odd thing is that Application.Current is null, and therefore shouldn't have thrown that exception. Note that the Application.Current isn't made null until after the shutdown of the add-in has completed. At the startup, the Appplication.Current is indeed null, but we get the exception nonetheless.

The fact the Shutdown did not work explains why we see strange behavior when we originally had the InvokeShtudown on the dispatcher -- it is not correctly shut down, even though there are no indication it didn't. It seems necessary to unload the AppDomain to avoid this type of memory leak.

Relevant Docs: Application constructor Shutdown method

[MDoerner]

So, this analysis brings us back to a need for COM shims, I guess. That is, unless things will change with Core 5.

[bclothier]

The problem, however, is that in .NET Core, there's no concept of AppDomain. Therefore, I'm not clear what a shim would do in .NET Core exactly. It'd need to be able to completely unload the .NET runtime, and I assume it's somehow different (if at all possible) in .NET Core. It would be very different from the shims that we'd have used with .NET Framework.