108482 2013-01-31 07:29:15 -0800 [WebCore][Notifications] Do not assume the page has a NotificationClient 2014-02-05 11:14:54 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified UNCONFIRMED P2 Normal --- 0 csaavedra webkit-unassigned jonlee mathstuf oldest_to_newest 821514 0 csaavedra 2013-01-31 07:29:15 -0800 [WebCore][Notifications] Do not assume the page has a NotificationClient 821516 1 185782 csaavedra 2013-01-31 07:33:22 -0800 Created attachment 185782 Patch 821517 2 csaavedra 2013-01-31 07:37:37 -0800 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. 821652 3 185782 ap 2013-01-31 10:42:03 -0800 Comment on attachment 185782 Patch There is another unchecked use of NotificationController::from in Notification::permission. Should it be fixed, too? 822876 4 csaavedra 2013-02-01 09:33:40 -0800 (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. 823014 5 185782 jonlee 2013-02-01 11:24:23 -0800 Comment on attachment 185782 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 = ...) 823025 6 jonlee 2013-02-01 11:29:04 -0800 Actually, doesn't it make more sense to not call provideNotification(), and check for the existence of a controller? 827198 7 csaavedra 2013-02-07 02:19:16 -0800 (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? 827519 8 jonlee 2013-02-07 07:56:46 -0800 (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. 976968 9 185782 kling 2014-02-05 11:14:54 -0800 Comment on attachment 185782 Patch Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag. 185782 2013-01-31 07:33:22 -0800 2014-02-05 11:14:53 -0800 Patch bug-108482-20130131173005.patch text/plain 1690 csaavedra U3VidmVyc2lvbiBSZXZpc2lvbjogMTQwOTY2CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMWZmNjhhYmY1NTU5ZjQy MTYyODc4OTZkN2EzNzI2NWNhYTNkOTg0NS4uY2FmYTk1OTY4NDI1NWUzY2Q1N2E3OTYxYmRjNzQx NjlhYmZmMWZmYiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE0IEBACisyMDEzLTAxLTMxICBDbGF1 ZGlvIFNhYXZlZHJhICA8Y3NhYXZlZHJhQGlnYWxpYS5jb20+CisKKyAgICAgICAgW1dlYkNvcmVd W05vdGlmaWNhdGlvbnNdIERvIG5vdCBhc3N1bWUgdGhlIHBhZ2UgaGFzIGEgTm90aWZpY2F0aW9u Q2xpZW50CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0x MDg0ODIKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAq IE1vZHVsZXMvbm90aWZpY2F0aW9ucy9Ob3RpZmljYXRpb24uY3BwOgorICAgICAgICAoV2ViQ29y ZTo6Tm90aWZpY2F0aW9uOjpyZXF1ZXN0UGVybWlzc2lvbik6IFBhZ2VzCisgICAgICAgIG1pZ2h0 IG5vdCBoYXZlIGEgTm90aWZpY2F0aW9uQ2xpZW50LCBzbyBhdm9pZCBhIGNyYXNoIGhlcmUuCisK IDIwMTMtMDEtMjggIFZzZXZvbG9kIFZsYXNvdiAgPHZzZXZpa0BjaHJvbWl1bS5vcmc+CiAKICAg ICAgICAgV2ViIEluc3BlY3RvcjogW1JlZ3Jlc3Npb25dIFNlYXJjaCBhbGwgc291cmNlcyBzaG91 bGQgbm90IHNlYXJjaCBhY3Jvc3Mgc2VydmljZSBwcm9qZWN0cy4KZGlmZiAtLWdpdCBhL1NvdXJj ZS9XZWJDb3JlL01vZHVsZXMvbm90aWZpY2F0aW9ucy9Ob3RpZmljYXRpb24uY3BwIGIvU291cmNl L1dlYkNvcmUvTW9kdWxlcy9ub3RpZmljYXRpb25zL05vdGlmaWNhdGlvbi5jcHAKaW5kZXggZDdi NDE3OGUzYzc3NGU3MGJiOGExNTk4NmY5NDFjZThhM2IxNDY3Yi4uOWM3ZDcyYWMzNDI4MTI1NTZj MGYzN2RiMDExY2I3OGYwNDk5NjgzNSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvTW9kdWxl cy9ub3RpZmljYXRpb25zL05vdGlmaWNhdGlvbi5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvTW9k dWxlcy9ub3RpZmljYXRpb25zL05vdGlmaWNhdGlvbi5jcHAKQEAgLTI4Nyw3ICsyODcsOSBAQCB2 b2lkIE5vdGlmaWNhdGlvbjo6cmVxdWVzdFBlcm1pc3Npb24oU2NyaXB0RXhlY3V0aW9uQ29udGV4 dCogY29udGV4dCwgUGFzc1JlZlB0cgogewogICAgIEFTU0VSVChjb250ZXh0LT5pc0RvY3VtZW50 KCkpOwogICAgIEFTU0VSVChzdGF0aWNfY2FzdDxEb2N1bWVudCo+KGNvbnRleHQpLT5wYWdlKCkp OwotICAgIE5vdGlmaWNhdGlvbkNvbnRyb2xsZXI6OmZyb20oc3RhdGljX2Nhc3Q8RG9jdW1lbnQq Pihjb250ZXh0KS0+cGFnZSgpKS0+Y2xpZW50KCktPnJlcXVlc3RQZXJtaXNzaW9uKGNvbnRleHQs IGNhbGxiYWNrKTsKKyAgICBOb3RpZmljYXRpb25DbGllbnQqIGNsaWVudCA9IE5vdGlmaWNhdGlv bkNvbnRyb2xsZXI6OmNsaWVudEZyb20oc3RhdGljX2Nhc3Q8RG9jdW1lbnQqPihjb250ZXh0KS0+ cGFnZSgpKTsKKyAgICBpZiAoY2xpZW50KQorICAgICAgICBjbGllbnQtPnJlcXVlc3RQZXJtaXNz aW9uKGNvbnRleHQsIGNhbGxiYWNrKTsKIH0KICNlbmRpZgogCg==