Bug 30380 – [Qt] Windowed Plugins : Crash when using QWebPage without a QWebview (i.e no QWebPageClient)

• Status: RESOLVED
• Resolution: FIXED
• Component: Plug-ins

[Girish Ramakrishnan]

Crash when taking snapshots of pages with flash in them.

[Girish Ramakrishnan]

Created attachment 41212 [details]
Don't crash when client is 0

[Girish Ramakrishnan]

It's a good idea to add a test for this. review-, please :-) I will add a test shortly.

[Simon Hausmann]

Comment on attachment 41212 [details]
Don't crash when client is 0


> -    if (!m_isStarted || !parent() || !m_plugin->pluginFuncs()->setwindow)
> +    if (!m_isStarted || !parent() || !m_plugin->pluginFuncs()->setwindow || m_status != PluginStatusLoadedSuccessfully)
>          return;

This hunk appears unrelated to the existance of the page client. Is it intentionally included but not mentioned in the ChangeLog?

[Girish Ramakrishnan]

(In reply to comment #3)
> (From update of attachment 41212 [details])
> 
> > -    if (!m_isStarted || !parent() || !m_plugin->pluginFuncs()->setwindow)
> > +    if (!m_isStarted || !parent() || !m_plugin->pluginFuncs()->setwindow || m_status != PluginStatusLoadedSuccessfully)
> >          return;
> 
> This hunk appears unrelated to the existance of the page client. Is it
> intentionally included but not mentioned in the ChangeLog?

I have updated the ChangeLog to explain it's purpose.

[Girish Ramakrishnan]

Created attachment 41235 [details]
Don't crash when client is 0 (2)

The patch adds a binary test.swf (copied from some test in WebCore/*). I have placed this flash file under tests/resources since I intend to also add other tests for qwebview.

Not sure, if the patch file works well for binaries. I have also pushed it to http://gitorious.org/~girish/qtwebkit/girishs-clone/commits/windowed_plugins_noclient_30380.

[Girish Ramakrishnan]

Note that the tests don't QVERIFY or QCOMPARE :-) Without this fix, they will segfault.

[Girish Ramakrishnan]

Created attachment 41371 [details]
Don't crash when client is 0 (3)

Updated based on Antonio Gomes (tonikitoo) comment at https://bugs.webkit.org/show_bug.cgi?id=30355#c3

[Kenneth Rohde Christiansen]

Maybe it makes sense making a manual-test as well, so that other ports as GTK can use it as well.

There is already a directory manual-tests somewhere with subdirs qt and gtk. Maybe we should add a linux subdir instead and put all plugin manual tests there?

[Girish Ramakrishnan]

From https://bugs.webkit.org/show_bug.cgi?id=30354#c6, 
(In reply to comment #6)
> +        In addition, setFrameRect()/updatePluginWidget() is called even if the
> +        plugin was not succesfully loaded. So, a status check is added to 
> +        setNPWindowIfNeeded.
> 
> Maybe it makes more sense adding the (m_status !=
> PluginStatusLoadedSuccessfully) there then? Currently with your patch it is not
> obvious why you have that test.
> 

updatePluginWidget() updates the windowrect among other things. We can use this for painting, if we wanted to paint plugins that didn't load successfully. The intent is that we don't want to call setwindow since the plugin never loaded. So, setNPWindowIfNeeded seems correct.

> you could make it a separate 'if' and add a comment explaining why that is
> done.

Agree.

[Girish Ramakrishnan]

Created attachment 41403 [details]
Don't crash when client is 0 (4)

Update after Kenneth's comment - Make it a separate if statement and add comment.

[Girish Ramakrishnan]

Created attachment 41404 [details]
Don't crash when client is 0 (5)

Oops. Two mistakes in previous patch - the bug id in changelog is wrong. And I forgot to add test.swf.

I have also pushed changes to gitorious. http://gitorious.org/~girish/qtwebkit/girishs-clone/commits/windowed_plugins_noclient_30380

[Girish Ramakrishnan]

Created attachment 41406 [details]
Don't crash when client is 0 (6)

After Holger (zecke) review:
1. Add ChangeLog note that test.swf was copied from existing WebKit test.swf
2. Update comment for QTest::qWait();

[Girish Ramakrishnan]

Created attachment 41408 [details]
Don't crash when client is 0 (7)

Use prepare-ChangeLog to detect test.swf was copied

[Holger Freyther]

Comment on attachment 41408 [details]
Don't crash when client is 0 (7)

r=me

[WebKit Commit Bot]

Comment on attachment 41408 [details]
Don't crash when client is 0 (7)

Rejecting patch 41408 from commit-queue.

Patch https://bugs.webkit.org/attachment.cgi?id=41408 from bug 30380 failed to download and apply.

[Girish Ramakrishnan]

Patch landed in r49771.