Bug 30354 – [Qt] Plugins : QGVLauncher crashes on exit

• Component: Plug-ins
• Status: RESOLVED FIXED

[Girish Ramakrishnan]

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

[Antonio Gomes]

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.

[Girish Ramakrishnan]

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.

[Girish Ramakrishnan]

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).

[Girish Ramakrishnan]

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.

[Antonio Gomes]

(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

[Kenneth Rohde Christiansen]

+        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.

[Girish Ramakrishnan]

> 
> -    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).

[Girish Ramakrishnan]

(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.

[Girish Ramakrishnan]

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.

[Holger Freyther]

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.

[Girish Ramakrishnan]

Created attachment 41409 [details]
Various fixes (2)

Incorp. holger's comments.

[Holger Freyther]

Comment on attachment 41409 [details]
Various fixes (2)

r=me. The swf file will be there with the other commit.

[WebKit Commit Bot]

Comment on attachment 41409 [details]
Various fixes (2)

Clearing flags on attachment: 41409

Committed r49770: <http://trac.webkit.org/changeset/49770>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.