diff --git a/devtools/server/actors/targets/window-global.js b/devtools/server/actors/targets/window-global.js index c1ef899a0c00..bee7fefb5784 100644 --- a/devtools/server/actors/targets/window-global.js +++ b/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) && diff --git a/devtools/server/actors/watcher/ParentProcessWatcherRegistry.sys.mjs b/devtools/server/actors/watcher/ParentProcessWatcherRegistry.sys.mjs index 2afe53829621..42144e04f6f1 100644 --- a/devtools/server/actors/watcher/ParentProcessWatcherRegistry.sys.mjs +++ b/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; } diff --git a/devtools/server/connectors/js-process-actor/DevToolsProcessChild.sys.mjs b/devtools/server/connectors/js-process-actor/DevToolsProcessChild.sys.mjs index 5885e958b1c7..de4678fbf034 100644 --- a/devtools/server/connectors/js-process-actor/DevToolsProcessChild.sys.mjs +++ b/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); } /** diff --git a/devtools/server/connectors/js-process-actor/DevToolsProcessParent.sys.mjs b/devtools/server/connectors/js-process-actor/DevToolsProcessParent.sys.mjs index add86631303c..3d982e102126 100644 --- a/devtools/server/connectors/js-process-actor/DevToolsProcessParent.sys.mjs +++ b/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); } }