Refactor Deactivation to ensure WinEventHooks are Unhooked on the main thread.

* Add IntelliSenseServer Install / Uninstall() to be called from AutoOpen / AutoClose
* Mark IntelliSenseServer.Register() as obsolete to highlight required changes
Fixes #67

Author: govert

Source/ExcelDna.IntelliSense.Host/AddIn.cs

@@ -1,10 +1,4 @@
-using System.Collections.Generic;
-using System.ComponentModel;
-using System.Diagnostics;
-using System.Linq;
-using System.Runtime.InteropServices;
 using ExcelDna.Integration;
-using ExcelDna.Integration.CustomUI;
 using ExcelDna.IntelliSense;
 
 namespace TestAddIn
@@ -13,14 +7,17 @@ public class AddIn : IExcelAddIn
     {
         public void AutoOpen()
         {
-            IntelliSenseServer.Register();
+            IntelliSenseServer.Install();
             //ExcelDna.Logging.LogDisplay.Show();
             //ExcelDna.Logging.LogDisplay.DisplayOrder = ExcelDna.Logging.DisplayOrder.NewestFirst;
         }
 
         public void AutoClose()
         {
-            // CONSIDER: Do we implement an explicit call here, or is the AppDomain Unload event good enough?
+            // The explicit Uninstall call was added in version 1.0.10, 
+            // to give us a chance to unhook the WinEvents (which needs to happen on the main thread)
+            // No easy plan to do this from the AppDomain unload event, which runs on a ThreadPool thread.
+            IntelliSenseServer.Uninstall();
         }
     }
 }

Source/ExcelDna.IntelliSense.Host/ExcelDna.IntelliSense.Host-AddIn.xll.config

@@ -3,12 +3,12 @@
   <system.diagnostics>
     <trace autoflush="true" indentsize="4"/>
     <sources>
-      <source name="ExcelDna.IntelliSense" switchValue="Off">
+      <source name="ExcelDna.IntelliSense" switchValue="All">
         <listeners>
           <!--<remove name="Default" />-->
-          <add name="LogDisplay" type="ExcelDna.Logging.LogDisplayTraceListener,ExcelDna.Integration">
+          <!--<add name="LogDisplay" type="ExcelDna.Logging.LogDisplayTraceListener,ExcelDna.Integration">
             <filter type="System.Diagnostics.EventTypeFilter" initializeData="All"/>
-          </add>
+          </add>-->
           <!-- <add name="File" type="System.Diagnostics.TextWriterTraceListener" initializeData="ExcelDna.IntelliSense.log" >
             <filter type="System.Diagnostics.EventTypeFilter" initializeData="All"/>
           </add> -->

Source/ExcelDna.IntelliSense/IntelliSenseServer.cs

@@ -6,18 +6,20 @@
 using System.Text.RegularExpressions;
 using Microsoft.Win32;
 using ExcelDna.Integration;
+using System.Threading;
 
 namespace ExcelDna.IntelliSense
 {
     // This class implements the registration and activation of the calling add-in as an IntelliSense Server.
     //
     // Among different add-ins that are loaded into an Excel process, at most one IntelliSenseServer can be Active.
-    // This should always be the IntelliSenseServer with the greatest version number among those registered.
+    // This should always be an IntelliSenseServer with the greatest version number among those registered.
+    // 
     // At the moment the bookkeeping for registration and activation in the process is done with environment variables. 
     // (An attractive alternative is the hidden Excel name space: http://www.cpearson.com/excel/hidden.htm )
-    // This prevents cross-AppDomain calls, which are problematic because assemblies are then loaded into multiple AppDomains, and
-    // since the mechanism is intended to cater for different assembly versions, this would be a problem. Also, we don't control
-    // the CLR hosting configuration, so can't always set the MultiDomain flag on setup. COM mechanisms could work, but are complicated.
+    // This allows us not to make cross-AppDomain calls, which are problematic because assemblies are then loaded into multiple AppDomains, and
+    // since the mechanism is intended to cater for different assembly versions, this would be a problem. Also, as we don't control
+    // the CLR hosting configuration, we can't always set the MultiDomain flag on setup. COM mechanisms could work, but are complicated.
     // Another approach would be to use a hidden Excel function that the Active server provides, and have all server register with the active server.
     // When a new server should become active, it then tells the active server, and somehow gets all the other registrations...
 
@@ -28,7 +30,7 @@ namespace ExcelDna.IntelliSense
     // REMEMBER: COM events are not necessarily safe macro contexts.
     public static class IntelliSenseServer
     {
-        const string ServerVersion = "1.0.9";  // TODO: Define and manage this somewhere else
+        const string ServerVersion = "1.0.10";  // TODO: Define and manage this somewhere else
 
         // NOTE: Do not change these constants in custom versions. 
         //       They are part of the co-operative safety mechanism allowing different add-ins providing IntelliSense to work together safely.
@@ -55,12 +57,19 @@ public static class IntelliSenseServer
         static bool _isActive = false;
         static IntelliSenseHelper _helper = null;
 
-        // Called directly from AutoOpen.
+        // The name change from Register to Install is just to force attention on this message for anyone upgrading
+        // - omitting the Uninstall call causes an Excel crash when the add-in is unloaded for any reason
+        [Obsolete("IntelliSenseServer now requires matching calls to Install (inside AutoOpen) and Uninstall (inside AutoClose)", true)]
         public static void Register()
+        {
+        }
+
+        // Called directly from AutoOpen()
+        public static void Install()
         {
             TraceLogger.Initialize();
 
-            Logger.Initialization.Info($"IntelliSenseServer.Register Begin: Version {ServerVersion} in {AppDomain.CurrentDomain.FriendlyName}");
+            Logger.Initialization.Info($"IntelliSenseServer.Install Begin: Version {ServerVersion} in {AppDomain.CurrentDomain.FriendlyName}");
             if (IsDisabled())
                 return;
 
@@ -93,7 +102,30 @@ public static void Register()
 
             AppDomain.CurrentDomain.DomainUnload += CurrentDomain_DomainUnload;
             AppDomain.CurrentDomain.ProcessExit += CurrentDomain_ProcessExit;
-            Logger.Initialization.Info("IntelliSenseServer.Register End");
+            Logger.Initialization.Info("IntelliSenseServer.Install End");
+        }
+
+        // Should be called in a macro context, e.g. from AutoClose()
+        // Was added again in 1.0.10 to clean up WinEvents
+        // Maybe we won't need it if we can move to our own thread handling WinEvents
+        public static void Uninstall()
+        {
+            Logger.Initialization.Info($"IntelliSenseServer.Uninstall Begin: Version {ServerVersion} in {AppDomain.CurrentDomain.FriendlyName}");
+
+            UnpublishRegistration();
+            if (_isActive)
+            {
+                Deactivate();
+
+                // See if there is another server to active
+                var highestRegistration = GetHighestPublishedRegistration();
+                if (highestRegistration != null)
+                {
+                    ActivateServer(highestRegistration);
+                }
+            }
+
+            Logger.Initialization.Info($"IntelliSenseServer.Uninstall End");
         }
 
         // Invokes a Refresh on the Active server (if there is on)
@@ -115,9 +147,8 @@ public static void Refresh()
             Logger.Initialization.Info($"IntelliSenseServer.Refresh End");
         }
 
-        private static void CurrentDomain_ProcessExit(object sender, EventArgs e)
+        static void CurrentDomain_ProcessExit(object sender, EventArgs e)
         {
-            
             // CONSIDER: We get this quite late in the shutdown
             //           We should try to find a way to identify Excel shutdown a lot earlier
             Logger.Initialization.Verbose("IntelliSenseServer ProcessExit Begin");
@@ -129,10 +160,8 @@ private static void CurrentDomain_ProcessExit(object sender, EventArgs e)
                 // Don't try to clean up clean up on process exit - all our resources are in-process anyway
                 // Leads to an error with the main thread SynchronizationContext, which might be shut down already.
                 // In particular we don't have to call UnhookWinEvent: "If the client's thread ends, the system automatically calls this function."
-                // Deactivate();
             }
             Logger.Initialization.Verbose("IntelliSenseServer ProcessExit End");
-            
         }
 
         // DomainUnload runs when AutoClose() would run on the add-in.
@@ -142,20 +171,12 @@ private static void CurrentDomain_ProcessExit(object sender, EventArgs e)
         static void CurrentDomain_DomainUnload(object sender, EventArgs e)
         {
             Logger.Initialization.Verbose("IntelliSenseServer DomainUnload Begin");
-            //// Early shutdown notification
-            //XlCall.ShutdownStarted();
 
-            UnpublishRegistration();
             if (_isActive)
             {
-                Deactivate();
-
-                var highestRegistration = GetHighestPublishedRegistration();
-                if (highestRegistration != null)
-                {
-                    ActivateServer(highestRegistration);
-                }
+                Logger.Initialization.Error("IntelliSenseServer DomainUnload while active - Add and IntelliSenseServer.Unregister() call in AutoClose() !");
             }
+
             Logger.Initialization.Verbose("IntelliSenseServer DomainUnload End");
         }
 
@@ -167,7 +188,6 @@ internal static bool Activate()
                 SetActiveRegistrationInfo();
                 _isActive = true;
 
-                // Now initialize (TODO: perhaps lazily...?)
                 _helper = new IntelliSenseHelper();
                 // TODO: Perhaps also register macro to trigger updates
                 return true;
@@ -179,9 +199,15 @@ internal static bool Activate()
             }
         }
 
-        // Called internally from the AppDomain_DomainUnload or AppDomain_ProcessExit event handler, and via the control function from another server when that server figures out that it must become the active server.
+        // Should be called only on the Excel main thread, either from Unregister (AutoClose) 
+        // or via the control function from another server when that server figures out that it must become the active server.
         internal static bool Deactivate()
         {
+            if (Thread.CurrentThread.ManagedThreadId != 1)
+            {
+                // At least try to log
+                Logger.Initialization.Error($"IntelliSenseServer.Deactivate not called on the main Excel thread!");
+            }
             try
             {
                 if (_helper != null)
@@ -193,7 +219,6 @@ internal static bool Deactivate()
             }
             catch (Exception ex)
             {
-                // TODO: Log
                 Logger.Initialization.Error($"IntelliSenseServer.Deactivate error: {ex}");
                 return false;
             }
@@ -509,7 +534,6 @@ static object IntelliSenseServerControl(object control)
             }
             return false;
         }
-
         #endregion
     }
 }

Source/ExcelDna.IntelliSense/IntellisenseHelper.cs

@@ -6,6 +6,11 @@
 
 namespace ExcelDna.IntelliSense
 {
+    // This is really the running IntelliSenseServer
+    // It brings together:
+    // * the UIMonitor which monitors the state of Excel, including the current function prefix,
+    // * the IntelliSenseDisplay which presents the pop-ups, and 
+    // * the IntelliSenseProviders which figure out what information is available.
     class IntelliSenseHelper : IDisposable
     {
         readonly SynchronizationContext _syncContextMain; // Main thread, not macro context
@@ -78,16 +83,18 @@ internal void RefreshProviders()
             }
         }
 
+        // Must run on the main thread
         public void Dispose()
         {
+            Debug.Assert(Thread.CurrentThread.ManagedThreadId == 1);
             Logger.Initialization.Verbose("IntelliSenseHelper Dispose Start");
 
             foreach (var provider in _providers)
             {
                 provider.Dispose();
             }
-            _uiMonitor.Dispose();
             _display.Dispose();
+            _uiMonitor.Dispose();
 
             Logger.Initialization.Verbose("IntelliSenseHelper Dispose End");
         }

Source/ExcelDna.IntelliSense/Providers/XmlIntelliSenseProvider.cs

@@ -160,7 +160,7 @@ public IList<FunctionInfo> GetFunctionInfos()
 
         void OnInvalidate(object _unused_)
         {
-            Logger.Provider.Verbose($"XmlIntelliSenseProvider.ROnInvalidate - Invoking Invalidate event");
+            Logger.Provider.Verbose($"XmlIntelliSenseProvider.OnInvalidate - Invoking Invalidate event");
             Invalidate?.Invoke(this, EventArgs.Empty);
         }
 

Source/ExcelDna.IntelliSense/UIMonitor/FormulaEditWatcher.cs

@@ -67,9 +67,11 @@ public IntPtr FormulaEditWindow
             }
         }
 
-        SynchronizationContext _syncContextAuto;
-        SynchronizationContext _syncContextMain;
-        WindowWatcher _windowWatcher;
+        readonly SynchronizationContext _syncContextAuto;
+        readonly SynchronizationContext _syncContextMain;
+
+        readonly WindowWatcher _windowWatcher;          // Passed in
+        WindowLocationWatcher  _windowLocationWatcher;  // Managed here
 
         IntPtr            _hwndFormulaBar;
         IntPtr            _hwndInCellEdit;
@@ -240,6 +242,7 @@ void _windowWatcher_InCellEditWindowChanged(object sender, WindowWatcher.WindowC
             }
         }
 
+        // Runs on the automation thread
         void SetEditWindow(IntPtr newWindowHandle, ref IntPtr hwnd)
         {
             if (hwnd != newWindowHandle)
@@ -276,26 +279,21 @@ void SetEditWindow(IntPtr newWindowHandle, ref IntPtr hwnd)
             hwnd = newWindowHandle;
         }
 
-        WindowLocationWatcher _windowLocationWatcher;
-
         // Runs on our Automation thread
         void InstallLocationMonitor(IntPtr hWnd)
         {
-            if (_windowLocationWatcher != null)
-            {
-               _windowLocationWatcher.Dispose();
-            }
+            UninstallLocationMonitor();
             _windowLocationWatcher = new WindowLocationWatcher(hWnd, _syncContextAuto, _syncContextMain);
             _windowLocationWatcher.LocationChanged += _windowLocationWatcher_LocationChanged;
         }
 
         // Runs on our Automation thread
         void UninstallLocationMonitor()
         {
-            if (_windowLocationWatcher != null)
+            WindowLocationWatcher tempWatcher = Interlocked.Exchange(ref _windowLocationWatcher, null);
+            if (tempWatcher != null)
             {
-                _windowLocationWatcher.Dispose();
-                _windowLocationWatcher = null;
+                _syncContextMain.Post(disp => ((IDisposable)disp).Dispose(), tempWatcher);
             }
         }
 
@@ -315,6 +313,7 @@ void _windowLocationWatcher_LocationChanged(object sender, EventArgs e)
         //    UpdateFormula(textChangedOnly: true);
         //}
 
+        // TODO: Get rid of this somehow - added to make the mouse clicks in the in-cell editing work, by delaying the call to the PenHelper
         void UpdateEditStateDelayed()
         {
             _syncContextAuto.Post(_ =>
@@ -406,11 +405,21 @@ void OnStateChanged(StateChangeType stateChangeType)
             StateChanged?.Invoke(this, stateChangedArgs);
         }
 
+        // Runs on the main thread
         public void Dispose()
         {
+            Debug.Assert(Thread.CurrentThread.ManagedThreadId == 1);
+
             Logger.WindowWatcher.Verbose("FormulaEdit Dispose Begin");
             _windowWatcher.FormulaBarWindowChanged -= _windowWatcher_FormulaBarWindowChanged;
             _windowWatcher.InCellEditWindowChanged -= _windowWatcher_InCellEditWindowChanged;
+
+            // Can't call UninstallLocationMonitor - we might be shutting down on the main thread, and don't want to post
+            WindowLocationWatcher tempWatcher = Interlocked.Exchange(ref _windowLocationWatcher, null);
+            if (tempWatcher != null)
+            {
+                tempWatcher.Dispose();
+            }
             Logger.WindowWatcher.Verbose("FormulaEdit Dispose End");
         }
     }

Source/ExcelDna.IntelliSense/UIMonitor/PopupListWatcher.cs

@@ -1,4 +1,5 @@
 using System;
+using System.Diagnostics;
 using System.Threading;
 using System.Windows;
 
@@ -186,6 +187,7 @@ void OnSelectedItemChanged()
 
         public void Dispose()
         {
+            Debug.Assert(Thread.CurrentThread.ManagedThreadId == 1);
             Logger.WindowWatcher.Info($"PopupList Dispose Begin");
             _windowWatcher.PopupListWindowChanged -= _windowWatcher_PopupListWindowChanged;
             _windowWatcher = null;