From ab439621fdeb03dc335598152ad345756bca4db9 Mon Sep 17 00:00:00 2001 From: Nicolas Chevobbe Date: Tue, 25 Feb 2020 10:22:31 +0000 Subject: [PATCH] Bug 1617210 - Fix issue with duplicated ObjectFronts in the scopes panel. r=jlast. Since the thread actor caches the object actors it creates, if an object had multiple properties referencing the same object, we would create multiple fronts for the same object actor, which would confuse protocol.js. The fix consist in checking if a front already exists before creating a new one. A test is added for the debugger to ensure this works as expected and we don't regress. Differential Revision: https://phabricator.services.mozilla.com/D63907 --HG-- extra : moz-landing-system : lando --- .../debugger/test/mochitest/browser.ini | 1 + .../browser_dbg-scopes-duplicated.js | 91 +++++++++++++++++++ .../client/debugger/test/mochitest/helpers.js | 1 + .../test/browser/stub-generator-helpers.js | 1 + .../tests/xpcshell/test_objectgrips-21.js | 1 + devtools/shared/fronts/object.js | 8 ++ 6 files changed, 103 insertions(+) create mode 100644 devtools/client/debugger/test/mochitest/browser_dbg-scopes-duplicated.js diff --git a/devtools/client/debugger/test/mochitest/browser.ini b/devtools/client/debugger/test/mochitest/browser.ini index 700786a577dd..ac89af34dffb 100644 --- a/devtools/client/debugger/test/mochitest/browser.ini +++ b/devtools/client/debugger/test/mochitest/browser.ini @@ -180,6 +180,7 @@ skip-if = (os == 'linux' && debug) || (os == 'linux' && asan) || ccov #Bug 1456 [browser_dbg-old-breakpoint.js] [browser_dbg-idb-run-to-completion.js] [browser_dbg-inline-script-offset.js] +[browser_dbg-scopes-duplicated.js] [browser_dbg-scopes-xrays.js] [browser_dbg-merge-scopes.js] [browser_dbg-message-run-to-completion.js] diff --git a/devtools/client/debugger/test/mochitest/browser_dbg-scopes-duplicated.js b/devtools/client/debugger/test/mochitest/browser_dbg-scopes-duplicated.js new file mode 100644 index 000000000000..815de52e9578 --- /dev/null +++ b/devtools/client/debugger/test/mochitest/browser_dbg-scopes-duplicated.js @@ -0,0 +1,91 @@ +/* Any copyright is dedicated to the Public Domain. + * http://creativecommons.org/publicdomain/zero/1.0/ */ + +// Test that properties with the same value objec be expanded. See Bug 1617210. + +const httpServer = createTestHTTPServer(); +httpServer.registerContentType("html", "text/html"); +httpServer.registerContentType("js", "application/javascript"); + +httpServer.registerPathHandler(`/`, function(request, response) { + response.setStatusLine(request.httpVersion, 200, "OK"); + response.write(` + + + + `); +}); + +httpServer.registerPathHandler("/test.js", function(request, response) { + response.setHeader("Content-Type", "application/javascript"); + response.write(` + document.addEventListener("click", function onClick(e) { + var hello = {hello: "world"}; + var x = {a: hello, b: hello, c: hello}; + debugger; + }); + `); +}); +const port = httpServer.identity.primaryPort; +const TEST_URL = `http://localhost:${port}/`; + +add_task(async function() { + const dbg = await initDebuggerWithAbsoluteURL(TEST_URL); + + const ready = Promise.all([ + waitForPaused(dbg), + waitForLoadedSource(dbg, "test"), + ]); + + SpecialPowers.spawn(gBrowser.selectedBrowser, [], function() { + content.document.querySelector("button.pause").click(); + }); + + await ready; + + is(getLabel(dbg, 4), "e"); + is(getLabel(dbg, 5), "hello"); + is(getLabel(dbg, 6), "x"); + + info("Expand `x` node"); + await toggleScopeNode(dbg, 6); + is(getLabel(dbg, 7), "a"); + is(getLabel(dbg, 8), "b"); + is(getLabel(dbg, 9), "c"); + + info("Expand `c`, `b` and `a` nodes"); + await toggleScopeNode(dbg, 9); + await toggleScopeNode(dbg, 8); + await toggleScopeNode(dbg, 7); + + is(getLabel(dbg, 7), "a"); + is(getLabel(dbg, 8), "hello"); + is(getLabel(dbg, 9), ""); + is(getLabel(dbg, 10), "b"); + is(getLabel(dbg, 11), "hello"); + is(getLabel(dbg, 12), ""); + is(getLabel(dbg, 13), "c"); + is(getLabel(dbg, 14), "hello"); + is(getLabel(dbg, 15), ""); + + info("Expand `e`"); + await toggleScopeNode(dbg, 4); + + info("Expand the `target` node"); + let nodes = getAllLabels(dbg); + const originalNodesCount = nodes.length; + const targetNodeIndex = nodes.indexOf("target"); + ok(targetNodeIndex > -1, "Found the target node"); + await toggleScopeNode(dbg, targetNodeIndex); + nodes = getAllLabels(dbg); + ok(nodes.length > originalNodesCount, "the target node was expanded"); + ok(nodes.includes("classList"), "classList is displayed"); +}); + +function getAllLabels(dbg) { + return Array.from(findAllElements(dbg, "scopeNodes")).map(el => el.innerText); +} + +function getLabel(dbg, index) { + return findElement(dbg, "scopeNode", index).innerText; +} diff --git a/devtools/client/debugger/test/mochitest/helpers.js b/devtools/client/debugger/test/mochitest/helpers.js index 8200ee2f3aab..89d784c28303 100644 --- a/devtools/client/debugger/test/mochitest/helpers.js +++ b/devtools/client/debugger/test/mochitest/helpers.js @@ -1299,6 +1299,7 @@ const selectors = { }, columnBreakpoints: ".column-breakpoint", scopes: ".scopes-list", + scopeNodes: ".scopes-list .object-label", scopeNode: i => `.scopes-list .tree-node:nth-child(${i}) .object-label`, scopeValue: i => `.scopes-list .tree-node:nth-child(${i}) .object-delimiter + *`, diff --git a/devtools/client/webconsole/test/browser/stub-generator-helpers.js b/devtools/client/webconsole/test/browser/stub-generator-helpers.js index 7cc9f4bac8c2..23e47b591ca5 100644 --- a/devtools/client/webconsole/test/browser/stub-generator-helpers.js +++ b/devtools/client/webconsole/test/browser/stub-generator-helpers.js @@ -433,6 +433,7 @@ function parsePacketAndCreateFronts(packet) { conn: { poolFor: () => {}, addActorPool: () => {}, + getFrontByID: () => {}, }, manage: () => {}, }); diff --git a/devtools/server/tests/xpcshell/test_objectgrips-21.js b/devtools/server/tests/xpcshell/test_objectgrips-21.js index 5efde8f698ca..b414503eff6f 100644 --- a/devtools/server/tests/xpcshell/test_objectgrips-21.js +++ b/devtools/server/tests/xpcshell/test_objectgrips-21.js @@ -310,6 +310,7 @@ async function test_unsafe_grips( ); } } + await objClient.release(); } await threadFront.resume(); diff --git a/devtools/shared/fronts/object.js b/devtools/shared/fronts/object.js index 3bf685fbce5a..3bac4f6e9f99 100644 --- a/devtools/shared/fronts/object.js +++ b/devtools/shared/fronts/object.js @@ -315,6 +315,14 @@ function getAdHocFrontOrPrimitiveGrip(packet, parentFront) { } const { conn, targetFront } = parentFront; + + // We may have already created a front for this object actor since some actor (e.g. the + // thread actor) cache the object actors they create. + const existingFront = conn.getFrontByID(packet.actor); + if (existingFront) { + return existingFront; + } + const { type } = packet; if (type === "longString") {