Bug 1694400 - [devtools] Fix infinite loop after deleting node in same-process frame r=nchevobbe

Deleting a node with the inspector puts the corresponding NodeActor in the retained nodes, in case the user cancels the deletion.
When a frame is unloaded we try to remove all the retained node to free memory.
For each retained node, we try to climb up the window hierarchy to know if it should be released or not.
However the implementation was buggy, relying on a while loop which was incorrectly mixing window objects and frame elements.
This previously lead to errors, and now leads to infinite loops.

A new test is added to cover this scenario.

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

devtools/client/inspector/test/browser.ini

@@ -71,6 +71,7 @@ skip-if = os == "mac" # Full keyboard navigation on OSX only works if Full Keybo
 [browser_inspector_delete-selected-node-02.js]
 skip-if = (os == 'win' && processor == 'aarch64') # bug 1533490
 [browser_inspector_delete-selected-node-03.js]
+[browser_inspector_delete_node_in_frame.js]
 [browser_inspector_destroy-after-navigation.js]
 [browser_inspector_destroy-before-ready.js]
 [browser_inspector_expand-collapse.js]

devtools/client/inspector/test/browser_inspector_delete-selected-node-02.js

@@ -23,7 +23,9 @@ add_task(async function() {
         "its parent node is selected and breadcrumbs are updated."
     );
 
-    await deleteNodeWithContextMenu("#deleteManually");
+    await selectNode("#deleteManually", inspector);
+    const nodeToBeDeleted = inspector.selection.nodeFront;
+    await deleteNodeWithContextMenu(nodeToBeDeleted, inspector);
 
     info("Performing checks.");
     await assertNodeSelectedAndPanelsUpdated(
@@ -83,7 +85,9 @@ add_task(async function() {
         "when the node is followed by a non-element node"
     );
 
-    await deleteNodeWithContextMenu("#deleteWithNonElement");
+    await selectNode("#deleteWithNonElement", inspector);
+    const nodeToBeDeleted = inspector.selection.nodeFront;
+    await deleteNodeWithContextMenu(nodeToBeDeleted, inspector);
 
     let expectedCrumbs = ["html", "body", "div#deleteToMakeSingleTextNode"];
     await assertNodeSelectedAndCrumbsUpdated(expectedCrumbs, Node.TEXT_NODE);
@@ -98,40 +102,6 @@ add_task(async function() {
     await assertNodeSelectedAndCrumbsUpdated(expectedCrumbs, Node.ELEMENT_NODE);
   }
 
-  async function deleteNodeWithContextMenu(selector) {
-    await selectNode(selector, inspector);
-    const nodeToBeDeleted = inspector.selection.nodeFront;
-
-    info("Getting the node container in the markup view.");
-    const container = await getContainerForSelector(selector, inspector);
-
-    const allMenuItems = openContextMenuAndGetAllItems(inspector, {
-      target: container.tagLine,
-    });
-    const menuItem = allMenuItems.find(item => item.id === "node-menu-delete");
-
-    info("Clicking 'Delete Node' in the context menu.");
-    is(menuItem.disabled, false, "delete menu item is enabled");
-    menuItem.click();
-
-    // close the open context menu
-    EventUtils.synthesizeKey("KEY_Escape");
-
-    info("Waiting for inspector to update.");
-    await inspector.once("inspector-updated");
-
-    // Since the mutations are sent asynchronously from the server, the
-    // inspector-updated event triggered by the deletion might happen before
-    // the mutation is received and the element is removed from the
-    // breadcrumbs. See bug 1284125.
-    if (inspector.breadcrumbs.indexOf(nodeToBeDeleted) > -1) {
-      info("Crumbs haven't seen deletion. Waiting for breadcrumbs-updated.");
-      await inspector.once("breadcrumbs-updated");
-    }
-
-    return menuItem;
-  }
-
   function assertNodeSelectedAndCrumbsUpdated(
     expectedCrumbs,
     expectedNodeType

devtools/client/inspector/test/browser_inspector_delete_node_in_frame.js

@@ -0,0 +1,33 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+"use strict";
+
+const TEST_URL_1 = `http://example.org/document-builder.sjs?html=
+<iframe srcdoc="<div>"></iframe><body>`;
+const TEST_URL_2 = `http://example.org/document-builder.sjs?html=<div id=url2>`;
+
+// Test that deleting a node in a same-process iframe and doing a navigation
+// does not freeze the browser or break the toolbox.
+add_task(async function() {
+  const { inspector } = await openInspectorForURL(TEST_URL_1);
+
+  info("Select a node in a same-process iframe");
+  const node = await selectNodeInFrames(["iframe", "div"], inspector);
+  const parentNode = node.parentNode();
+
+  info("Removing selected element with the context menu.");
+  await deleteNodeWithContextMenu(node, inspector);
+
+  is(
+    inspector.selection.nodeFront,
+    parentNode,
+    "The parent node of the deleted node was selected."
+  );
+
+  const onInspectorReloaded = inspector.once("reloaded");
+  await navigateTo(TEST_URL_2);
+  await onInspectorReloaded;
+
+  const url2NodeFront = await getNodeFront("#url2", inspector);
+  ok(url2NodeFront, "Can retrieve a node front after the navigation");
+});

devtools/client/inspector/test/head.js

@@ -1420,3 +1420,41 @@ async function checkEyeDropperColorAt(
   );
   is(colorValue, expectedColor, assertionDescription);
 }
+
+/**
+ * Delete the provided node front using the context menu in the markup view.
+ * Will resolve after the inspector UI was fully updated.
+ *
+ * @param {NodeFront} node
+ *        The node front to delete.
+ * @param {Inspector} inspector
+ *        The current inspector panel instance.
+ */
+async function deleteNodeWithContextMenu(node, inspector) {
+  const container = inspector.markup.getContainer(node);
+
+  const allMenuItems = openContextMenuAndGetAllItems(inspector, {
+    target: container.tagLine,
+  });
+  const menuItem = allMenuItems.find(item => item.id === "node-menu-delete");
+  const onInspectorUpdated = inspector.once("inspector-updated");
+
+  info("Clicking 'Delete Node' in the context menu.");
+  is(menuItem.disabled, false, "delete menu item is enabled");
+  menuItem.click();
+
+  // close the open context menu
+  EventUtils.synthesizeKey("KEY_Escape");
+
+  info("Waiting for inspector to update.");
+  await onInspectorUpdated;
+
+  // Since the mutations are sent asynchronously from the server, the
+  // inspector-updated event triggered by the deletion might happen before
+  // the mutation is received and the element is removed from the
+  // breadcrumbs. See bug 1284125.
+  if (inspector.breadcrumbs.indexOf(node) > -1) {
+    info("Crumbs haven't seen deletion. Waiting for breadcrumbs-updated.");
+    await inspector.once("breadcrumbs-updated");
+  }
+}

devtools/server/actors/inspector/walker.js

@@ -2533,12 +2533,12 @@ var WalkerActor = protocol.ActorClassWithSpec(walkerSpec, {
 
   // Returns true if domNode is in window or a subframe.
   _childOfWindow: function(window, domNode) {
-    let win = nodeDocument(domNode).defaultView;
-    while (win) {
+    while (domNode) {
+      const win = nodeDocument(domNode).defaultView;
       if (win === window) {
         return true;
       }
-      win = getFrameElement(win);
+      domNode = getFrameElement(win);
     }
     return false;
   },