Bug 1588741 - Descriptors.getTarget may return unattached targets when called in parallel. r=jdescottes

The race isn't trivial to reproduce and the test do not reproduce it.
But this was making target list tests to fail.

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

--HG--
extra : moz-landing-system : lando

devtools/client/framework/test/browser_target_parents.js

@@ -42,15 +42,41 @@ add_task(async function() {
 
   const { processes } = await mainRoot.listProcesses();
 
+  // Assert that concurrent calls to getTarget resolves the same target and that it is already attached
+  // We that, we were chasing a precise race, where a second call to ProcessDescriptor.getTarget()
+  // happens between the instantiation of ContentProcessTarget and its call to attach() from getTarget
+  // function.
   await Promise.all(
     processes.map(async processDescriptor => {
-      const process = await processDescriptor.getTarget();
-
-      is(
-        process.descriptorFront,
-        processDescriptor,
-        "Got the correct descriptorFront from the process target."
-      );
+      const promises = [];
+      const concurrentCalls = 10;
+      for (let i = 0; i < concurrentCalls; i++) {
+        const targetPromise = processDescriptor.getTarget();
+        // Every odd runs, wait for a tick to introduce some more randomness
+        if (i % 2 == 0) {
+          await wait(0);
+        }
+        promises.push(
+          targetPromise.then(target => {
+            is(
+              target.descriptorFront,
+              processDescriptor,
+              "Got the correct descriptorFront from the process target."
+            );
+            // activeConsole is one of the only attribute to assess that a Content Process Target is attached
+            ok(target.activeConsole, "The target is attached");
+            return target;
+          })
+        );
+      }
+      const targets = await Promise.all(promises);
+      for (let i = 1; i < concurrentCalls; i++) {
+        is(
+          targets[0],
+          targets[i],
+          "All the targets returned by concurrent calls to getTarget are the same"
+        );
+      }
     })
   );
 
@@ -73,15 +99,39 @@ add_task(async function() {
 
   const mainProcess = await mainRoot.getMainProcess();
   const { frames } = await mainProcess.listRemoteFrames();
-  await Promise.all(
-    frames.map(async frameDescriptorFront => {
-      const frameTargetFront = await frameDescriptorFront.getTarget();
 
-      is(
-        frameTargetFront.descriptorFront,
-        frameDescriptorFront,
-        "Got the correct descriptorFront from the frame target."
-      );
+  // Assert that concurrent calls to getTarget resolves the same target and that it is already attached
+  await Promise.all(
+    frames.map(async frameDescriptor => {
+      const promises = [];
+      const concurrentCalls = 10;
+      for (let i = 0; i < concurrentCalls; i++) {
+        const targetPromise = frameDescriptor.getTarget();
+        // Every odd runs, wait for a tick to introduce some more randomness
+        if (i % 2 == 0) {
+          await wait(0);
+        }
+        promises.push(
+          targetPromise.then(target => {
+            is(
+              target.descriptorFront,
+              frameDescriptor,
+              "Got the correct descriptorFront from the frame target."
+            );
+            // traits is one attribute to assert that a Frame Target is attached
+            ok(target.traits, "The target is attached");
+            return target;
+          })
+        );
+      }
+      const targets = await Promise.all(promises);
+      for (let i = 1; i < concurrentCalls; i++) {
+        is(
+          targets[0],
+          targets[i],
+          "All the targets returned by concurrent calls to getTarget are the same"
+        );
+      }
     })
   );
 

devtools/shared/fronts/descriptors/frame.js

@@ -43,33 +43,42 @@ class FrameDescriptorFront extends FrontClassWithSpec(frameDescriptorSpec) {
   }
 
   async getTarget() {
+    // Only return the cached Target if it is still alive.
+    // It may have been destroyed in case of navigation trigerring a load in another
+    // process.
     if (this._frameTargetFront && this._frameTargetFront.actorID) {
       return this._frameTargetFront;
     }
+    // Otherwise, ensure that we don't try to spawn more than one Target by
+    // returning the pending promise
     if (this._targetFrontPromise) {
       return this._targetFrontPromise;
     }
     this._targetFrontPromise = (async () => {
+      let target = null;
       try {
         const targetForm = await super.getTarget();
+        // getTarget uses 'json' in the specification type and this prevents converting
+        // actor exception into front exceptions
         if (targetForm.error) {
-          this._targetFrontPromise = null;
-          return null;
+          throw new Error(targetForm.error);
         }
-        this._frameTargetFront = await this._createFrameTarget(targetForm);
-        await this._frameTargetFront.attach();
-        // clear the promise if we are finished so that we can re-connect if
-        // necessary
-        this._targetFrontPromise = null;
-        return this._frameTargetFront;
+        target = await this._createFrameTarget(targetForm);
+        await target.attach();
       } catch (e) {
         // This is likely to happen if we get a lot of events which drop previous
         // frames.
         console.log(
           `Request to connect to frameDescriptor "${this.id}" failed: ${e}`
         );
-        return null;
       }
+      // Save the reference to the target only after the call to attach
+      // so that getTarget always returns the attached target in case of concurrent calls
+      this._frameTargetFront = target;
+      // clear the promise if we are finished so that we can re-connect if
+      // necessary
+      this._targetFrontPromise = null;
+      return target;
     })();
     return this._targetFrontPromise;
   }

devtools/shared/fronts/descriptors/process.js

@@ -55,31 +55,35 @@ class ProcessDescriptorFront extends FrontClassWithSpec(processDescriptorSpec) {
   }
 
   async getTarget() {
+    // Only return the cached Target if it is still alive.
     if (this._processTargetFront && this._processTargetFront.actorID) {
       return this._processTargetFront;
     }
+    // Otherwise, ensure that we don't try to spawn more than one Target by
+    // returning the pending promise
     if (this._targetFrontPromise) {
       return this._targetFrontPromise;
     }
     this._targetFrontPromise = (async () => {
+      let targetFront = null;
       try {
         const targetForm = await super.getTarget();
-        this._processTargetFront = await this._createProcessTargetFront(
-          targetForm
-        );
-        await this._processTargetFront.attach();
-        // clear the promise if we are finished so that we can re-connect if
-        // necessary
-        this._targetFrontPromise = null;
-        return this._processTargetFront;
+        targetFront = await this._createProcessTargetFront(targetForm);
+        await targetFront.attach();
       } catch (e) {
         // This is likely to happen if we get a lot of events which drop previous
         // processes.
         console.log(
           `Request to connect to ProcessDescriptor "${this.id}" failed: ${e}`
         );
-        return null;
       }
+      // Save the reference to the target only after the call to attach
+      // so that getTarget always returns the attached target in case of concurrent calls
+      this._processTargetFront = targetFront;
+      // clear the promise if we are finished so that we can re-connect if
+      // necessary
+      this._targetFrontPromise = null;
+      return targetFront;
     })();
     return this._targetFrontPromise;
   }