RESOLVED FIXED 110424
Web Inspector: Cannot deep expand an element that has previously been partially expanded 
https://bugs.webkit.org/show_bug.cgi?id=110424
Summary Web Inspector: Cannot deep expand an element that has previously been partial...
Antoine Quint
Reported 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.
Attachments
Work-in-progress patch, no tests yet. (1.14 KB, patch)
2013-02-20 18:56 PST, Antoine Quint 		no flags
DetailsFormatted DiffDiff
Patch (7.33 KB, patch)
2013-02-25 23:14 PST, Antoine Quint 		pfeldman: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2013-02-20 18:38:26 PST
<rdar://problem/13260224>
Antoine Quint
Comment 2 2013-02-20 18:56:44 PST
Created attachment 189445 [details] Work-in-progress patch, no tests yet.
Pavel Feldman
Comment 3 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.
Antoine Quint
Comment 4 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?
Antoine Quint
Comment 5 2013-02-25 23:14:33 PST
Created attachment 190216 [details] Patch
Pavel Feldman
Comment 6 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.
Antoine Quint
Comment 7 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.
Antoine Quint
Comment 8 2013-02-26 08:00:34 PST
Landed in http://trac.webkit.org/changeset/144057.
Note You need to log in before you can comment on or make changes to this bug.