Bug 1704458 - [devtools] Update targetFront url and title on DOCUMENT_EVENT resource. r=ochameau,jdescottes.

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

devtools/client/fronts/targets/browsing-context.js

@@ -42,8 +42,16 @@ class BrowsingContextTargetFront extends TargetMixin(
 
     this.outerWindowID = json.outerWindowID;
     this.favicon = json.favicon;
-    this._title = json.title;
-    this._url = json.url;
+
+    // Initial value for the page title and url. Since the BrowsingContextTargetActor can
+    // be created very early, those might not represent the actual value we'd want to
+    // display for the user (e.g. the <title> might not have been parsed yet, and the
+    // url could still be about:blank, which is what the platform uses at the very start
+    // of a navigation to a new location).
+    // Those values are set again  from the targetCommand when receiving DOCUMENT_EVENT
+    // resource, at which point both values should be in their expected form.
+    this.setTitle(json.title);
+    this.setUrl(json.url);
   }
 
   /**
@@ -68,8 +76,8 @@ class BrowsingContextTargetFront extends TargetMixin(
     // is a WebExtension (where the addon name is always included in the title
     // and the url is supposed to be updated every time the selected frame changes).
     if (!packet.isFrameSwitching || this.isWebExtension) {
-      this._url = packet.url;
-      this._title = packet.title;
+      this.setTitle(packet.title);
+      this.setUrl(packet.url);
     }
 
     // Send any stored event payload (DOMWindow or nsIRequest) for backwards
@@ -81,6 +89,24 @@ class BrowsingContextTargetFront extends TargetMixin(
     }
   }
 
+  /**
+   * Set the targetFront url.
+   *
+   * @param {string} url
+   */
+  setUrl(url) {
+    this._url = url;
+  }
+
+  /**
+   * Set the targetFront title.
+   *
+   * @param {string} title
+   */
+  setTitle(title) {
+    this._title = title;
+  }
+
   async attach() {
     if (this._attach) {
       return this._attach;

devtools/server/actors/resources/document-event.js

@@ -45,6 +45,13 @@ class DocumentEventWatcher {
           time,
           shouldBeIgnoredAsRedundantWithTargetAvailable,
           isFrameSwitching,
+          // only send `title` on dom interactive (once the HTML was parsed) so we don't
+          // make the payload bigger for events where we either don't have a title yet,
+          // or where we already had a chance to get the title.
+          title: name === "dom-interactive" ? targetActor.title : undefined,
+          // only send `url` on dom loading so we don't make the payload bigger for
+          // other events
+          url: name === "dom-loading" ? targetActor.url : undefined,
         },
       ]);
     };

devtools/shared/commands/resource/tests/browser_resources_document_events.js

@@ -19,7 +19,9 @@ async function testDocumentEventResources() {
   info("Test ResourceCommand for DOCUMENT_EVENT");
 
   // Open a test tab
-  const tab = await addTab("data:text/html,Document Events");
+  const title = "DocumentEventsTitle";
+  const url = `data:text/html,<title>${title}</title>Document Events`;
+  const tab = await addTab(url);
 
   const listener = new ResourceListener();
   const { client, resourceCommand, targetCommand } = await initResourceCommand(
@@ -47,6 +49,40 @@ async function testDocumentEventResources() {
     "shouldBeIgnoredAsRedundantWithTargetAvailable is true for already loaded page"
   );
 
+  is(
+    domLoadingResource.url,
+    url,
+    `resource ${domLoadingResource.name} has expected url`
+  );
+  is(
+    domLoadingResource.title,
+    undefined,
+    `resource ${domLoadingResource.name} does not have a title property`
+  );
+
+  let domInteractiveResource = await onInteractiveAtInit;
+  is(
+    domInteractiveResource.url,
+    undefined,
+    `resource ${domInteractiveResource.name} does not have a url property`
+  );
+  is(
+    domInteractiveResource.title,
+    title,
+    `resource ${domInteractiveResource.name} has expected title`
+  );
+  let domCompleteResource = await onCompleteAtInit;
+  is(
+    domCompleteResource.url,
+    undefined,
+    `resource ${domCompleteResource.name} does not have a url property`
+  );
+  is(
+    domCompleteResource.title,
+    undefined,
+    `resource ${domCompleteResource.name} does not have a title property`
+  );
+
   info("Check whether the document events are fired correctly when reloading");
   const onLoadingAtReloaded = listener.once("dom-loading");
   const onInteractiveAtReloaded = listener.once("dom-interactive");
@@ -66,6 +102,40 @@ async function testDocumentEventResources() {
     "shouldBeIgnoredAsRedundantWithTargetAvailable is not set after reloading"
   );
 
+  is(
+    domLoadingResource.url,
+    url,
+    `resource ${domLoadingResource.name} has expected url after reloading`
+  );
+  is(
+    domLoadingResource.title,
+    undefined,
+    `resource ${domLoadingResource.name} does not have a title property after reloading`
+  );
+
+  domInteractiveResource = await onInteractiveAtInit;
+  is(
+    domInteractiveResource.url,
+    undefined,
+    `resource ${domInteractiveResource.name} does not have a url property after reloading`
+  );
+  is(
+    domInteractiveResource.title,
+    title,
+    `resource ${domInteractiveResource.name} has expected title after reloading`
+  );
+  domCompleteResource = await onCompleteAtInit;
+  is(
+    domCompleteResource.url,
+    undefined,
+    `resource ${domCompleteResource.name} does not have a url property after reloading`
+  );
+  is(
+    domCompleteResource.title,
+    undefined,
+    `resource ${domCompleteResource.name} does not have a title property after reloading`
+  );
+
   targetCommand.destroy();
   await client.close();
 }
@@ -111,15 +181,16 @@ async function testCrossOriginNavigation() {
     onAvailable: resources => documentEvents.push(...resources),
     ignoreExistingResources: true,
   });
+  // Wait for some time for extra safety
+  await wait(1000);
   is(documentEvents.length, 0, "Existing document events are not fired");
 
   info("Navigate to another process");
   const onSwitched = targetCommand.once("switched-target");
+  const netUrl =
+    "http://example.net/document-builder.sjs?html=<head><title>titleNet</title></head>net";
   const onLoaded = BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
-  BrowserTestUtils.loadURI(
-    gBrowser.selectedBrowser,
-    "http://example.net/document-builder.sjs?html=net"
-  );
+  BrowserTestUtils.loadURI(gBrowser.selectedBrowser, netUrl);
   await onLoaded;
 
   // We are switching to a new target only when fission is enabled...
@@ -139,6 +210,7 @@ async function testCrossOriginNavigation() {
     3,
     "There is no duplicated event and only the 3 expected DOCUMENT_EVENT states"
   );
+  const [loadingEvent, interactiveEvent, completeEvent] = documentEvents;
 
   // followWindowGlobalLifeCycle will be true when enabling server side target switching,
   // even when fission is off.
@@ -147,18 +219,51 @@ async function testCrossOriginNavigation() {
     targetCommand.targetFront.targetForm.followWindowGlobalLifeCycle
   ) {
     is(
-      documentEvents[0].shouldBeIgnoredAsRedundantWithTargetAvailable,
+      loadingEvent.shouldBeIgnoredAsRedundantWithTargetAvailable,
       true,
       "shouldBeIgnoredAsRedundantWithTargetAvailable is true for the new target which follows the WindowGlobal lifecycle"
     );
   } else {
     is(
-      documentEvents[0].shouldBeIgnoredAsRedundantWithTargetAvailable,
+      loadingEvent.shouldBeIgnoredAsRedundantWithTargetAvailable,
       undefined,
       "shouldBeIgnoredAsRedundantWithTargetAvailable is undefined if fission is disabled and we keep the same target"
     );
   }
 
+  is(
+    loadingEvent.url,
+    encodeURI(netUrl),
+    `resource ${loadingEvent.name} has expected url after reloading`
+  );
+  is(
+    loadingEvent.title,
+    undefined,
+    `resource ${loadingEvent.name} does not have a title property after reloading`
+  );
+
+  is(
+    interactiveEvent.url,
+    undefined,
+    `resource ${interactiveEvent.name} does not have a url property after reloading`
+  );
+  is(
+    interactiveEvent.title,
+    "titleNet",
+    `resource ${interactiveEvent.name} has expected title after reloading`
+  );
+
+  is(
+    completeEvent.url,
+    undefined,
+    `resource ${completeEvent.name} does not have a url property after reloading`
+  );
+  is(
+    completeEvent.title,
+    undefined,
+    `resource ${completeEvent.name} does not have a title property after reloading`
+  );
+
   targetCommand.destroy();
   await client.close();
 }

devtools/shared/commands/target/target-command.js

@@ -125,6 +125,8 @@ class TargetCommand extends EventEmitter {
     //                but we wait for the watcher actor to notify us about it
     //                via target-available-form avent.
     this._gotFirstTopLevelTarget = false;
+    this.commands = commands;
+    this._onResourceAvailable = this._onResourceAvailable.bind(this);
   }
 
   // Called whenever a new Target front is available.
@@ -398,6 +400,21 @@ class TargetCommand extends EventEmitter {
       }
     }
 
+    if (!this._watchingDocumentEvent && !this.isDestroyed()) {
+      // We want to watch DOCUMENT_EVENT in order to update the url and title of target fronts,
+      // as the initial value that is set in them might be erroneous (if the target was
+      // created so early that the document url is still pointing to about:blank and the
+      // html hasn't be parsed yet, so we can't know the <title> content).
+
+      this._watchingDocumentEvent = true;
+      await this.commands.resourceCommand.watchResources(
+        [this.commands.resourceCommand.TYPES.DOCUMENT_EVENT],
+        {
+          onAvailable: this._onResourceAvailable,
+        }
+      );
+    }
+
     if (this.isServerTargetSwitchingEnabled()) {
       await this._onFirstTarget;
     }
@@ -456,6 +473,16 @@ class TargetCommand extends EventEmitter {
    *        but still unregister listeners set via Legacy Listeners.
    */
   stopListening({ onlyLegacy = false } = {}) {
+    if (this._watchingDocumentEvent) {
+      this.commands.resourceCommand.unwatchResources(
+        [this.commands.resourceCommand.TYPES.DOCUMENT_EVENT],
+        {
+          onAvailable: this._onResourceAvailable,
+        }
+      );
+      this._watchingDocumentEvent = false;
+    }
+
     for (const type of TargetCommand.ALL_TYPES) {
       if (!this._isListening(type)) {
         continue;
@@ -511,6 +538,23 @@ class TargetCommand extends EventEmitter {
     return type === target.targetType;
   }
 
+  _onResourceAvailable(resources) {
+    for (const resource of resources) {
+      if (
+        resource.resourceType ===
+        this.commands.resourceCommand.TYPES.DOCUMENT_EVENT
+      ) {
+        const { targetFront } = resource;
+        if (resource.title !== undefined && targetFront?.setTitle) {
+          targetFront.setTitle(resource.title);
+        }
+        if (resource.url !== undefined && targetFront?.setUrl) {
+          targetFront.setUrl(resource.url);
+        }
+      }
+    }
+  }
+
   /**
    * Listen for the creation and/or destruction of target fronts matching one of the provided types.
    *
@@ -773,6 +817,7 @@ class TargetCommand extends EventEmitter {
     this.stopListening();
     this._createListeners.off();
     this._destroyListeners.off();
+
     this._isDestroyed = true;
   }
 }

devtools/shared/commands/target/tests/browser_target_list_bfcache.js

@@ -147,7 +147,28 @@ async function testTopLevelNavigations(bfcacheInParent) {
   // Go forward and resurect the second page, this should also be a bfcache navigation, and,
   // get a new target.
   info("Go forward to the second page");
+
   onNavigate = bfcacheInParent ? null : targets[0].once("navigate");
+
+  // When a new target will be created, we need to wait until it's fully processed
+  // to avoid pending promises.
+  const onNewTargetProcessed = bfcacheInParent
+    ? new Promise(resolve => {
+        targetCommand.on(
+          "processed-available-target",
+          function onProcessedAvailableTarget(targetFront) {
+            if (targetFront === targets[3]) {
+              resolve();
+              targetCommand.off(
+                "processed-available-target",
+                onProcessedAvailableTarget
+              );
+            }
+          }
+        );
+      })
+    : null;
+
   gBrowser.selectedBrowser.goForward();
 
   if (bfcacheInParent) {
@@ -168,6 +189,7 @@ async function testTopLevelNavigations(bfcacheInParent) {
     // when navigating to another page and switching to new process and target.
     await targets[3].attachAndInitThread(targetCommand);
     await waitForAllTargetsToBeAttached(targetCommand);
+    await onNewTargetProcessed;
   } else {
     info("Wait for 'navigate'");
     await onNavigate;

devtools/shared/commands/target/tests/browser_target_list_frames.js

@@ -209,6 +209,12 @@ async function testTabFrames(mainRoot) {
   });
 
   info("Navigate to another domain and process (if fission is enabled)");
+  // When a new target will be created, we need to wait until it's fully processed
+  // to avoid pending promises.
+  const onNewTargetProcessed = isFissionEnabled()
+    ? targetCommand.once("processed-available-target")
+    : null;
+
   const browser = tab.linkedBrowser;
   const onLoaded = BrowserTestUtils.browserLoaded(browser);
   await BrowserTestUtils.loadURI(browser, SECOND_PAGE_URL);
@@ -280,6 +286,8 @@ async function testTabFrames(mainRoot) {
     );
   }
 
+  await onNewTargetProcessed;
+
   targetCommand.unwatchTargets([TYPES.FRAME], onAvailable);
 
   targetCommand.destroy();