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    
Attachments:
Description Flags
fix to check for null pointer
none
Alternative patch
hausmann: review-
Patch
none
SVN up'ed to latest webkit to fix auto check errors none

David Leong
Reported 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(); 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.
Attachments
fix to check for null pointer (1.33 KB, patch)
2010-04-08 18:31 PDT, David Leong 		no flags
DetailsFormatted DiffDiff
Alternative patch (1.49 KB, patch)
2010-04-08 19:38 PDT, David Leong 		hausmann: review-
DetailsFormatted DiffDiff
Patch (1.56 KB, patch)
2010-04-09 15:46 PDT, David Leong 		no flags
DetailsFormatted DiffDiff
SVN up'ed to latest webkit to fix auto check errors (1.53 KB, patch)
2010-04-12 16:26 PDT, David Leong 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
David Leong
Comment 1 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.
WebKit Review Bot
Comment 2 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.
Simon Hausmann
Comment 3 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.
Simon Hausmann
Comment 4 2010-04-09 05:13:14 PDT
Committed r57333: <http://trac.webkit.org/changeset/57333>
Simon Hausmann
Comment 5 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 14feb62c96ffe2c37e3e2fdac4e370fdbc76ef62
Simon Hausmann
Comment 6 2010-04-09 07:16:37 PDT
Revision r57333 cherry-picked into qtwebkit-2.0 with commit e3b79791d73b624eceebbfbbfd6d6752a532b390
David Leong
Comment 7 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.
David Leong
Comment 8 2010-04-09 15:46:20 PDT
Created attachment 53005 [details] Patch Updated alternative patch. It will now properly delete the proxy and the container objects.
David Leong
Comment 9 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.
David Leong
Comment 10 2010-04-12 11:29:52 PDT
Comment on attachment 53005 [details] Patch Fixes memory leak.
alan
Comment 11 2010-04-12 11:35:54 PDT
changing on behalf of David
David Leong
Comment 12 2010-04-12 16:26:48 PDT
Created attachment 53191 [details] SVN up'ed to latest webkit to fix auto check errors
Simon Hausmann
Comment 13 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
Simon Hausmann
Comment 14 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.
Laszlo Gombos
Comment 15 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.
WebKit Commit Bot
Comment 16 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>
WebKit Commit Bot
Comment 17 2010-04-21 18:30:17 PDT
All reviewed patches have been landed. Closing bug.
Simon Hausmann
Comment 18 2010-04-26 03:17:10 PDT
Revision r58035 cherry-picked into qtwebkit-2.0 with commit 7513c8718ac700b5fa7f58cb459f76fadd526df3
Laszlo Gombos
Comment 19 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.
Laszlo Gombos
Comment 20 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.
Mahesh Kulkarni
Comment 21 2010-06-30 21:35:21 PDT
*** Bug 41366 has been marked as a duplicate of this bug. ***
Simon Hausmann
Comment 22 2010-07-01 00:48:12 PDT
<cherry-pick-for-backport: r62159>
Simon Hausmann
Comment 23 2010-07-01 00:55:02 PDT
Revision r62159 cherry-picked into qtwebkit-2.0 with commit 5bddb0b27458a4ac655ca9527393b41a729d3717
Note You need to log in before you can comment on or make changes to this bug.