Summary: | [WebCore][Notifications] Do not assume the page has a NotificationClient | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Claudio Saavedra <csaavedra> | ||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | UNCONFIRMED --- | ||||||
Severity: | Normal | CC: | jonlee, mathstuf | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Claudio Saavedra
2013-01-31 07:29:15 PST
Created attachment 185782 [details]
Patch
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 on attachment 185782 [details]
Patch
There is another unchecked use of NotificationController::from in Notification::permission. Should it be fixed, too?
(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 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 = ...) Actually, doesn't it make more sense to not call provideNotification(), and check for the existence of a controller? (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? (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 on attachment 185782 [details]
Patch
Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.
|