Bug 30354 - [Qt] Plugins : QGVLauncher crashes on exit
Summary: [Qt] Plugins : QGVLauncher crashes on exit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL: www.cnn.com
Keywords: Qt
Depends on: 30385
Blocks:
  Show dependency treegraph
 
Reported: 2009-10-14 04:36 PDT by Girish Ramakrishnan
Modified: 2009-10-19 03:44 PDT (History)
3 users (show)

See Also:


Attachments
Patch so far (4.96 KB, patch)
2009-10-16 06:27 PDT, Girish Ramakrishnan
no flags Details | Formatted Diff | Diff
Various fixes (7.42 KB, patch)
2009-10-18 00:58 PDT, Girish Ramakrishnan
zecke: review+
Details | Formatted Diff | Diff
Various fixes (2) (7.45 KB, patch)
2009-10-19 03:16 PDT, Girish Ramakrishnan
no flags 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 04:36:27 PDT
When QGVLauncher is used with a site that uses plugins, it crashes on exit. e.g. QGVLauncher communitymx.com/content/source/E5141/wmodetrans.htm.

Backtrace

#0  0x00000028 in ?? ()
No symbol table info available.
#1  0xb6e043a1 in WebCore::Chrome::repaint (this=0x94be138, windowRect=@0xbffa0a3c, contentChanged=true, immediate=false, repaintContentOnly=false)
    at ../../../WebCore/page/Chrome.cpp:72
No locals.
#2  0xb6ede13c in WebCore::ScrollView::repaintContentRectangle (this=0x95e36b8, rect=@0xbffa0b84, now=false) at ../../../WebCore/platform/ScrollView.cpp:721
	visibleContent = {m_location = {m_x = 61, m_y = 58}, m_size = {m_width = 161, m_height = 81}}
#3  0xb6e4d0ba in WebCore::FrameView::repaintContentRectangle (this=0x95e36b8, r=@0xbffa0b84, immediate=false) at ../../../WebCore/page/FrameView.cpp:863
	delay = 0
	__PRETTY_FUNCTION__ = "virtual void WebCore::FrameView::repaintContentRectangle(const WebCore::IntRect&, bool)"
#4  0xb701acf4 in WebCore::RenderView::repaintViewRectangle (this=0x94deaa4, ur=@0xbffa0b84, immediate=false)
    at ../../../WebCore/rendering/RenderView.cpp:228
	elt = (class WebCore::Element *) 0x0
#5  0xb6fcf273 in WebCore::RenderObject::repaintUsingContainer (this=0x96813bc, repaintContainer=0x94deaa4, r=@0xbffa0b84, immediate=false)
    at ../../../WebCore/rendering/RenderObject.cpp:1117
	v = (class WebCore::RenderView *) 0x94deaa4
	__PRETTY_FUNCTION__ = "void WebCore::RenderObject::repaintUsingContainer(WebCore::RenderBoxModelObject*, const WebCore::IntRect&, bool)"
#6  0xb6fd2748 in WebCore::RenderObject::repaintRectangle (this=0x96813bc, r=@0xbffa0bc8, immediate=false)
    at ../../../WebCore/rendering/RenderObject.cpp:1163
	view = (class WebCore::RenderView *) 0x94deaa4
	dirtyRect = {m_location = {m_x = 61, m_y = 58}, m_size = {m_width = 161, m_height = 81}}
	repaintContainer = (class WebCore::RenderBoxModelObject *) 0x0
#7  0xb6f1394f in WebCore::PluginView::invalidateWindowlessPluginRect (this=0x97207d8, rect=@0xbffa0c10) at ../../../WebCore/plugins/PluginView.cpp:1178
	renderer = (class WebCore::RenderBox *) 0x96813bc
	dirtyRect = {m_location = {m_x = 50, m_y = 12}, m_size = {m_width = 161, m_height = 81}}
#8  0xb70f19da in WebCore::PluginView::invalidateRect (this=0x97207d8, rect=0xbffa0c8c) at ../../../WebCore/plugins/qt/PluginViewQt.cpp:658
	r = {m_location = {m_x = 50, m_y = 12}, m_size = {m_width = 161, m_height = 81}}
#9  0xb70ef98b in NPN_InvalidateRect (instance=0x9720930, invalidRect=0xbffa0c8c) at ../../../WebCore/plugins/npapi.cpp:124
No locals.
#10 0xb13bf7a5 in ?? () from /usr/lib/adobe-flashplugin/libflashplayer.so
No symbol table info available.
#11 0xb13c2c7c in ?? () from /usr/lib/adobe-flashplugin/libflashplayer.so
No symbol table info available.
#12 0xb16f8052 in ?? () from /usr/lib/adobe-flashplugin/libflashplayer.so
No symbol table info available.
#13 0xb13c2ccf in ?? () from /usr/lib/adobe-flashplugin/libflashplayer.so
No symbol table info available.
---Type <return> to continue, or q <return> to quit---
#14 0xb17447a4 in ?? () from /usr/lib/adobe-flashplugin/libflashplayer.so
No symbol table info available.
#15 0xb13d8731 in ?? () from /usr/lib/adobe-flashplugin/libflashplayer.so
No symbol table info available.
#16 0xb13c2ae3 in ?? () from /usr/lib/adobe-flashplugin/libflashplayer.so
No symbol table info available.
#17 0xb3c652b6 in g_timeout_dispatch (source=0x9461240, callback=0xbffa0980, user_data=0xb01ec000) at /build/buildd/glib2.0-2.20.1/glib/gmain.c:3253
No locals.
#18 0xb3c64b88 in IA__g_main_context_dispatch (context=0x9427490) at /build/buildd/glib2.0-2.20.1/glib/gmain.c:1814
No locals.
#19 0xb3c680eb in g_main_context_iterate (context=0x9427490, block=1, dispatch=1, self=0x9424e78) at /build/buildd/glib2.0-2.20.1/glib/gmain.c:2448
	max_priority = 0
	timeout = 0
	some_ready = 1
	nfds = 12
	allocated_nfds = <value optimized out>
	fds = (GPollFD *) 0x9711cc8
	__PRETTY_FUNCTION__ = "g_main_context_iterate"
#20 0xb3c68268 in IA__g_main_context_iteration (context=0x9427490, may_block=1) at /build/buildd/glib2.0-2.20.1/glib/gmain.c:2511
	retval = <value optimized out>
#21 0xb4899ef4 in QEventDispatcherGlib::processEvents (this=0x94251f8, flags={i = -1074130812})
    at /home/girish/Qt/qt-git/src/corelib/kernel/qeventdispatcher_glib.cpp:328
	d = (QEventDispatcherGlibPrivate * const) 0x9425208
	canWait = true
	savedFlags = {i = 0}
	result = 250
#22 0xb4c036ea in QGuiEventDispatcherGlib::processEvents (this=0x94251f8, flags={i = -1074130764})
    at /home/girish/Qt/qt-git/src/gui/kernel/qguieventdispatcher_glib.cpp:202
	d = (QGuiEventDispatcherGlibPrivate * const) 0x9425208
	saved_flags = {i = 0}
	returnValue = 191
#23 0xb485be36 in QEventLoop::processEvents (this=0xbffa1158, flags={i = -1074130680}) at /home/girish/Qt/qt-git/src/corelib/kernel/qeventloop.cpp:149
	d = (QEventLoopPrivate * const) 0x94eca88
#24 0xb485c0a8 in QEventLoop::exec (this=0xbffa1158, flags={i = -1074130592}) at /home/girish/Qt/qt-git/src/corelib/kernel/qeventloop.cpp:201
	d = (QEventLoopPrivate * const) 0x94eca88
	app = (class QCoreApplication *) 0xbffa11d4
	eventLoop = (class QEventLoop *) 0xb46cffe4
---Type <return> to continue, or q <return> to quit---
#25 0xb486044f in QCoreApplication::exec () at /home/girish/Qt/qt-git/src/corelib/kernel/qcoreapplication.cpp:976
	threadData = (QThreadData *) 0x941ee88
	eventLoop = {<QObject> = {_vptr.QObject = 0xb49aa448, d_ptr = {d = 0x94eca88}}, }
	returnCode = -1208387152
#26 0xb4b2a1e0 in QApplication::exec () at /home/girish/Qt/qt-git/src/gui/kernel/qapplication.cpp:3564
No locals.
#27 0x0804e6e5 in main (argc=Cannot access memory at address 0x28
) at /home/girish/WebKit/WebKit/qt/QGVLauncher/main.cpp:334
	app = {<QCoreApplication> = {<QObject> = {_vptr.QObject = 0xb5604448, d_ptr = {d = 0x941eda0}}, }, }
	url = {d = 0x948fb98}
	args = {<QList<QString>> = {{p = {d = 0x9473520}, d = 0x9473520}}, <No data fields>}
	window = (MainWindow *) 0x94738f8
Comment 1 Antonio Gomes 2009-10-14 05:22:29 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.
Comment 2 Girish Ramakrishnan 2009-10-15 05:57:22 PDT
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.
Comment 3 Girish Ramakrishnan 2009-10-16 06:27:02 PDT
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).
Comment 4 Girish Ramakrishnan 2009-10-18 00:58:21 PDT
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.
Comment 5 Antonio Gomes 2009-10-18 04:01:57 PDT
(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
Comment 6 Kenneth Rohde Christiansen 2009-10-18 08:51:52 PDT
+        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.
Comment 7 Girish Ramakrishnan 2009-10-18 22:08:04 PDT
> 
> -    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).
Comment 8 Girish Ramakrishnan 2009-10-18 22:10:37 PDT
(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.
Comment 9 Girish Ramakrishnan 2009-10-19 00:50:53 PDT
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 10 Holger Freyther 2009-10-19 02:54:08 PDT
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.
Comment 11 Girish Ramakrishnan 2009-10-19 03:16:25 PDT
Created attachment 41409 [details]
Various fixes (2)

Incorp. holger's comments.
Comment 12 Holger Freyther 2009-10-19 03:34:02 PDT
Comment on attachment 41409 [details]
Various fixes (2)

r=me. The swf file will be there with the other commit.
Comment 13 WebKit Commit Bot 2009-10-19 03:44:51 PDT
Comment on attachment 41409 [details]
Various fixes (2)

Clearing flags on attachment: 41409

Committed r49770: <http://trac.webkit.org/changeset/49770>
Comment 14 WebKit Commit Bot 2009-10-19 03:44:56 PDT
All reviewed patches have been landed.  Closing bug.