Merge pull request #4268 from mansellan/3949

Fix the leaky event sink for VB6 commandbars

Rubberduck.VBEEditor/Rubberduck.VBEditor.csproj

@@ -78,6 +78,11 @@
     <Compile Include="Factories\ISafeComWrapperProvider.cs" />
     <Compile Include="Factories\VBEFactory.cs" />
     <Compile Include="HashCode.cs" />
+    <Compile Include="SafeComWrappers\Abstract\IComIndexedProperty.cs" />
+    <Compile Include="SafeComWrappers\Abstract\IEventSource.cs" />
+    <Compile Include="SafeComWrappers\VB\Abstract\ICommandBarButtonEvents.cs" />
+    <Compile Include="SafeComWrappers\VB\Abstract\ICommandBarEvents.cs" />
+    <Compile Include="SafeComWrappers\VB\Abstract\IEvents.cs" />
     <Compile Include="SourceCodeHandling\SourceFileHandlerSourceCodeHandlerAdapter.cs" />
     <Compile Include="SourceCodeHandling\CodePaneSourceCodeHandler.cs" />
     <Compile Include="SourceCodeHandling\ISourceCodeHandler.cs" />

Rubberduck.VBEEditor/SafeComWrappers/Abstract/IComIndexedProperty.cs

@@ -0,0 +1,7 @@
+namespace Rubberduck.VBEditor.SafeComWrappers.Abstract
+{
+    public interface IComIndexedProperty<out TItem>
+    {
+        TItem this[object index] { get; }
+    }
+}

Rubberduck.VBEEditor/SafeComWrappers/Abstract/IEventSource.cs

@@ -0,0 +1,7 @@
+namespace Rubberduck.VBEditor.SafeComWrappers.Abstract
+{
+    public interface IEventSource<out TEventSource>
+    {
+        TEventSource EventSource { get; }
+    }
+}

Rubberduck.VBEEditor/SafeComWrappers/SafeRedirectedEventedComWrapper.cs

@@ -1,4 +1,4 @@
-using System;
+using System.Diagnostics;
 using System.Runtime.InteropServices;
 using System.Runtime.InteropServices.ComTypes;
 using Rubberduck.VBEditor.SafeComWrappers.Abstract;
@@ -13,73 +13,83 @@ public abstract class SafeRedirectedEventedComWrapper<TSource, TEventSource, TEv
         private const int NotAdvising = 0;
         private readonly object _lock = new object();
         private TEventSource _eventSource; // The event source
+        private TEventInterface _eventSink; // The event sink
         private IConnectionPoint _icp; // The connection point
-        private int _cookie = NotAdvising;     // The cookie for the connection
+        private int _cookie = NotAdvising; // The cookie for the connection
 
-        protected SafeRedirectedEventedComWrapper(TSource target, TEventSource eventSource, bool rewrapping = false) : base(target, rewrapping)
+        protected SafeRedirectedEventedComWrapper(TSource target, bool rewrapping = false) 
+            : base(target, rewrapping)
         {
-            _eventSource = eventSource;
         }
 
         protected override void Dispose(bool disposing)
         {
             DetachEvents();
-
-            Marshal.ReleaseComObject(_eventSource);
-            _eventSource = null;
-
             base.Dispose(disposing);
         }
 
-        public void AttachEvents()
+        protected void AttachEvents(IEventSource<TEventSource> eventSource, TEventInterface eventSink)
         {
+            Debug.Assert(eventSource != null, "Unable to attach events - eventSource is null");
+            Debug.Assert(eventSink != null, "Unable to attach events - eventSink is null");
+            if (eventSource == null || eventSink == null)
+            {
+                return;
+            }            
+
             lock (_lock)
             {
                 if (IsWrappingNullReference)
                 {
                     return;
                 }
-
-                if (_cookie != NotAdvising)
-                {
+
+                // Check that events not already attached
+                if (_eventSource != null || _eventSink != null)
+                {                   
                     return;
                 }
 
+                _eventSource = eventSource.EventSource;
+                _eventSink = eventSink;
+
                 // Call QueryInterface for IConnectionPointContainer
-                var icpc = (IConnectionPointContainer) _eventSource;
+                if (!(_eventSource is IConnectionPointContainer icpc))
+                {
+                    Debug.Assert(false, $"Unable to attach events - supplied type {_eventSource.GetType().Name} is not a connection point container.");
+                    return;
+                }
 
                 // Find the connection point for the source interface
                 var g = typeof(TEventInterface).GUID;
                 icpc.FindConnectionPoint(ref g, out _icp);
-
-                var sink = this as TEventInterface;
-
-                if (sink == null)
-                {
-                    throw new InvalidOperationException($"The class {this.GetType()} does not implement the required event interface {typeof(TEventInterface)}");
-                }
-
-                _icp.Advise(sink, out _cookie);
+                _icp.Advise(_eventSink, out _cookie);
             }
         }
 
+        public abstract void AttachEvents();
+
         public void DetachEvents()
         {
             lock (_lock)
             {
-                if (_icp == null)
+                if (_icp != null)
                 {
-                    return;
+                    if (_cookie != NotAdvising)
+                    {
+                        _icp.Unadvise(_cookie);
+                        _cookie = NotAdvising;
+                    }
+
+                    Marshal.ReleaseComObject(_icp);
+                    _icp = null;
                 }
 
-                if (_cookie != NotAdvising)
+                if (_eventSource != null)
                 {
-                    _icp.Unadvise(_cookie);
-                    _cookie = NotAdvising;
+                    Marshal.ReleaseComObject(_eventSource);
+                    _eventSource = null;
                 }
-
-                Marshal.ReleaseComObject(_icp);                
-                _icp = null;                
             }
         }
     }

Rubberduck.VBEEditor/SafeComWrappers/VB/Abstract/ICommandBarButtonEvents.cs

@@ -0,0 +1,8 @@
+using System;
+
+namespace Rubberduck.VBEditor.SafeComWrappers.Abstract
+{
+    public interface ICommandBarButtonEvents : ISafeComWrapper, IEquatable<ICommandBarButtonEvents>
+    {
+    }
+}

Rubberduck.VBEEditor/SafeComWrappers/VB/Abstract/ICommandBarEvents.cs

@@ -0,0 +1,8 @@
+using System;
+
+namespace Rubberduck.VBEditor.SafeComWrappers.Abstract
+{
+    public interface ICommandBarEvents : ISafeComWrapper, IComIndexedProperty<ICommandBarButtonEvents>, IEquatable<ICommandBarEvents>
+    {
+    }
+}

Rubberduck.VBEEditor/SafeComWrappers/VB/Abstract/IEvents.cs

@@ -0,0 +1,11 @@
+using System;
+
+namespace Rubberduck.VBEditor.SafeComWrappers.Abstract
+{
+    // This interface is not included on IVBE, as it is only safe in VB6
+    // https://stackoverflow.com/questions/41055765/whats-the-difference-between-commandbarevents-click-and-commandbarbutton-click/41066408#41066408
+    public interface IEvents : ISafeComWrapper, IEquatable<IEvents>
+    {
+        ICommandBarEvents CommandBarEvents { get; }
+    }
+}

Rubberduck.VBEditor.VB6/Rubberduck.VBEditor.VB6.csproj

@@ -78,6 +78,9 @@
     <Compile Include="SafeComWrappers\VB\CodeModule.cs" />
     <Compile Include="SafeComWrappers\VB\CodePane.cs" />
     <Compile Include="SafeComWrappers\VB\CodePanes.cs" />
+    <Compile Include="SafeComWrappers\VB\CommandBarButtonEvents.cs" />
+    <Compile Include="SafeComWrappers\VB\CommandBarEvents.cs" />
+    <Compile Include="SafeComWrappers\VB\Events.cs" />
     <Compile Include="SafeComWrappers\VB\LinkedWindows.cs" />
     <Compile Include="SafeComWrappers\VB\Properties.cs" />
     <Compile Include="SafeComWrappers\VB\Property.cs" />

Rubberduck.VBEditor.VB6/SafeComWrappers/Office/CommandBarButton.cs

@@ -1,9 +1,6 @@
 extern alias Office_v8;
 using System;
 using System.Drawing;
-using System.Runtime.InteropServices;
-using System.Windows.Forms;
-using Microsoft.CSharp.RuntimeBinder;
 using NLog;
 using Rubberduck.VBEditor.SafeComWrappers.Abstract;
 using MSO = Office_v8::Office;
@@ -22,11 +19,10 @@ public class CommandBarButton : SafeRedirectedEventedComWrapper<MSO.CommandBarBu
         // Command bar click event is sourced from VBE.Events.CommandBarEvents[index]
         // where index is the command bar button COM object.
         public CommandBarButton(MSO.CommandBarButton target, IVBE vbe, bool rewrapping = false) 
-            : base(target, ((VB.VBE)vbe.HardReference).Events.CommandBarEvents[target], rewrapping)
+            : base(target, rewrapping)
         {
             _control = new CommandBarControl(target, vbe, rewrapping);
             _vbe = vbe;
-
         }
 
         private MSO.CommandBarButton Button => Target;
@@ -252,8 +248,9 @@ public override int GetHashCode()
                 lock (_eventLock)
                 {
                     _click += value;
-                    if (_click != null && _click.GetInvocationList().Length != 0)
+                    if (_click != null && _click.GetInvocationList().Length == 1)
                     {
+                        // First subscriber attached - attach COM events
                         AttachEvents();
                     }
                 }
@@ -265,8 +262,9 @@ public override int GetHashCode()
                     _click -= value;
                     if (_click == null || _click.GetInvocationList().Length == 0)
                     {
+                        // Last subscriber detached - detach COM events 
                         DetachEvents();
-                    };
+                    }
                 }
             }
         }
@@ -296,5 +294,17 @@ protected override void Dispose(bool disposing)
             base.Dispose(disposing);
             _control.Dispose();
         }
+
+        public override void AttachEvents()
+        {
+            // Cast to VB6 VBE SafeComWrapper as  events are not exposed on IVBE as they are only safe to use in VB6
+            using (var events = ((VB6.VBE)_vbe).Events)
+            using (var commandBarEvents = events.CommandBarEvents)
+            {
+                // Disposal of buttonEvents is handled by the base class
+                var buttonEvents = commandBarEvents[Target] as IEventSource<VB.CommandBarEvents>;
+                AttachEvents(buttonEvents, this);
+            }
+        }
     }
 }

Rubberduck.VBEditor.VB6/SafeComWrappers/VB/CommandBarButtonEvents.cs

@@ -0,0 +1,31 @@
+using Rubberduck.VBEditor.SafeComWrappers.Abstract;
+using VB = Microsoft.Vbe.Interop.VB6;
+
+namespace Rubberduck.VBEditor.SafeComWrappers.VB6
+{
+    public class CommandBarButtonEvents : SafeComWrapper<VB.CommandBarEvents>, ICommandBarButtonEvents, IEventSource<VB.CommandBarEvents>
+    {
+        public CommandBarButtonEvents(VB.CommandBarEvents target, bool rewrapping = false)
+            : base(target, rewrapping)
+        {            
+        }
+
+        // Explicit implementation as usage should only be from within a SafeComWrapper
+        VB.CommandBarEvents IEventSource<VB.CommandBarEvents>.EventSource => Target;
+
+        public override bool Equals(ISafeComWrapper<VB.CommandBarEvents> other)
+        {
+            return IsEqualIfNull(other) || (other != null && ReferenceEquals(other.Target, Target));
+        }
+
+        public bool Equals(ICommandBarButtonEvents other)
+        {
+            return Equals(other as SafeComWrapper<VB.CommandBarEvents>);
+        }
+
+        public override int GetHashCode()
+        {
+            return IsWrappingNullReference ? 0 : Target.GetHashCode();
+        }
+    }
+}

Rubberduck.VBEditor.VB6/SafeComWrappers/VB/CommandBarEvents.cs

@@ -0,0 +1,31 @@
+using Rubberduck.VBEditor.SafeComWrappers.Abstract;
+using VB = Microsoft.Vbe.Interop.VB6;
+
+// ReSharper disable once CheckNamespace - Special dispensation due to conflicting file vs namespace priorities
+namespace Rubberduck.VBEditor.SafeComWrappers.VB6
+{
+    public class CommandBarEvents : SafeComWrapper<VB.Events>, ICommandBarEvents
+    {
+        public CommandBarEvents(VB.Events target, bool rewrapping = false)
+            : base(target, rewrapping)
+        {
+        }
+
+        public ICommandBarButtonEvents this[object button] => new CommandBarButtonEvents(IsWrappingNullReference ? null : Target.CommandBarEvents[button]);
+
+        public override bool Equals(ISafeComWrapper<VB.Events> other)
+        {
+            return IsEqualIfNull(other) || (other != null && ReferenceEquals(other.Target, Target));
+        }
+
+        public bool Equals(ICommandBarEvents other)
+        {
+            return Equals(other as SafeComWrapper<VB.Events>);
+        }
+
+        public override int GetHashCode()
+        {
+            return IsWrappingNullReference ? 0 : Target.GetHashCode();
+        }
+    }
+}

Rubberduck.VBEditor.VB6/SafeComWrappers/VB/Events.cs

@@ -0,0 +1,31 @@
+using Rubberduck.VBEditor.SafeComWrappers.Abstract;
+using VB = Microsoft.Vbe.Interop.VB6;
+
+// ReSharper disable once CheckNamespace - Special dispensation due to conflicting file vs namespace priorities
+namespace Rubberduck.VBEditor.SafeComWrappers.VB6
+{
+    public class Events : SafeComWrapper<VB.Events>, IEvents
+    {
+        public Events(VB.Events target, bool rewrapping = false)
+            : base(target, rewrapping)
+        {
+        }
+
+        public ICommandBarEvents CommandBarEvents => new CommandBarEvents(IsWrappingNullReference ? null : Target, true);
+
+        public override bool Equals(ISafeComWrapper<VB.Events> other)
+        {
+            return IsEqualIfNull(other) || (other != null && ReferenceEquals(other.Target, Target));
+        }
+
+        public bool Equals(IEvents other)
+        {
+            return Equals(other as SafeComWrapper<VB.Events>);
+        }
+
+        public override int GetHashCode()
+        {
+            return IsWrappingNullReference ? 0 : Target.GetHashCode();
+        }
+    }
+}

Rubberduck.VBEditor.VB6/SafeComWrappers/VB/VBE.cs

@@ -78,6 +78,8 @@ public IWindow MainWindow
         public IVBProjects VBProjects => new VBProjects(IsWrappingNullReference ? null : Target.VBProjects);
 
         public IWindows Windows => new Windows(IsWrappingNullReference ? null : Target.Windows);
+
+        public IEvents Events => new Events(IsWrappingNullReference ? null : Target.Events);
 
         public override bool Equals(ISafeComWrapper<VB.VBE> other)
         {

Rubberduck.VBEditor.VBA/SafeComWrappers/Office/CommandBarButton.cs

@@ -291,8 +291,9 @@ public override int GetHashCode()
                 lock (_eventLock)
                 {
                     _click += value;
-                    if (_click != null && _click.GetInvocationList().Length != 0)
+                    if (_click != null && _click.GetInvocationList().Length == 1)
                     {
+                        // First subscriber attached - attach COM events
                         AttachEvents();
                     }
                 }
@@ -304,6 +305,7 @@ public override int GetHashCode()
                     _click -= value;
                     if (_click == null || _click.GetInvocationList().Length == 0)
                     {
+                        // Last subscriber detached - detach COM events
                         DetachEvents();
                     };
                 }

Rubberduck.VBEditor.VBA/SafeComWrappers/VB/VBE.cs

@@ -80,7 +80,7 @@ public IWindow MainWindow
         public IVBProjects VBProjects => new VBProjects(IsWrappingNullReference ? null : Target.VBProjects);
 
         public IWindows Windows => new Windows(IsWrappingNullReference ? null : Target.Windows);
-        
+
         public override bool Equals(ISafeComWrapper<VB.VBE> other)
         {
             return IsEqualIfNull(other) || (other != null && other.Target.Version == Version);