Issue metadata
Sign in to add a comment
|
Regression: tapping notification has no effect (seen in Twitter Lite) |
||||||||||||||||||||||
Issue descriptionChrome 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.
,
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
,
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.
,
Jan 30 2018
,
Jan 31 2018
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.
,
Jan 31 2018
,
Jan 31 2018
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
,
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.
,
Feb 1 2018
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.
,
Feb 1 2018
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?
,
Feb 1 2018
Assigning to Peter K (myself). This bug only occurs if twitter is a WebAPK. No leads yet.
,
Feb 2 2018
Ok, did some investigation. One of the reasons that this is failing is that PlatformNotificationBridge#getActiveNotificationsIds() ignores WebAPKs.
,
Feb 2 2018
When was that check added?
,
Feb 2 2018
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...
,
Feb 3 2018
,
Feb 5 2018
- 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)
,
Feb 5 2018
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.
,
Feb 5 2018
nator@ what do you think of my suggested fix in comment #17?
,
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
,
Feb 5 2018
Requesting merge of revert - a99fad2c5b37e0ede596a45d891645300b26311c - to M65
,
Feb 5 2018
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
,
Feb 5 2018
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.
,
Feb 5 2018
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.
,
Feb 5 2018
Merge approved! Please make sure to verify nothing else breaks after reverting from M65.
,
Feb 6 2018
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
,
Feb 13 2018
,
Feb 13 2018
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 |
|||||||||||||||||||||||
Comment 1 by kbr@chromium.org
, Jan 29 2018