Bug 108482

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:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none

Claudio Saavedra

Reported 2013-01-31 07:29:15 PST

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

Attachments
Patch (1.65 KB, patch) 2013-01-31 07:33 PST, Claudio Saavedra	no flags	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Claudio Saavedra

Comment 1 2013-01-31 07:33:22 PST

Created attachment 185782 [details] Patch

Claudio Saavedra

Comment 2 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.

Alexey Proskuryakov

Comment 3 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?

Claudio Saavedra

Comment 4 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.

Jon Lee

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

Jon Lee

Comment 6 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?

Claudio Saavedra

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

Jon Lee

Comment 8 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.

Andreas Kling

Comment 9 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.

Note You need to log in before you can comment on or make changes to this bug.