window.Notification.permission directly checks HostContentSettingsMap for permission status |
||||||||||
Issue descriptionSteps to repro (Chrome M59+): 1. Visit https://permission.site 2. Click "notifications". Dismiss the prompt by pressing the "X". 3. Refresh the page 4. Repeat steps (2) and (3) twice more 5. In the console, run "window.Notification.permission" Expected: The state should be "denied", as the permission will have been automatically blocked for 3 dismisses. Actual: The state is "default". In Chrome M59, we now have permissions embargo, which temporarily blocks permission requests after 3 dismisses. This is implemented in the permissions layer, and all code which checks permission status must use PermissionManager::GetPermissionStatus in chrome/browser/permissions to get the correct state (as embargo means that the state is not solely reflected in the permission content setting). window.Notification.permission directly checks HostContentSettingsMap for the notifications permission, resulting in the incorrect value of window.Notification.permission. This is pretty bad, since websites use this value to determine if they can or cannot prompt the user. This is now inconsistent with the actual state of whether they can or can't prompt. Putting this at P1 since we should really get a fix in ASAP and possibly merge to M60. I think window.Notification.permission uses a sync IPC, so it should be able to post to the UI thread and query the permission manager for the permission state.
,
Jun 8 2017
I'm fixing this one now.
,
Jun 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a970559340c9a67098bc9b722854a4bfaa014d27 commit a970559340c9a67098bc9b722854a4bfaa014d27 Author: dominickn <dominickn@chromium.org> Date: Thu Jun 08 07:48:28 2017 Add a test for Notification.permission. This CL adds an interactive UI test for the window.Notification.permission API. In particular, it checks that the correct value is returned when the notifications permission has been embargoed due to multiple dismissals. BUG= 730273 Review-Url: https://codereview.chromium.org/2925203003 Cr-Commit-Position: refs/heads/master@{#477920} [modify] https://crrev.com/a970559340c9a67098bc9b722854a4bfaa014d27/chrome/browser/notifications/notification_interactive_uitest.cc [modify] https://crrev.com/a970559340c9a67098bc9b722854a4bfaa014d27/chrome/browser/notifications/notification_interactive_uitest_support.cc [modify] https://crrev.com/a970559340c9a67098bc9b722854a4bfaa014d27/chrome/browser/notifications/notification_interactive_uitest_support.h [modify] https://crrev.com/a970559340c9a67098bc9b722854a4bfaa014d27/chrome/test/data/notifications/notification_tester.html
,
Jun 13 2017
Requesting merge of c#1 to M60. This fixes the notifications permission API when embargo is enabled (currently broken in M59). It's been in canary with no issues.
,
Jun 13 2017
This bug requires manual review: M60 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), josafat@(ChromeOS), bustamante@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jun 13 2017
Please tag with appropriate OSs. Thanks.
,
Jun 13 2017
,
Jun 14 2017
This is borderline (as I wish the patch were smaller), but due to the impact on web developers, we can take the patch. Approved for 60 branch 3112.
,
Jun 15 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/611ba643701ed338a8f7bab4b607b8ef530ee2e8 commit 611ba643701ed338a8f7bab4b607b8ef530ee2e8 Author: Dominick Ng <dominickn@chromium.org> Date: Thu Jun 15 00:04:05 2017 Check embargo status in PlatformNotificationServiceImpl::CheckPermissionOnIOThread window.Notification.permission directly checks the HostContentSettingsMap for notification permission. This means that if the permission is embargoed, it will incorrectly return "default" instead of "denied". This CL refactors PermissionDecisionAutoBlocker::GetEmbargoResult to be a static method that can be called with a HostContentSettingsMap argument. This allows the PlatformNotificationServiceImpl (which is running on the IO thread) to check the embargo status of the notification permission and return the correct value. The long-term fix here is that PermissionManager::GetPermissionStatus must be made thread safe so that there is one and only one way to correctly query permission status from any thread. BUG= 730273 Review-Url: https://codereview.chromium.org/2926773002 Cr-Original-Commit-Position: refs/heads/master@{#477856} Review-Url: https://codereview.chromium.org/2939923003 . Cr-Commit-Position: refs/branch-heads/3112@{#348} Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897} [modify] https://crrev.com/611ba643701ed338a8f7bab4b607b8ef530ee2e8/chrome/browser/notifications/platform_notification_service_impl.cc [modify] https://crrev.com/611ba643701ed338a8f7bab4b607b8ef530ee2e8/chrome/browser/permissions/permission_decision_auto_blocker.cc [modify] https://crrev.com/611ba643701ed338a8f7bab4b607b8ef530ee2e8/chrome/browser/permissions/permission_decision_auto_blocker.h
,
Jun 15 2017
,
Jun 21 2017
Verified the issue on windows 10, Mac 10.12.5 and Ubuntu 14.04 using chrome beta version #60.0.3112.40 as per comment #0. Observed that state was "denied". Hence, the fix is working as expected. Attaching screen cast for reference. Hence, adding the verified labels. Thanks...!!
,
Jun 29 2017
Issue 738059 has been merged into this issue.
,
Aug 2 2017
Issue 751490 has been merged into this issue. |
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by bugdroid1@chromium.org
, Jun 8 2017