Bug 108482

Summary: [WebCore][Notifications] Do not assume the page has a NotificationClient
Product: WebKit Reporter: Claudio Saavedra <csaavedra>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: UNCONFIRMED ---    
Severity: Normal CC: jonlee, mathstuf
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Claudio Saavedra 2013-01-31 07:29:15 PST
[WebCore][Notifications] Do not assume the page has a NotificationClient
Comment 1 Claudio Saavedra 2013-01-31 07:33:22 PST
Created attachment 185782 [details]
Patch
Comment 2 Claudio Saavedra 2013-01-31 07:37:37 PST
Hi again Jon! :)

This one is a crasher that happens with WK1 clients when enabling NOTIFICATIONS. I am working exclusively on adding notifications support in wk2 for the GTK port and clients still using WK1 should not crash, even if notifications are enabled.
Comment 3 Alexey Proskuryakov 2013-01-31 10:42:03 PST
Comment on attachment 185782 [details]
Patch

There is another unchecked use of NotificationController::from in Notification::permission. Should it be fixed, too?
Comment 4 Claudio Saavedra 2013-02-01 09:33:40 PST
(In reply to comment #3)
> (From update of attachment 185782 [details])
> There is another unchecked use of NotificationController::from in Notification::permission. Should it be fixed, too?

I see. So far I can't tell whether it's an issue but I guess it wouldn't hurt to ensure it won't crash.
Comment 5 Jon Lee 2013-02-01 11:24:23 PST
Comment on attachment 185782 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=185782&action=review

Unofficial r=me.

> Source/WebCore/Modules/notifications/Notification.cpp:291
> +    if (client)

It would be better to define it in the if clause:

if (NotificationClient* client = ...)
Comment 6 Jon Lee 2013-02-01 11:29:04 PST
Actually, doesn't it make more sense to not call provideNotification(), and check for the existence of a controller?
Comment 7 Claudio Saavedra 2013-02-07 02:19:16 PST
(In reply to comment #6)
> Actually, doesn't it make more sense to not call provideNotification(), and check for the existence of a controller?

Actually the caller of Notification::requestPermission() seems to be JSNotification::requestPermission(). Not sure I understand what you mean?
Comment 8 Jon Lee 2013-02-07 07:56:46 PST
(In reply to comment #7)
> (In reply to comment #6)
> > Actually, doesn't it make more sense to not call provideNotification(), and check for the existence of a controller?
> 
> Actually the caller of Notification::requestPermission() seems to be JSNotification::requestPermission(). Not sure I understand what you mean?

If you don't have a notification client, then you shouldn't be adding a NotificationController as a supplement to the Page. So I think you should be able to check for a null NotificationController, rather than  checking for a null NotificationClient.
Comment 9 Andreas Kling 2014-02-05 11:14:54 PST
Comment on attachment 185782 [details]
Patch

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.