From c0aa119d7d5a5d24ba7f5f435b313e62f259b6de Mon Sep 17 00:00:00 2001 From: Dorel Luca Date: Sat, 9 May 2020 20:09:59 +0300 Subject: [PATCH] Backed out 3 changesets (bug 1635467, bug 1625961) for Devtools failures in resources/tests/browser_resources_exceptions.js. CLOSED TREE Backed out changeset 1fd6a3754afb (bug 1635467) Backed out changeset 977b0031b41c (bug 1625961) Backed out changeset 18646e7b984f (bug 1625961) --- devtools/client/inspector/inspector.js | 17 +-- ...rowser_inspector_walker_watch_root_node.js | 42 ++++++ .../resources/legacy-listeners/moz.build | 1 - .../resources/legacy-listeners/root-node.js | 20 --- devtools/shared/resources/resource-watcher.js | 41 +----- devtools/shared/resources/tests/browser.ini | 3 - .../tests/browser_resources_exceptions.js | 52 -------- .../tests/browser_resources_root_node.js | 124 ------------------ .../browser_resources_several_resources.js | 91 ------------- 9 files changed, 45 insertions(+), 346 deletions(-) delete mode 100644 devtools/shared/resources/legacy-listeners/root-node.js delete mode 100644 devtools/shared/resources/tests/browser_resources_exceptions.js delete mode 100644 devtools/shared/resources/tests/browser_resources_root_node.js delete mode 100644 devtools/shared/resources/tests/browser_resources_several_resources.js diff --git a/devtools/client/inspector/inspector.js b/devtools/client/inspector/inspector.js index b0c4016c7e93..5c8ab757dee7 100644 --- a/devtools/client/inspector/inspector.js +++ b/devtools/client/inspector/inspector.js @@ -166,7 +166,6 @@ function Inspector(toolbox) { this.onHostChanged = this.onHostChanged.bind(this); this.onMarkupLoaded = this.onMarkupLoaded.bind(this); this.onNewSelection = this.onNewSelection.bind(this); - this.onResourceAvailable = this.onResourceAvailable.bind(this); this.onRootNodeAvailable = this.onRootNodeAvailable.bind(this); this.onPanelWindowResize = this.onPanelWindowResize.bind(this); this.onShowBoxModelHighlighterForNode = this.onShowBoxModelHighlighterForNode.bind( @@ -201,10 +200,6 @@ Inspector.prototype = { this._onTargetDestroyed ); - await this.toolbox.resourceWatcher.watch( - [this.toolbox.resourceWatcher.TYPES.ROOT_NODE], - this.onResourceAvailable - ); // Store the URL of the target page prior to navigation in order to ensure // telemetry counts in the Grid Inspector are not double counted on reload. this.previousURL = this.currentTarget.url; @@ -227,6 +222,8 @@ Inspector.prototype = { this._getCssProperties(), this._getAccessibilityFront(), ]); + + this.walker.watchRootNode(this.onRootNodeAvailable); }, _onTargetDestroyed({ type, targetFront, isTopLevel }) { @@ -1282,16 +1279,6 @@ Inspector.prototype = { } }, - onResourceAvailable: function({ resourceType, targetFront, resource }) { - if (resourceType === this.toolbox.resourceWatcher.TYPES.ROOT_NODE) { - // Note: the resource (ie the root node here) will be fetched from the - // walker later on in _getDefaultNodeForSelection. - // We should update the inspector to directly use the node front - // provided here. Bug 1635461. - this.onRootNodeAvailable(); - } - }, - /** * Reset the inspector on new root mutation. */ diff --git a/devtools/server/tests/browser/browser_inspector_walker_watch_root_node.js b/devtools/server/tests/browser/browser_inspector_walker_watch_root_node.js index b5b3f84feae8..2d6356200642 100644 --- a/devtools/server/tests/browser/browser_inspector_walker_watch_root_node.js +++ b/devtools/server/tests/browser/browser_inspector_walker_watch_root_node.js @@ -121,3 +121,45 @@ add_task(async function testCallingWatchSuccessivelyWithoutReload() { // cleanup walker.unwatchRootNode(onAvailable); }); + +/** + * Test that the watchRootNode API provides the expected node fronts. + */ +add_task(async function testRootNodeFrontIsCorrect() { + const { walker } = await initInspectorFront(`data:text/html,
`); + const browser = gBrowser.selectedBrowser; + + info("Call watchRootNode"); + let rootNodeResolve; + let rootNodePromise = new Promise(r => (rootNodeResolve = r)); + const onAvailable = rootNodeFront => rootNodeResolve(rootNodeFront); + await walker.watchRootNode(onAvailable); + + info("Wait until onAvailable has been called"); + const root1 = await rootNodePromise; + ok(!!root1, "onAvailable has been called with a valid argument"); + + info("Check we can query an expected node under the retrieved root"); + const div1 = await walker.querySelector(root1, "div"); + is(div1.getAttribute("id"), "div1", "Correct root node retrieved"); + + info("Reload the selected browser"); + rootNodePromise = new Promise(r => (rootNodeResolve = r)); + browser.reload(); + + const root2 = await rootNodePromise; + ok( + root1 !== root2, + "onAvailable has been called with a different node front after reload" + ); + + info("Navigate to another URL"); + rootNodePromise = new Promise(r => (rootNodeResolve = r)); + BrowserTestUtils.loadURI(browser, `data:text/html,
`); + const root3 = await rootNodePromise; + info("Check we can query an expected node under the retrieved root"); + const div2 = await walker.querySelector(root3, "div"); + is(div2.getAttribute("id"), "div2", "Correct root node retrieved"); + + walker.unwatchRootNode(onAvailable); +}); diff --git a/devtools/shared/resources/legacy-listeners/moz.build b/devtools/shared/resources/legacy-listeners/moz.build index 73e6fd16df6a..3a083a183183 100644 --- a/devtools/shared/resources/legacy-listeners/moz.build +++ b/devtools/shared/resources/legacy-listeners/moz.build @@ -6,5 +6,4 @@ DevToolsModules( 'console-messages.js', 'error-messages.js', 'platform-messages.js', - 'root-node.js', ) diff --git a/devtools/shared/resources/legacy-listeners/root-node.js b/devtools/shared/resources/legacy-listeners/root-node.js deleted file mode 100644 index 6b048a8915b4..000000000000 --- a/devtools/shared/resources/legacy-listeners/root-node.js +++ /dev/null @@ -1,20 +0,0 @@ -/* This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ - -"use strict"; - -module.exports = async function({ - targetList, - targetType, - targetFront, - isTopLevel, - onAvailable, -}) { - if (!isTopLevel) { - return; - } - - const inspectorFront = await targetFront.getFront("inspector"); - await inspectorFront.walker.watchRootNode(onAvailable); -}; diff --git a/devtools/shared/resources/resource-watcher.js b/devtools/shared/resources/resource-watcher.js index b8785459a968..e45c96796284 100644 --- a/devtools/shared/resources/resource-watcher.js +++ b/devtools/shared/resources/resource-watcher.js @@ -33,11 +33,6 @@ class ResourceWatcher { this._destroyedListeners = new EventEmitter(); this._listenerCount = new Map(); - - // This set is only used to know which resources have been watched and then - // unwatched, since the ResourceWatcher doesn't support calling - // watch, unwatch and watch again. - this._previouslyListenedTypes = new Set(); } get contentToolboxFissionPrefValue() { @@ -226,42 +221,12 @@ class ResourceWatcher { * to be listened. */ async _startListening(resourceType) { - const isDocumentEvent = - resourceType === ResourceWatcher.TYPES.DOCUMENT_EVENTS; - let listeners = this._listenerCount.get(resourceType) || 0; listeners++; this._listenerCount.set(resourceType, listeners); - if (listeners > 1) { - // If there are several calls to watch, only the first caller receives - // "existing" resources. Throw to avoid inconsistent behaviors - if (isDocumentEvent) { - // For DOCUMENT_EVENTS, return without throwing because this is already - // used by several callsites in the netmonitor. - // This should be reviewed in Bug 1625909. - return; - } - - throw new Error( - `The ResourceWatcher is already listening to "${resourceType}", ` + - "the client should call `watch` only once per resource type." - ); + return; } - - const wasListening = this._previouslyListenedTypes.has(resourceType); - if (wasListening && !isDocumentEvent) { - // We already called watch/unwatch for this resource. - // This can lead to the onAvailable callback being called twice because we - // don't perform any cleanup in _unwatchResourcesForTarget. - throw new Error( - `The ResourceWatcher previously watched "${resourceType}" ` + - "and doesn't support watching again on a previous resource." - ); - } - - this._previouslyListenedTypes.add(resourceType); - // If this is the first listener for this type of resource, // we should go through all the existing targets as onTargetAvailable // has already been called for these existing targets. @@ -318,7 +283,6 @@ class ResourceWatcher { if (listeners > 0) { return; } - // If this was the last listener, we should stop watching these events from the actors // and the actors should stop watching things from the platform for (const targetType of this.targetList.ALL_TYPES) { @@ -351,7 +315,6 @@ ResourceWatcher.TYPES = ResourceWatcher.prototype.TYPES = { ERROR_MESSAGES: "error-messages", PLATFORM_MESSAGES: "platform-messages", DOCUMENT_EVENTS: "document-events", - ROOT_NODE: "root-node", }; module.exports = { ResourceWatcher }; @@ -381,6 +344,4 @@ const LegacyListeners = { webConsoleFront.on("documentEvent", onAvailable); await webConsoleFront.startListeners(["DocumentEvents"]); }, - [ResourceWatcher.TYPES - .ROOT_NODE]: require("devtools/shared/resources/legacy-listeners/root-node"), }; diff --git a/devtools/shared/resources/tests/browser.ini b/devtools/shared/resources/tests/browser.ini index 60d324d9e0fc..ea57132d02c2 100644 --- a/devtools/shared/resources/tests/browser.ini +++ b/devtools/shared/resources/tests/browser.ini @@ -14,10 +14,7 @@ support-files = [browser_resources_console_messages.js] [browser_resources_document_events.js] [browser_resources_error_messages.js] -[browser_resources_exceptions.js] [browser_resources_platform_messages.js] -[browser_resources_root_node.js] -[browser_resources_several_resources.js] [browser_target_list_frames.js] [browser_target_list_preffedoff.js] [browser_target_list_processes.js] diff --git a/devtools/shared/resources/tests/browser_resources_exceptions.js b/devtools/shared/resources/tests/browser_resources_exceptions.js deleted file mode 100644 index 11223eba8848..000000000000 --- a/devtools/shared/resources/tests/browser_resources_exceptions.js +++ /dev/null @@ -1,52 +0,0 @@ -/* Any copyright is dedicated to the Public Domain. - http://creativecommons.org/publicdomain/zero/1.0/ */ - -"use strict"; - -// Test the ResourceWatcher API exceptions when using unsupported patterns - -const { - ResourceWatcher, -} = require("devtools/shared/resources/resource-watcher"); - -add_task(async function() { - // Open a test tab - gBrowser.selectedTab = BrowserTestUtils.addTab(gBrowser); - const tab = await addTab("data:text/html,ResourceWatcher exception tests"); - - const { - client, - resourceWatcher, - targetList, - } = await initResourceWatcherAndTarget(tab); - - info("Call watch once"); - const onAvailable = () => {}; - await resourceWatcher.watch([ResourceWatcher.TYPES.ROOT_NODE], onAvailable); - - info("Call watch again, should throw because we are already listening"); - const expectedMessage1 = - `The ResourceWatcher is already listening to "${ResourceWatcher.TYPES.ROOT_NODE}", ` + - "the client should call `watch` only once per resource type."; - - await Assert.rejects( - resourceWatcher.watch([ResourceWatcher.TYPES.ROOT_NODE], onAvailable), - err => err.message === expectedMessage1 - ); - - info("Call unwatch"); - resourceWatcher.unwatch([ResourceWatcher.TYPES.ROOT_NODE], onAvailable); - - info("Call watch again, should throw because we already listened previously"); - const expectedMessage2 = - `The ResourceWatcher previously watched "${ResourceWatcher.TYPES.ROOT_NODE}" ` + - "and doesn't support watching again on a previous resource."; - - await Assert.rejects( - resourceWatcher.watch([ResourceWatcher.TYPES.ROOT_NODE], onAvailable), - err => err.message === expectedMessage2 - ); - - targetList.stopListening(); - await client.close(); -}); diff --git a/devtools/shared/resources/tests/browser_resources_root_node.js b/devtools/shared/resources/tests/browser_resources_root_node.js deleted file mode 100644 index 9b8defd84675..000000000000 --- a/devtools/shared/resources/tests/browser_resources_root_node.js +++ /dev/null @@ -1,124 +0,0 @@ -/* Any copyright is dedicated to the Public Domain. - http://creativecommons.org/publicdomain/zero/1.0/ */ - -"use strict"; - -// Test the ResourceWatcher API around ROOT_NODE - -const { - ResourceWatcher, -} = require("devtools/shared/resources/resource-watcher"); - -/** - * This first test is a simplified version of - * devtools/server/tests/browser/browser_inspector_walker_watch_root_node.js - * - * The original test still asserts some scenarios using several watchRootNode - * call sites, which is not something we intend to support at the moment in the - * resource watcher. - * - * Otherwise this test checks the basic behavior of the resource when reloading - * an empty page. - */ -add_task(async function() { - // Open a test tab - gBrowser.selectedTab = BrowserTestUtils.addTab(gBrowser); - const tab = await addTab("data:text/html,Root Node tests"); - - const { - client, - resourceWatcher, - targetList, - } = await initResourceWatcherAndTarget(tab); - - const browser = gBrowser.selectedBrowser; - - info("Call watch([ROOT_NODE], ...)"); - let onAvailableCounter = 0; - const onAvailable = () => onAvailableCounter++; - await resourceWatcher.watch([ResourceWatcher.TYPES.ROOT_NODE], onAvailable); - - info("Wait until onAvailable has been called"); - await waitUntil(() => onAvailableCounter === 1); - is(onAvailableCounter, 1, "onAvailable has been called 1 time"); - - info("Reload the selected browser"); - browser.reload(); - - info("Wait until the watch([ROOT_NODE], ...) callback has been called"); - await waitUntil(() => onAvailableCounter === 2); - - is(onAvailableCounter, 2, "onAvailable has been called 2 times"); - - info("Call unwatch([ROOT_NODE], ...) for the onAvailable callback"); - resourceWatcher.unwatch([ResourceWatcher.TYPES.ROOT_NODE], onAvailable); - - info("Reload the selected browser"); - const reloaded = BrowserTestUtils.browserLoaded(browser); - browser.reload(); - await reloaded; - - is(onAvailableCounter, 2, "onAvailable was not called after calling unwatch"); - - // Cleanup - targetList.stopListening(); - await client.close(); -}); - -/** - * Test that the watchRootNode API provides the expected node fronts. - */ -add_task(async function testRootNodeFrontIsCorrect() { - gBrowser.selectedTab = BrowserTestUtils.addTab(gBrowser); - const tab = await addTab("data:text/html,
"); - - const { - client, - resourceWatcher, - targetList, - } = await initResourceWatcherAndTarget(tab); - const browser = gBrowser.selectedBrowser; - - info("Call watch([ROOT_NODE], ...)"); - - let rootNodeResolve; - let rootNodePromise = new Promise(r => (rootNodeResolve = r)); - const onAvailable = rootNodeFront => rootNodeResolve(rootNodeFront); - await resourceWatcher.watch([ResourceWatcher.TYPES.ROOT_NODE], onAvailable); - - info("Wait until onAvailable has been called"); - const { resource: root1, resourceType } = await rootNodePromise; - ok(!!root1, "onAvailable has been called with a valid argument"); - is( - resourceType, - ResourceWatcher.TYPES.ROOT_NODE, - "The resource has the expected type" - ); - - info("Check we can query an expected node under the retrieved root"); - const div1 = await root1.walkerFront.querySelector(root1, "div"); - is(div1.getAttribute("id"), "div1", "Correct root node retrieved"); - - info("Reload the selected browser"); - rootNodePromise = new Promise(r => (rootNodeResolve = r)); - browser.reload(); - - const { resource: root2 } = await rootNodePromise; - ok( - root1 !== root2, - "onAvailable has been called with a different node front after reload" - ); - - info("Navigate to another URL"); - rootNodePromise = new Promise(r => (rootNodeResolve = r)); - BrowserTestUtils.loadURI(browser, `data:text/html,
`); - const { resource: root3 } = await rootNodePromise; - info("Check we can query an expected node under the retrieved root"); - const div3 = await root3.walkerFront.querySelector(root3, "div"); - is(div3.getAttribute("id"), "div3", "Correct root node retrieved"); - - // Cleanup - resourceWatcher.unwatch([ResourceWatcher.TYPES.ROOT_NODE], onAvailable); - targetList.stopListening(); - await client.close(); -}); diff --git a/devtools/shared/resources/tests/browser_resources_several_resources.js b/devtools/shared/resources/tests/browser_resources_several_resources.js deleted file mode 100644 index 749b36ebc6a3..000000000000 --- a/devtools/shared/resources/tests/browser_resources_several_resources.js +++ /dev/null @@ -1,91 +0,0 @@ -/* Any copyright is dedicated to the Public Domain. - http://creativecommons.org/publicdomain/zero/1.0/ */ - -"use strict"; - -const { - ResourceWatcher, -} = require("devtools/shared/resources/resource-watcher"); - -/** - * Check that the resource watcher is still properly watching for new targets - * after unwatching one resource, if there is still another watched resource. - */ -add_task(async function() { - // Open a test tab - gBrowser.selectedTab = BrowserTestUtils.addTab(gBrowser); - const tab = await addTab("data:text/html,Root Node tests"); - - const { - client, - resourceWatcher, - targetList, - } = await initResourceWatcherAndTarget(); - - const { CONSOLE_MESSAGES, ROOT_NODE } = ResourceWatcher.TYPES; - - // We are only interested in console messages as a resource, the ROOT_NODE one - // is here to test the ResourceWatcher::unwatch API with several resources. - let receivedMessages = 0; - const onAvailable = ({ resourceType }) => { - if (resourceType === CONSOLE_MESSAGES) { - receivedMessages++; - } - }; - - info("Call watch([CONSOLE_MESSAGES, ROOT_NODE], ...)"); - await resourceWatcher.watch([CONSOLE_MESSAGES, ROOT_NODE], onAvailable); - - info("Use console.log in the content page"); - logInTab(tab, "test"); - info("Wait until onAvailable received 1 resource of type CONSOLE_MESSAGES"); - await waitUntil(() => receivedMessages === 1); - - // Check that the resource watcher captures resources from new targets. - info("Open a first tab on the example.com domain"); - const comTab = await addTab( - "http://example.com/document-builder.sjs?html=com" - ); - info("Use console.log in the example.com page"); - logInTab(comTab, "test-from-example-com"); - info("Wait until onAvailable received 2 resources of type CONSOLE_MESSAGES"); - await waitUntil(() => receivedMessages === 2); - - info("Stop watching ROOT_NODE resources"); - await resourceWatcher.unwatch([ROOT_NODE], onAvailable); - - // Check that messages from new targets are still captured after calling - // unwatch for another resource. - info("Open a second tab on the example.net domain"); - const netTab = await addTab( - "http://example.net/document-builder.sjs?html=net" - ); - info("Use console.log in the example.net page"); - logInTab(netTab, "test-from-example-net"); - info("Wait until onAvailable received 3 resources of type CONSOLE_MESSAGES"); - await waitUntil(() => receivedMessages === 3); - - info("Stop watching CONSOLE_MESSAGES resources"); - await resourceWatcher.unwatch([CONSOLE_MESSAGES], onAvailable); - await logInTab(tab, "test-again"); - - // We don't have a specific event to wait for here, so allow some time for - // the message to be received. - await wait(1000); - - is( - receivedMessages, - 3, - "The resource watcher should not watch CONSOLE_MESSAGES anymore" - ); - - // Cleanup - targetList.stopListening(); - await client.close(); -}); - -function logInTab(tab, message) { - return ContentTask.spawn(tab.linkedBrowser, message, function(_message) { - content.console.log(_message); - }); -}