Bug 30380 - [Qt] Windowed Plugins : Crash when using QWebPage without a QWebview (i.e no QWebPageClient)
: [Qt] Windowed Plugins : Crash when using QWebPage without a QWebview (i.e no ...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Plug-ins
: 528+ (Nightly build)
: PC Linux
: P2 Normal
Assigned To: Nobody
http://gitorious.org/~girish/qtwebkit...
: Qt
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-10-14 23:59 PDT by Girish Ramakrishnan
Modified: 2009-10-19 04:31 PDT (History)
2 users (show)

See Also:


Attachments
Don't crash when client is 0 (2.08 KB, patch)
2009-10-15 00:01 PDT, Girish Ramakrishnan
no flags Details | Formatted Diff | Diff
Don't crash when client is 0 (2) (19.99 KB, patch)
2009-10-15 10:54 PDT, Girish Ramakrishnan
no flags Details | Formatted Diff | Diff
Don't crash when client is 0 (3) (19.88 KB, patch)
2009-10-17 23:13 PDT, Girish Ramakrishnan
no flags Details | Formatted Diff | Diff
Don't crash when client is 0 (4) (6.85 KB, patch)
2009-10-19 02:15 PDT, Girish Ramakrishnan
no flags Details | Formatted Diff | Diff
Don't crash when client is 0 (5) (20.29 KB, patch)
2009-10-19 02:22 PDT, Girish Ramakrishnan
no flags Details | Formatted Diff | Diff
Don't crash when client is 0 (6) (20.55 KB, patch)
2009-10-19 02:56 PDT, Girish Ramakrishnan
no flags Details | Formatted Diff | Diff
Don't crash when client is 0 (7) (20.41 KB, patch)
2009-10-19 03:05 PDT, Girish Ramakrishnan
zecke: review+
commit-queue: commit‑queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Girish Ramakrishnan 2009-10-14 23:59:59 PDT
Crash when taking snapshots of pages with flash in them.
Comment 1 Girish Ramakrishnan 2009-10-15 00:01:28 PDT
Created attachment 41212 [details]
Don't crash when client is 0
Comment 2 Girish Ramakrishnan 2009-10-15 06:07:03 PDT
It's a good idea to add a test for this. review-, please :-) I will add a test shortly.
Comment 3 Simon Hausmann 2009-10-15 10:41:03 PDT
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?
Comment 4 Girish Ramakrishnan 2009-10-15 10:51:05 PDT
(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.
Comment 5 Girish Ramakrishnan 2009-10-15 10:54:20 PDT
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.
Comment 6 Girish Ramakrishnan 2009-10-15 11:02:59 PDT
Note that the tests don't QVERIFY or QCOMPARE :-) Without this fix, they will segfault.
Comment 7 Girish Ramakrishnan 2009-10-17 23:13:50 PDT
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
Comment 8 Kenneth Rohde Christiansen 2009-10-18 08:46:38 PDT
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?
Comment 9 Girish Ramakrishnan 2009-10-19 02:13:59 PDT
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.
Comment 10 Girish Ramakrishnan 2009-10-19 02:15:51 PDT
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.
Comment 11 Girish Ramakrishnan 2009-10-19 02:22:10 PDT
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
Comment 12 Girish Ramakrishnan 2009-10-19 02:56:42 PDT
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();
Comment 13 Girish Ramakrishnan 2009-10-19 03:05:52 PDT
Created attachment 41408 [details]
Don't crash when client is 0 (7)

Use prepare-ChangeLog to detect test.swf was copied
Comment 14 Holger Freyther 2009-10-19 03:35:39 PDT
Comment on attachment 41408 [details]
Don't crash when client is 0 (7)

r=me
Comment 15 WebKit Commit Bot 2009-10-19 03:36:49 PDT
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.
Comment 16 Girish Ramakrishnan 2009-10-19 04:31:03 PDT
Patch landed in r49771.