Crash when taking snapshots of pages with flash in them.
Created attachment 41212 [details] Don't crash when client is 0
It's a good idea to add a test for this. review-, please :-) I will add a test shortly.
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?
(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.
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.
Note that the tests don't QVERIFY or QCOMPARE :-) Without this fix, they will segfault.
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
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?
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.
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.
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
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();
Created attachment 41408 [details] Don't crash when client is 0 (7) Use prepare-ChangeLog to detect test.swf was copied
Comment on attachment 41408 [details] Don't crash when client is 0 (7) r=me
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.
Patch landed in r49771.