Bug 110424 - Web Inspector: Cannot deep expand an element that has previously been partially expanded
Summary: Web Inspector: Cannot deep expand an element that has previously been partial...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-02-20 18:37 PST by Antoine Quint
Modified: 2013-02-26 08:00 PST (History)
11 users (show)

See Also:


Attachments
Work-in-progress patch, no tests yet. (1.14 KB, patch)
2013-02-20 18:56 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (7.33 KB, patch)
2013-02-25 23:14 PST, Antoine Quint
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2013-02-20 18:37:47 PST
While we added support for true deep-expansion with https://bugs.webkit.org/show_bug.cgi?id=66868, there is (at least) one case where this fails.  In InspectorDOMAgent::pushChildNodesToFrontend(), if we find that we'd already requested children for the given node, we return early:

    if (m_childrenRequested.contains(nodeId))
        return;

This could easily happen for the <body> or <html> elements, or basically any element that you inspect directly to open the inspector.
Comment 1 Radar WebKit Bug Importer 2013-02-20 18:38:26 PST
<rdar://problem/13260224>
Comment 2 Antoine Quint 2013-02-20 18:56:44 PST
Created attachment 189445 [details]
Work-in-progress patch, no tests yet.
Comment 3 Pavel Feldman 2013-02-20 22:44:29 PST
Comment on attachment 189445 [details]
Work-in-progress patch, no tests yet.

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

> Source/WebCore/inspector/InspectorDOMAgent.cpp:453
> +        node = node->firstChild();

for (node = innerFirstChild(node); node; node = innerNextSibling(node)) {
...

> Source/WebCore/inspector/InspectorDOMAgent.cpp:455
> +            nodeId = m_documentNodeToIdMap.get(node);

Dangling nodes (the ones printed with console.log(document)) will end up with nodes being a part of another NodeToIdMap instance. m_idToNodesMap knows which map to use.

Please do not reuse nodeId, this one is about child node. childNodeId

> Source/WebCore/inspector/InspectorDOMAgent.cpp:456
> +            if (nodeId)

Should be an assertion. We know that m_childrenRequested.contains(nodeId). Hence we've sent its children.
Comment 4 Antoine Quint 2013-02-25 22:22:31 PST
(In reply to comment #3)
> (From update of attachment 189445 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189445&action=review
> 
> > Source/WebCore/inspector/InspectorDOMAgent.cpp:455
> > +            nodeId = m_documentNodeToIdMap.get(node);
> 
> Dangling nodes (the ones printed with console.log(document)) will end up with nodes being a part of another NodeToIdMap instance. m_idToNodesMap knows which map to use.

Should I simply move the `NodeToIdMap* nodeMap = m_idToNodesMap.get(nodeId);` call to before I enter the `m_childrenRequested.contains(nodeId)` condition and used this nodeMap to get the childNodeId?
Comment 5 Antoine Quint 2013-02-25 23:14:33 PST
Created attachment 190216 [details]
Patch
Comment 6 Pavel Feldman 2013-02-26 00:36:14 PST
Comment on attachment 190216 [details]
Patch

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

> Source/WebCore/inspector/InspectorDOMAgent.cpp:457
> +            childNodeId = nodeMap->get(node);

you never use childNodeId outside for scope - declare it inline.
Comment 7 Antoine Quint 2013-02-26 08:00:12 PST
(In reply to comment #6)
> (From update of attachment 190216 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190216&action=review
> 
> > Source/WebCore/inspector/InspectorDOMAgent.cpp:457
> > +            childNodeId = nodeMap->get(node);
> 
> you never use childNodeId outside for scope - declare it inline.

Made the change in commit. Thanks for the review Pavel.
Comment 8 Antoine Quint 2013-02-26 08:00:34 PST
Landed in http://trac.webkit.org/changeset/144057.