Bug 226624 – Web Inspector: Add instrumentation to node destruction for InspectorDOMAgent

• Component: Web Inspector
• Status: RESOLVED FIXED

[Patrick Angle]

<rdar://69543874>

[Patrick Angle]

Created attachment 430538 [details]
WIP v0.1 - Patch for EWS testing of base changes

[Patrick Angle]

Created attachment 430586 [details]
WIP v0.2 - Patch for EWS testing

[Patrick Angle]

Created attachment 430643 [details]
Patch v1.0

[Devin Rousso]

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, { })
```

[Patrick Angle]

Created attachment 430922 [details]
WIP v1.1 - First pass of review notes for EWS testing

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Devin Rousso]

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 :)

[Patrick Angle]

Created attachment 431141 [details]
Patch v2.0 - Revised node mapping strategy

[Patrick Angle]

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...

[Patrick Angle]

Created attachment 431155 [details]
Patch v2.1 - Add missing forward declaration

[Devin Rousso]

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`

[Patrick Angle]

Created attachment 431174 [details]
Patch v2.2 - Review notes

[Patrick Angle]

Created attachment 431219 [details]
Patch v2.3 - Add valid key check to InspectorDOMAgent::boundNodeId

[Devin Rousso]

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

[Patrick Angle]

Created attachment 431225 [details]
Patch v2.4 - Review notes

[EWS]

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].