Bug 63023 - [chromium] Searching may cause a segmentation fault in WebPluginDocument
Summary: [chromium] Searching may cause a segmentation fault in WebPluginDocument
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-20 15:58 PDT by Andrew Russell
Modified: 2013-04-11 14:40 PDT (History)
2 users (show)

See Also:


Attachments
Patch (1.19 KB, patch)
2011-06-20 16:03 PDT, Andrew Russell
no flags Details | Formatted Diff | Diff
Patch (1.19 KB, patch)
2011-06-20 16:06 PDT, Andrew Russell
no flags Details | Formatted Diff | Diff
Patch (1.45 KB, patch)
2011-06-21 15:48 PDT, Andrew Russell
no flags Details | Formatted Diff | Diff
Patch (7.46 KB, patch)
2011-07-07 15:07 PDT, Andrew Russell
no flags Details | Formatted Diff | Diff
Patch (7.47 KB, patch)
2011-07-07 16:07 PDT, Andrew Russell
eric: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Russell 2011-06-20 15:58:59 PDT
Searching may cause a segmentation fault in WebPluginDocument
Comment 1 Andrew Russell 2011-06-20 16:03:37 PDT
Created attachment 97878 [details]
Patch
Comment 2 Andrew Russell 2011-06-20 16:06:16 PDT
Created attachment 97882 [details]
Patch
Comment 3 Andrew Russell 2011-06-20 16:09:14 PDT
This happens when the embed object is removed from the DOM.  The WebPluginDocument will then segmentation fault since |container| is null.  Since extensions are able to modify the DOM of a WebPluginPage, this could happen to a user without them knowing what has just occurred.
Comment 4 Dimitri Glazkov (Google) 2011-06-21 15:18:14 PDT
Comment on attachment 97882 [details]
Patch

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

The fix seems right, but why no test?

> Source/WebKit/chromium/ChangeLog:5
> +        Searching may cause a segmentation fault in WebPluginDocument

This is way too sparse and cryptic to be useful. Can you perhaps enrich this a bit with explanation of how this happens and why this is the right fix? The commit archeologists thank you in advance :)
Comment 5 Andrew Russell 2011-06-21 15:48:00 PDT
Created attachment 98070 [details]
Patch
Comment 6 Andrew Russell 2011-06-21 16:21:21 PDT
(In reply to comment #4)
> (From update of attachment 97882 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97882&action=review
> 
> The fix seems right, but why no test?
> 

Since this is caused by searching from the browser, this code requires Chromium to call the function which causes the segmentation fault. I do not believe that there is a way to duplicate this functionality with just HTML.
Comment 7 Dimitri Glazkov (Google) 2011-06-28 08:10:31 PDT
(In reply to comment #6)
> (In reply to comment #4)
> > (From update of attachment 97882 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=97882&action=review
> > 
> > The fix seems right, but why no test?
> > 
> 
> Since this is caused by searching from the browser, this code requires Chromium to call the function which causes the segmentation fault. I do not believe that there is a way to duplicate this functionality with just HTML.

Could you possibly use layoutTestController.findString?
Comment 8 Andrew Russell 2011-06-28 11:36:37 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #4)
> > > (From update of attachment 97882 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=97882&action=review
> > > 
> > > The fix seems right, but why no test?
> > > 
> > 
> > Since this is caused by searching from the browser, this code requires Chromium to call the function which causes the segmentation fault. I do not believe that there is a way to duplicate this functionality with just HTML.
> 
> Could you possibly use layoutTestController.findString?

layoutTestController.findString is undefined in the chromium environment, and since this bug is in chromium code, I do not see the point in using it.
Comment 9 Dimitri Glazkov (Google) 2011-06-28 11:38:48 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (In reply to comment #4)
> > > > (From update of attachment 97882 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=97882&action=review
> > > > 
> > > > The fix seems right, but why no test?
> > > > 
> > > 
> > > Since this is caused by searching from the browser, this code requires Chromium to call the function which causes the segmentation fault. I do not believe that there is a way to duplicate this functionality with just HTML.
> > 
> > Could you possibly use layoutTestController.findString?
> 
> layoutTestController.findString is undefined in the chromium environment, and since this bug is in chromium code, I do not see the point in using it.

Ah! This sounds like an excellent patch then. Expose findString and write test for it using this bug.
Comment 10 Andrew Russell 2011-07-07 15:07:20 PDT
Created attachment 100037 [details]
Patch
Comment 11 Andrew Russell 2011-07-07 15:10:11 PDT
I have uploaded a new patch that includes tests for this change.

I did not add the `findString` method to the layoutTestController since I would have needed to change a large amount of the chromium DumpRenderTree code to allow searching through plugins and did not think it was worth the effort.

Instead, I created a simpler method, `removePlugin`, that removes the plugin from the WebPluginDocument, and then makes sure that it has been removed.  The test will crash without this bug fix, and the test passes with this bug fix.
Comment 12 Andrew Russell 2011-07-07 16:07:49 PDT
Created attachment 100045 [details]
Patch
Comment 13 Andrew Russell 2011-07-07 16:10:09 PDT
Updated the patch so that it works with the trunk of WebKit.
Comment 14 Eric Seidel (no email) 2011-09-12 15:45:02 PDT
Comment on attachment 100045 [details]
Patch

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

> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:754
> +    // Search through all of the frames and check if any of them contain a WebPluginDocument.
> +    bool pluginRemoved = false;
> +    while (frame) {
> +        if (frame->document().isPluginDocument() && frame->document().to<WebPluginDocument>().plugin()) {
> +            frame->executeScript(WebScriptSource(WebString::fromUTF8("document.body.innerHTML='';")));
> +            if (!frame->document().to<WebPluginDocument>().plugin())
> +                pluginRemoved = true;
> +            break;
> +        }
> +        frame = frame->traverseNext(false);
> +    }
> +    result->set(pluginRemoved);

Although I'm very glad you added a test (even adding a method on layout test controller!) I'm not sure why this method is needed (can't we remove the plugin in some other way via JS?)  It also would make more sense to me if it took some sort of fram that it was supposed to remove the plugin from.
Comment 15 Eric Seidel (no email) 2012-02-16 14:02:48 PST
Comment on attachment 100045 [details]
Patch

I guess this patch is abandoned?