Bug 1771088 - [devtools] Ensure instantiating only one Loader when debugging the system principal. r=jdescottes

We were instantiating one new Loader per DevToolsFrameChild instance.
This patch tries to share this loader with all the instances as well as share it
with the one used to load DevToolsServer when we are running in the parent process.

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

devtools/client/framework/browser-toolbox/Launcher.jsm

@@ -10,9 +10,11 @@ const BROWSER_TOOLBOX_WINDOW_URL =
   "chrome://devtools/content/framework/browser-toolbox/window.html";
 const CHROME_DEBUGGER_PROFILE_NAME = "chrome_debugger_profile";
 
-const { require, DevToolsLoader } = ChromeUtils.import(
-  "resource://devtools/shared/loader/Loader.jsm"
-);
+const {
+  require,
+  useDistinctSystemPrincipalLoader,
+  releaseDistinctSystemPrincipalLoader,
+} = ChromeUtils.import("resource://devtools/shared/loader/Loader.jsm");
 const { XPCOMUtils } = require("resource://gre/modules/XPCOMUtils.jsm");
 
 ChromeUtils.defineModuleGetter(
@@ -139,9 +141,7 @@ BrowserToolboxLauncher.prototype = {
     // This allows us to safely use the tools against even the actors and
     // DebuggingServer itself, especially since we can mark this loader as
     // invisible to the debugger (unlike the usual loader settings).
-    this.loader = new DevToolsLoader({
-      invisibleToDebugger: true,
-    });
+    this.loader = useDistinctSystemPrincipalLoader(this);
     const { DevToolsServer } = this.loader.require(
       "devtools/server/devtools-server"
     );
@@ -418,10 +418,10 @@ BrowserToolboxLauncher.prototype = {
       this.listener.close();
     }
 
-    if (this.devToolsServer) {
-      this.devToolsServer.destroy();
-      this.devToolsServer = null;
-    }
+    // Note that the DevToolsServer can be shared with the DevToolsServer
+    // spawned by DevToolsFrameChild. We shouldn't destroy it from here.
+    // Instead we should let it auto-destroy itself once the last connection is closed.
+    this.devToolsServer = null;
 
     this._dbgProcess.stdout.close();
     await this._dbgProcess.kill();
@@ -436,7 +436,7 @@ BrowserToolboxLauncher.prototype = {
 
     this._dbgProcess = null;
     if (this.loader) {
-      this.loader.destroy();
+      releaseDistinctSystemPrincipalLoader(this);
     }
     this.loader = null;
     this._telemetry = null;

devtools/server/connectors/js-window-actor/DevToolsFrameChild.jsm

@@ -313,12 +313,10 @@ class DevToolsFrameChild extends JSWindowActorChild {
   _createConnectionAndActor(forwardingPrefix, sessionData) {
     this.useCustomLoader = this.document.nodePrincipal.isSystemPrincipal;
 
-    // When debugging chrome pages, use a new dedicated loader, using a distinct chrome compartment.
     if (!this.loader) {
+      // When debugging chrome pages, use a new dedicated loader, using a distinct chrome compartment.
       this.loader = this.useCustomLoader
-        ? new Loader.DevToolsLoader({
-            invisibleToDebugger: true,
-          })
+        ? Loader.useDistinctSystemPrincipalLoader(this)
         : Loader;
     }
     const { DevToolsServer } = this.loader.require(
@@ -687,7 +685,7 @@ class DevToolsFrameChild extends JSWindowActorChild {
 
     if (this.loader) {
       if (this.useCustomLoader) {
-        this.loader.destroy();
+        Loader.releaseDistinctSystemPrincipalLoader(this);
       }
       this.loader = null;
     }

devtools/shared/loader/Loader.jsm

@@ -18,6 +18,8 @@ var { requireRawId } = ChromeUtils.import(
 
 const EXPORTED_SYMBOLS = [
   "DevToolsLoader",
+  "useDistinctSystemPrincipalLoader",
+  "releaseDistinctSystemPrincipalLoader",
   "require",
   "loader",
   // Export StructuredCloneHolder for its use from builtin-modules
@@ -26,6 +28,37 @@ const EXPORTED_SYMBOLS = [
 
 var gNextLoaderID = 0;
 
+// When debugging system principal resources (JSMs, chrome documents, ...)
+// We have to load DevTools actors in another system principal global.
+// That's mostly because of spidermonkey's Debugger API which requires
+// debuggee and debugger to be in distinct principals.
+//
+// We try to hold a single instance of this special loader via this API.
+//
+// @param requester object
+//        Object/instance which is using the loader.
+//        The same requester object should be passed to release method.
+var systemLoader = null;
+var systemLoaderRequesters = new Set();
+function useDistinctSystemPrincipalLoader(requester) {
+  if (!systemLoader) {
+    systemLoader = new DevToolsLoader({
+      invisibleToDebugger: true,
+    });
+    systemLoaderRequesters.clear();
+  }
+  systemLoaderRequesters.add(requester);
+  return systemLoader;
+}
+
+function releaseDistinctSystemPrincipalLoader(requester) {
+  systemLoaderRequesters.delete(requester);
+  if (systemLoaderRequesters.size == 0) {
+    systemLoader.destroy();
+    systemLoader = null;
+  }
+}
+
 /**
  * The main devtools API. The standard instance of this loader is exported as
  * |loader| below, but if a fresh copy of the loader is needed, then a new

devtools/shared/tests/xpcshell/test_loader.js

@@ -0,0 +1,53 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+const {
+  useDistinctSystemPrincipalLoader,
+  releaseDistinctSystemPrincipalLoader,
+} = ChromeUtils.import("resource://devtools/shared/loader/Loader.jsm");
+
+function run_test() {
+  const requester = {},
+    requester2 = {};
+  const loader = useDistinctSystemPrincipalLoader(requester);
+
+  info("Assert the key difference with the other regular loaders:");
+  ok(
+    loader.loader.invisibleToDebugger,
+    "The loader is set invisible to debugger"
+  );
+
+  ok(loader.loader, "Loader is not destroyed before calling release");
+
+  info("Now assert the precise behavior of release");
+  releaseDistinctSystemPrincipalLoader({});
+  ok(
+    loader.loader,
+    "Loader is still not destroyed after calling release with another requester"
+  );
+
+  releaseDistinctSystemPrincipalLoader(requester);
+  ok(
+    !loader.loader,
+    "Loader is destroyed after calling release with the right requester"
+  );
+
+  info("Now test the behavior with two concurrent usages");
+  const loader2 = useDistinctSystemPrincipalLoader(requester);
+  Assert.notEqual(loader, loader2, "We get a new loader instance");
+  ok(
+    loader2.loader.invisibleToDebugger,
+    "The loader is set invisible to debugger"
+  );
+
+  const loader3 = useDistinctSystemPrincipalLoader(requester2);
+  Assert.equal(loader2, loader3, "The two loader last loaders are shared");
+
+  releaseDistinctSystemPrincipalLoader(requester);
+  ok(loader2.loader, "Loader isn't destroy on the first call to destroy");
+
+  releaseDistinctSystemPrincipalLoader(requester2);
+  ok(!loader2.loader, "Loader is destroyed on the second call to destroy");
+}

devtools/shared/tests/xpcshell/xpcshell.ini

@@ -30,6 +30,7 @@ run-if = nightly_build
 [test_indentation.js]
 [test_independent_loaders.js]
 [test_invisible_loader.js]
+[test_loader.js]
 [test_isSet.js]
 [test_safeErrorString.js]
 [test_defineLazyPrototypeGetter.js]