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
: WebKit
Plug-ins
: 528+ (Nightly build)
: PC Linux
: P2 Normal
Assigned To:
: http://gitorious.org/~girish/qtwebkit...
: Qt
:
:
  Show dependency treegraph
 
Reported: 2009-10-14 23:59 PST by
Modified: 2009-10-19 04:31 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-10-14 23:59:59 PST
Crash when taking snapshots of pages with flash in them.
------- Comment #1 From 2009-10-15 00:01:28 PST -------
Created an attachment (id=41212) [details]
Don't crash when client is 0
------- Comment #2 From 2009-10-15 06:07:03 PST -------
It's a good idea to add a test for this. review-, please :-) I will add a test shortly.
------- Comment #3 From 2009-10-15 10:41:03 PST -------
(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?
------- Comment #4 From 2009-10-15 10:51:05 PST -------
(In reply to comment #3)
> (From update of attachment 41212 [details] [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 From 2009-10-15 10:54:20 PST -------
Created an attachment (id=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 From 2009-10-15 11:02:59 PST -------
Note that the tests don't QVERIFY or QCOMPARE :-) Without this fix, they will segfault.
------- Comment #7 From 2009-10-17 23:13:50 PST -------
Created an attachment (id=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 From 2009-10-18 08:46:38 PST -------
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 From 2009-10-19 02:13:59 PST -------
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 From 2009-10-19 02:15:51 PST -------
Created an attachment (id=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 From 2009-10-19 02:22:10 PST -------
Created an attachment (id=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 From 2009-10-19 02:56:42 PST -------
Created an attachment (id=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 From 2009-10-19 03:05:52 PST -------
Created an attachment (id=41408) [details]
Don't crash when client is 0 (7)

Use prepare-ChangeLog to detect test.swf was copied
------- Comment #14 From 2009-10-19 03:35:39 PST -------
(From update of attachment 41408 [details])
r=me
------- Comment #15 From 2009-10-19 03:36:49 PST -------
(From update of attachment 41408 [details])
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 From 2009-10-19 04:31:03 PST -------
Patch landed in r49771.