Bug 108482 – [WebCore][Notifications] Do not assume the page has a NotificationClient

• Status: UNCONFIRMED
• Component: New Bugs

[Claudio Saavedra]

[WebCore][Notifications] Do not assume the page has a NotificationClient

[Claudio Saavedra]

Created attachment 185782 [details]
Patch

[Claudio Saavedra]

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.

[Alexey Proskuryakov]

Comment on attachment 185782 [details]
Patch

There is another unchecked use of NotificationController::from in Notification::permission. Should it be fixed, too?

[Claudio Saavedra]

(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.

[Jon Lee]

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 = ...)

[Jon Lee]

Actually, doesn't it make more sense to not call provideNotification(), and check for the existence of a controller?

[Claudio Saavedra]

(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?

[Jon Lee]

(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.

[Andreas Kling]

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.