Reboot sandbox when required

Another chapter in the never-ending AppDomain security saga.

If a computer goes to sleep while SourceGen is running with a project
open, life gets confusing when the system wakes up.  The keep-alive
timer fires and a ping is sent to the remote AppDomain, successfully.
At the same time, the lease expires on the remote side, and the objects
are discarded (apparently without bothering to query the ILease object).
This failure mode is 100% repeatable.

Since we can't prevent sandbox objects from disappearing, we have to
detect and recover from the problem.  Fortunately we don't keep any
necessary state on the plugin side, so we can just tear the whole
thing down and recreate it.

The various methods in ScriptManager now do a "health check" before
making calls into the plugin AppDomain.  If the ping attempt fails,
the AppDomain is "rebooted" by destroying it and creating a new one,
reloading all plugins that were in there before.  The plugin binaries
*should* still be in the PluginDllCache directory since the ping failure
was due to objects being discarded, not AppDomain shutdown, and Windows
doesn't let you mess with files that hold executable code.

A new "reboot security sandbox" option has been added to the DEBUG
menu to facilitate testing.

The PluginManager's Ping() method gets called more often, but not to
the extent that performance will be affected.

This change also adds a finalizer to DisasmProject, since we're relying
on it to shut down the ScriptManager, and it's relying on callers to
invoke its cleanup function.  The finalizer throws an assertion if the
cleanup function doesn't get called.

(Issue #82)

PluginCommon/PluginManager.cs

@@ -76,9 +76,12 @@ namespace PluginCommon {
         /// <summary>
         /// Tests simple round-trip communication.  This may be called from an arbitrary thread.
         /// </summary>
+        /// <remarks>
+        /// This is used for keep-alives and health checks, so it may be called frequently.
+        /// </remarks>
         public int Ping(int val) {
-            Debug.WriteLine("PluginManager Ping tid=" + Thread.CurrentThread.ManagedThreadId +
-                " (id=" + AppDomain.CurrentDomain.Id + "): " + val);
+            //Debug.WriteLine("PluginManager Ping tid=" + Thread.CurrentThread.ManagedThreadId +
+            //    " (id=" + AppDomain.CurrentDomain.Id + "): " + val);
             int result = (int)(DateTime.Now - mLastPing).TotalSeconds;
             mLastPing = DateTime.Now;
             return result;

SourceGen/DisasmProject.cs

@@ -29,7 +29,7 @@ namespace SourceGen {
     /// 
     /// This class does no file I/O or user interaction.
     /// </summary>
-    public class DisasmProject {
+    public class DisasmProject : IDisposable {
         // Arbitrary 1MB limit.  Could be increased to 16MB if performance is acceptable.
         public const int MAX_DATA_FILE_SIZE = 1 << 20;
 
@@ -322,6 +322,24 @@ namespace SourceGen {
             }
         }
 
+        // IDisposable generic finalizer.
+        ~DisasmProject() {
+            Dispose(false);
+        }
+        // IDisposable generic Dispose() implementation.
+        public void Dispose() {
+            Dispose(true);
+            GC.SuppressFinalize(this);
+        }
+        /// <summary>
+        /// Confirms that Cleanup() was called.  This is just a behavior check; the
+        /// destructor is not required for correct behavior.
+        /// </summary>
+        protected virtual void Dispose(bool disposing) {
+            //Debug.WriteLine("DisasmProject Dispose(" + disposing + ")");
+            Debug.Assert(mScriptManager == null, "DisasmProject.Cleanup was not called");
+        }
+
         /// <summary>
         /// Prepares the DisasmProject for use as a new project.
         /// </summary>
@@ -2622,6 +2640,10 @@ namespace SourceGen {
             mScriptManager.UnprepareScripts();
         }
 
+        public void DebugRebootSandbox() {
+            mScriptManager.RebootSandbox();
+        }
+
         /// <summary>
         /// For debugging purposes, get some information about the currently loaded
         /// extension scripts.

SourceGen/MainController.cs

@@ -4417,6 +4417,7 @@ namespace SourceGen {
         }
 
         public void Debug_ApplyEditCommands() {
+            // Might want to suggest disabling Edit > Toggle Data Scan for some merges.
             OpenFileDialog fileDlg = new OpenFileDialog() {
                 Filter = Res.Strings.FILE_FILTER_SGEC + "|" + Res.Strings.FILE_FILTER_ALL,
                 FilterIndex = 1
@@ -4486,6 +4487,11 @@ namespace SourceGen {
             ApplyUndoableChanges(cs);
         }
 
+        public void Debug_RebootSecuritySandbox() {
+            Debug.Assert(mProject != null);
+            mProject.DebugRebootSandbox();
+        }
+
         #endregion Debug features
     }
 }

SourceGen/RuntimeData/Help/advanced.html

@@ -430,6 +430,9 @@ not help you debug 6502 projects.</p>
     an infrequently-encountered Windows bug.  (See code for notes and
     stackoverflow.com links.)
     This setting is not persistent.</li>
+  <li>Reboot Security Sandbox.  Discards the sandbox, creates a new one,
+    and reloads it.  Only useful for exercising the sandbox code that
+    runs when the keep-alives are unsuccessful.</li>
   <li>Applesoft to HTML.  An experimental feature that formats an
     Applesoft program as HTML.</li>
   <li>Export Edit Commands.  Outputs comments and notes in

SourceGen/Sandbox/DomainManager.cs

@@ -53,7 +53,7 @@ namespace SourceGen.Sandbox {
         /// </summary>
         public PluginManager PluginMgr {
             get {
-                Debug.Assert(mPluginManager.CheckLease());
+                //Debug.Assert(mPluginManager.CheckLease());
                 return mPluginManager.Instance;
             }
         }

SourceGen/Sandbox/ScriptManager.cs

@@ -17,6 +17,7 @@ using System;
 using System.Collections.Generic;
 using System.Diagnostics;
 using System.Reflection;
+using System.Runtime.Remoting;
 using System.Text;
 
 using CommonUtil;
@@ -27,6 +28,8 @@ namespace SourceGen.Sandbox {
     /// Maintains a collection of IPlugin instances, or communicates with the remote
     /// PluginManager that holds the collection.  Whether the plugins are instantiated
     /// locally depends on how the class is constructed.
+    ///
+    /// One of these will be instantiated when the DisasmProject is created.
     /// </summary>
     public class ScriptManager {
         public const string FILENAME_EXT = ".cs";
@@ -37,6 +40,13 @@ namespace SourceGen.Sandbox {
         /// </summary>
         public static bool UseKeepAliveHack { get; set; }
 
+        /// <summary>
+        /// If true, this ScriptManager is not using a DomainManager.
+        /// </summary>
+        public bool UseMainAppDomain {
+            get { return DomainMgr == null; }
+        }
+
         /// <summary>
         /// Reference to DomainManager, if we're using one.
         /// </summary>
@@ -52,6 +62,21 @@ namespace SourceGen.Sandbox {
         /// </summary>
         private DisasmProject mProject;
 
+        private class LoadedPluginPath {
+            public string ScriptIdent { get; private set; }
+            public string DllPath { get; private set; }
+
+            public LoadedPluginPath(string scriptIdent, string dllPath) {
+                ScriptIdent = scriptIdent;
+                DllPath = dllPath;
+            }
+        }
+
+        /// <summary>
+        /// List of paths to loaded plugins.  Used if we need to "reboot" the sandbox.
+        /// </summary>
+        private List<LoadedPluginPath> mLoadedPlugins = new List<LoadedPluginPath>();
+
 
         /// <summary>
         /// Constructor.
@@ -60,14 +85,21 @@ namespace SourceGen.Sandbox {
             mProject = proj;
 
             if (!proj.UseMainAppDomainForPlugins) {
-                DomainMgr = new DomainManager(UseKeepAliveHack);
-                DomainMgr.CreateDomain("Plugin Domain", PluginDllCache.GetPluginDirPath());
-                DomainMgr.PluginMgr.SetFileData(proj.FileData);
+                CreateDomainManager();
             } else {
                 mActivePlugins = new Dictionary<string, IPlugin>();
             }
         }
 
+        private void CreateDomainManager() {
+            // The project's UseMainAppDomainForPlugins value is theoretically mutable, so
+            // don't try to assert it here.
+            DomainMgr = new DomainManager(UseKeepAliveHack);
+            DomainMgr.CreateDomain("Plugin Domain", PluginDllCache.GetPluginDirPath());
+            DomainMgr.PluginMgr.SetFileData(mProject.FileData);
+
+        }
+
         /// <summary>
         /// Cleans up, discarding the AppDomain if one was created.  Do not continue to use
         /// the object after calling this.
@@ -89,8 +121,10 @@ namespace SourceGen.Sandbox {
             if (DomainMgr == null) {
                 mActivePlugins.Clear();
             } else {
+                CheckHealth();
                 DomainMgr.PluginMgr.ClearPluginList();
             }
+            mLoadedPlugins.Clear();
         }
 
         /// <summary>
@@ -118,15 +152,76 @@ namespace SourceGen.Sandbox {
                 report = new FileLoadReport(dllPath);       // empty report
                 return true;
             } else {
+                CheckHealth();
                 IPlugin plugin = DomainMgr.PluginMgr.LoadPlugin(dllPath, scriptIdent,
                     out string failMsg);
                 if (plugin == null) {
                     report.Add(FileLoadItem.Type.Error, "Failed loading plugin: " + failMsg);
+                } else {
+                    mLoadedPlugins.Add(new LoadedPluginPath(scriptIdent, dllPath));
                 }
                 return plugin != null;
             }
         }
 
+        /// <summary>
+        /// Reboots the sandbox by discarding the old DomainManager, creating a new one, and
+        /// reloading all of the plugins.
+        /// </summary>
+        /// <returns>True if no problems were encountered.</returns>
+        public bool RebootSandbox() {
+            if (DomainMgr == null) {
+                return false;
+            }
+            Debug.WriteLine("Rebooting sandbox...");
+
+            // Discard existing DomainManager, and create a new one.
+            DomainMgr.Dispose();
+            CreateDomainManager();
+
+            bool failed = false;
+
+            // Reload plugins.
+            foreach (LoadedPluginPath lpp in mLoadedPlugins) {
+                IPlugin plugin = DomainMgr.PluginMgr.LoadPlugin(lpp.DllPath, lpp.ScriptIdent,
+                    out string failMsg);
+                if (plugin == null) {
+                    // This is unexpected; we're opening a DLL that we recently had open.
+                    // Not a lot we can do to recover, and we're probably too deep to report
+                    // a failure to the user.
+                    Debug.WriteLine("Failed to reopen '" + lpp.DllPath + "': " + failMsg);
+                    failed = true;
+                    // continue on to the next one
+                } else {
+                    Debug.WriteLine("  Reloaded " + lpp.ScriptIdent);
+                }
+            }
+
+            return failed;
+        }
+
+        /// <summary>
+        /// Checks the health of the sandbox, and reboots it if it seems unhealthy.  Call this
+        /// before making any calls into plugins via DomainMgr.
+        /// </summary>
+        /// <remarks>
+        /// We're relying on the idea that, if the ping succeeds, the PluginManager instance
+        /// will continue to exist for a while.  There is some evidence to the contrary -- the
+        /// ping issued immediately after the machine wakes up succeeds right before the remote
+        /// objects get discarded -- but I'm hoping that's due to a race condition that won't
+        /// happen in normal circumstances (because of the keep-alives we send).
+        /// </remarks>
+        private void CheckHealth() {
+            Debug.Assert(DomainMgr != null);
+            try {
+                DomainMgr.PluginMgr.Ping(111);
+            } catch (RemotingException re) {
+                Debug.WriteLine("Health check failed: " + re.Message);
+                RebootSandbox();
+                DomainMgr.PluginMgr.Ping(112);
+            }
+        }
+
         public IPlugin GetInstance(string scriptIdent) {
             if (DomainMgr == null) {
                 if (mActivePlugins.TryGetValue(scriptIdent, out IPlugin plugin)) {
@@ -135,6 +230,7 @@ namespace SourceGen.Sandbox {
                 Debug.Assert(false);
                 return null;
             } else {
+                CheckHealth();
                 return DomainMgr.PluginMgr.GetPlugin(scriptIdent);
             }
         }
@@ -148,6 +244,7 @@ namespace SourceGen.Sandbox {
             if (DomainMgr == null) {
                 dict = mActivePlugins;
             } else {
+                CheckHealth();
                 dict = DomainMgr.PluginMgr.GetActivePlugins();
             }
             List<IPlugin> list = new List<IPlugin>(dict.Count);
@@ -174,6 +271,7 @@ namespace SourceGen.Sandbox {
                     }
                 }
             } else {
+                CheckHealth();
                 List<AddressMap.AddressMapEntry> addrEnts = mProject.AddrMap.GetEntryList();
                 DomainMgr.PluginMgr.PreparePlugins(appRef, addrEnts, plSyms);
             }
@@ -189,7 +287,7 @@ namespace SourceGen.Sandbox {
                     ipl.Unprepare();
                 }
             } else {
-                List<AddressMap.AddressMapEntry> addrEnts = mProject.AddrMap.GetEntryList();
+                CheckHealth();
                 DomainMgr.PluginMgr.UnpreparePlugins();
             }
         }
@@ -199,6 +297,10 @@ namespace SourceGen.Sandbox {
         /// Returns true if any of the plugins report that the before or after label is
         /// significant.
         /// </summary>
+        /// <remarks>
+        /// This is called when a label is edited, so DisasmProject can decide whether it
+        /// needs to re-run the code analyzer.
+        /// </remarks>
         public bool IsLabelSignificant(Symbol before, Symbol after) {
             string labelBefore = (before == null) ? string.Empty : before.Label;
             string labelAfter = (after == null) ? string.Empty : after.Label;
@@ -213,6 +315,7 @@ namespace SourceGen.Sandbox {
                 }
                 return false;
             } else {
+                CheckHealth();
                 return DomainMgr.PluginMgr.IsLabelSignificant(labelBefore, labelAfter);
             }
         }
@@ -309,6 +412,7 @@ namespace SourceGen.Sandbox {
                 }
                 return pdict;
             } else {
+                CheckHealth();
                 return DomainMgr.PluginMgr.GetActivePlugins();
             }
         }
@@ -328,6 +432,7 @@ namespace SourceGen.Sandbox {
                     DebugGetScriptInfo(kvp.Value, sb);
                 }
             } else {
+                CheckHealth();
                 Dictionary<string, IPlugin> plugins = DomainMgr.PluginMgr.GetActivePlugins();
                 foreach (IPlugin plugin in plugins.Values) {
                     string loc = DomainMgr.PluginMgr.GetPluginAssemblyLocation(plugin);
@@ -359,7 +464,9 @@ namespace SourceGen.Sandbox {
             if (plugin is PluginCommon.IPlugin_InlineBrk) {
                 sb.Append(" InlineBrk");
             }
-            if (plugin is PluginCommon.IPlugin_Visualizer) {
+            if (plugin is PluginCommon.IPlugin_Visualizer_v2) {
+                sb.Append(" Visualizer2");
+            } else if (plugin is PluginCommon.IPlugin_Visualizer) {
                 sb.Append(" Visualizer");
             }
             sb.Append("\r\n");

SourceGen/WpfGui/MainWindow.xaml

@@ -203,6 +203,7 @@ limitations under the License.
             <RoutedUICommand x:Key="Debug_ApplyPlatformSymbolsCmd" Text="Apply Platform Symbols"/>
             <RoutedUICommand x:Key="Debug_ExportEditCommandsCmd" Text="Export Edit Commands..."/>
             <RoutedUICommand x:Key="Debug_ExtensionScriptInfoCmd" Text="Extension Script Info..."/>
+            <RoutedUICommand x:Key="Debug_RebootSecuritySandboxCmd" Text="Reboot Security Sandbox"/>
             <RoutedUICommand x:Key="Debug_ShowAnalysisTimersCmd" Text="Show Analysis Timers"/>
             <RoutedUICommand x:Key="Debug_ShowAnalyzerOutputCmd" Text="Show Analyzer Output"/>
             <RoutedUICommand x:Key="Debug_ShowUndoRedoHistoryCmd" Text="Show Undo/Redo History"/>
@@ -345,6 +346,8 @@ limitations under the License.
                         CanExecute="IsProjectOpen" Executed="Debug_ExportEditCommandsCmd_Executed"/>
         <CommandBinding Command="{StaticResource Debug_ExtensionScriptInfoCmd}"
                         CanExecute="IsProjectOpen" Executed="Debug_ExtensionScriptInfoCmd_Executed"/>
+        <CommandBinding Command="{StaticResource Debug_RebootSecuritySandboxCmd}"
+                        CanExecute="IsProjectOpen" Executed="Debug_RebootSecuritySandboxCmd_Executed"/>
         <CommandBinding Command="{StaticResource Debug_ShowAnalysisTimersCmd}"
                         CanExecute="IsProjectOpen" Executed="Debug_ShowAnalysisTimersCmd_Executed"/>
         <CommandBinding Command="{StaticResource Debug_ShowAnalyzerOutputCmd}"
@@ -465,6 +468,7 @@ limitations under the License.
                           Command="{StaticResource Debug_ToggleSecuritySandboxCmd}" IsCheckable="True"/>
                 <MenuItem Name="debugKeepAliveHackMenuItem"
                           Command="{StaticResource Debug_ToggleKeepAliveHackCmd}" IsCheckable="True"/>
+                <MenuItem Command="{StaticResource Debug_RebootSecuritySandboxCmd}"/>
                 <Separator/>
                 <MenuItem Command="{StaticResource Debug_ApplesoftToHtmlCmd}"/>
                 <MenuItem Command="{StaticResource Debug_ExportEditCommandsCmd}"/>

SourceGen/WpfGui/MainWindow.xaml.cs

@@ -1405,6 +1405,11 @@ namespace SourceGen.WpfGui {
             mMainCtrl.Debug_ExtensionScriptInfo();
         }
 
+        private void Debug_RebootSecuritySandboxCmd_Executed(object sender,
+                ExecutedRoutedEventArgs e) {
+            mMainCtrl.Debug_RebootSecuritySandbox();
+        }
+
         private void Debug_RefreshCmd_Executed(object sender, ExecutedRoutedEventArgs e) {
             mMainCtrl.Debug_Refresh();
         }