Bug 63023 – [chromium] Searching may cause a segmentation fault in WebPluginDocument

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Andrew Russell]

Searching may cause a segmentation fault in WebPluginDocument

[Andrew Russell]

Created attachment 97878 [details]
Patch

[Andrew Russell]

Created attachment 97882 [details]
Patch

[Andrew Russell]

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.

[Dimitri Glazkov (Google)]

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 :)

[Andrew Russell]

Created attachment 98070 [details]
Patch

[Andrew Russell]

(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.

[Dimitri Glazkov (Google)]

(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?

[Andrew Russell]

(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.

[Dimitri Glazkov (Google)]

(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.

[Andrew Russell]

Created attachment 100037 [details]
Patch

[Andrew Russell]

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.

[Andrew Russell]

Created attachment 100045 [details]
Patch

[Andrew Russell]

Updated the patch so that it works with the trunk of WebKit.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Comment on attachment 100045 [details]
Patch

I guess this patch is abandoned?