First step towards clean exit (#3277)

MDoerner · retailcoder · commit 8f05c0106f0b · 2017-08-21T22:29:16.000-04:00
This PR is a first step towards a clean exist; it fixes one reason of the crash on exit: we have not been unregistering the Click events from the CommandBarButtons, neither in the command bar nor in the menus. This leads to a fatal error on shutdown. This PR changes the CommandBarControls to be added permanently and to be removed explicitly on shutdown, removing the Click handlers as well. Moreover, as a safety measure, the GC gets invoked at the end of the shutdown. There are more problematic sections I have identified and I am looking into. However, although there are still plenty of COM access violations on shutdown, I get a clean exit in Excel in production now, but not when debugging. ref. #3213 * Make menus and the command bar permanent and clean them up on shutdown. * Fix event handler registration in CommandBarButton. * Fixed RemoveChildren on the parent menus and the command bar. * Add garbage collection on shutdown. * Rewired deleting the command bar and menus. * Fixed three tests with illegal code as input.
diff --git a/RetailCoder.VBE/AppMenu.cs b/RetailCoder.VBE/AppMenu.cs
@@ -70,12 +70,15 @@ public void Dispose()
             _parser.State.StateChanged -= OnParserStateChanged;
             _selectionService.SelectedDeclarationChanged -= OnSelectedDeclarationChange;
 
-            // note: doing this wrecks the teardown process. counter-intuitive? sure. but hey it works.
-            //foreach (var menu in _menus.Where(menu => menu.Item != null))
-            //{
-            //    menu.RemoveChildren();
-            //    menu.Item.Delete();
-            //}
+            RemoveMenus();
+        }
+
+        private void RemoveMenus()
+        {
+            foreach (var menu in _menus.Where(menu => menu.Item != null))
+            {
+                menu.RemoveMenu();
+            }
         }
     }
 }
diff --git a/RetailCoder.VBE/Extension.cs b/RetailCoder.VBE/Extension.cs
@@ -243,6 +243,16 @@ private void ShutdownAddIn()
                 _kernel = null;
             }
 
+            Debug.WriteLine("Extension: Initiating garbage collection.");
+
+            GC.Collect();
+
+            Debug.WriteLine("Extension: Initiated garbage collection.");
+
+            GC.WaitForPendingFinalizers();
+
+            Debug.WriteLine("Extension: Finalizers have run.");
+
             _isInitialized = false;
         }
 
diff --git a/RetailCoder.VBE/UI/Command/MenuItems/CommandBars/AppCommandBarBase.cs b/RetailCoder.VBE/UI/Command/MenuItems/CommandBars/AppCommandBarBase.cs
@@ -109,41 +109,39 @@ public void EvaluateCanExecute(RubberduckParserState state)
 
         public ICommandBars Parent { get; set; }
         public ICommandBar Item { get; private set; }
-        public void RemoveChildren()
+
+        public void RemoveCommandBar()
+        {
+            Logger.Debug("Removing commandbar.");
+            RemoveChildren();
+            Item?.Delete();
+            Item = null;
+            Parent = null;
+        }
+
+        private void RemoveChildren()
         {
-            Logger.Debug("Removing child controls from commandbar.");
             if (Parent == null || Parent.IsWrappingNullReference)
             {
                 return;
             }
 
             try
             {
-                foreach (var child in _items.Values.Select(item => item as CommandBarButton).Where(child => child != null))
+                foreach (var button in _items.Values.Select(item => item as ICommandBarButton).Where(child => child != null))
                 {
-                    var button = Parent.FindControl(child.Id);
-                    if (button is ICommandBarButton && !button.IsWrappingNullReference)
+                    if (!button.IsWrappingNullReference)
                     {
-                        (button as ICommandBarButton).Click -= child_Click;
+                        button.Click -= child_Click;
                     }
                     button.Delete();
                 }
             }
             catch (COMException exception)
             {
-                /*
-Application: EXCEL.EXE
-Framework Version: v4.0.30319
-Description: The process was terminated due to an unhandled exception.
-Exception Info: System.Runtime.InteropServices.COMException
-   at Microsoft.Office.Core.CommandBarControl.get_Parent()
-   at Rubberduck.VBEditor.SafeComWrappers.Office.Core.CommandBarControl.get_Parent()
-   at Rubberduck.VBEditor.SafeComWrappers.Office.Core.CommandBarButton.remove_Click(System.EventHandler`1<Rubberduck.VBEditor.SafeComWrappers.Office.Core.CommandBarButtonClickEventArgs>)
-   at Rubberduck.UI.Command.MenuItems.CommandBars.AppCommandBarBase.RemoveChildren()
-   at Rubberduck.UI.Command.MenuItems.CommandBars.RubberduckCommandBar.Dispose()
-                */
                 Logger.Error(exception, "Error removing child controls from commandbar.");
             }
+            _items.Clear();
         }
 
         // note: HAAAAACK!!!
diff --git a/RetailCoder.VBE/UI/Command/MenuItems/CommandBars/IAppCommandBar.cs b/RetailCoder.VBE/UI/Command/MenuItems/CommandBars/IAppCommandBar.cs
@@ -6,6 +6,6 @@ public interface IAppCommandBar : IAppMenu
     {
         ICommandBars Parent { get; set; }
         ICommandBar Item { get; }
-        void RemoveChildren();
+        void RemoveCommandBar();
     }
 }
diff --git a/RetailCoder.VBE/UI/Command/MenuItems/CommandBars/RubberduckCommandBar.cs b/RetailCoder.VBE/UI/Command/MenuItems/CommandBars/RubberduckCommandBar.cs
@@ -138,10 +138,7 @@ public void Dispose()
             _parser.State.StateChanged -= OnParserStateChanged;
             _parser.State.StatusMessageUpdate -= OnParserStatusMessageUpdate;
 
-            //note: doing this wrecks the teardown process. counter-intuitive? sure. but hey it works.
-            //RemoveChildren();
-            //Item.Delete();
-            //Item.Release(true);
+            RemoveCommandBar();
         }
     }
 
diff --git a/RetailCoder.VBE/UI/Command/MenuItems/ParentMenus/IParentMenuItem.cs b/RetailCoder.VBE/UI/Command/MenuItems/ParentMenus/IParentMenuItem.cs
@@ -6,6 +6,6 @@ public interface IParentMenuItem : IMenuItem, IAppMenu
     {
         ICommandBarControls Parent { get; set; }
         ICommandBarPopup Item { get; }
-        void RemoveChildren();
+        void RemoveMenu();
     }
 }
diff --git a/RetailCoder.VBE/UI/Command/MenuItems/ParentMenus/ParentMenuItemBase.cs b/RetailCoder.VBE/UI/Command/MenuItems/ParentMenus/ParentMenuItemBase.cs
@@ -1,5 +1,6 @@
 ﻿using System;
 using System.Collections.Generic;
+using System.Diagnostics;
 using System.Globalization;
 using System.Linq;
 using Rubberduck.Parsing.VBA;
@@ -88,19 +89,24 @@ public void Initialize()
             EvaluateCanExecute(null);
         }
 
-        public void RemoveChildren()
+        public void RemoveMenu()
+        {
+            Logger.Debug($"Removing menu {_key}.");
+            RemoveChildren();
+            Item?.Delete();
+            Item = null;
+        }
+
+        private void RemoveChildren()
         {
             foreach (var child in _items.Keys.Select(item => item as IParentMenuItem).Where(child => child != null))
             {
-                child.RemoveChildren();
-                //var item = _items[child];
-                //Debug.Assert(item is CommandBarPopup);
-                //(item as CommandBarPopup).Delete();
+                child.RemoveMenu();
             }
-            foreach (var child in _items.Values.Select(item => item as CommandBarButton).Where(child => child != null))
+            foreach (var child in _items.Values.Select(item => item as ICommandBarButton).Where(child => child != null))
             {
                 child.Click -= child_Click;
-                //child.Delete();
+                child.Delete();
             }
         }
 
diff --git a/Rubberduck.Parsing/VBA/ParseCoordinator.cs b/Rubberduck.Parsing/VBA/ParseCoordinator.cs
@@ -320,7 +320,7 @@ private void ParseAll(object requestor, CancellationToken token)
                 if (watch != null && watch.IsRunning) watch.Stop();
                 if (lockTaken) Monitor.Exit(_parsingRunSyncObject);
             }
-            if (watch != null) Logger.Debug("Parsing run finished after {0}s. (thread {1}).", watch.Elapsed.Seconds, Thread.CurrentThread.ManagedThreadId);
+            if (watch != null) Logger.Debug("Parsing run finished after {0}s. (thread {1}).", watch.Elapsed.TotalSeconds, Thread.CurrentThread.ManagedThreadId);
         }
 
         private void ParseAllInternal(object requestor, CancellationToken token)
diff --git a/Rubberduck.VBEEditor/SafeComWrappers/Office.Core/CommandBarButton.cs b/Rubberduck.VBEEditor/SafeComWrappers/Office.Core/CommandBarButton.cs
@@ -31,21 +31,27 @@ public event EventHandler<CommandBarButtonClickEventArgs> Click
         {
             add
             {
-                ((Microsoft.Office.Core.CommandBarButton)Target).Click += Target_Click;
+                if (_clickHandler == null)
+                {
+                    ((Microsoft.Office.Core.CommandBarButton)Target).Click += Target_Click;
+                }
                 _clickHandler += value;
                 System.Diagnostics.Debug.WriteLine("Added handler for: {0} '{1}' (tag: {2}, hashcode:{3})", Parent.Name, Target.Caption, Tag, Target.GetHashCode());
             }
             remove
             {
+                _clickHandler -= value;
                 try
                 {
-                    ((Microsoft.Office.Core.CommandBarButton)Target).Click -= Target_Click;
+                    if (_clickHandler == null)
+                    {
+                        ((Microsoft.Office.Core.CommandBarButton)Target).Click -= Target_Click;
+                    }
                 }
                 catch
                 {
                     // he's gone, dave.
                 }
-                _clickHandler -= value;
                 System.Diagnostics.Debug.WriteLine("Removed handler for: {0} '{1}' (tag: {2}, hashcode:{3})", Parent.GetType().Name, Target.Caption, Tag, Target.GetHashCode());
             }
         }
diff --git a/Rubberduck.VBEEditor/SafeComWrappers/Office.Core/CommandBarControl.cs b/Rubberduck.VBEEditor/SafeComWrappers/Office.Core/CommandBarControl.cs
@@ -6,6 +6,8 @@ namespace Rubberduck.VBEditor.SafeComWrappers.Office.Core
 {
     public class CommandBarControl : SafeComWrapper<Microsoft.Office.Core.CommandBarControl>, ICommandBarControl
     {
+        public const bool AddCommandBarControlsTemporarily = false;
+
         public CommandBarControl(Microsoft.Office.Core.CommandBarControl target) 
             : base(target)
         {
@@ -125,7 +127,7 @@ public bool IsPriorityDropped
 
         public void Delete()
         {
-            if (!IsWrappingNullReference) Target.Delete(true);
+            if (!IsWrappingNullReference) Target.Delete(AddCommandBarControlsTemporarily);
         }
 
         public void Execute()
diff --git a/Rubberduck.VBEEditor/SafeComWrappers/Office.Core/CommandBarControls.cs b/Rubberduck.VBEEditor/SafeComWrappers/Office.Core/CommandBarControls.cs
@@ -30,12 +30,12 @@ public ICommandBarControl this[object index]
 
         public ICommandBarControl Add(ControlType type)
         {
-            return new CommandBarControl(IsWrappingNullReference ? null : Target.Add(type, Temporary:true));
+            return new CommandBarControl(IsWrappingNullReference ? null : Target.Add(type, Temporary: CommandBarControl.AddCommandBarControlsTemporarily));
         }
 
         public ICommandBarControl Add(ControlType type, int before)
         {
-            return new CommandBarControl(IsWrappingNullReference ? null : Target.Add(type, Before: before, Temporary: true));
+            return new CommandBarControl(IsWrappingNullReference ? null : Target.Add(type, Before: before, Temporary: CommandBarControl.AddCommandBarControlsTemporarily));
         }
 
         IEnumerator<ICommandBarControl> IEnumerable<ICommandBarControl>.GetEnumerator()
diff --git a/Rubberduck.VBEEditor/WindowsApi/SubclassingWindow.cs b/Rubberduck.VBEEditor/WindowsApi/SubclassingWindow.cs
@@ -39,10 +39,18 @@ protected SubclassingWindow(IntPtr subclassId, IntPtr hWnd)
             AssignHandle();
         }
 
+        private bool _disposed = false;
         public void Dispose()
         {
+            if (_disposed)
+            {
+                return;
+            }
+
             ReleaseHandle();
             _thisHandle.Free();
+
+            _disposed = true;
         }
 
         private void AssignHandle()
diff --git a/RubberduckTests/Inspections/ObjectVariableNotSetInspectionTests.cs b/RubberduckTests/Inspections/ObjectVariableNotSetInspectionTests.cs
@@ -36,7 +36,7 @@ public void ObjectVariableNotSet_GivenPropertyLet_ReturnsNoResult()
             var expectResultCount = 0;
             var input =
                 @"
-Public Property Let Foo() As String
+Public Property Let Foo(rhs As String)
 End Property
 
 Private Sub DoSomething()
@@ -48,12 +48,12 @@ End Sub
 
         [TestMethod]
         [TestCategory("Inspections")]
-        public void ObjectVariableNotSet_GivenPropertySet_ReturnsResult()
+        public void ObjectVariableNotSet_GivenPropertySet_WithoutSet_ReturnsResult()
         {
             var expectResultCount = 1;
             var input =
                 @"
-Public Property Set Foo() As Object
+Public Property Set Foo(rhs As Object)
 End Property
 
 Private Sub DoSomething()
@@ -65,12 +65,12 @@ End Sub
 
         [TestMethod]
         [TestCategory("Inspections")]
-        public void ObjectVariableNotSet_GivenPropertySet_ReturnsNoResult()
+        public void ObjectVariableNotSet_GivenPropertySet_WithSet_ReturnsNoResult()
         {
             var expectResultCount = 0;
             var input =
                 @"
-Public Property Set Foo() As Object
+Public Property Set Foo(rhs As Object)
 End Property
 
 Private Sub DoSomething()

Original file line number	Diff line number	Diff line change
`@@ -109,41 +109,39 @@ public void EvaluateCanExecute(RubberduckParserState state)`
`109`	`109`
`110`	`110`	`public ICommandBars Parent { get; set; }`
`111`	`111`	`public ICommandBar Item { get; private set; }`
`112`		`- public void RemoveChildren()`
	`112`	`+`
	`113`	`+ public void RemoveCommandBar()`
	`114`	`+ {`
	`115`	`+ Logger.Debug("Removing commandbar.");`
	`116`	`+ RemoveChildren();`
	`117`	`+ Item?.Delete();`
	`118`	`+ Item = null;`
	`119`	`+ Parent = null;`
	`120`	`+ }`
	`121`	`+`
	`122`	`+ private void RemoveChildren()`
`113`	`123`	`{`
`114`		`- Logger.Debug("Removing child controls from commandbar.");`
`115`	`124`	`if (Parent == null \|\| Parent.IsWrappingNullReference)`
`116`	`125`	`{`
`117`	`126`	`return;`
`118`	`127`	`}`
`119`	`128`
`120`	`129`	`try`
`121`	`130`	`{`
`122`		`- foreach (var child in _items.Values.Select(item => item as CommandBarButton).Where(child => child != null))`
	`131`	`+ foreach (var button in _items.Values.Select(item => item as ICommandBarButton).Where(child => child != null))`
`123`	`132`	`{`
`124`		`- var button = Parent.FindControl(child.Id);`
`125`		`- if (button is ICommandBarButton && !button.IsWrappingNullReference)`
	`133`	`+ if (!button.IsWrappingNullReference)`
`126`	`134`	`{`
`127`		`- (button as ICommandBarButton).Click -= child_Click;`
	`135`	`+ button.Click -= child_Click;`
`128`	`136`	`}`
`129`	`137`	`button.Delete();`
`130`	`138`	`}`
`131`	`139`	`}`
`132`	`140`	`catch (COMException exception)`
`133`	`141`	`{`
`134`		`- /*`
`135`		`-Application: EXCEL.EXE`
`136`		`-Framework Version: v4.0.30319`
`137`		`-Description: The process was terminated due to an unhandled exception.`
`138`		`-Exception Info: System.Runtime.InteropServices.COMException`
`139`		`- at Microsoft.Office.Core.CommandBarControl.get_Parent()`
`140`		`- at Rubberduck.VBEditor.SafeComWrappers.Office.Core.CommandBarControl.get_Parent()`
`141`		- at Rubberduck.VBEditor.SafeComWrappers.Office.Core.CommandBarButton.remove_Click(System.EventHandler`1<Rubberduck.VBEditor.SafeComWrappers.Office.Core.CommandBarButtonClickEventArgs>)
`142`		`- at Rubberduck.UI.Command.MenuItems.CommandBars.AppCommandBarBase.RemoveChildren()`
`143`		`- at Rubberduck.UI.Command.MenuItems.CommandBars.RubberduckCommandBar.Dispose()`
`144`		`- */`
`145`	`142`	`Logger.Error(exception, "Error removing child controls from commandbar.");`
`146`	`143`	`}`
	`144`	`+ _items.Clear();`
`147`	`145`	`}`
`148`	`146`
`149`	`147`	`// note: HAAAAACK!!!`
Original file line number	Diff line number	Diff line change
`@@ -6,6 +6,6 @@ public interface IAppCommandBar : IAppMenu`
`6`	`6`	`{`
`7`	`7`	`ICommandBars Parent { get; set; }`
`8`	`8`	`ICommandBar Item { get; }`
`9`		`- void RemoveChildren();`
	`9`	`+ void RemoveCommandBar();`
`10`	`10`	`}`
`11`	`11`	`}`
Original file line number	Diff line number	Diff line change
`@@ -138,10 +138,7 @@ public void Dispose()`
`138`	`138`	`_parser.State.StateChanged -= OnParserStateChanged;`
`139`	`139`	`_parser.State.StatusMessageUpdate -= OnParserStatusMessageUpdate;`
`140`	`140`
`141`		`- //note: doing this wrecks the teardown process. counter-intuitive? sure. but hey it works.`
`142`		`- //RemoveChildren();`
`143`		`- //Item.Delete();`
`144`		`- //Item.Release(true);`
	`141`	`+ RemoveCommandBar();`
`145`	`142`	`}`
`146`	`143`	`}`
`147`	`144`
Original file line number	Diff line number	Diff line change
`@@ -6,6 +6,6 @@ public interface IParentMenuItem : IMenuItem, IAppMenu`
`6`	`6`	`{`
`7`	`7`	`ICommandBarControls Parent { get; set; }`
`8`	`8`	`ICommandBarPopup Item { get; }`
`9`		`- void RemoveChildren();`
	`9`	`+ void RemoveMenu();`
`10`	`10`	`}`
`11`	`11`	`}`
Original file line number	Diff line number	Diff line change
`@@ -1,5 +1,6 @@`
`1`	`1`	`using System;`
`2`	`2`	`using System.Collections.Generic;`
	`3`	`+using System.Diagnostics;`
`3`	`4`	`using System.Globalization;`
`4`	`5`	`using System.Linq;`
`5`	`6`	`using Rubberduck.Parsing.VBA;`
`@@ -88,19 +89,24 @@ public void Initialize()`
`88`	`89`	`EvaluateCanExecute(null);`
`89`	`90`	`}`
`90`	`91`
`91`		`- public void RemoveChildren()`
	`92`	`+ public void RemoveMenu()`
	`93`	`+ {`
	`94`	`+ Logger.Debug($"Removing menu {_key}.");`
	`95`	`+ RemoveChildren();`
	`96`	`+ Item?.Delete();`
	`97`	`+ Item = null;`
	`98`	`+ }`
	`99`	`+`
	`100`	`+ private void RemoveChildren()`
`92`	`101`	`{`
`93`	`102`	`foreach (var child in _items.Keys.Select(item => item as IParentMenuItem).Where(child => child != null))`
`94`	`103`	`{`
`95`		`- child.RemoveChildren();`
`96`		`- //var item = _items[child];`
`97`		`- //Debug.Assert(item is CommandBarPopup);`
`98`		`- //(item as CommandBarPopup).Delete();`
	`104`	`+ child.RemoveMenu();`
`99`	`105`	`}`
`100`		`- foreach (var child in _items.Values.Select(item => item as CommandBarButton).Where(child => child != null))`
	`106`	`+ foreach (var child in _items.Values.Select(item => item as ICommandBarButton).Where(child => child != null))`
`101`	`107`	`{`
`102`	`108`	`child.Click -= child_Click;`
`103`		`- //child.Delete();`
	`109`	`+ child.Delete();`
`104`	`110`	`}`
`105`	`111`	`}`
`106`	`112`
Original file line number	Diff line number	Diff line change
`@@ -320,7 +320,7 @@ private void ParseAll(object requestor, CancellationToken token)`
`320`	`320`	`if (watch != null && watch.IsRunning) watch.Stop();`
`321`	`321`	`if (lockTaken) Monitor.Exit(_parsingRunSyncObject);`
`322`	`322`	`}`
`323`		`- if (watch != null) Logger.Debug("Parsing run finished after {0}s. (thread {1}).", watch.Elapsed.Seconds, Thread.CurrentThread.ManagedThreadId);`
	`323`	`+ if (watch != null) Logger.Debug("Parsing run finished after {0}s. (thread {1}).", watch.Elapsed.TotalSeconds, Thread.CurrentThread.ManagedThreadId);`
`324`	`324`	`}`
`325`	`325`
`326`	`326`	`private void ParseAllInternal(object requestor, CancellationToken token)`
Original file line number	Diff line number	Diff line change
`@@ -31,21 +31,27 @@ public event EventHandler<CommandBarButtonClickEventArgs> Click`
`31`	`31`	`{`
`32`	`32`	`add`
`33`	`33`	`{`
`34`		`- ((Microsoft.Office.Core.CommandBarButton)Target).Click += Target_Click;`
	`34`	`+ if (_clickHandler == null)`
	`35`	`+ {`
	`36`	`+ ((Microsoft.Office.Core.CommandBarButton)Target).Click += Target_Click;`
	`37`	`+ }`
`35`	`38`	`_clickHandler += value;`
`36`	`39`	`System.Diagnostics.Debug.WriteLine("Added handler for: {0} '{1}' (tag: {2}, hashcode:{3})", Parent.Name, Target.Caption, Tag, Target.GetHashCode());`
`37`	`40`	`}`
`38`	`41`	`remove`
`39`	`42`	`{`
	`43`	`+ _clickHandler -= value;`
`40`	`44`	`try`
`41`	`45`	`{`
`42`		`- ((Microsoft.Office.Core.CommandBarButton)Target).Click -= Target_Click;`
	`46`	`+ if (_clickHandler == null)`
	`47`	`+ {`
	`48`	`+ ((Microsoft.Office.Core.CommandBarButton)Target).Click -= Target_Click;`
	`49`	`+ }`
`43`	`50`	`}`
`44`	`51`	`catch`
`45`	`52`	`{`
`46`	`53`	`// he's gone, dave.`
`47`	`54`	`}`
`48`		`- _clickHandler -= value;`
`49`	`55`	`System.Diagnostics.Debug.WriteLine("Removed handler for: {0} '{1}' (tag: {2}, hashcode:{3})", Parent.GetType().Name, Target.Caption, Tag, Target.GetHashCode());`
`50`	`56`	`}`
`51`	`57`	`}`
Original file line number	Diff line number	Diff line change
`@@ -6,6 +6,8 @@ namespace Rubberduck.VBEditor.SafeComWrappers.Office.Core`
`6`	`6`	`{`
`7`	`7`	`public class CommandBarControl : SafeComWrapper<Microsoft.Office.Core.CommandBarControl>, ICommandBarControl`
`8`	`8`	`{`
	`9`	`+ public const bool AddCommandBarControlsTemporarily = false;`
	`10`	`+`
`9`	`11`	`public CommandBarControl(Microsoft.Office.Core.CommandBarControl target)`
`10`	`12`	`: base(target)`
`11`	`13`	`{`
`@@ -125,7 +127,7 @@ public bool IsPriorityDropped`
`125`	`127`
`126`	`128`	`public void Delete()`
`127`	`129`	`{`
`128`		`- if (!IsWrappingNullReference) Target.Delete(true);`
	`130`	`+ if (!IsWrappingNullReference) Target.Delete(AddCommandBarControlsTemporarily);`
`129`	`131`	`}`
`130`	`132`
`131`	`133`	`public void Execute()`