Bug 106699

Summary: [Qt] Crash in gmail on enabling desktop notifications
Product: WebKit Reporter: jingdow
Component: New BugsAssignee: Allan Sandfeld Jensen <allan.jensen>
Status: RESOLVED FIXED    
Severity: Major CC: allan.jensen, jturcotte, nowrep
Priority: P3    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on:    
Bug Blocks: 88186, 103747, 107438    
Attachments:
Description Flags
gdb trace log
none
gdb trace log from QtTestBrowser
none
Check the validity of ptr right before handling callback.
none
Don't add invalid pointer into to m_callbacks
none
Patch jturcotte: review+

jingdow
Reported 2013-01-11 14:07:12 PST
Created attachment 182414 [details] gdb trace log QtWebKit version: latest as of today from http://gitorious.org/+qtwebkit-developers/webkit/qtwebkit-23/commits/qtwebkit-2.3-staging Browser: Qupzilla Reproducible: Always Browser crashes by clicking on a link in Gmail Settings "Click here to enable desktop notifications for Gmail". Desktop notifications on my other account was already enabled and working fine. gdb log attached.
Attachments
gdb trace log (2.59 KB, application/octet-stream)
2013-01-11 14:07 PST, jingdow 		no flags
Details
gdb trace log from QtTestBrowser (2.86 KB, application/octet-stream)
2013-01-11 15:04 PST, jingdow 		no flags
Details
Check the validity of ptr right before handling callback. (714 bytes, patch)
2013-01-18 15:24 PST, David Rosca 		no flags
DetailsFormatted DiffDiff
Don't add invalid pointer into to m_callbacks (749 bytes, patch)
2013-01-18 15:26 PST, David Rosca 		no flags
DetailsFormatted DiffDiff
Patch (2.04 KB, patch)
2013-01-21 03:24 PST, Allan Sandfeld Jensen 		jturcotte: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
jingdow
Comment 1 2013-01-11 15:00:51 PST
I've built qtwebkit with "bool NotificationPresenterClientQt::dumpNotification" set to "true" in /Source/WebKit/qt/WebCoreSupport/NotificationPresenterClientQt.cpp and I get in console output: DESKTOP NOTIFICATION PERMISSION REQUESTED: https://mail.google.com
jingdow
Comment 2 2013-01-11 15:04:11 PST
Created attachment 182426 [details] gdb trace log from QtTestBrowser
jingdow
Comment 3 2013-01-11 15:59:59 PST
Another thing I've noticed. In file /Source/WebKit/qt/WebCoreSupport/NotificationPresenterClientQt.cpp in function void NotificationPresenterClientQt::requestPermission(ScriptExecutionContext* context, PassRefPtr<VoidCallback> callback) I've added two printf commands, like this: printf("Before emited signal\n"); emit toPage(context)->featurePermissionRequested(toFrame(context), QWebPage::Notifications); printf("After emited signal\n"); Upon crash, only the first one gets printed ("Before emited signal"). Probably doesn't mean anything, but just in case...
David Rosca
Comment 4 2013-01-18 15:24:49 PST
Created attachment 183556 [details] Check the validity of ptr right before handling callback.
David Rosca
Comment 5 2013-01-18 15:26:43 PST
Created attachment 183557 [details] Don't add invalid pointer into to m_callbacks
David Rosca
Comment 6 2013-01-18 15:42:10 PST
From w3.org: static void requestPermission(NotificationPermissionCallback callback); There is one parameter to requestPermission method. However, if this method is called without it, the Callback pointer that is passed to NotificationPresenterClientQt will be invalid and finally will lead to this crash. The simple "window.webkitNotifications.requestPermission();" should crash it. I've added two patches, the first checks the validity of callback pointer right before using it, while the second rather prevents adding invalid callback pointer into m_callbacks array. I'd prefer the behavior of the second patch. Both patches fixes the crash and enables desktop notifications on gmail.com
jingdow
Comment 7 2013-01-18 15:44:43 PST
Confirmed, patches fix the problem.
Allan Sandfeld Jensen
Comment 8 2013-01-19 13:10:38 PST
N(In reply to comment #7) > Confirmed, patches fix the problem. Nice, looks like it might apply to trunk as well. I will test on Monday.
Allan Sandfeld Jensen
Comment 9 2013-01-21 03:24:47 PST
Created attachment 183747 [details] Patch
Allan Sandfeld Jensen
Comment 10 2013-01-21 04:35:31 PST
Committed r140322: <http://trac.webkit.org/changeset/140322>
Note You need to log in before you can comment on or make changes to this bug.