Bug 226624 - Web Inspector: Add instrumentation to node destruction for InspectorDOMAgent
Summary: Web Inspector: Add instrumentation to node destruction for InspectorDOMAgent
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Patrick Angle
URL:
Keywords: InRadar
Depends on:
Blocks: 189687
  Show dependency treegraph
 
Reported: 2021-06-03 21:42 PDT by Patrick Angle
Modified: 2021-06-11 15:37 PDT (History)
15 users (show)

See Also:


Attachments
WIP v0.1 - Patch for EWS testing of base changes (2.73 KB, patch)
2021-06-03 21:45 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
WIP v0.2 - Patch for EWS testing (2.87 KB, patch)
2021-06-04 09:44 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
Patch v1.0 (12.65 KB, patch)
2021-06-04 22:09 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
WIP v1.1 - First pass of review notes for EWS testing (17.92 KB, patch)
2021-06-08 17:29 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
Patch v2.0 - Revised node mapping strategy (45.73 KB, patch)
2021-06-10 15:18 PDT, Patrick Angle
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch v2.1 - Add missing forward declaration (46.15 KB, patch)
2021-06-10 16:06 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
Patch v2.2 - Review notes (46.03 KB, patch)
2021-06-10 20:28 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
Patch v2.3 - Add valid key check to InspectorDOMAgent::boundNodeId (46.12 KB, patch)
2021-06-11 12:27 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
Patch v2.4 - Review notes (46.79 KB, patch)
2021-06-11 13:02 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 Patrick Angle 2021-06-03 21:42:04 PDT
<rdar://69543874>
Comment 1 Patrick Angle 2021-06-03 21:45:30 PDT
Created attachment 430538 [details]
WIP v0.1 - Patch for EWS testing of base changes
Comment 2 Patrick Angle 2021-06-04 09:44:30 PDT
Created attachment 430586 [details]
WIP v0.2 - Patch for EWS testing
Comment 3 Patrick Angle 2021-06-04 22:09:27 PDT
Created attachment 430643 [details]
Patch v1.0
Comment 4 Devin Rousso 2021-06-07 08:30:48 PDT
Comment on attachment 430643 [details]
Patch v1.0

View in context: https://bugs.webkit.org/attachment.cgi?id=430643&action=review

> Source/WebCore/inspector/InspectorInstrumentation.cpp:175
> -        pageDOMDebuggerAgent->didRemoveDOMNode(node);
> +        pageDOMDebuggerAgent->willDestroyDOMNode(node);

I think we should keep `PageDOMDebuggerAgent::didRemoveDOMNode` and add a new `PageDOMDebuggerAgent::willDestroyDOMNode` that just calls `didRemoveDOMNode(node)` with a comment like `// Treat destruction as though the DOM node was just removed from the main DOM tree.` for the sake of readability/searchability

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:2492
> +    // If parent is not mapped yet/anymore -> ignore the event.
> +    if (!m_documentNodeToIdMap.contains(parent))

I feel like we should still attempt to `unbind` even if `parent` hasn't been instrumented, just in case.  Like what about if the `Node` is not attached to the main DOM tree and is just a totally isolated node?  We may need to add a new event to the frontend like `DOM.nodeDestroyed` that ensures that the frontend removes the `WI.DOMNode`.  This function being called should ensure that `Node` is not found _anywhere_ in _any_ of the `HashMap`, regardless of the state of the `Node` and/or any ancestors.

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:2510
> +    unbind(&node, &m_documentNodeToIdMap);

Are we sure that we really need to do a full `unbind`?  This also does stuff like walking the entire subtree and calling `unbind` for all those nodes too.  I don't think it'd be a problem here, but doing more complex stuff inside a destructor can be risky, so perhaps we should only do things related to the given `node` directly and let any related stuff (e.g. subtree nodes) be handled by their own `willDestroyDOMNode`.

ditto for `InspectorDOMDebuggerAgent::willDestroyDOMNode` too

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:2516
> +    for (auto parentID : m_childNodeCountUpdatedIdentifiers)
> +        m_frontendDispatcher->childNodeCountUpdated(parentID, 0);

I think we need to make sure that the parent node still has no children before sending this.  We wouldn't want any new child nodes added to the parent in between `InspectorDOMAgent::willDestroyDOMNode` and the timer firing to be wiped out.

NIT: I think this could use a better name since it only every sends a count of `0`.  Maybe `m_pendingChildUpdateIdentifiersForEmptyParentNodes`?

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:2517
> +    m_childNodeCountUpdatedIdentifiers.clear();

NIT: you can do this in one line :)
```
    for (auto parentID : std::exchange(m_childNodeCountUpdatedIdentifiers, { })
```

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:2520
> +    for (auto pair : m_childNodeRemovedIdentifiers)
> +        m_frontendDispatcher->childNodeRemoved(pair.first, pair.second);

NIT: I'd extract first and second into better named variables
```
    for (auto& [parentId, nodeId] : std::exchange(m_pendingChildNodeRemovedIdentifiers, { })
```
Comment 5 Patrick Angle 2021-06-08 17:29:36 PDT
Created attachment 430922 [details]
WIP v1.1 - First pass of review notes for EWS testing
Comment 6 EWS Watchlist 2021-06-08 17:30:26 PDT
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
Comment 7 Devin Rousso 2021-06-08 18:02:50 PDT
Comment on attachment 430922 [details]
WIP v1.1 - First pass of review notes for EWS testing

View in context: https://bugs.webkit.org/attachment.cgi?id=430922&action=review

Looking good :)

Any idea if you can write a test for this?

> Source/JavaScriptCore/inspector/protocol/DOM.json:682
> +            "name": "detachedNodeDestroyed",

NIT: While this is technically correct, i think for the sake of "future proofing" it may be better to just call this `willDestroyDOMNode` both to match the `InspectorInstrumentation` name and so that this better fits into <https://webkit.org/b/189687>.  We probably eventually want to have this event be the teardown instead of `childNodeRemoved`, so if/when that day comes having to add yet another event (i.e. having both `detachedNodeDestroyed` and `willDestroyDOMNode`) would be odd.   If your goal is to avoid dispatching unnecessary events (e.g. for a node that is attached, this is probably redundant), I'd rename it and then just add another sentence to the `"description"` explaining how right now this is only called for detached nodes, but I personally don't think it's a big deal.

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:2493
> +    ContainerNode* parent = node.parentNode();

`auto`

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:2499
> +        m_pendingNodeDestroyedIdentifiers.append(std::make_pair(parentId, nodeId));

NIT: I think you can just `{ parentId, nodeId }`

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:2507
> +    m_idToNode.remove(nodeId);
> +    m_documentNodeToIdMap.remove(&node);

When do we remove from `m_childrenRequested`?

What about `m_idToNodesMap` as well?

> Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:186
> +    detachedNodeDestroyed(nodeId)

Style: this should go in the `// DOMObserver` section since that's what calls this, probably right above `didAddEventListener` (nearest match in `DOM.json`)

> Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:188
> +        var node = this._idToDOMNode[nodeId];

`let`

> Source/WebInspectorUI/UserInterface/Views/DOMTreeUpdater.js:-106
> -        this._recentlyDeletedNodes.set(event.data.node, {parent: event.data.parent});

nice job catching this :)
Comment 8 Patrick Angle 2021-06-10 15:18:16 PDT
Created attachment 431141 [details]
Patch v2.0 - Revised node mapping strategy
Comment 9 Patrick Angle 2021-06-10 15:42:02 PDT
Comment on attachment 431141 [details]
Patch v2.0 - Revised node mapping strategy

Missing an import in Node.cpp 😔 - new patch that should actually build in just a moment...
Comment 10 Patrick Angle 2021-06-10 16:06:39 PDT
Created attachment 431155 [details]
Patch v2.1 - Add missing forward declaration
Comment 11 Devin Rousso 2021-06-10 16:09:19 PDT
Comment on attachment 431155 [details]
Patch v2.1 - Add missing forward declaration

View in context: https://bugs.webkit.org/attachment.cgi?id=431155&action=review

This is so awesome!!!  I've been wanting to do something like this for a while now.  Not sure why I never have, but glad to see it finally happening :)

> Source/WebCore/dom/Node.h:56
> +class InspectorInstrumentation;

I think you actually need to actually add `#include "InspectorInstrumentation.h"` to `Node.cpp` 😅

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:384
> +    auto id = m_nodeToId.get(node);

NIT: you could use `HashMap::ensure` to make this a little cleaner
```
    return m_nodeToId.ensure(node, [&] {
        auto id = m_lastNodeId++;
        m_idToNode.set(id, node);
        return id;
    });
```

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:417
> +    m_nodeToId.remove(node);

I'd move this to be right next to `m_idToNode.remove(id)` (or vice versa) so that it's clear that operations to one are meant to happen to the other ASAP.

I wonder if it makes any difference where the `.remove(` are inside this function.  If not, I'd suggest putting it at the very top so that it avoids reentrancy.

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:2480
> +    if (auto parentId = m_nodeToId.get(parent))

NIT: I'd probably add a FIXME comment for <https://webkit.org/b/189687> in here somewhere as we'd probably eventually want to unify `m_destroyedAttachedNodeIdentifiers` and `m_destroyedDetachedNodeIdentifiers`.

> Source/WebCore/inspector/agents/InspectorDOMAgent.h:267
> +    Vector<std::pair<Inspector::Protocol::DOM::NodeId, Inspector::Protocol::DOM::NodeId>> m_destroyedNodeIdentifiers;

i'd rename this `m_destroyedAttachedNodeIdentifiers` so it's more clear how it's different from `m_destroyedDetachedNodeIdentifiers`
Comment 12 Patrick Angle 2021-06-10 20:28:24 PDT
Created attachment 431174 [details]
Patch v2.2 - Review notes
Comment 13 Patrick Angle 2021-06-11 12:27:45 PDT
Created attachment 431219 [details]
Patch v2.3 - Add valid key check to InspectorDOMAgent::boundNodeId
Comment 14 Devin Rousso 2021-06-11 12:36:51 PDT
Comment on attachment 431219 [details]
Patch v2.3 - Add valid key check to InspectorDOMAgent::boundNodeId

View in context: https://bugs.webkit.org/attachment.cgi?id=431219&action=review

r=mews, awesome awesome awesome

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:382
> +Protocol::DOM::NodeId InspectorDOMAgent::bind(Node* node)

NIT: I wonder if we should make this into `Node&` while here so that we know for sure that `m_nodeToId.ensure` won't complain about an invalid key (e.g. not `nullptr`).  Alternatively you could check it just like `boundNodeId`.

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:391
> +void InspectorDOMAgent::unbind(Node* node)

ditto (:382)

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:2367
> +    auto prevId = previousSibling ? boundNodeId(previousSibling) : 0;

you can drop the `previousSibling ? ` check now that `boundNodeId` does it for you

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:2425
>      if (!parent)

ditto (:2367)

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:2439
> +        auto prevId = prevSibling ? boundNodeId(prevSibling) : 0;

ditto (:2367)

> Source/WebCore/inspector/agents/page/PageConsoleAgent.cpp:56
>  Protocol::ErrorStringOr<void> PageConsoleAgent::clearMessages()

you can just remove this function entirely now
Comment 15 Patrick Angle 2021-06-11 13:02:14 PDT
Created attachment 431225 [details]
Patch v2.4 - Review notes
Comment 16 EWS 2021-06-11 15:37:47 PDT
Committed r278785 (238742@main): <https://commits.webkit.org/238742@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 431225 [details].