Bug 56334 – Web Inspector: crash upon "//html//@id" search in elements panel.

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

[Pavel Feldman]

We can't return Attr results / highlight them.

[Pavel Feldman]

Created attachment 85719 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 85719 [details]
Patch

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

> Source/WebCore/inspector/InspectorDOMAgent.cpp:212
> +            if (node->nodeType() == Node::ATTRIBUTE_NODE)
> +                node = static_cast<Attr*>(node)->ownerElement();
>              if (!ec)
>                  resultCollector.add(node);

This should probably go inside the (!ec) block. Or make it an early continue check. "if (ec) continue;" 

Are there other node types we should avoid here? Should we whitelist certain types?

Other then this, the patch looks good to me.

[Alexey Proskuryakov]

Comment on attachment 85719 [details]
Patch

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

> Source/WebCore/inspector/InspectorDOMAgent.cpp:211
>              if (!ec)

In WebCore, any code that looks at exception codes is responsible for initializing them. So, there should be "ec = 0;" inside the loop.

[Pavel Feldman]

Comment on attachment 85719 [details]
Patch

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

>> Source/WebCore/inspector/InspectorDOMAgent.cpp:211
>>              if (!ec)
> 
> In WebCore, any code that looks at exception codes is responsible for initializing them. So, there should be "ec = 0;" inside the loop.

I don't follow. There is ExceptionCode ec = 0; above. I quit as soon as I hit ec != 0. See the for condition.

>> Source/WebCore/inspector/InspectorDOMAgent.cpp:212
>>                  resultCollector.add(node);
> 
> This should probably go inside the (!ec) block. Or make it an early continue check. "if (ec) continue;" 
> 
> Are there other node types we should avoid here? Should we whitelist certain types?
> 
> Other then this, the patch looks good to me.

Good catch. I can't think of any other type - we are only interested in node types with broken "parentNode" link.

[Pavel Feldman]

Created attachment 85780 [details]
[PATCH] Review comments addressed.

[Adam Barth]

Comment on attachment 85780 [details]
[PATCH] Review comments addressed.

/me is super excited about the testing!

[Pavel Feldman]

Comment on attachment 85780 [details]
[PATCH] Review comments addressed.

Clearing flags on attachment: 85780

Committed r81132: <http://trac.webkit.org/changeset/81132>

[Pavel Feldman]

All reviewed patches have been landed.  Closing bug.