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

Issue 806982 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug-Regression

Blocked on:
issue 800363



Sign in to add a comment

Regression: tapping notification has no effect (seen in Twitter Lite)

Project Member Reported by kbr@chromium.org, Jan 29 2018

Issue description

Chrome Version: 65.0.3325.16 (Official Build) dev (32-bit)
OS: Android 8.1

What steps will reproduce the problem?
(1) Install Twitter Lite PWA with Chrome Dev
(2) Wait for Twitter to send a notification
(3) Tap the notification

What is the expected result?

Expect the Twitter Lite app to be opened, and to navigate to the tweet.


What happens instead?

Nothing happens. It doesn't matter what app is in the foreground, whether the Twitter Lite PWA is foregrounded, etc.


This is a regression within the past week. Notifications seem to be completely broken. This is a release blocker.

 

Comment 1 by kbr@chromium.org, Jan 29 2018

Also, as a potential hint of a root cause, the notification doesn't auto-dismiss when it's tapped. It can only be dismissed when swiping to dismiss. Perhaps an exception is being thrown while processing the notification's intent?

Comment 2 by peter@chromium.org, Jan 29 2018

Could you capture and share a bug report[1] or output of the "adb logcat" command? I can't reproduce this on Canary or Dev.

Could you also try notifications on the following site? By default the Chrome window should focus and the notification should go away.

    https://tests.peter.sh/notification-generator/

Finally, dismissing the notification following an interaction is the developer's responsibility because there's cases where you wouldn't want to do that, so that'd be in line with not receiving an event.

[1] https://developer.android.com/studio/debug/bug-report.html

Comment 3 by peter@chromium.org, Jan 29 2018

Feel free to e-mail me the bug report / logcat output directly if it's a personal device, as it could contain PII.

Comment 4 by peter@chromium.org, Jan 30 2018

Labels: Needs-Feedback

Comment 5 by kbr@chromium.org, Jan 31 2018

Components: Mobile>WebAPKs
peter@ your web site works as expected in Chrome Dev.

It might be a problem with this WebAPK specifically.

Just to be clear: you want the bug report captured immediately after attempting to tap one of these notifications? If so I'll do so the next time one comes up. I'll send it to you separately since, yes, this is my personal phone.

Comment 6 by peter@chromium.org, Jan 31 2018

Cc: hanxi@chromium.org yfried...@chromium.org pkotw...@chromium.org
Status: Unconfirmed (was: Untriaged)
Project Member

Comment 7 by sheriffbot@chromium.org, Jan 31 2018

Cc: peter@chromium.org
Labels: -Needs-Feedback
Thank you for providing more feedback. Adding requester "peter@chromium.org" to the cc list and removing "Needs-Feedback" label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 8 by hanxi@chromium.org, Jan 31 2018

I test WebAPKs created for both tests.peter.notification and twitter.com on Chrome dev/TOT on an Android O device. Notification is only broken on the WebAPK of twitter.com. It doesn't look like a common issue of WebAPKs, but it is still unclear to me why these two sites behavior a different way.

Comment 9 by kbr@chromium.org, Feb 1 2018

Status: Available (was: Unconfirmed)
Here is a bug report grabbed after bringing the Twitter Lite WebAPK to the foreground, then tapping two different notifications it sent, one after another. (Only Google employees can access it.)

https://drive.google.com/file/d/0B5DES7PYkZBLcGVfOTBtQmNaS2xESzMwNjh2MTYtNGtnVUhF/view?usp=sharing

Marking Available as this has clearly been reproduced.

Comment 10 Deleted

I build Clank from TOT and get the following adb logcat.

1) The logcat for tests.peter.sh site is:
02-01 09:59:09.783 32312 32312 I NotificationService: Received a notification intent in the NotificationService's receiver.
02-01 09:59:09.785  2313  2333 E ANDR-PERF-RESOURCEQS: Failed to apply optimization [2, 0]
02-01 09:59:09.790 32312  1738 W chromium: [WARNING:library_prefetcher.cc(222)] Incorrect code ordering
02-01 09:59:09.790 32312 32312 I cr_NotificationPlatformBridge: Dispatching notification event to native: p#https://tests.peter.sh/#04
02-01 09:59:09.795 32312 32376 I chromium: [INFO:notification_event_dispatcher_impl.cc(169)] Lookup for ServiceWoker Registration: success: 1
02-01 09:59:09.795 32312 32376 I chromium: [INFO:notification_event_dispatcher_impl.cc(108)] Trying to dispatch notification for SW with status: 0
02-01 09:59:09.806  1195  2490 I ActivityManager: START u0 {act=android.intent.action.VIEW dat=https://tests.peter.sh/... flg=0x10000000 pkg=org.chromium.webapk.a4b4240ee93f1a289 cmp=org.chromium.webapk.a4b4240ee93f1a289/org.chromium.webapk.shell_apk.MainActivity (has extras)} from uid 10702
02-01 09:59:09.808   782   782 D QCOM PowerHAL: LAUNCH HINT: ON
02-01 09:59:09.809   782   782 D QCOM PowerHAL: Activity launch hint handled
02-01 09:59:09.814 32312 32376 I chromium: [INFO:notification_event_dispatcher_impl.cc(58)] The notification event has finished: 0
02-01 09:59:09.835  1195  2490 I ActivityManager: START u0 {act=com.google.android.apps.chrome.webapps.WebappManager.ACTION_START_WEBAPP pkg=com.google.android.apps.chrome cmp=com.google.android.apps.chrome/org.chromium.chrome.browser.webapps.WebappLauncherActivity (has extras)} from uid 10701
02-01 09:59:09.836   782   782 D QCOM PowerHAL: LAUNCH HINT: ON
02-01 09:59:09.836   782   782 D QCOM PowerHAL: Activity launch hint handled
02-01 09:59:09.858 32312 32312 D WebApkValidator: Ok! Looks like a WebApk (has start url) and validation is disabled.
02-01 09:59:09.862 32312 32312 W ResourceType: No package identifier when getting value for resource number 0x00000000
02-01 09:59:09.863  1195  2490 I ActivityManager: START u0 {act=android.intent.action.VIEW dat=webapp://webapk-org.chromium.webapk.a4b4240ee93f1a289 flg=0x14080000 cmp=com.google.android.apps.chrome/org.chromium.chrome.browser.webapps.WebApkActivity (has extras)} from uid 10702

2) The logcat for the twitter site:
02-01 09:58:20.652 32312 32312 I NotificationService: Received a notification intent in the NotificationService's receiver.
02-01 09:58:20.660 32312  1560 W chromium: [WARNING:library_prefetcher.cc(222)] Incorrect code ordering
02-01 09:58:20.661 32312 32312 I cr_NotificationPlatformBridge: Dispatching notification event to native: p#https://mobile.twitter.com/#1dm_message_one_to_one_push-388885669-859109629552361480
02-01 09:58:20.668 32312 32376 I chromium: [INFO:notification_event_dispatcher_impl.cc(169)] Lookup for ServiceWoker Registration: success: 0

See the last line above "success: 0". For some reason, Chrome doesn't find the ServiceWorker registered for the twitter.com site when the notification is tapped. Besides, I also get "this site has been updated in the background" notification at the same time.

So I use Chrome stable to add a WebAPK for the twitter.com site, and it works properly. The logcat is:
02-01 10:10:20.305  2866  2866 I NotificationService: Received a notification intent in the NotificationService's receiver.
02-01 10:10:20.314  2866  2866 I cr_NotificationPlatformBridge: Dispatching notification event to native: p:2466D430568E1F668EF31A2047728E139D88EB640https://mobile.twitter.com/1dm_message_one_to_one_push-388885669-859109629552361480
02-01 10:10:20.322  2866  2934 I chromium: [INFO:notification_event_dispatcher_impl.cc(168)] Lookup for ServiceWoker Registration: success: 1
02-01 10:10:20.322  2866  2934 I chromium: [INFO:notification_event_dispatcher_impl.cc(107)] Trying to dispatch notification for SW with status: 0

So I think the problem doesn't come from the twitter.com site neither. @Peter B: are you aware anything may cause the failure of finding the registered service worker?
Owner: pkotw...@chromium.org
Status: Assigned (was: Available)
Assigning to Peter K (myself). This bug only occurs if twitter is a WebAPK. No leads yet.
Ok, did some investigation. One of the reasons that this is failing is that PlatformNotificationBridge#getActiveNotificationsIds() ignores WebAPKs.
When was that check added?
Cc: na...@chromium.org
Ugh, you're right, sorry for forgetting about that! That was the following CL:
https://chromium.googlesource.com/chromium/src/+/a99fad2c5b37e0ede596a45d891645300b26311c

In short - we maintain our own notification database in //content because developers need to be able to get a list of which notifications are showing. The ability to get the IDs of showing notifications from the platform enables us to synchronize the database with the actual state.

We do this on all platforms but Android, where we couldn't rely on NotificationManager.getActiveNotifications() prior to Android O because it either didn't exist (<N) or was racy. This leads to some rare but unfortunate issues (i.e. Issue 800152). nator@ added the check for Android O in the aforementioned CL.

It's important that we have this functionality, but I don't know how to support this for WebAPKs unless we iterate over all of them to ask for showing notifications -- which I don't think we can...

Comment 16 by kbr@chromium.org, Feb 3 2018

Blockedon: 800363
Thanks for figuring out the root cause. Linking to the earlier bug.

- The offending CL made it to Chrome 65 which is now in Beta. We should revert the offending CL.

One way that I can see us resolving this issue is calling on Android NotificationPlatformBridge#getActiveNotificationsIds() once for every origin for which we think that we have a notification. Peter Beverloo what do you think?

Listing WebAPKs on Android is kind of expensive. We would need to:
(1) get a list of all installed apps
(2) for the subset of apps which we think are WebAPKs try to connect to the identity service to check whether the WebAPK uses this channel of Chrome or a different Channel (e.g. dev, canary etc)
Sorry for missing this case. The first thing I'm doing is reverting the offending CL (sent the revert for review). Next, we'll work with the WebAPK team to work on an appropriate solution, and also add some testing around WebAPK.
nator@ what do you think of my suggested fix in comment #17?
Project Member

Comment 20 by bugdroid1@chromium.org, Feb 5 2018

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

commit 631048a10bb7eddb9da7dc812bb8d20eebc264a2
Author: Mugdha Lakhani <nator@chromium.org>
Date: Mon Feb 05 16:19:58 2018

Revert "[Android Notifications] Implement GetDisplayed() for Android O+"

This reverts commit a99fad2c5b37e0ede596a45d891645300b26311c.
See  Issue 806982  for details.

Bug:  806982 ,  800363 
Change-Id: I08672ec28eb607cb6686fbb3dfd9673caee8eb9b
Reviewed-on: https://chromium-review.googlesource.com/901606
Reviewed-by: Peter Beverloo <peter@chromium.org>
Commit-Queue: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534395}
[modify] https://crrev.com/631048a10bb7eddb9da7dc812bb8d20eebc264a2/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationManagerProxy.java
[modify] https://crrev.com/631048a10bb7eddb9da7dc812bb8d20eebc264a2/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationManagerProxyImpl.java
[modify] https://crrev.com/631048a10bb7eddb9da7dc812bb8d20eebc264a2/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java
[modify] https://crrev.com/631048a10bb7eddb9da7dc812bb8d20eebc264a2/chrome/browser/notifications/notification_platform_bridge_android.cc
[modify] https://crrev.com/631048a10bb7eddb9da7dc812bb8d20eebc264a2/chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/notifications/MockNotificationManagerProxy.java

Labels: Merge-Request-65
Requesting merge of revert - a99fad2c5b37e0ede596a45d891645300b26311c - to M65
Project Member

Comment 22 by sheriffbot@chromium.org, Feb 5 2018

Labels: -Merge-Request-65 Merge-Review-65 Hotlist-Merge-Review
This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Regarding #17, how do you propose we list origins that might be showing a notification?
IMO the right solution would be for Chrome to keep an internal list of all active WebAPKs, which we can then connect to, to get a list of notifications they're displaying, and then combine these into a larger list.
Thank you for requesting merge Peter! +1 to the request.

#17 - The NotificationDisplayService API doesn't care about whether something is a Web Notification or something else, neither do its users, so that's not feasible. Inclusion of the |service_worker_scope| already is unfortunate and something that we should aim to generalize to the |display_source|.

We'll also run in a very similar issue with delegated notifications to TWAs, which is going to increase the complexity of this code further.

My current idea is that, specific to Android, we'll need a "registry" of external entities that've recently shown notifications, and query those for calls to GetNotifications(). We may also want to separate out the act of assembling the NotificationBuilder object from the act of dispatching a call to the SDK/WebAPK/TWA.

Comment 25 by cmasso@google.com, Feb 5 2018

Labels: -Hotlist-Merge-Review -Merge-Review-65 Merge-Approved-65
Merge approved! Please make sure to verify nothing else breaks after reverting from M65.
Project Member

Comment 26 by bugdroid1@chromium.org, Feb 6 2018

Labels: -merge-approved-65 merge-merged-3325
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9411fa0e94916c3987ce86b125480e3db1e8a282

commit 9411fa0e94916c3987ce86b125480e3db1e8a282
Author: Mugdha Lakhani <nator@chromium.org>
Date: Tue Feb 06 16:24:57 2018

Revert "[Android Notifications] Implement GetDisplayed() for Android O+"

This reverts commit a99fad2c5b37e0ede596a45d891645300b26311c.
See  Issue 806982  for details.

(cherry picked from commit 631048a10bb7eddb9da7dc812bb8d20eebc264a2)

Bug:  806982 ,  800363 
Change-Id: I08672ec28eb607cb6686fbb3dfd9673caee8eb9b
Reviewed-on: https://chromium-review.googlesource.com/901606
Reviewed-by: Peter Beverloo <peter@chromium.org>
Commit-Queue: Peter Beverloo <peter@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#534395}
Reviewed-on: https://chromium-review.googlesource.com/904664
Cr-Commit-Position: refs/branch-heads/3325@{#346}
Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369}
[modify] https://crrev.com/9411fa0e94916c3987ce86b125480e3db1e8a282/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationManagerProxy.java
[modify] https://crrev.com/9411fa0e94916c3987ce86b125480e3db1e8a282/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationManagerProxyImpl.java
[modify] https://crrev.com/9411fa0e94916c3987ce86b125480e3db1e8a282/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java
[modify] https://crrev.com/9411fa0e94916c3987ce86b125480e3db1e8a282/chrome/browser/notifications/notification_platform_bridge_android.cc
[modify] https://crrev.com/9411fa0e94916c3987ce86b125480e3db1e8a282/chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/notifications/MockNotificationManagerProxy.java

Cc: aska...@chromium.org
Status: Verified (was: Assigned)
Verified fix in 65.0.3325.67 and 66.0.3345.0. Thanks!
Tapping on twitter Lite notifications, opens the intended tweet.

Sign in to add a comment