Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1889233002:
[Android,VideoFling] Never add null players to alternative ones (Closed)
|
Created:
4 years, 8 months ago by whywhat Modified:
4 years, 8 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, mcasas+watch+vc_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[Android,VideoFling] Never add null players to alternative ones
BUG=602851
TEST=existing tests
The fix is a mere speculation from staring at the code.
Committed: https://crrev.com/9af71a3cfca697a63586d334267829408d702d6a
Cr-Commit-Position: refs/heads/master@{#387961}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Remove and delete new player when no current player is found #
Total comments: 2
Patch Set 3 : Check the result of SwapCurrentPlayer in SwitchToRemotePlayer #
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: + aberent@chromium.org, dgn@chromium.org
PTaL
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1889233002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1889233002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
avayvod@chromium.org changed reviewers: - dgn@chromium.org
After the weekend friendly ping :) Thanks!
dgn@chromium.org changed reviewers: + dgn@chromium.org
Letting aberent@ have a look. https://codereview.chromium.org/1889233002/diff/1/chrome/browser/media/androi... File chrome/browser/media/android/remote/remote_media_player_manager.cc (right): https://codereview.chromium.org/1889233002/diff/1/chrome/browser/media/androi... chrome/browser/media/android/remote/remote_media_player_manager.cc:153: if (!old_player) In which case can this be null? Is it safe to run SwapPlayer even though you are not going to be able to swap back later? Wouldn't it be better to find the player first?
https://codereview.chromium.org/1889233002/diff/1/chrome/browser/media/androi... File chrome/browser/media/android/remote/remote_media_player_manager.cc (right): https://codereview.chromium.org/1889233002/diff/1/chrome/browser/media/androi... chrome/browser/media/android/remote/remote_media_player_manager.cc:153: if (!old_player) On 2016/04/18 at 13:18:49, dgn wrote: > In which case can this be null? Is it safe to run SwapPlayer even though you are not going to be able to swap back later? Wouldn't it be better to find the player first? SwapPlayer doesn't do anything if it can't find the player to swap - it will return null if it doesn't find one. As I understand, that would happen if we have a player id that's out of sync with the C++/renderer side. Before the refactoring mentioned in the bug I think we did nothing in the case player id wasn't found in the list of players. After that we potentially store null pointers sometimes, leading to the crashes causing the bug.
https://codereview.chromium.org/1889233002/diff/1/chrome/browser/media/androi... File chrome/browser/media/android/remote/remote_media_player_manager.cc (right): https://codereview.chromium.org/1889233002/diff/1/chrome/browser/media/androi... chrome/browser/media/android/remote/remote_media_player_manager.cc:153: if (!old_player) On 2016/04/18 13:21:51, whywhat wrote: > On 2016/04/18 at 13:18:49, dgn wrote: > > In which case can this be null? Is it safe to run SwapPlayer even though you > are not going to be able to swap back later? Wouldn't it be better to find the > player first? > > SwapPlayer doesn't do anything if it can't find the player to swap - it will > return null if it doesn't find one. As I understand, that would happen if we > have a player id that's out of sync with the C++/renderer side. Before the > refactoring mentioned in the bug I think we did nothing in the case player id > wasn't found in the list of players. After that we potentially store null > pointers sometimes, leading to the crashes causing the bug. Is all this one line too high? It seems that this code would leave |new_player| as both the current player and the alternative player. I think we want to remove it as the alternative player, otherwise things will get confused the next time this is called. I think (but haven't fully checked) that all calls to GetAlternativePlayer check for not getting one.
Remove and delete new player when no current player is found
PTAL https://codereview.chromium.org/1889233002/diff/1/chrome/browser/media/androi... File chrome/browser/media/android/remote/remote_media_player_manager.cc (right): https://codereview.chromium.org/1889233002/diff/1/chrome/browser/media/androi... chrome/browser/media/android/remote/remote_media_player_manager.cc:153: if (!old_player) On 2016/04/18 at 13:44:57, aberent wrote: > On 2016/04/18 13:21:51, whywhat wrote: > > On 2016/04/18 at 13:18:49, dgn wrote: > > > In which case can this be null? Is it safe to run SwapPlayer even though you > > are not going to be able to swap back later? Wouldn't it be better to find the > > player first? > > > > SwapPlayer doesn't do anything if it can't find the player to swap - it will > > return null if it doesn't find one. As I understand, that would happen if we > > have a player id that's out of sync with the C++/renderer side. Before the > > refactoring mentioned in the bug I think we did nothing in the case player id > > wasn't found in the list of players. After that we potentially store null > > pointers sometimes, leading to the crashes causing the bug. > > Is all this one line too high? It seems that this code would leave |new_player| as both the current player and the alternative player. I think we want to remove it as the alternative player, otherwise things will get confused the next time this is called. > > I think (but haven't fully checked) that all calls to GetAlternativePlayer check for not getting one. Good point, remove and delete the alternative player now if the current player is not found.
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/1889233002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1889233002/20001
https://codereview.chromium.org/1889233002/diff/20001/chrome/browser/media/an... File chrome/browser/media/android/remote/remote_media_player_manager.cc (right): https://codereview.chromium.org/1889233002/diff/20001/chrome/browser/media/an... chrome/browser/media/android/remote/remote_media_player_manager.cc:156: alternative_players_.erase(it); Does this now need a change to SwitchToRemotePlayer to handle this case? SwitchToRemotePlayer seems to assume that SwapCurrentPlayer works correctly, and doesn't check that it has a new valid player before starting remote playback. If the error case can never happen when the call is from SwitchToRemotePlayer then maybe we should inline this function and only handle the error case in SwitchToLocalPlayer. The real answer to this mess is, of course, to merge remote_media_player_manager into browser_media_player_manager, but that is a much bigger job.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Check the result of SwapCurrentPlayer in SwitchToRemotePlayer
PTAL https://codereview.chromium.org/1889233002/diff/20001/chrome/browser/media/an... File chrome/browser/media/android/remote/remote_media_player_manager.cc (right): https://codereview.chromium.org/1889233002/diff/20001/chrome/browser/media/an... chrome/browser/media/android/remote/remote_media_player_manager.cc:156: alternative_players_.erase(it); On 2016/04/18 at 15:31:46, aberent wrote: > Does this now need a change to SwitchToRemotePlayer to handle this case? SwitchToRemotePlayer seems to assume that SwapCurrentPlayer works correctly, and doesn't check that it has a new valid player before starting remote playback. > > If the error case can never happen when the call is from SwitchToRemotePlayer then maybe we should inline this function and only handle the error case in SwitchToLocalPlayer. > > The real answer to this mess is, of course, to merge remote_media_player_manager into browser_media_player_manager, but that is a much bigger job. Done. Added a check. Doesn't seem to be necessary in other two places where SwapCurrentPlayer is called (for SwitchToLocalPlayer the check would be a no-op, for ReplaceRemotePlayerWithLocal it would crash earlier if one of the players were missing or null.
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/1889233002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1889233002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by avayvod@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1889233002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1889233002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [Android,VideoFling] Never add null players to alternative ones BUG=602851 TEST=existing tests The fix is a mere speculation from staring at the code. ========== to ========== [Android,VideoFling] Never add null players to alternative ones BUG=602851 TEST=existing tests The fix is a mere speculation from staring at the code. Committed: https://crrev.com/9af71a3cfca697a63586d334267829408d702d6a Cr-Commit-Position: refs/heads/master@{#387961} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/9af71a3cfca697a63586d334267829408d702d6a Cr-Commit-Position: refs/heads/master@{#387961} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
