[Android,VideoFling] Never add null players to alternative ones

chrome/browser/media/android/remote/remote_media_player_manager.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/media/android/remote/remote_media_player_manager.h"
 
 #include "chrome/browser/android/tab_android.h"
 #include "chrome/common/chrome_content_client.h"
 #include "content/common/media/media_player_messages_android.h"
 #include "third_party/skia/include/core/SkBitmap.h"
@@ skipping 132 matching lines @@
 }
 
 void RemoteMediaPlayerManager::SwapCurrentPlayer(int player_id) {
   // Find the remote player
   auto it = GetAlternativePlayer(player_id);
   if (it == alternative_players_.end())
     return;
   MediaPlayerAndroid* new_player = *it;
   std::unique_ptr<MediaPlayerAndroid> old_player =
       SwapPlayer(player_id, new_player);
+  if (!old_player)

[dgn, 2016/04/18 13:18:49]

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?

[whywhat, 2016/04/18 13:21:51]

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.

[aberent, 2016/04/18 13:44:57]

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.

[whywhat, 2016/04/18 14:54:45]

Good point, remove and delete the alternative player now if the current player
is not found.

+    return;
+
   alternative_players_.weak_erase(it);
   alternative_players_.push_back(old_player.release());
 }
 
 void RemoteMediaPlayerManager::SwitchToRemotePlayer(
     int player_id,
     const std::string& casting_message) {
   DCHECK(!IsPlayingRemotely(player_id));
   SwapCurrentPlayer(player_id);
   players_playing_remotely_.insert(player_id);
@@ skipping 105 matching lines @@
     MediaPlayerAndroid* local_player = GetLocalPlayer(player_id);
     Send(new MediaPlayerMsg_MediaMetadataChanged(
         RoutingID(), player_id, duration, local_player->GetVideoWidth(),
         local_player->GetVideoHeight(), success));
   } else {
     BrowserMediaPlayerManager::OnMediaMetadataChanged(player_id, duration,
                                                       width, height, success);
   }
 }
 } // namespace remote_media