Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1431023004:
[Media, Android] Fix lockscreen and other remote controls on KK- (Closed)
|
Created:
5 years, 1 month ago by whywhat Modified:
5 years, 1 month ago CC:
chromium-reviews, feature-media-reviews_chromium.org, mcasas+watch_chromium.org, posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Media, Android] Fix lockscreen and other remote controls on KK-
The cause of the crash is the following:
- user starts playback on KK
- user tries to use remote control, e.g. lock screen
- the service gets an intent without the notification id and stops itself
- onServiceDestroy gets called with an invalid notification id so it doesn't clean up the manager
- next time the playback is started, the manager calls getIntent() with an invalid notification id
- the method returns null in this case
The fix: use distinct MediaButtonReceiver subclasses for the two supported notification ids to pass the right one with the intent MediaButtonReceiver sends to the service.
BUG==551411
Committed: https://crrev.com/da7a2629e7e54be4d8f8238cb81917ccbdb11b7d
Cr-Commit-Position: refs/heads/master@{#358714}
Patch Set 1 #Patch Set 2 : Better solution #
Total comments: 8
Patch Set 3 : Addressed comments #Patch Set 4 : Handle invalid intent. Remove an else. #
Messages
Total messages: 31 (11 generated)
The CQ bit was checked by avayvod@chromium.org to run a CQ dry run
avayvod@chromium.org changed reviewers: + mlamouri@chromium.org
PTaL
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1431023004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1431023004/1
Does that happen when the intent goes trough the MediaButtonReceiver? Is the intent received there actually has the notification id? Why is the notification id removed in that case? Furthermore, is that K only or does it happen also on JB?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Better solution
The CQ bit was checked by avayvod@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1431023004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1431023004/20001
Description was changed from ========== [Media, Android] Fix lockscreen and other remote controls on KK- The cause of the crash is the following: - user starts playback on KK - user tries to use remote control, e.g. lock screen - the service gets an intent without the notification id and stops itself - onServiceDestroy gets called with an invalid notification id so it doesn't clean up the manager - next time the playback is started, the manager calls getIntent() with an invalid notification id - the method returns null in this case The fix: if the intent doesn't have the notification id but the service already got it before, continue processing the intent. BUG==551411 ========== to ========== [Media, Android] Fix lockscreen and other remote controls on KK- The cause of the crash is the following: - user starts playback on KK - user tries to use remote control, e.g. lock screen - the service gets an intent without the notification id and stops itself - onServiceDestroy gets called with an invalid notification id so it doesn't clean up the manager - next time the playback is started, the manager calls getIntent() with an invalid notification id - the method returns null in this case The fix: use distinct MediaButtonReceiver subclasses for the two supported notification ids to pass the right one with the intent MediaButtonReceiver sends to the service. BUG==551411 ==========
avayvod@chromium.org changed reviewers: + bauerb@chromium.org
+Bernhard for the manifest change. PTaL, modified as discussed.
lgtm with comments applied. Thanks! :) https://codereview.chromium.org/1431023004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java (right): https://codereview.chromium.org/1431023004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:102: && mNotificationId != MediaNotificationInfo.INVALID_ID) { Is such a thing even possible? I would merge these two conditions, return false and assert unless we have a good reason not to. https://codereview.chromium.org/1431023004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:105: } else { No need for |else| if we return above.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Addressed comments
PTaL https://codereview.chromium.org/1431023004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java (right): https://codereview.chromium.org/1431023004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:102: && mNotificationId != MediaNotificationInfo.INVALID_ID) { On 2015/11/06 at 18:01:08, Mounir Lamouri wrote: > Is such a thing even possible? I would merge these two conditions, return false and assert unless we have a good reason not to. Can some other app send an ignorant/malicious intent to Chrome? It shouldn't be possible within Chrome anymore. Changed to an assert but they're pretty useless, at least without tests, anyway :( https://codereview.chromium.org/1431023004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:105: } else { On 2015/11/06 at 18:01:08, Mounir Lamouri wrote: > No need for |else| if we return above. Ack.
The CQ bit was checked by avayvod@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1431023004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1431023004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1431023004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java (right): https://codereview.chromium.org/1431023004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:102: && mNotificationId != MediaNotificationInfo.INVALID_ID) { On 2015/11/08 13:11:51, whywhat wrote: > On 2015/11/06 at 18:01:08, Mounir Lamouri wrote: > > Is such a thing even possible? I would merge these two conditions, return > false and assert unless we have a good reason not to. > > Can some other app send an ignorant/malicious intent to Chrome? It shouldn't be > possible within Chrome anymore. > Changed to an assert but they're pretty useless, at least without tests, anyway > :( It definitely _can_ happen that someone outside of Chrome sends such an Intent, right? The question is just whether something bad can happen if we process the Intent. https://codereview.chromium.org/1431023004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:217: } else if (notificationId == PresentationListenerService.NOTIFICATION_ID) { Same here; no else necessary.
Handle invalid intent. Remove an else.
PTaL https://codereview.chromium.org/1431023004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java (right): https://codereview.chromium.org/1431023004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:102: && mNotificationId != MediaNotificationInfo.INVALID_ID) { On 2015/11/08 at 23:43:13, Bernhard Bauer wrote: > On 2015/11/08 13:11:51, whywhat wrote: > > On 2015/11/06 at 18:01:08, Mounir Lamouri wrote: > > > Is such a thing even possible? I would merge these two conditions, return > > false and assert unless we have a good reason not to. > > > > Can some other app send an ignorant/malicious intent to Chrome? It shouldn't be > > possible within Chrome anymore. > > Changed to an assert but they're pretty useless, at least without tests, anyway > > :( > > It definitely _can_ happen that someone outside of Chrome sends such an Intent, right? The question is just whether something bad can happen if we process the Intent. Agree. Will stop the service in that case and log a message. https://codereview.chromium.org/1431023004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:217: } else if (notificationId == PresentationListenerService.NOTIFICATION_ID) { On 2015/11/08 at 23:43:13, Bernhard Bauer wrote: > Same here; no else necessary. Done.
lgtm
The CQ bit was checked by mlamouri@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/1431023004/#ps60001 (title: "Handle invalid intent. Remove an else.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1431023004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1431023004/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/da7a2629e7e54be4d8f8238cb81917ccbdb11b7d Cr-Commit-Position: refs/heads/master@{#358714} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
