Bug 1699497 - [devtools] Fix races when connecting to the same browser element twice in parallel. r=jdescottes

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

devtools/client/framework/test/browser.ini

@@ -81,6 +81,7 @@ prefs =
 [browser_source_map-reload.js]
 skip-if = fission # Disable frequent fission intermittents Bug 1675020
 [browser_source_map-late-script.js]
+[browser_tab_descriptor_factory.js]
 [browser_tab_descriptor_fission.js]
 [browser_target_from_url.js]
 [browser_target_cached-front.js]

devtools/client/framework/test/browser_tab_descriptor_factory.js

@@ -0,0 +1,91 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+// Test TabDescriptorFactory
+
+add_task(async function() {
+  await testTabDescriptorWithURL("data:text/html;charset=utf-8,foo");
+
+  // Bug 1699497: Also test against a page in the parent process
+  // which can hit some race with frame-connector's frame scripts.
+  await testTabDescriptorWithURL("about:robots");
+});
+
+async function testTabDescriptorWithURL(url) {
+  info(`Test TabDescriptor against url ${url}\n`);
+  const tab = await addTab(url);
+
+  const descriptorPromises = [];
+  const sharedDescriptor = await TabDescriptorFactory.createDescriptorForTab(
+    tab
+  );
+  descriptorPromises.push(sharedDescriptor);
+  is(
+    sharedDescriptor.localTab,
+    tab,
+    "TabDescriptor's localTab is set correctly"
+  );
+
+  info(
+    "Calling a second time createDescriptorForTab with the same tab, will return the same descriptor"
+  );
+  const secondDescriptor = await TabDescriptorFactory.createDescriptorForTab(
+    tab
+  );
+  is(sharedDescriptor, secondDescriptor, "second descriptor is the same");
+
+  info(
+    "forceCreationForWebextension allows to spawn new descriptor for the same tab"
+  );
+  const webExtDescriptor = await TabDescriptorFactory.createDescriptorForTab(
+    tab,
+    { forceCreationForWebextension: true }
+  );
+  isnot(
+    sharedDescriptor,
+    webExtDescriptor,
+    "web extension descriptor is a new one"
+  );
+  is(
+    webExtDescriptor.localTab,
+    tab,
+    "web ext descriptor still refers to the same tab"
+  );
+  descriptorPromises.push(webExtDescriptor);
+
+  info("Instantiate many descriptor in parallel");
+  for (let i = 0; i < 10; i++) {
+    const descriptor = TabDescriptorFactory.createDescriptorForTab(tab, {
+      forceCreationForWebextension: true,
+    });
+    descriptorPromises.push(descriptor);
+  }
+
+  info("Wait for all descriptor to be resolved");
+  const descriptors = await Promise.all(descriptorPromises);
+
+  info("Wait for all targets to be created");
+  const targets = await Promise.all(
+    descriptors.map(async descriptor => {
+      return descriptor.getTarget();
+    })
+  );
+
+  info("Call any method to ensure that each target works");
+  await Promise.all(
+    targets.map(async target => {
+      await target.logInPage("foo");
+    })
+  );
+
+  info("Destroy all the descriptors");
+  await Promise.all(
+    descriptors.map(async descriptor => {
+      await descriptor.destroy();
+    })
+  );
+
+  gBrowser.removeCurrentTab();
+}

devtools/server/devtools-server.js

@@ -145,6 +145,9 @@ var DevToolsServer = {
     return this._connections && Object.keys(this._connections).length > 0;
   },
 
+  hasConnectionForPrefix(prefix) {
+    return this._connections && !!this._connections[prefix + "/"];
+  },
   /**
    * Performs cleanup tasks before shutting down the devtools server. Such tasks
    * include clearing any actor constructors added at runtime. This method

devtools/server/startup/frame.js

@@ -51,12 +51,21 @@ try {
     const connections = new Map();
 
     const onConnect = DevToolsUtils.makeInfallible(function(msg) {
-      removeMessageListener("debug:connect", onConnect);
-
       const mm = msg.target;
       const prefix = msg.data.prefix;
       const addonId = msg.data.addonId;
 
+      // If we try to create several frame targets simultaneously, the frame script will be loaded several times.
+      // In this case a single "debug:connect" message might be received by all the already loaded frame scripts.
+      // Check if the DevToolsServer already knows the provided connection prefix,
+      // because it means that another framescript instance already handled this message.
+      // Another "debug:connect" message is guaranteed to be emitted for another prefix,
+      // so we keep the message listener and wait for this next message.
+      if (DevToolsServer.hasConnectionForPrefix(prefix)) {
+        return;
+      }
+      removeMessageListener("debug:connect", onConnect);
+
       const conn = DevToolsServer.connectToParent(prefix, mm);
       conn.parentMessageManager = mm;
       connections.set(prefix, conn);