Bug 108009 - The notification constructor needs to check permissions synchronously
Summary: The notification constructor needs to check permissions synchronously
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: jochen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-26 11:38 PST by jochen
Modified: 2013-01-29 07:59 PST (History)
7 users (show)

See Also:


Attachments
Patch (3.81 KB, patch)
2013-01-26 12:06 PST, jochen
no flags Details | Formatted Diff | Diff
Patch (10.74 KB, patch)
2013-01-26 12:12 PST, jochen
no flags Details | Formatted Diff | Diff
Patch (7.38 KB, patch)
2013-01-26 13:32 PST, jochen
no flags Details | Formatted Diff | Diff
Patch for landing (7.50 KB, patch)
2013-01-27 02:36 PST, jochen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jochen 2013-01-26 11:38:04 PST
The notification constructor needs to check permissions synchronously
Comment 1 jochen 2013-01-26 12:06:56 PST
Created attachment 184877 [details]
Patch
Comment 2 jochen 2013-01-26 12:12:45 PST
Created attachment 184878 [details]
Patch
Comment 3 Build Bot 2013-01-26 12:38:51 PST
Comment on attachment 184878 [details]
Patch

Attachment 184878 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16122899
Comment 4 Build Bot 2013-01-26 13:02:34 PST
Comment on attachment 184878 [details]
Patch

Attachment 184878 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16114962
Comment 5 Adam Barth 2013-01-26 13:04:51 PST
Comment on attachment 184878 [details]
Patch

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

Looks like you've got a problem with mac.

> Source/WebCore/Modules/notifications/Notification.cpp:113
> +    ASSERT(static_cast<Document*>(context)->page());

I don't think this ASSERT is valid.  Create a popup window, grab a reference to its Notification constructor, and then close the popup.  Now, if you call the Notification constructor, the page will be 0.
Comment 6 Adam Barth 2013-01-26 13:05:57 PST
http://www.w3.org/TR/notifications/#dom-notification doesn't say anything about the constructor throwing an exception...
Comment 7 jochen 2013-01-26 13:32:54 PST
Created attachment 184882 [details]
Patch
Comment 8 jochen 2013-01-26 13:33:30 PST
You're right. I moved the check from the timer to show()

I also changed the assert into just exiting gracefully if the page is gone.
Comment 9 Adam Barth 2013-01-26 15:02:33 PST
Comment on attachment 184882 [details]
Patch

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

> LayoutTests/fast/notifications/notifications-constructor-with-permission.html:25
> +            setTimeout(function() { testRunner.notifyDone(); }, 100);

This isn't flaky because the timer gets scheduled first, right?
Comment 10 jochen 2013-01-27 02:36:59 PST
Created attachment 184908 [details]
Patch for landing
Comment 11 WebKit Review Bot 2013-01-27 03:00:00 PST
Comment on attachment 184908 [details]
Patch for landing

Clearing flags on attachment: 184908

Committed r140927: <http://trac.webkit.org/changeset/140927>
Comment 12 WebKit Review Bot 2013-01-27 03:00:04 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Jon Lee 2013-01-28 03:09:52 PST
Why was this change necessary? With the legacy API you end up checking permissions twice-- once at construction (which throws an error) and once at show() (which calls onerror).

For the standard API show() is called on a 0-delay timer when the notification is created, so this change is a no-op, essentially. But it seems odd to have to check the permission twice for the legacy case.
Comment 14 jochen 2013-01-28 03:14:17 PST
(In reply to comment #13)
> Why was this change necessary? With the legacy API you end up checking permissions twice-- once at construction (which throws an error) and once at show() (which calls onerror).
> 
> For the standard API show() is called on a 0-delay timer when the notification is created, so this change is a no-op, essentially. But it seems odd to have to check the permission twice for the legacy case.

Because new Notification("blah").show() would show a notification
Comment 15 Jake Archibald 2013-01-29 07:50:48 PST
Btw, the show() method is no longer part of the spec. Notifications show when they're constructed http://notifications.spec.whatwg.org/
Comment 16 Jake Archibald 2013-01-29 07:59:39 PST
(disregard my comment, I didn't see the 'legacy' part of the show() discussion)