New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 644256 link

Starred by 1 user
Status: Fixed
Owner:
█ joh...@chromium.org
no longer working on chrome
Closed: Sep 2016
Cc:
█ owe...@chromium.org
█ miguelg@chromium.org
█ raymes@chromium.org
peter@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug-Regression



Sign in to add a comment

PushManager.subscribe() fails to request permission

Project Member Reported by joh...@chromium.org, Sep 6 2016 
Version: 54
OS: Android

What steps will reproduce the problem?

(1) Call PushManager.subscribe()

For example:

(1) Reset permissions for rawgit.com if you have previously set any.
(2) Visit https://rawgit.com/johnmellor/51ade9f1221a15b162efc4520ab997d1/raw/ in a regular (non-incognito) tab.

What is the expected output?

It should prompt for notification permission (once only). When you accept that, the PushManager.subscribe() result should change from "pending..." to a URL, and the two Notification results should change from "pending..." to "granted".

What do you see instead?

- In a debug build, it hits a NOTREACHED at https://chromium.googlesource.com/chromium/src/+/48bc3900dd3f59923fda26f9555607e431701ab6/chrome/browser/permissions/permission_queue_controller.cc#150 because it tries to show a push messaging infobar, but there is no such infobar.

- In a release build (which ignores NOTREACHED), the PushManager.subscribe() promise hangs, i.e. is never resolved or rejected. Thus subscribing for push messaging will fail unless websites happened to have requested notification permission previously.

This regressed due to https://codereview.chromium.org/2149883002, which merged the NotificationPermissionContext with the PushMessagingPermissionContext. Fortunately that landed after m53
Project Member

Comment 2 by bugdroid1@chromium.org, Sep 6 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e0db126dd99d297686d49eedd745d196708167ee

commit e0db126dd99d297686d49eedd745d196708167ee
Author: johnme <johnme@chromium.org>
Date: Tue Sep 06 23:16:27 2016

Fix PushManager.subscribe() permission request on Android

https://codereview.chromium.org/2149883002 merged the
PushMessagingPermissionContext into the NotificationPermissionContext.
Unfortunately, this meant that PushManager.subscribe() tried to show
a push messaging permission infobar instead of the notifications
infobar, which fails because there is no push messaging infobar. This
patch switches push messaging back to the notifications infobar.

BUG= 644256 

Review-Url: https://codereview.chromium.org/2316673002
Cr-Commit-Position: refs/heads/master@{#416762}

[modify] https://crrev.com/e0db126dd99d297686d49eedd745d196708167ee/chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java
[modify] https://crrev.com/e0db126dd99d297686d49eedd745d196708167ee/chrome/browser/permissions/permission_queue_controller.cc

Comment 3 by raymes@chromium.org, Sep 7 2016

Labels: Merge-Request-54
Tested this on branch 2840 and verified that push shows a prompt and accepting it allows push to work.

Comment 4 by dimu@chromium.org, Sep 7 2016

Labels: -Merge-Request-54 Merge-Approved-54 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M54 (branch: 2840)

Comment 5 by amineer@chromium.org, Sep 7 2016 

[Bulk edit]

This issue has been approved for a merge to M54 branch 2840.  Please try to complete the merge by tomorrow at 5 PM PT if at all possible.  If this has already been merged and this message is in error, please remove the label Merge-Approved-54.

Cheers,
Alex
Project Member

Comment 6 by bugdroid1@chromium.org, Sep 7 2016

Labels: -merge-approved-54 merge-merged-2840
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d256ccde6cb75b87e4346d64e6e868028197ab6b

commit d256ccde6cb75b87e4346d64e6e868028197ab6b
Author: John Mellor <johnme@chromium.org>
Date: Wed Sep 07 13:25:16 2016

Fix PushManager.subscribe() permission request on Android

https://codereview.chromium.org/2149883002 merged the
PushMessagingPermissionContext into the NotificationPermissionContext.
Unfortunately, this meant that PushManager.subscribe() tried to show
a push messaging permission infobar instead of the notifications
infobar, which fails because there is no push messaging infobar. This
patch switches push messaging back to the notifications infobar.

BUG= 644256 

Review-Url: https://codereview.chromium.org/2316673002
Cr-Commit-Position: refs/heads/master@{#416762}
(cherry picked from commit e0db126dd99d297686d49eedd745d196708167ee)

Review URL: https://codereview.chromium.org/2320633002 .

Cr-Commit-Position: refs/branch-heads/2840@{#204}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/d256ccde6cb75b87e4346d64e6e868028197ab6b/chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java
[modify] https://crrev.com/d256ccde6cb75b87e4346d64e6e868028197ab6b/chrome/browser/permissions/permission_queue_controller.cc

Comment 7 by joh...@chromium.org, Sep 7 2016

Status: Fixed (was: Started)
Project Member

Comment 8 by bugdroid1@chromium.org, Oct 27 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d256ccde6cb75b87e4346d64e6e868028197ab6b

commit d256ccde6cb75b87e4346d64e6e868028197ab6b
Author: John Mellor <johnme@chromium.org>
Date: Wed Sep 07 13:25:16 2016

Fix PushManager.subscribe() permission request on Android

https://codereview.chromium.org/2149883002 merged the
PushMessagingPermissionContext into the NotificationPermissionContext.
Unfortunately, this meant that PushManager.subscribe() tried to show
a push messaging permission infobar instead of the notifications
infobar, which fails because there is no push messaging infobar. This
patch switches push messaging back to the notifications infobar.

BUG= 644256 

Review-Url: https://codereview.chromium.org/2316673002
Cr-Commit-Position: refs/heads/master@{#416762}
(cherry picked from commit e0db126dd99d297686d49eedd745d196708167ee)

Review URL: https://codereview.chromium.org/2320633002 .

Cr-Commit-Position: refs/branch-heads/2840@{#204}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/d256ccde6cb75b87e4346d64e6e868028197ab6b/chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java
[modify] https://crrev.com/d256ccde6cb75b87e4346d64e6e868028197ab6b/chrome/browser/permissions/permission_queue_controller.cc
Project Member

Comment 9 by bugdroid1@chromium.org, Nov 8 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/00d49400f8a7c4f189e132c00ed0118f606c4dc8

commit 00d49400f8a7c4f189e132c00ed0118f606c4dc8
Author: raymes <raymes@chromium.org>
Date: Tue Nov 08 23:17:00 2016

Pass the permission type to NotificationPermissionInfoBarDelegate

NotificationPermissionInfoBarDelegate is used for both the notifications
permission and for the push messaging permission. For correctness (mainly with
metrics right now), we need to pass the correct permission type through to the
PermissionInfobarDelegate.

BUG= 644256 

Review-Url: https://codereview.chromium.org/2490493002
Cr-Commit-Position: refs/heads/master@{#430750}

[modify] https://crrev.com/00d49400f8a7c4f189e132c00ed0118f606c4dc8/chrome/browser/notifications/notification_permission_infobar_delegate.cc
[modify] https://crrev.com/00d49400f8a7c4f189e132c00ed0118f606c4dc8/chrome/browser/notifications/notification_permission_infobar_delegate.h
[modify] https://crrev.com/00d49400f8a7c4f189e132c00ed0118f606c4dc8/chrome/browser/permissions/permission_infobar_delegate.cc
►

Sign in to add a comment