Bug 56334 - Web Inspector: crash upon "//html//@id" search in elements panel.
Summary: Web Inspector: crash upon "//html//@id" search in elements panel.
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: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-03-14 14:10 PDT by Pavel Feldman
Modified: 2011-03-15 07:13 PDT (History)
10 users (show)

See Also:

Attachments
Patch (4.55 KB, patch)
2011-03-14 14:18 PDT, Pavel Feldman 		no flags Details | Formatted Diff | Diff
[PATCH] Review comments addressed. (4.69 KB, patch)
2011-03-14 23:43 PDT, Pavel Feldman 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Feldman 2011-03-14 14:10:52 PDT 
We can't return Attr results / highlight them.
Comment 1 Pavel Feldman 2011-03-14 14:18:15 PDT 
Created attachment 85719 [details]
Patch
Comment 2 Joseph Pecoraro 2011-03-14 14:31:12 PDT 
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.
Comment 3 Alexey Proskuryakov 2011-03-14 16:52:21 PDT 
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.
Comment 4 Pavel Feldman 2011-03-14 23:21:11 PDT 
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.
Comment 5 Pavel Feldman 2011-03-14 23:43:21 PDT 
Created attachment 85780 [details]
[PATCH] Review comments addressed.
Comment 6 Adam Barth 2011-03-15 02:01:19 PDT 
Comment on attachment 85780 [details]
[PATCH] Review comments addressed.

/me is super excited about the testing!
Comment 7 Pavel Feldman 2011-03-15 07:13:27 PDT 
Comment on attachment 85780 [details]
[PATCH] Review comments addressed.

Clearing flags on attachment: 85780

Committed r81132: <http://trac.webkit.org/changeset/81132>
Comment 8 Pavel Feldman 2011-03-15 07:13:35 PDT 
All reviewed patches have been landed.  Closing bug.