Summary: | [Qt] Plugins : QGVLauncher crashes on exit | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Girish Ramakrishnan <girish> | ||||||||
Component: | Plug-ins | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, kenneth, tonikitoo | ||||||||
Priority: | P2 | Keywords: | Qt | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | Linux | ||||||||||
URL: | www.cnn.com | ||||||||||
Bug Depends on: | 30385 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Girish Ramakrishnan
2009-10-14 04:36:27 PDT
girish, could you please fill the "URL" field w/ a known crashing url .. i have a fix for it, i believe. it works here, implementing an auto tests now. This is sort of related to 30385. This bug deals with the case where the page has plugins. The problem is that when the view dies, it takes the platformPluginWidget along with it. But the page is still *alive* and hence PluginView is still alive. So, the problem is that the platformPluginWidget pointer in PluginView is stale. Created attachment 41284 [details]
Patch so far
Identifies one of the problems. The test works. But there is still a bad
pointer somewhere (qgvlauncher still crashes).
Created attachment 41374 [details] Various fixes Fixes various sources of crashes. 1. This commit depends on the resource file (test.swf) that will get added as part of https://bugs.webkit.org/show_bug.cgi?id=30380. 2. The crash is fixed because the page now gets deleted as a part of r49730. QtWebKit crashes if page is not deleted :/ I have opened 30490 to track this. (In reply to comment #4) > Created an attachment (id=41374) [details] > Various fixes > > Fixes various sources of crashes. > > 1. This commit depends on the resource file (test.swf) that will get added as > part of https://bugs.webkit.org/show_bug.cgi?id=30380. > 2. The crash is fixed because the page now gets deleted as a part of r49730. > QtWebKit crashes if page is not deleted :/ I have opened 30490 to track this. - if (d->page) + if (d->page) { d->page->d->view = 0; + d->page->d->client = 0; + } this has landed already, girish. great autotests + 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. you could make it a separate 'if' and add a comment explaining why that is done. > > - if (d->page) > + if (d->page) { > d->page->d->view = 0; > + d->page->d->client = 0; > + } > > this has landed already, girish. Nope, not that I can see (till r49764). The change was only made to qgraphicswebview destructor but not to qwebview (my change is in qwebview). (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. > > you could make it a separate 'if' and add a comment explaining why that is > done. Looks like you interchanged comments. This comment is meant for #30380 :-). I will reply on that task. From https://bugs.webkit.org/show_bug.cgi?id=30380#c8. (In reply to comment #8) > 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? Good idea for adding a manual-test! In fact, I already have a bunch of html fields that I have used for testing. I will clean them up. I have created https://bugs.webkit.org/show_bug.cgi?id=30503 since I had lots of ideas for it. Please comment there. Comment on attachment 41374 [details] Various fixes > QWebView::~QWebView() > { > - if (d->page) > + if (d->page) { > d->page->d->view = 0; > + d->page->d->client = 0; > + } As discussed on IRC this might have a bad side effect if someone is doing. view1->setPage(page) view2->setPage(page) delete view1; but we want to solve it by adding a warning to QWebView::setPage. > + if (html.contains("</embed>")) { > + QTest::qWait(2000); // some reasonable time for the PluginStream to feed test.swf to flash > + } I fear our coding style guidelines wants you to omit the curly braces here. This needs to be fixed when landing. In general it would be nice if we could wait for things like QEvent::UpdateRequest, QEvent::ShowEvent instead of the qWait but in practive we can not. It is making the test a bit fragile but I see it is the best we can do right now. I'm leaving the commit-queue to "?" as this test wants a resource introduced by another patch and the patch needs a tweak. Created attachment 41409 [details]
Various fixes (2)
Incorp. holger's comments.
Comment on attachment 41409 [details]
Various fixes (2)
r=me. The swf file will be there with the other commit.
Comment on attachment 41409 [details] Various fixes (2) Clearing flags on attachment: 41409 Committed r49770: <http://trac.webkit.org/changeset/49770> All reviewed patches have been landed. Closing bug. |