Bug 37303

Summary: [Qt] Application crash on exit if NPPlugin is loaded
Product: WebKit Reporter: David Leong <david.leong>
Component: WebKit QtAssignee: Abhinav Mithal <abhinav.mithal>
Status: CLOSED FIXED    
Severity: Critical CC: abhinav.mithal, commit-queue, hausmann, laszlo.gombos, maheshk, webkit.review.bot, zalan
Priority: P1 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: S60 Hardware   
OS: S60 3rd edition   
Bug Depends on:    
Bug Blocks: 35784    
Description Flags
fix to check for null pointer
Alternative patch
hausmann: review-
SVN up'ed to latest webkit to fix auto check errors none

Description David Leong 2010-04-08 18:31:40 PDT
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();
        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.
Comment 1 David Leong 2010-04-08 19:38:24 PDT
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.
Comment 2 WebKit Review Bot 2010-04-08 19:39:23 PDT
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.
Comment 3 Simon Hausmann 2010-04-09 05:07:41 PDT
Comment on attachment 52932 [details]
Alternative patch

> +    if( PlatformPluginWidget() ) {

This is not going to compile because of the uppercase P.
Comment 4 Simon Hausmann 2010-04-09 05:13:14 PDT
Committed r57333: <http://trac.webkit.org/changeset/57333>
Comment 5 Simon Hausmann 2010-04-09 05:18:36 PDT
(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
Comment 6 Simon Hausmann 2010-04-09 07:16:37 PDT
Revision r57333 cherry-picked into qtwebkit-2.0 with commit e3b79791d73b624eceebbfbbfd6d6752a532b390
Comment 7 David Leong 2010-04-09 11:40:22 PDT
(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.
Comment 8 David Leong 2010-04-09 15:46:20 PDT
Created attachment 53005 [details]

Updated alternative patch. It will now properly delete the proxy and the container objects.
Comment 9 David Leong 2010-04-12 11:28:28 PDT
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.
Comment 10 David Leong 2010-04-12 11:29:52 PDT
Comment on attachment 53005 [details]

Fixes memory leak.
Comment 11 zalan 2010-04-12 11:35:54 PDT
changing on behalf of David
Comment 12 David Leong 2010-04-12 16:26:48 PDT
Created attachment 53191 [details]
SVN up'ed to latest webkit to fix auto check errors
Comment 13 Simon Hausmann 2010-04-13 06:50:17 PDT
Comment on attachment 52930 [details]
fix to check for null pointer

Clearing review as this attachment has been landed
Comment 14 Simon Hausmann 2010-04-13 06:53:15 PDT
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.
Comment 15 Laszlo Gombos 2010-04-21 06:11:48 PDT
Comment on attachment 53191 [details]
SVN up'ed to latest webkit to fix auto check errors

Marking it to cq+ for landing.
Comment 16 WebKit Commit Bot 2010-04-21 18:30:10 PDT
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>
Comment 17 WebKit Commit Bot 2010-04-21 18:30:17 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Simon Hausmann 2010-04-26 03:17:10 PDT
Revision r58035 cherry-picked into qtwebkit-2.0 with commit 7513c8718ac700b5fa7f58cb459f76fadd526df3
Comment 19 Laszlo Gombos 2010-06-29 12:36:11 PDT
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.
Comment 20 Laszlo Gombos 2010-06-29 20:47:34 PDT
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.
Comment 21 Mahesh Kulkarni 2010-06-30 21:35:21 PDT
*** Bug 41366 has been marked as a duplicate of this bug. ***
Comment 22 Simon Hausmann 2010-07-01 00:48:12 PDT
<cherry-pick-for-backport: r62159>
Comment 23 Simon Hausmann 2010-07-01 00:55:02 PDT
Revision r62159 cherry-picked into qtwebkit-2.0 with commit 5bddb0b27458a4ac655ca9527393b41a729d3717