189687 – Web Inspector: preserve DOM.NodeId if a node is removed and re-added

REOPENED Bug 189687

Web Inspector: preserve DOM.NodeId if a node is removed and re-added

https://bugs.webkit.org/show_bug.cgi?id=189687

Summary Web Inspector: preserve DOM.NodeId if a node is removed and re-added

Devin Rousso

Reported 2018-09-17 20:39:39 PDT

If the same DOM node is removed and then re-added, it is assigned a new nodeId, which breaks the highlighting of any previous "Log Element" or `WI.linkifyNode` values. * STEPS TO REPRODUCE: 1. Open WebInspector to any page with children of <body> 2. Log any child element of <body> using the context menu "Log Element" (check that it shows `$1` next to the logged value in the console) 3. Remove the logged child element from <body> (e.g. press delete when it's selected) 4. Re-add the removed element by running `document.body.appendChild($1)` in the console 5. Highlight over the previously logged element (the entry with $1) * EXPECTED: The re-added node should be highlighted on the page * ACTUAL: Nothing is highlighted and an error "Could not find node with given id" is shown in inspector2.

Attachments
Patch (21.39 KB, patch) 2018-09-17 22:43 PDT, Devin Rousso	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews100 for mac-sierra (2.76 MB, application/zip) 2018-09-17 23:33 PDT, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews115 for mac-sierra (3.33 MB, application/zip) 2018-09-18 00:28 PDT, EWS Watchlist	no flags	Details
Patch v1.0 - For EWS (9.83 KB, patch) 2022-04-05 11:00 PDT, Patrick Angle	no flags	Details Formatted Diff Diff
Patch v1.1 - For EWS (26.06 KB, patch) 2022-04-06 15:03 PDT, Patrick Angle	no flags	Details Formatted Diff Diff
Patch v1.2 - For EWS (26.20 KB, patch) 2022-04-06 15:05 PDT, Patrick Angle	no flags	Details Formatted Diff Diff
Patch v1.3 - For Review (37.32 KB, patch) 2022-04-07 19:52 PDT, Patrick Angle	no flags	Details Formatted Diff Diff
Patch v1.4 - Review notes (35.42 KB, patch) 2022-04-11 13:21 PDT, Patrick Angle	no flags	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Devin Rousso

Comment 1 2018-09-17 22:43:42 PDT

Created attachment 350002 [details] Patch Checking for test failures on EWS while I write tests specific for this patch

EWS Watchlist

Comment 2 2018-09-17 22:45:30 PDT

Attachment 350002 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 19 files If any of these errors are false positives, please file a bug against check-webkit-style.

Mark Lam

Comment 3 2018-09-17 22:56:55 PDT

Comment on attachment 350002 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350002&action=review > Source/WebCore/inspector/agents/InspectorCSSAgent.h:148 > - void didRemoveDOMNode(Node&, int nodeId) override; > + void domNodeDestroyed(Node&, int nodeId) override; nit: is there a reason to not stick with the existing convention and name this "didDestroyDOMNode"? Also see didModifyDOMAttr below?

Daniel Bates

Comment 4 2018-09-17 23:02:03 PDT

(In reply to Build Bot from comment #2) > Attachment 350002 [details] did not pass style-queue: > > > ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and > either add and list tests, or explain why no new tests were possible. > [changelog/nonewtests] [5] > Total errors found: 1 in 19 files > > > If any of these errors are false positives, please file a bug against > check-webkit-style. This is not a false positive

EWS Watchlist

Comment 5 2018-09-17 23:33:27 PDT

Comment on attachment 350002 [details] Patch Attachment 350002 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/9252827 New failing tests: inspector/dom/shadow-and-non-shadow-children.html inspector/canvas/create-context-bitmaprenderer.html inspector/console/webcore-logging.html inspector/canvas/create-context-webgl.html

EWS Watchlist

Comment 6 2018-09-17 23:33:29 PDT

Created attachment 350005 [details] Archive of layout-test-results from ews100 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-sierra Platform: Mac OS X 10.12.6

EWS Watchlist

Comment 7 2018-09-18 00:28:51 PDT

Comment on attachment 350002 [details] Patch Attachment 350002 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/9253346 New failing tests: inspector/canvas/create-context-bitmaprenderer.html inspector/console/webcore-logging.html inspector/canvas/create-context-webgl.html

EWS Watchlist

Comment 8 2018-09-18 00:28:53 PDT

Created attachment 350008 [details] Archive of layout-test-results from ews115 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-sierra Platform: Mac OS X 10.12.6

Radar WebKit Bug Importer

Comment 9 2018-09-19 21:08:34 PDT

<rdar://problem/44628244>

Devin Rousso

Comment 10 2018-10-01 11:29:14 PDT

Comment on attachment 350002 [details] Patch r- this is not in a state ready for review.

Patrick Angle

Comment 11 2022-04-05 11:00:46 PDT

Created attachment 456725 [details] Patch v1.0 - For EWS

Devin Rousso

Comment 12 2022-04-05 11:40:39 PDT

Comment on attachment 456725 [details] Patch v1.0 - For EWS View in context: https://bugs.webkit.org/attachment.cgi?id=456725&action=review > Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:-511 > - _unbind(node) I think we need to leave this for when inspecting older iOS. Have there been any new protocol things added since the last snapshot that we could use to detect whether or not we need to do this?

Patrick Angle

Comment 13 2022-04-05 12:05:25 PDT

Comment on attachment 456725 [details] Patch v1.0 - For EWS View in context: https://bugs.webkit.org/attachment.cgi?id=456725&action=review >> Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:-511 >> - _unbind(node) > > I think we need to leave this for when inspecting older iOS. > > Have there been any new protocol things added since the last snapshot that we could use to detect whether or not we need to do this? Good catch! We have added `DOM.showFlexOverlay` since the last protocol snapshot, so we can use that.

Patrick Angle

Comment 14 2022-04-05 15:46:47 PDT

Need to investigate these test failures... its possible the new results are actually expected given at least one of the tests seems to be keeping track what nodes the frontend is still aware of. Will investigate. - inspector/dom-debugger/dom-breakpoint-node-removed-ancestor.html - inspector/dom/pseudo-element-dynamic.html

Patrick Angle

Comment 15 2022-04-06 15:03:29 PDT

Created attachment 456868 [details] Patch v1.1 - For EWS

Patrick Angle

Comment 16 2022-04-06 15:05:11 PDT

Created attachment 456870 [details] Patch v1.2 - For EWS

Devin Rousso

Comment 17 2022-04-06 16:37:28 PDT

Comment on attachment 456870 [details] Patch v1.2 - For EWS View in context: https://bugs.webkit.org/attachment.cgi?id=456870&action=review > Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:2544 > + if (parentId) { I realize that this is existing behavior, but I wonder if maybe we can't do this work in the frontend. Also, in `Node::~Node` it looks like it's expected for the `parentNode()` to be invalid shortly after our instrumentation, so I wonder if this is even working 🤔 > Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:190 > + debugDescription += (line + "\n"); Style: no parenthesis needed around `line + "\n"` > Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:197 > + let spacePrefix = ""; > + for (let i = 0; i <= level; ++i) > + spacePrefix += singleLevelSpacePrefix; ``` let spacePrefix = singleLevelSpacePrefix.repeat(level + 1); ``` > Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:210 > + let rootNodes = Object.values(this._idToDOMNode).filter((value) => (value instanceof WI.DOMNode) && !value.parentNode); Style: the parenthesis around `value instanceof WI.DOMNode` are not needed. Also, when would that ever not be the case? Is that check even necessary? > Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:551 > + // COMPATIBILITY (iOS 15.4): After iOS 15.4, the backend no longer unbinds nodes from their IDs until the node > + // is destroyed. Because there are no protocol changes associated with this change, check for flexbox overlay > + // support, which was also new after iOS 15.4. Please add something like this to this comment as well: ``` // FIXME: Use explicit version checking once https://webkit.org/b/148680 is fixed. ``` > Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:552 > + if(WI.assumingMainTarget().hasCommand("DOM.showFlexOverlay")) Style: `if (` > Source/WebInspectorUI/UserInterface/Models/DOMNode.js:1071 > + let node = this._domManager.nodeForId(payload.nodeId) || new WI.DOMNode(this._domManager, this.ownerDocument, this._isInShadowTree, payload); Wait so are we still creating a full `payload` for existing nodes? If so, that seems not ideal :/ Any way we can avoid that? > Source/WebInspectorUI/UserInterface/Models/DOMNode.js:1105 > + this._children.push(this._domManager.nodeForId(payload.nodeId) || new WI.DOMNode(this._domManager, this.ownerDocument, this._isInShadowTree, payload)); ditto (:1071) > LayoutTests/inspector/dom-debugger/dom-breakpoint-node-removed-ancestor.html:19 > + function addDOMNodeDidChangeHandlerToBreakpoint(breakpoint) { NIT: Why not have this be part of `InspectorTest.DOMBreakpoint.createBreakpoint`? Seems to me like this is something useful to have always, not opt-in :)

Patrick Angle

Comment 18 2022-04-06 17:56:34 PDT

Comment on attachment 456870 [details] Patch v1.2 - For EWS View in context: https://bugs.webkit.org/attachment.cgi?id=456870&action=review >> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:2544 >> + if (parentId) { > > I realize that this is existing behavior, but I wonder if maybe we can't do this work in the frontend. Also, in `Node::~Node` it looks like it's expected for the `parentNode()` to be invalid shortly after our instrumentation, so I wonder if this is even working 🤔 lol yup. There will never be a parentId... Node::~Node asserts that it doesn't have a parent almost immediately after our instrumentation, so even the old code path here shouldn't have been reachable. Not sure how I missed that last summer 🤦 - but this should greatly simply the logic here. >> Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:210 >> + let rootNodes = Object.values(this._idToDOMNode).filter((value) => (value instanceof WI.DOMNode) && !value.parentNode); > > Style: the parenthesis around `value instanceof WI.DOMNode` are not needed. > > Also, when would that ever not be the case? Is that check even necessary? oops >> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:1071 >> + let node = this._domManager.nodeForId(payload.nodeId) || new WI.DOMNode(this._domManager, this.ownerDocument, this._isInShadowTree, payload); > > Wait so are we still creating a full `payload` for existing nodes? If so, that seems not ideal :/ Any way we can avoid that? Yeah... probably need to do some renovations in the insertion/deletion protocol commands to support just providing nodeIds in some cases. >> LayoutTests/inspector/dom-debugger/dom-breakpoint-node-removed-ancestor.html:19 >> + function addDOMNodeDidChangeHandlerToBreakpoint(breakpoint) { > > NIT: Why not have this be part of `InspectorTest.DOMBreakpoint.createBreakpoint`? Seems to me like this is something useful to have always, not opt-in :) 👍🏻

Patrick Angle

Comment 19 2022-04-07 19:52:23 PDT

Created attachment 457006 [details] Patch v1.3 - For Review

Devin Rousso

Comment 20 2022-04-11 10:38:24 PDT

Comment on attachment 457006 [details] Patch v1.3 - For Review View in context: https://bugs.webkit.org/attachment.cgi?id=457006&action=review r=me, nnniiiccceee =D > Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:-984 > - // FIXME: <https://webkit.org/b/189687> Preserve DOM.NodeId if a node is removed and re-added The presence of this FIXME implies that something needed to be done here to support that. Is that true, or was this an erroneous FIXME? > Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:186 > + get boundNodesDebugDescription() NIT: If this is only for testing, I'd put this at the very bottom in a new `// Testing` section. Or even better, you could have a `Object.defineProperty(WI.DOMManager.prototype, "boundNodesDebugDescription. { get() { ... } })` inside `Source/WebInspectorUI/UserInterface/Debug/Bootstrap.js` (or a new `DOMManager.js` in that same folder) so that we don't ship this code to customers :) Also, it seems like this isn't used by this patch. Can we split this into a separate patch instead? > Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:514 > + _pseudoElementAdded(parentId, pseudoElementPayload) NIT: The name in the protocol actually is `pseudoElement`, so it would be kinda nice to keep it that way (though I do agree that it was badly named in the first place). > Source/WebInspectorUI/UserInterface/Models/DOMNode.js:166 > + static newOrExistingFromPayload(domManager, document, payload, {isInShadowTree} = {}) NIT: While I appreciate that you're following the existing pattern of passing in the `WI.DOMManager` instance to the `WI.DOMNode`, I kinda feel like that's old behavior/logic and probably shouldn't be propagated. I'd remove the `domManager` parameter and instead just inline `WI.domManager` where needed. (And we should also probably have a followup to remove `_domManager` from `WI.DOMNode`.) > LayoutTests/inspector/dom-debugger/dom-breakpoint-node-removed-ancestor-expected.txt:20 > +Breakpoint "domNode" set to "null". > +Breakpoint "domNode" set to "div#node-removed-ancestor-test". Hmm the fact that this is dispatching an event about the `domNode` being set to `null` makes me think that it was set to something before, and then is reset? That kinda seems odd. Especially since this seems to happen for _every_ event dispatch. Most of the tests involving `WI.DOMBreakpoint` seem to provide a `WI.DOMNode` up front, so not really sure why it'd ever be set to `null`. Can you figure out what's going on here? > LayoutTests/inspector/dom-debugger/resources/dom-breakpoint-utilities.js:62 > + InspectorTest.DOMBreakpoint._handleDOMNodeDidChange = function(event) { NIT: If this isn't used outside of this file, I'd just make it `function handleDOMNodeDidChange` (at the top of the file) instead of attaching it to `InspectorTest.DOMBreakpoint`.

Patrick Angle

Comment 21 2022-04-11 11:20:46 PDT

Comment on attachment 457006 [details] Patch v1.3 - For Review View in context: https://bugs.webkit.org/attachment.cgi?id=457006&action=review >> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:-984 >> - // FIXME: <https://webkit.org/b/189687> Preserve DOM.NodeId if a node is removed and re-added > > The presence of this FIXME implies that something needed to be done here to support that. Is that true, or was this an erroneous FIXME? This was an erroneous FIXME (that I added). This code shouldn't need to deal with the fact that the nodeId is now preserved, since it asks for an existing one first, then if there wasn't an existing ID, and we are trying to instrument all layout containers of interest, we request a new identifier. >> Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:186 >> + get boundNodesDebugDescription() > > NIT: If this is only for testing, I'd put this at the very bottom in a new `// Testing` section. Or even better, you could have a `Object.defineProperty(WI.DOMManager.prototype, "boundNodesDebugDescription. { get() { ... } })` inside `Source/WebInspectorUI/UserInterface/Debug/Bootstrap.js` (or a new `DOMManager.js` in that same folder) so that we don't ship this code to customers :) > > Also, it seems like this isn't used by this patch. Can we split this into a separate patch instead? Yep - I'll move this into a debug file so that it doesn't ship to customers. You are correct that it isn't used in this patch, but was used while working on the patch to verify the current DOM tree and detached trees were correct. I'll split it out into a separate patch. >> LayoutTests/inspector/dom-debugger/dom-breakpoint-node-removed-ancestor-expected.txt:20 >> +Breakpoint "domNode" set to "div#node-removed-ancestor-test". > > Hmm the fact that this is dispatching an event about the `domNode` being set to `null` makes me think that it was set to something before, and then is reset? That kinda seems odd. Especially since this seems to happen for _every_ event dispatch. Most of the tests involving `WI.DOMBreakpoint` seem to provide a `WI.DOMNode` up front, so not really sure why it'd ever be set to `null`. Can you figure out what's going on here? Its set to null upon the node being removed from the DOM tree, and then set back to the node upon reinsertion. The initial setting of the breakpoint to the DOM node isn't instrumented, since it occurs before we attach the event listener. Because the test removes the node and then re-adds it, this makes sense. The node is actually changing from its initial `div#node-removed-ancestor-test` to `null` on removal and back to `div#node-removed-ancestor-test` on insertion.

Patrick Angle

Comment 22 2022-04-11 13:21:50 PDT

Created attachment 457280 [details] Patch v1.4 - Review notes

EWS

Comment 23 2022-04-11 18:35:25 PDT

Committed r292754 (249538@main): <https://commits.webkit.org/249538@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 457280 [details].

Patrick Angle

Comment 24 2022-05-22 11:12:39 PDT

Reverted in https://commits.webkit.org/250847@main due to issues between frontend and backend representation of DOM tree.

Note You need to log in before you can comment on or make changes to this bug.

Status REOPENED

Resolution

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware All

OS All

Product WebKit

Component Web Inspector

Assignee

Devin Rousso

Reported

2018-09-17 20:39 PDT

Modified

2022-05-22 11:12 PDT History

CC List

15 users Show

URL

Keywords InRadar

Depends on

226624

Blocks

238947 239129 240769

Dependencies

tree graph