Bug 110921 – Web Inspector: allow referencing of nodes that have not been pushed to the front-end

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Antoine Quint]

In order to support https://bugs.webkit.org/show_bug.cgi?id=110407, the front-end will need to receive some information related to a node before that node is pushed, with a simple "backend node identifier" used to identify the node. The front-end should then be able to request the node with the given backend node id to be pushed to the front-end and subsequently, a node pushed to the front-end should list both its backend node id and its regular node id, as it will be necessary for the LayerTreeAgent to be able to identify a node for the given backend node id has now been pushed.

[Radar WebKit Bug Importer]

<rdar://problem/13300008>

[Antoine Quint]

Created attachment 190386 [details]
Patch

[Pavel Feldman]

Comment on attachment 190386 [details]
Patch

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

> Source/WebCore/inspector/Inspector.json:1732
> +                    { "name": "backendNodeId", "$ref": "BackendNodeId", "optional": true, "description": "Node identifier used to identify this node prior to being pushing to the front-end. This identifier expires as soon as the node is pushed and should only be used to update id references for nodes that have been referenced, prior to pushing, by a backend node identifier." }

Since you get a hold of Node object, it means that this id is useless. Why sending it here? Callers should manage them and tear them down in the callbacks on their own.

> Source/WebCore/inspector/InspectorDOMAgent.h:248
> +    HashMap<RefPtr<Node>, BackendNodeId> m_nodeToBackendIdMap;

You don't want to retain nodes by backend ids - eventually you blow the heap. There even is no way to dispose them. You should keep the raw pointer and clean it up upon node deletion.

[Antoine Quint]

(In reply to comment #3)
> (From update of attachment 190386 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190386&action=review
> 
> > Source/WebCore/inspector/Inspector.json:1732
> > +                    { "name": "backendNodeId", "$ref": "BackendNodeId", "optional": true, "description": "Node identifier used to identify this node prior to being pushing to the front-end. This identifier expires as soon as the node is pushed and should only be used to update id references for nodes that have been referenced, prior to pushing, by a backend node identifier." }
> 
> Since you get a hold of Node object, it means that this id is useless. Why sending it here? Callers should manage them and tear them down in the callbacks on their own.

I'm not sure how that can be done in all cases. Imagine you call pushNodeByBackendIdToFrontend which could be made to return the nodeId, what if an ancestor of that node has a backendId as well and get pushed in that call? How will the front-end be able to establish that node with backendNodeId -3 is the same as node with nodeId 22 that was pushed but not explicitly requested?

> 
> > Source/WebCore/inspector/InspectorDOMAgent.h:248
> > +    HashMap<RefPtr<Node>, BackendNodeId> m_nodeToBackendIdMap;
> 
> You don't want to retain nodes by backend ids - eventually you blow the heap. There even is no way to dispose them. You should keep the raw pointer and clean it up upon node deletion.

Will fix, thanks for catching this.

[Pavel Feldman]

> I'm not sure how that can be done in all cases. Imagine you call pushNodeByBackendIdToFrontend which could be made to return the nodeId, what if an ancestor of that node has a backendId as well and get pushed in that call? How will the front-end be able to establish that node with backendNodeId -3 is the same as node with nodeId 22 that was pushed but not explicitly requested?

Lets discard backendId upon explicit pushNodeByBackendIdToFrontend then? I am sure that most of the call sites would like to not think about whether node with given backend id was already sent to the front-end. They would not mind making a roundtrip for the conversion and would operate backend ids only.

[Antoine Quint]

(In reply to comment #5)
> > I'm not sure how that can be done in all cases. Imagine you call pushNodeByBackendIdToFrontend which could be made to return the nodeId, what if an ancestor of that node has a backendId as well and get pushed in that call? How will the front-end be able to establish that node with backendNodeId -3 is the same as node with nodeId 22 that was pushed but not explicitly requested?
> 
> Lets discard backendId upon explicit pushNodeByBackendIdToFrontend then? I am sure that most of the call sites would like to not think about whether node with given backend id was already sent to the front-end. They would not mind making a roundtrip for the conversion and would operate backend ids only.

So we wouldn't have a backendNodeId member on the Node object sent to the front-end and instead calling pushNodeByBackendIdToFrontend on a node that has already been pushed would just return the nodeId and then discard the backendNodeId. Am I reading this right? This sounds good to me.

[Antoine Quint]

Created attachment 190757 [details]
Patch

[Timothy Hatcher]

Comment on attachment 190757 [details]
Patch

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

I don't see the code to clean up m_backendIdToNode when Node is removed. Something in InspectorDOMAgent::unbind is likely needed.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1746
> +    if (Node * node = m_backendIdToNode.get(backendNodeId)) {

Style: Node* node = ...

[Pavel Feldman]

Comment on attachment 190757 [details]
Patch

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

>> Source/WebCore/inspector/InspectorDOMAgent.cpp:1746
>> +    if (Node * node = m_backendIdToNode.get(backendNodeId)) {
> 
> Style: Node* node = ...

What if the node was deleted? You should clean up your binding upon node deletion.

[Antoine Quint]

Created attachment 190769 [details]
Patch

[Antoine Quint]

(In reply to comment #9)
> (From update of attachment 190757 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190757&action=review
> 
> >> Source/WebCore/inspector/InspectorDOMAgent.cpp:1746
> >> +    if (Node * node = m_backendIdToNode.get(backendNodeId)) {
> > 
> > Style: Node* node = ...
> 
> What if the node was deleted? You should clean up your binding upon node deletion.

I think this is addressed in the newer patch where we check for a backendNodeId for the node passed to unbind().

[Pavel Feldman]

Comment on attachment 190769 [details]
Patch

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

> Source/WebCore/inspector/InspectorDOMAgent.cpp:324
> +    if (m_nodeToBackendIdMap.contains(node)) {

This method will only call itself recursively in case of mapped nodes / childrenRequested, etc. At the same time, your node might be in the removed subtree.

There is a couple of solutions I see depending on what operation you want to be fast:
1) in case backend map is not empty, traverse the entire subtree being removed here
2) when backendId is being created, generate backendids for the entire path from node to root. Then know for sure whether you need to traverse nodes here.

[Antoine Quint]

This is no longer relevant, https://bugs.webkit.org/show_bug.cgi?id=110407 will be addressed differently.

[Dmitry Gozman]

Reopening to attach new patch.

[Dmitry Gozman]

Created attachment 195552 [details]
Patch

[Dmitry Gozman]

This will be needed for https://bugs.webkit.org/show_bug.cgi?id=113398.
Patch rebaselined, and comment addressed. Please, take a look.

(In reply to comment #12)
> (From update of attachment 190769 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190769&action=review
> 
> > Source/WebCore/inspector/InspectorDOMAgent.cpp:324
> > +    if (m_nodeToBackendIdMap.contains(node)) {
> 
> This method will only call itself recursively in case of mapped nodes / childrenRequested, etc. At the same time, your node might be in the removed subtree.
> 
> There is a couple of solutions I see depending on what operation you want to be fast:
> 1) in case backend map is not empty, traverse the entire subtree being removed here
> 2) when backendId is being created, generate backendids for the entire path from node to root. Then know for sure whether you need to traverse nodes here.

Used option 2 here. I do generate backendId for all parents of the node, and so I'm guaranteed that recursive unbind will process all nodes with backendId.

[Dmitry Gozman]

Created attachment 195555 [details]
Patch

[Pavel Feldman]

Comment on attachment 195555 [details]
Patch

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

We need to test this.

> Source/WebCore/inspector/Inspector.json:1722
> +                "description": "Unique DOM node identifier used to reference a node that may not have been pushed to the front-end."

"hidden": true

> Source/WebCore/inspector/InspectorDOMAgent.cpp:326
> +        m_backendIdToNode.remove(backendId);

Thinking about the future... If we want to re-use ids after say timeline was stopped, we might want these nodes retained. I.e. we might want to have a named object group that will be discarded either on demand or when all its backend ids are resolved to node ids. Just thinking out loud.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:353
> +    if (nodesMap->contains(node))

You don't need to change this.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:630
> +        current = innerParentNode(current);

You don't need to retain parent path.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1762
> +        *nodeId = pushNodePathToFrontend(node);

You should discard backend id at this point (clean up the map)

[Andrey Kosyakov]

Comment on attachment 195555 [details]
Patch

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

> Source/WebCore/inspector/InspectorDOMAgent.cpp:627
> +        BackendNodeId id = m_lastBackendNodeId--;

nit: it's either --m_lastBackendNodeId, or m_nextBackendNodeId--

[Dmitry Gozman]

Created attachment 195749 [details]
Patch

[Dmitry Gozman]

Comment on attachment 195555 [details]
Patch

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

>> Source/WebCore/inspector/Inspector.json:1722
>> +                "description": "Unique DOM node identifier used to reference a node that may not have been pushed to the front-end."
> 
> "hidden": true

Done.

>> Source/WebCore/inspector/InspectorDOMAgent.cpp:627
>> +        BackendNodeId id = m_lastBackendNodeId--;
> 
> nit: it's either --m_lastBackendNodeId, or m_nextBackendNodeId--

Done.

[Pavel Feldman]

Comment on attachment 195749 [details]
Patch

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

> Source/WebCore/inspector/InspectorDOMAgent.cpp:631
> +void InspectorDOMAgent::releaseBackendIdsForNodeGroup(const String& nodeGroup)

This method should be exposed via the protocol.

> Source/WebCore/inspector/InspectorDOMAgent.h:250
> +    HashMap<String, OwnPtr<NodeToBackendIdMap> > m_nodeGroupToBackendIdMap;

Lets store by value instead.

[Dmitry Gozman]

Created attachment 195968 [details]
Patch

[Dmitry Gozman]

Comment on attachment 195749 [details]
Patch

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

>> Source/WebCore/inspector/InspectorDOMAgent.cpp:631
>> +void InspectorDOMAgent::releaseBackendIdsForNodeGroup(const String& nodeGroup)
> 
> This method should be exposed via the protocol.

Done.

>> Source/WebCore/inspector/InspectorDOMAgent.h:250
>> +    HashMap<String, OwnPtr<NodeToBackendIdMap> > m_nodeGroupToBackendIdMap;
> 
> Lets store by value instead.

Done.

[Pavel Feldman]

Comment on attachment 195968 [details]
Patch

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

> Source/WebCore/inspector/Inspector.json:2022
> +                "name": "releaseBackendIdsForNodeGroup",

releaseBackendNodeIds

> Source/WebCore/inspector/InspectorDOMAgent.cpp:616
> +        m_nodeGroupToBackendIdMap.set(nodeGroup, NodeToBackendIdMap());

You don't need this, object will be created automatically upon access.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:619
> +    BackendNodeId id = map.get(node);

id = m_nodeGroupToBackendIdMap[nodeGroup].get(node);

> Source/WebCore/inspector/InspectorDOMAgent.cpp:623
> +        m_backendIdToNode.set(id, std::make_pair(node, nodeGroup));

m_nodeGroupToBackendIdMap[nodeGroup].set(id, ...);

> Source/WebCore/inspector/InspectorDOMAgent.cpp:632
> +        NodeToBackendIdMap& map = *m_nodeGroupToBackendIdMap.find(nodeGroup).values();

ditto

[Pavel Feldman]

Comment on attachment 195968 [details]
Patch

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

> Source/WebCore/inspector/InspectorDOMAgent.cpp:618
> +    NodeToBackendIdMap& map = *m_nodeGroupToBackendIdMap.find(nodeGroup).values();

"values()" -> "->value"

[Dmitry Gozman]

Comment on attachment 195968 [details]
Patch

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

>> Source/WebCore/inspector/Inspector.json:2022
>> +                "name": "releaseBackendIdsForNodeGroup",
> 
> releaseBackendNodeIds

Done.

>> Source/WebCore/inspector/InspectorDOMAgent.cpp:618
>> +    NodeToBackendIdMap& map = *m_nodeGroupToBackendIdMap.find(nodeGroup).values();
> 
> "values()" -> "->value"

Done.

[Dmitry Gozman]

Created attachment 196110 [details]
Patch

[WebKit Review Bot]

Comment on attachment 196110 [details]
Patch

Clearing flags on attachment: 196110

Committed r147428: <http://trac.webkit.org/changeset/147428>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.