Skip to content
Permalink
Browse files
Web Inspector: preserve DOM.NodeId if a node is removed and re-added
https://bugs.webkit.org/show_bug.cgi?id=189687

Reviewed by Devin Rousso.

Source/WebCore:

Instead of unbinding and rebinding nodes upon removal/reinsertion to the DOM tree, we should only perform the
unbinding actions on a `Node` that is being destroyed.

This resolves an issue where console messages with DOM nodes would behave unexpectedly because the node for a
visible console message could have its children removed in `DOMManager.prototype._unbind`, even though the node
would still have its children when it is removed from the DOM tree (unless it is being destroyed, which is
handled by `DOMManager.prototype.willDestroyDOMNode`).

While not a cause of an issue here, it is worth noting (due to confusion it caused me while investigating) that
even after we stopped keeping `RefPtr<Node>` for nodes in r278785 the nodes themselves continued to be kept
alive as part of the `RemoteObject` mechanism because the inspector injected script holds on to nodes until the
`RemoteObject` is released.

* inspector/agents/InspectorCSSAgent.cpp:
(WebCore::InspectorCSSAgent::nodeLayoutContextTypeChanged):

* inspector/agents/InspectorDOMAgent.cpp:
(WebCore::InspectorDOMAgent::reset):
(WebCore::InspectorDOMAgent::bind):
(WebCore::InspectorDOMAgent::didCommitLoad):
(WebCore::InspectorDOMAgent::didInsertDOMNode):
(WebCore::InspectorDOMAgent::didRemoveDOMNode):
(WebCore::InspectorDOMAgent::pseudoElementDestroyed):
(WebCore::InspectorDOMAgent::unbind): Deleted.
- We should no longer be unbinding and rebinding nodes on insertion/removal, since nodes should keep the same ID
for their lifetime, and we will clean up the binding upon the Node's destruction.

(WebCore::InspectorDOMAgent::willDestroyDOMNode):
(WebCore::InspectorDOMAgent::destroyedNodesTimerFired):
- A Node in middle of being destroyed will no longer have a parent.

* inspector/agents/InspectorDOMAgent.h:

Source/WebInspectorUI:

Instead of unbinding and rebinding nodes upon removal/reinsertion to the DOM tree, we should only perform the
unbinding actions on a `Node` that is being destroyed.

* UserInterface/Controllers/DOMDebuggerManager.js:

* UserInterface/Controllers/DOMManager.js:
(WI.DOMManager.prototype._pseudoElementAdded):

(WI.DOMManager.prototype.willDestroyDOMNode):
(WI.DOMManager.prototype._unbind):
- To match the backend, we should only unbind nodes when they are destroyed, not removed.

* UserInterface/Models/DOMNode.js:
(WI.DOMNode.prototype.newOrExistingFromPayload):
- Reuse an existing orphaned DOMNode if it already exists. In the future, we want to stop sending entire node
payloads to the frontend in those situations.

(WI.DOMNode.prototype._insertChild):
(WI.DOMNode.prototype._setChildrenPayload):

LayoutTests:

Update test to reflect that DOM breakpoints are now able to be reattached to their original node upon insertion
following removal.

* inspector/dom-debugger/dom-breakpoint-node-removed-ancestor.html:
* inspector/dom-debugger/dom-breakpoint-node-removed-ancestor-expected.txt:
* inspector/dom-debugger/dom-breakpoint-node-removed-direct-expected.txt:

* inspector/dom-debugger/resources/dom-breakpoint-utilities.js:
(TestPage.registerInitializer.InspectorTest.DOMBreakpoint.createBreakpoint):
- Add an event handler to log the setting of the domNode for breakpoints. In order to prevent spurious output
when removing breakpoints, instrument the removal of breakpoints to remove the testing event handler before the
node is changed.

Canonical link: https://commits.webkit.org/249538@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@292754 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
patrickangle committed Apr 12, 2022
1 parent a0262c8 commit 121550d11a4e437d4755ae3899eefed8c5236e70
Showing 13 changed files with 228 additions and 85 deletions.
@@ -1,3 +1,23 @@
2022-04-11 Patrick Angle <pangle@apple.com>

Web Inspector: preserve DOM.NodeId if a node is removed and re-added
https://bugs.webkit.org/show_bug.cgi?id=189687

Reviewed by Devin Rousso.

Update test to reflect that DOM breakpoints are now able to be reattached to their original node upon insertion
following removal.

* inspector/dom-debugger/dom-breakpoint-node-removed-ancestor.html:
* inspector/dom-debugger/dom-breakpoint-node-removed-ancestor-expected.txt:
* inspector/dom-debugger/dom-breakpoint-node-removed-direct-expected.txt:

* inspector/dom-debugger/resources/dom-breakpoint-utilities.js:
(TestPage.registerInitializer.InspectorTest.DOMBreakpoint.createBreakpoint):
- Add an event handler to log the setting of the domNode for breakpoints. In order to prevent spurious output
when removing breakpoints, instrument the removal of breakpoints to remove the testing event handler before the
node is changed.

2022-04-11 Matteo Flores <matteo_flores@apple.com>

[ Mac wk2 Debug ] accessibility/ancestor-computation.html is a constant timeout
@@ -16,19 +16,25 @@ PASS: Pause targetNodeId should be expected value.
CALL STACK:
0: [F] testNodeRemovedAncestor
1: [P] Global Code
Breakpoint "domNode" set to "null".
Breakpoint "domNode" set to "div#node-removed-ancestor-test".
-- Running test teardown.

-- Running test case: DOMBreakpoint.NodeRemoved.Ancestor.BreakpointDisabled
Adding "node-removed:1,HTML,1,BODY,1,DIV,0,DIV,0,DIV" DOM Breakpoint...
Disabling breakpoint...
Triggering breakpoint...
Breakpoint "domNode" set to "null".
Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should not pause for disabled breakpoint.
-- Running test teardown.

-- Running test case: DOMBreakpoint.NodeRemoved.Ancestor.DebuggerDisabled
Adding "node-removed:1,HTML,1,BODY,1,DIV,0,DIV,0,DIV" DOM Breakpoint...
Disabling all breakpoints...
Triggering breakpoint...
Breakpoint "domNode" set to "null".
Breakpoint "domNode" set to "div#node-removed-ancestor-test".
Enabling all breakpoints...
PASS: Should not pause when all breakpoints disabled.
-- Running test teardown.
@@ -46,17 +52,25 @@ Adding "node-removed:1,HTML,1,BODY,1,DIV,0,DIV,0,DIV" DOM Breakpoint...
Setting condition to 'false'...

Triggering breakpoint...
Breakpoint "domNode" set to "null".
Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should not pause.

Triggering breakpoint...
Breakpoint "domNode" set to "null".
Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should not pause.

Setting condition to 'true'...

Triggering breakpoint...
Breakpoint "domNode" set to "null".
Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should pause.

Triggering breakpoint...
Breakpoint "domNode" set to "null".
Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should pause.

-- Running test case: DOMBreakpoint.NodeRemoved.Ancestor.Options.Condition.ConsoleCommandLineAPI
@@ -66,18 +80,26 @@ Adding "node-removed:1,HTML,1,BODY,1,DIV,0,DIV,0,DIV" DOM Breakpoint...
Setting condition to saved console value...

Triggering breakpoint...
Breakpoint "domNode" set to "null".
Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should not pause.

Triggering breakpoint...
Breakpoint "domNode" set to "null".
Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should not pause.

Adding saved console value 'true'...
Setting condition to saved console value...

Triggering breakpoint...
Breakpoint "domNode" set to "null".
Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should pause.

Triggering breakpoint...
Breakpoint "domNode" set to "null".
Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should pause.

-- Running test case: DOMBreakpoint.NodeRemoved.Ancestor.Options.Action.Log
@@ -86,25 +108,33 @@ Adding "node-removed:1,HTML,1,BODY,1,DIV,0,DIV,0,DIV" DOM Breakpoint...
Adding log action...

Triggering breakpoint...
Breakpoint "domNode" set to "null".
Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should execute breakpoint action.
PASS: Should pause.

Editing log action...

Triggering breakpoint...
Breakpoint "domNode" set to "null".
Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should execute breakpoint action.
PASS: Should pause.

Editing log action...
Enabling auto-continue...

Triggering breakpoint...
Breakpoint "domNode" set to "null".
Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should execute breakpoint action.
PASS: Should not pause.

Editing log action...

Triggering breakpoint...
Breakpoint "domNode" set to "null".
Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should execute breakpoint action.
PASS: Should not pause.

@@ -114,25 +144,33 @@ Adding "node-removed:1,HTML,1,BODY,1,DIV,0,DIV,0,DIV" DOM Breakpoint...
Adding evaluate action...

Triggering breakpoint...
Breakpoint "domNode" set to "null".
Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should execute breakpoint action.
PASS: Should pause.

Editing evaluate action...

Triggering breakpoint...
Breakpoint "domNode" set to "null".
Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should execute breakpoint action.
PASS: Should pause.

Editing evaluate action...
Enabling auto-continue...

Triggering breakpoint...
Breakpoint "domNode" set to "null".
Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should execute breakpoint action.
PASS: Should not pause.

Editing evaluate action...

Triggering breakpoint...
Breakpoint "domNode" set to "null".
Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should execute breakpoint action.
PASS: Should not pause.

@@ -143,13 +181,17 @@ Adding "node-removed:1,HTML,1,BODY,1,DIV,0,DIV,0,DIV" DOM Breakpoint...
Adding evaluate action using saved console value...

Triggering breakpoint...
Breakpoint "domNode" set to "null".
Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should execute breakpoint action.
PASS: Should pause.

Adding saved console value '2'...
Editing evaluate action using saved console value...

Triggering breakpoint...
Breakpoint "domNode" set to "null".
Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should execute breakpoint action.
PASS: Should pause.

@@ -158,13 +200,17 @@ Editing evaluate action using saved console value...
Enabling auto-continue...

Triggering breakpoint...
Breakpoint "domNode" set to "null".
Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should execute breakpoint action.
PASS: Should not pause.

Adding saved console value '4'...
Editing evaluate action using saved console value...

Triggering breakpoint...
Breakpoint "domNode" set to "null".
Breakpoint "domNode" set to "div#node-removed-ancestor-test".
PASS: Should execute breakpoint action.
PASS: Should not pause.

@@ -46,8 +46,6 @@

await InspectorTest.evaluateInPage(`testNodeRemovedAncestor()`);

InspectorTest.assert(!breakpoint.domNode, "Should not have domNode.");

InspectorTest.assert(paused, "Should pause.");
WI.debuggerManager.removeEventListener(WI.DebuggerManager.Event.Paused, pausedListener);
},
@@ -72,13 +70,9 @@
InspectorTest.log("Disabling breakpoint...");
breakpoint.disabled = true;

InspectorTest.assert(breakpoint.domNode === node, "Should have domNode.");

InspectorTest.log("Triggering breakpoint...");
await InspectorTest.evaluateInPage(`testNodeRemovedAncestor()`);

InspectorTest.assert(!breakpoint.domNode, "Should not have domNode.");

InspectorTest.expectFalse(paused, "Should not pause for disabled breakpoint.");
WI.debuggerManager.removeEventListener(WI.DebuggerManager.Event.Paused, pausedListener);
},
@@ -106,8 +100,6 @@
InspectorTest.log("Triggering breakpoint...");
await InspectorTest.evaluateInPage(`testNodeRemovedAncestor()`);

InspectorTest.assert(!breakpoint.domNode, "Should not have domNode.");

InspectorTest.log("Enabling all breakpoints...");
await DebuggerAgent.setBreakpointsActive(true);

@@ -135,13 +127,9 @@
InspectorTest.log("Removing breakpoint...");
WI.domDebuggerManager.removeDOMBreakpoint(breakpoint);

InspectorTest.assert(!breakpoint.domNode, "Should not have domNode.");

InspectorTest.log("Triggering breakpoint...");
await InspectorTest.evaluateInPage(`testNodeRemovedAncestor()`);

InspectorTest.assert(!breakpoint.domNode, "Should not have domNode.");

InspectorTest.expectFalse(paused, "Should not pause for removed breakpoint.");
WI.debuggerManager.removeEventListener(WI.DebuggerManager.Event.Paused, pausedListener);
},

0 comments on commit 121550d

Please sign in to comment.