From 312d5c2fa027ea84fe94a66be2a48fd44ee4d08b Mon Sep 17 00:00:00 2001 From: Nicolas Chevobbe Date: Tue, 18 May 2021 14:16:34 +0000 Subject: [PATCH] Bug 1704458 - [devtools] Update targetFront url and title on DOCUMENT_EVENT resource. r=ochameau,jdescottes. Differential Revision: https://phabricator.services.mozilla.com/D114601 --- .../client/fronts/targets/browsing-context.js | 34 ++++- .../server/actors/resources/document-event.js | 7 ++ .../browser_resources_document_events.js | 119 ++++++++++++++++-- .../shared/commands/target/target-command.js | 45 +++++++ .../tests/browser_target_list_bfcache.js | 22 ++++ 5 files changed, 216 insertions(+), 11 deletions(-) diff --git a/devtools/client/fronts/targets/browsing-context.js b/devtools/client/fronts/targets/browsing-context.js index 9f42c8d2764b..ecaad6e0dd8d 100644 --- a/devtools/client/fronts/targets/browsing-context.js +++ b/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 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; diff --git a/devtools/server/actors/resources/document-event.js b/devtools/server/actors/resources/document-event.js index 5acdfc9c87f2..8942d0e55de2 100644 --- a/devtools/server/actors/resources/document-event.js +++ b/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, }, ]); }; diff --git a/devtools/shared/commands/resource/tests/browser_resources_document_events.js b/devtools/shared/commands/resource/tests/browser_resources_document_events.js index c339e3c2679c..bec50f986184 100644 --- a/devtools/shared/commands/resource/tests/browser_resources_document_events.js +++ b/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}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=titleNetnet"; 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(); } diff --git a/devtools/shared/commands/target/target-command.js b/devtools/shared/commands/target/target-command.js index 7231b97e2fda..d3686c73adf6 100644 --- a/devtools/shared/commands/target/target-command.js +++ b/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 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; } } diff --git a/devtools/shared/commands/target/tests/browser_target_list_bfcache.js b/devtools/shared/commands/target/tests/browser_target_list_bfcache.js index a9ed48c09302..e4e3ffb10d37 100644 --- a/devtools/shared/commands/target/tests/browser_target_list_bfcache.js +++ b/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;