WebAPK notifications broken on Android O on ToT |
||||||
Issue descriptionWe're trying to post them to a non-existing `Sites` channel. Looking at the code, both the actual notification[1][2] and the public notification[3] end up using the NotificationBuilderForO which sets the channel. This is incorrect for WebAPKs, which don't have channels yet. This probably was silently ignored previously, but recent Android O builds refuse to display the notification. WebAPKs don't target O yet so we could branch everything again on some WebAPK flag and not set a channel if so. That feels like a hack to me though, and we will probably want to merge back a fix to M60 :(. [1] https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/notifications/StandardNotificationBuilder.java?l=29 [2] https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java?l=143 [3] https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java?l=363
,
May 30 2017
,
May 31 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/22291519e3fe40a961ef28dce4465e4111330a67 commit 22291519e3fe40a961ef28dce4465e4111330a67 Author: Anita Woodruff <awdf@chromium.org> Date: Wed May 31 17:03:22 2017 [Web apks] Stop setting channels on web apk notifications - This fixes a crash when web apk notifications are displayed on recent builds of Android O. - The crash occurred because the channel we were setting on web apk notifications was never created for the web apk - it was only created for the browser apk - while the notifications were posted by the web apk. - This fixes things by not setting a channel on web apk notifications. Web apks do not target O yet so this is fine for now. When they do, web apks will need their own channel(s). Bug: 727178 , 726340 Change-Id: I4d200f1dc63c484c8728f0b21d993ef35b0cf90c Reviewed-on: https://chromium-review.googlesource.com/519345 Reviewed-by: Yaron Friedman <yfriedman@chromium.org> Reviewed-by: Peter Beverloo <peter@chromium.org> Commit-Queue: Anita Woodruff <awdf@chromium.org> Cr-Commit-Position: refs/heads/master@{#475949} [modify] https://crrev.com/22291519e3fe40a961ef28dce4465e4111330a67/chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java [modify] https://crrev.com/22291519e3fe40a961ef28dce4465e4111330a67/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderForO.java [modify] https://crrev.com/22291519e3fe40a961ef28dce4465e4111330a67/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java [modify] https://crrev.com/22291519e3fe40a961ef28dce4465e4111330a67/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUmaTracker.java [modify] https://crrev.com/22291519e3fe40a961ef28dce4465e4111330a67/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationManager.java [modify] https://crrev.com/22291519e3fe40a961ef28dce4465e4111330a67/chrome/android/javatests/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilderTest.java
,
May 31 2017
,
May 31 2017
Your change meets the bar and is auto-approved for M60. Please go ahead and merge the CL to branch 3112 manually. Please contact 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 1 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2fb2815a0c9b277427231ce47b9bc232f5ba65c6 commit 2fb2815a0c9b277427231ce47b9bc232f5ba65c6 Author: Anita Woodruff <awdf@chromium.org> Date: Thu Jun 01 11:38:13 2017 [Web apks] Stop setting channels on web apk notifications - This fixes a crash when web apk notifications are displayed on recent builds of Android O. - The crash occurred because the channel we were setting on web apk notifications was never created for the web apk - it was only created for the browser apk - while the notifications were posted by the web apk. - This fixes things by not setting a channel on web apk notifications. Web apks do not target O yet so this is fine for now. When they do, web apks will need their own channel(s). R=peter@chromium.org Bug: 727178 , 726340 Change-Id: I4d200f1dc63c484c8728f0b21d993ef35b0cf90c Reviewed-on: https://chromium-review.googlesource.com/519345 Reviewed-by: Yaron Friedman <yfriedman@chromium.org> Reviewed-by: Peter Beverloo <peter@chromium.org> Commit-Queue: Anita Woodruff <awdf@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#475949} Review-Url: https://codereview.chromium.org/2922443002 . Cr-Commit-Position: refs/branch-heads/3112@{#83} Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897} [modify] https://crrev.com/2fb2815a0c9b277427231ce47b9bc232f5ba65c6/chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java [modify] https://crrev.com/2fb2815a0c9b277427231ce47b9bc232f5ba65c6/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java [modify] https://crrev.com/2fb2815a0c9b277427231ce47b9bc232f5ba65c6/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderForO.java [modify] https://crrev.com/2fb2815a0c9b277427231ce47b9bc232f5ba65c6/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java [modify] https://crrev.com/2fb2815a0c9b277427231ce47b9bc232f5ba65c6/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUmaTracker.java [modify] https://crrev.com/2fb2815a0c9b277427231ce47b9bc232f5ba65c6/chrome/android/java/src/org/chromium/chrome/browser/notifications/StandardNotificationBuilder.java [modify] https://crrev.com/2fb2815a0c9b277427231ce47b9bc232f5ba65c6/chrome/android/javatests/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilderTest.java [modify] https://crrev.com/2fb2815a0c9b277427231ce47b9bc232f5ba65c6/chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationBuilderBaseTest.java [modify] https://crrev.com/2fb2815a0c9b277427231ce47b9bc232f5ba65c6/chrome/android/javatests/src/org/chromium/chrome/browser/notifications/StandardNotificationBuilderTest.java
,
Jun 1 2017
,
Jun 1 2017
what about m59?
,
Jun 1 2017
* I made a mistake in the commit message - it didn't actually crash without this commit, it showed an error toast and no notification. Still appropriate to cherry-pick to M60 though.
,
Jun 1 2017
It might be too late to merge to m59.
,
Jun 1 2017
re M59, I'm not sure it's worth patching at this late stage in the release cycle given the tiny percentage of users on O using web apks that will be affected. But maybe one for the TPMS to decide. (As discussed it's possible this could start crashing by the final Android O release - need to check with the Android team) |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by yfried...@chromium.org
, May 29 2017