Bug 189687 - Web Inspector: preserve DOM.NodeId if a node is removed and re-added
Summary: Web Inspector: preserve DOM.NodeId if a node is removed and re-added
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on: 226624
Blocks: 238947 239129 240769
  Show dependency treegraph
 
Reported: 2018-09-17 20:39 PDT by Devin Rousso
Modified: 2022-05-22 11:12 PDT (History)
15 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 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.
Comment 1 Devin Rousso 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
Comment 2 EWS Watchlist 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.
Comment 3 Mark Lam 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?
Comment 4 Daniel Bates 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
Comment 5 EWS Watchlist 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
Comment 6 EWS Watchlist 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
Comment 7 EWS Watchlist 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
Comment 8 EWS Watchlist 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
Comment 9 Radar WebKit Bug Importer 2018-09-19 21:08:34 PDT
<rdar://problem/44628244>
Comment 10 Devin Rousso 2018-10-01 11:29:14 PDT
Comment on attachment 350002 [details]
Patch

r- this is not in a state ready for review.
Comment 11 Patrick Angle 2022-04-05 11:00:46 PDT
Created attachment 456725 [details]
Patch v1.0 - For EWS
Comment 12 Devin Rousso 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?
Comment 13 Patrick Angle 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.
Comment 14 Patrick Angle 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
Comment 15 Patrick Angle 2022-04-06 15:03:29 PDT
Created attachment 456868 [details]
Patch v1.1 - For EWS
Comment 16 Patrick Angle 2022-04-06 15:05:11 PDT
Created attachment 456870 [details]
Patch v1.2 - For EWS
Comment 17 Devin Rousso 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 :)
Comment 18 Patrick Angle 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 :)

👍🏻
Comment 19 Patrick Angle 2022-04-07 19:52:23 PDT
Created attachment 457006 [details]
Patch v1.3 - For Review
Comment 20 Devin Rousso 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`.
Comment 21 Patrick Angle 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.
Comment 22 Patrick Angle 2022-04-11 13:21:50 PDT
Created attachment 457280 [details]
Patch v1.4 - Review notes
Comment 23 EWS 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].
Comment 24 Patrick Angle 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.