[Qt] Application crash on exit if NPPlugin is loaded

[David Leong]

Created attachment 52930 [details]
fix to check for null pointer

Qt-Symbian applications which use Qtwebkit and load Netscape plugins currently crashes on exit.

The crash has been narrowed down to:

void PluginView::platformDestroy()
{
    QWebPageClient* client = m_parentFrame->view()->hostWindow()->platformPageClient();
    if (qobject_cast<QGraphicsWebView*>(client->pluginParent()))   // Crashes here
        delete static_cast<PluginContainerSymbian*>(platformPluginWidget())->proxy();
    else
        delete platformPluginWidget();
}

The crash is caused by a NULL client*. qobject_cast causes an exception as it requires a valid QObject pointer to access underlying Qtmetadata.

[David Leong]

Created attachment 52932 [details]
Alternative patch

The plugin view in symbian can only be of a type PluginContainerSymbian. The proxy object is initialized to NULL or a QGraphicsProxyWidget*. There is no harm deleting NULLs, so there is no need to do the qobject_cast. If the platform widget pointer is not null, it should be safe to delete both the proxy and the widget itself.

[WebKit Review Bot]

Attachment 52932 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/plugins/symbian/PluginViewSymbian.cpp:417:  Missing space before ( in if(  [whitespace/parens] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Hausmann]

Comment on attachment 52932 [details]
Alternative patch


> +    if( PlatformPluginWidget() ) {

This is not going to compile because of the uppercase P.

[Simon Hausmann]

Committed r57333: <http://trac.webkit.org/changeset/57333>

[Simon Hausmann]

(In reply to comment #0)
> Created an attachment (id=52930) [details]
> fix to check for null pointer

Cherry-picked r51006 into qtwebkit-4.6 with commit
14feb62c96ffe2c37e3e2fdac4e370fdbc76ef62

[Simon Hausmann]

Revision r57333 cherry-picked into qtwebkit-2.0 with commit e3b79791d73b624eceebbfbbfd6d6752a532b390

[David Leong]

(In reply to comment #3)
> (From update of attachment 52932 [details])
> 
> > +    if( PlatformPluginWidget() ) {
> 
> This is not going to compile because of the uppercase P.

Thanks! Good catch. Funny enough RVCT still compiles and works on the device. 

I think we are leaking the container now with the landed patch. Both the proxy and the container are new'ed but the if fix will only delete the proxy. This leaks the container. Will submit an updated fix once i test out a window'ed plugin to make sure it is working.

[David Leong]

Created attachment 53005 [details]
Patch

Updated alternative patch. It will now properly delete the proxy and the container objects.

[David Leong]

Tested the code with window'ed and windowless plugins. For window'ed plugins the current fix will leak the container. Updated the patch with Simon's comment for review.

[David Leong]

Comment on attachment 53005 [details]
Patch

Fixes memory leak.

[zalan]

changing on behalf of David

[David Leong]

Created attachment 53191 [details]
SVN up'ed to latest webkit to fix auto check errors

[Simon Hausmann]

Comment on attachment 52930 [details]
fix to check for null pointer

Clearing review as this attachment has been landed

[Simon Hausmann]

As a side note: Due to the use of QGraphicsProxyWidget this is going to be horribly slow on Symbian and generally mobile platforms. Hopefully the flash plugin on the other side will default to windowless painting, otherwise this isn't going to be actually usable.

[Laszlo Gombos]

Comment on attachment 53191 [details]
SVN up'ed to latest webkit to fix auto check errors

Marking it to cq+ for landing.

[WebKit Commit Bot]

Comment on attachment 53191 [details]
SVN up'ed to latest webkit to fix auto check errors

Clearing flags on attachment: 53191

Committed r58035: <http://trac.webkit.org/changeset/58035>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Hausmann]

Revision r58035 cherry-picked into qtwebkit-2.0 with commit 7513c8718ac700b5fa7f58cb459f76fadd526df3

[Laszlo Gombos]

Reopening for a potential double-deallocation case when a page is destroyed which had a plugin loaded.

It seems that the container is deleted twice, once with delete container->proxy(); and then by calling delete container.

Abhinav promised to have a patch.

[Laszlo Gombos]

Fix committed to trunk as http://trac.webkit.org/changeset/62159.

We should seriously consider this patch for the 4.6 and 2.0 branches as well.

[Mahesh Kulkarni]

*** Bug 41366 has been marked as a duplicate of this bug. ***

[Simon Hausmann]

<cherry-pick-for-backport: r62159>

[Simon Hausmann]

Revision r62159 cherry-picked into qtwebkit-2.0 with commit 5bddb0b27458a4ac655ca9527393b41a729d3717