Bug 2044741 - [devtools] Delay devtools JSProcess actor unregistration. r=devtools-reviewers,nchevobbe

Avoid unregistering the JS Process actor ASAP on DevTools closing
as it may throw many exception if any RDP Request or JSProcess Actor
query is still pending after the devtools closing.

It required some tweaks in the parent process as WatcherActor.conn
may now be null because of shifted executions and we weren't correctly
unregistering it because of lack of `connectionPrefix`.

As well as some tweaks around `sharedData` to force persisting it across
processes, otherwise the content process may still hold now destroyed
watcher actor sessionData and keep instantiating actor.

Last, but not least, because there is some shift in precise execution
timings, I had to tweak docshell DOM event listeners as we may leave
listeners on docshell's window object when the cleanup method
is call after the docshell starts being destroyed and docshell.domWindow
is already null.

Differential Revision: https://phabricator.services.mozilla.com/D306953

devtools/server/actors/targets/window-global.js

@@ -1797,7 +1797,12 @@ class DebuggerProgressListener {
     // inner-window-destroyed events. Bug 1016952 would remove the need for this.
     this._knownWindowIDs = new Map();
 
-    this._watchedDocShells = new WeakSet();
+    // Map of all the currently watched top level docshells.
+    //   Map(docshell => AbortController)
+    // We mostly keep track of all the docshells via the Map keys,
+    // but store as value the AbortController to unregister all event listeners
+    // against the docshell document.
+    this._watchedDocShellsAbortControllersMap = new Map();
   }
 
   QueryInterface = ChromeUtils.generateQI([
@@ -1809,14 +1814,18 @@ class DebuggerProgressListener {
     Services.obs.removeObserver(this, "inner-window-destroyed");
     this._knownWindowIDs.clear();
     this._knownWindowIDs = null;
+
+    // TargetActor should have manually unwatch the docshells,
+    // but just to be safe, cleanup any leftover
+    for (const abortController of this._watchedDocShellsAbortControllersMap.values()) {
+      abortController.abort();
+    }
+    this._watchedDocShellsAbortControllersMap.clear();
   }
 
   watch(docShell) {
-    // Add the docshell to the watched set. We're actually adding the window,
-    // because docShell objects are not wrappercached and would be rejected
-    // by the WeakSet.
-    const docShellWindow = docShell.domWindow;
-    this._watchedDocShells.add(docShellWindow);
+    const abortController = new AbortController();
+    this._watchedDocShellsAbortControllersMap.set(docShell, abortController);
 
     const webProgress = docShell
       .QueryInterface(Ci.nsIInterfaceRequestor)
@@ -1827,12 +1836,23 @@ class DebuggerProgressListener {
         Ci.nsIWebProgress.NOTIFY_STATE_DOCUMENT
     );
 
+    const { signal } = abortController;
     const handler = getDocShellChromeEventHandler(docShell);
-    handler.addEventListener("DOMWindowCreated", this._onWindowCreated, true);
-    handler.addEventListener("pageshow", this._onWindowCreated, true);
-    handler.addEventListener("pagehide", this._onWindowHidden, true);
+    handler.addEventListener("DOMWindowCreated", this._onWindowCreated, {
+      capture: true,
+      signal,
+    });
+    handler.addEventListener("pageshow", this._onWindowCreated, {
+      capture: true,
+      signal,
+    });
+    handler.addEventListener("pagehide", this._onWindowHidden, {
+      capture: true,
+      signal,
+    });
 
     // Dispatch the _windowReady event on the targetActor for pre-existing windows
+    const docShellWindow = docShell.domWindow;
     const windows = this._targetActor.ignoreSubFrames
       ? [docShellWindow]
       : this._getWindowsInDocShell(docShell);
@@ -1852,18 +1872,6 @@ class DebuggerProgressListener {
   }
 
   unwatch(docShell) {
-    // If the docshell is being destroyed, we won't be able to retrieve its related window object,
-    // which is the key ingredient for all cleanup operations done in this method.
-    if (docShell.isBeingDestroyed()) {
-      return;
-    }
-
-    const docShellWindow = docShell.domWindow;
-    if (!this._watchedDocShells.has(docShellWindow)) {
-      return;
-    }
-    this._watchedDocShells.delete(docShellWindow);
-
     const webProgress = docShell
       .QueryInterface(Ci.nsIInterfaceRequestor)
       .getInterface(Ci.nsIWebProgress);
@@ -1874,15 +1882,21 @@ class DebuggerProgressListener {
       // ignore
     }
 
-    const handler = getDocShellChromeEventHandler(docShell);
-    handler.removeEventListener(
-      "DOMWindowCreated",
-      this._onWindowCreated,
-      true
-    );
-    handler.removeEventListener("pageshow", this._onWindowCreated, true);
-    handler.removeEventListener("pagehide", this._onWindowHidden, true);
+    const abortController =
+      this._watchedDocShellsAbortControllersMap.get(docShell);
+    if (!abortController) {
+      return;
+    }
+    this._watchedDocShellsAbortControllersMap.delete(docShell);
+    abortController.abort();
 
+    // If the docshell is being destroyed, we won't be able to retrieve its related window object,
+    // which is the key ingredient for all cleanup operations done in this method.
+    if (docShell.isBeingDestroyed()) {
+      return;
+    }
+
+    const docShellWindow = docShell.domWindow;
     const windows = this._targetActor.ignoreSubFrames
       ? [docShellWindow]
       : this._getWindowsInDocShell(docShell);
@@ -1987,7 +2001,7 @@ class DebuggerProgressListener {
     // That's because we registered the listener on docShell.domWindow as
     // top level windows don't have a chromeEventHandler.
     if (
-      this._watchedDocShells.has(window) &&
+      this._watchedDocShellsAbortControllersMap.has(window?.docShell) &&
       // Avoid exception when the notified window is a cross origin object
       // (most likely an iframe running in a distinct origin)
       !Cu.isRemoteProxy(window) &&

devtools/server/actors/watcher/ParentProcessWatcherRegistry.sys.mjs

@@ -29,6 +29,12 @@ const { SessionDataHelpers } = ChromeUtils.importESModule(
   { global: "contextual" }
 );
 
+const lazy = {};
+
+ChromeUtils.defineESModuleGetters(lazy, {
+  setTimeout: "resource://gre/modules/Timer.sys.mjs",
+});
+
 const { SUPPORTED_DATA } = SessionDataHelpers;
 const SUPPORTED_DATA_TYPES = Object.values(SUPPORTED_DATA);
 
@@ -235,6 +241,11 @@ export const ParentProcessWatcherRegistry = {
   unregisterWatcher(watcherActorID) {
     sessionDataByWatcherActor.delete(watcherActorID);
     watcherActors.delete(watcherActorID);
+
+    // We should persist the data to sharedData as content processes may keep a cached
+    // version of sessionData for this watcher Actor ID.
+    persistMapToSharedData();
+
     this.maybeUnregisterJSActors();
   },
 
@@ -242,9 +253,11 @@ export const ParentProcessWatcherRegistry = {
    * Unregister the JS Actors if there is no more DevTools code observing any target/resource.
    */
   maybeUnregisterJSActors() {
-    if (sessionDataByWatcherActor.size == 0) {
+    if (shouldUnregisterJSProcessActor()) {
       unregisterBrowserToolboxJSProcessActor();
-      unregisterJSProcessActor();
+      // As there may be pending async RDP Request / Actor queries
+      // delay the actual unregistering a bit
+      lazy.setTimeout(unregisterJSProcessActor, 1000);
     }
   },
 
@@ -355,6 +368,10 @@ const BrowserToolboxJSProcessActorConfig = {
 const PROCESS_SCRIPT_URL =
   "resource://devtools/server/connectors/js-process-actor/content-process-jsprocessactor-startup.js";
 
+function shouldUnregisterJSProcessActor() {
+  return sessionDataByWatcherActor.size == 0;
+}
+
 function registerJSProcessActor() {
   if (isJSProcessActorRegistered) {
     return;
@@ -385,6 +402,10 @@ function registerBrowserToolboxJSProcessActor() {
 }
 
 function unregisterJSProcessActor() {
+  // As this is being throttled, re-check the unregistering condition before actually unregistering the actor
+  if (!shouldUnregisterJSProcessActor()) {
+    return;
+  }
   if (!isJSProcessActorRegistered) {
     return;
   }

devtools/server/connectors/js-process-actor/DevToolsProcessChild.sys.mjs

@@ -267,6 +267,7 @@ export class DevToolsProcessChild extends JSProcessActorChild {
     for (const targetType of [...watchingTargetTypes]) {
       this.#unwatchTargetsForWatcher(watcherDataObject, targetType);
     }
+    ContentProcessWatcherRegistry.remove(watcherDataObject);
   }
 
   /**

devtools/server/connectors/js-process-actor/DevToolsProcessParent.sys.mjs

@@ -155,6 +155,12 @@ export class DevToolsProcessParent extends JSProcessActorParent {
 
       this.#watchers.set(watcher.watcherConnectionPrefix, {
         watcher,
+
+        // Keep the connection prefix around, to do proper cleanup
+        // once the connection closes and the watcher or its connection is already destroyed
+        // (this prefix is shorter than the forwarding prefix)
+        connectionPrefix: watcher.conn.prefix,
+
         // This prefix is the prefix of the DevToolsServerConnection, running
         // in the content process, for which we should forward packets to, based on its prefix.
         // While `watcher.connection` is also a DevToolsServerConnection, but from this process,
@@ -206,7 +212,7 @@ export class DevToolsProcessParent extends JSProcessActorParent {
 
   #onConnectionClosed = (status, prefix) => {
     for (const watcherInfo of this.#watchers.values()) {
-      if (watcherInfo.watcher.conn?.prefix == prefix) {
+      if (watcherInfo.connectionPrefix == prefix) {
         this.#unregisterWatcher(watcherInfo.watcher);
       }
     }