Defensive fix for crash in MediaWebContentsobserver

This defensive fix until MediaSession owners return from vacation.
Reverting is challenging given the long string of CLs.

GetMediaPlayerRemote(player_id) is crashing for failing to find the
remote with that ID. The stack shows the call coming from
MediaSessionController, which should have the same lifetime as its
associated mojo::Remote<media::mojom::MediaPlayer>. So far, I have not
found how those lifetimes are getting out of sync.

Ownership looks like this (read -> as "owns", MWCO as MediaWebContentsObserver):
MWCO -> MediaSessionControllersManager -> map<MediaPlayerId, MediaSessionController(s)>
MWCO -> map<MediaPlayerId, mojo::Remote<media::mojom::MediaPlayer>>

I've combed through all the places where MWCO removes from its map of
MediaPlayer remotes, and looked for a failure to call
MediaSessionControllersManger::OnEnd(Id) to remove the associated
MediaSessionController. There are 3 cases where we remove from the
map...
1. MediaWebContentsObserver::RenderFrameDeleted()
2. MediaWebContentsObserver::WebContentsDestroyed()
3. media_player_remotes_[player_id].set_disconnect_handler(base::BindOnce(...

The only case where I don't see a call to
MediaSessionControllersManager is #2. But that shouldn't matter since
MWCO's destructor is called shortly (synchronously) after
WebContentsDestroyed(). Its destructor will tear down the
MediaSessionControllersManager.

The defensive fix does not address the root lifetime issue. It is
probably the case that the MediaSessionControllers are being leaked.

Bug: 1161551, 1039252
Change-Id: I85337eae106d8b56b7d48fc9759702dc579855ca
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2607579
Commit-Queue: Chrome Cunningham <chcunningham@chromium.org>
Commit-Queue: Frank Liberato <liberato@chromium.org>
Reviewed-by: Frank Liberato <liberato@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Auto-Submit: Chrome Cunningham <chcunningham@chromium.org>
Cr-Commit-Position: refs/heads/master@{#839818}

content/browser/media/media_web_contents_observer.cc

@@ -525,11 +525,14 @@ void MediaWebContentsObserver::OnReceivedTranslatedDeviceId(
       MediaPlayerId(render_frame_host, delegate_id), raw_device_id);
 }
 
-mojo::Remote<media::mojom::MediaPlayer>&
-MediaWebContentsObserver::GetMediaPlayerRemote(const MediaPlayerId& player_id) {
-  DCHECK(media_player_remotes_.contains(player_id));
-  DCHECK(media_player_remotes_[player_id].is_bound());
-  return media_player_remotes_.at(player_id);
+media::mojom::MediaPlayer* MediaWebContentsObserver::GetMediaPlayerRemote(
+    const MediaPlayerId& player_id) {
+  if (media_player_remotes_.contains(player_id)) {
+    DCHECK(media_player_remotes_[player_id].is_bound());
+    return media_player_remotes_.at(player_id).get();
+  }
+
+  return nullptr;
 }
 
 void MediaWebContentsObserver::OnMediaPlayerHostDisconnected(

content/browser/media/media_web_contents_observer.h

@@ -110,9 +110,11 @@ class CONTENT_EXPORT MediaWebContentsObserver : public WebContentsObserver {
                                     int delegate_id,
                                     const std::string& raw_device_id);
 
-  // Return an already bound mojo Remote for the MediaPlayer mojo interface. It
-  // is an error to call this method if no MediaPlayer with |player_id| exists.
-  mojo::Remote<media::mojom::MediaPlayer>& GetMediaPlayerRemote(
+  // Return an already bound mojo Remote for the MediaPlayer mojo interface.
+  // Return null if no player with |player_id| exists.
+  // TODO(https://crbug.com/1161551): Revert to returning a reference and make
+  // it an error to call this method if no MediaPlayer with |player_id| exists.
+  media::mojom::MediaPlayer* GetMediaPlayerRemote(
       const MediaPlayerId& player_id);
 
   // Creates a new MediaPlayerObserverHostImpl associated to |player_id| if

content/browser/media/session/media_session_controller.cc

@@ -50,34 +50,62 @@ bool MediaSessionController::OnPlaybackStarted() {
 
 void MediaSessionController::OnSuspend(int player_id) {
   DCHECK_EQ(player_id_, player_id);
+
+  media::mojom::MediaPlayer* remote =
+      web_contents_->media_web_contents_observer()->GetMediaPlayerRemote(id_);
+  if (!remote) {
+    // TODO(https://crbug.com/1161551): Remove this when lifetime bug is fixed.
+    NOTREACHED() << "Controller should not outlive remote MediaPlayer";
+    return;
+  }
+
   // TODO(crbug.com/953645): Set triggered_by_user to true ONLY if that action
   // was actually triggered by user as this will activate the frame.
-  web_contents_->media_web_contents_observer()
-      ->GetMediaPlayerRemote(id_)
-      ->RequestPause(/*triggered_by_user=*/true);
+  remote->RequestPause(/*triggered_by_user=*/true);
 }
 
 void MediaSessionController::OnResume(int player_id) {
   DCHECK_EQ(player_id_, player_id);
-  web_contents_->media_web_contents_observer()
-      ->GetMediaPlayerRemote(id_)
-      ->RequestPlay();
+
+  media::mojom::MediaPlayer* remote =
+      web_contents_->media_web_contents_observer()->GetMediaPlayerRemote(id_);
+  if (!remote) {
+    // TODO(https://crbug.com/1161551): Remove this when lifetime bug is fixed.
+    NOTREACHED() << "Controller should not outlive remote MediaPlayer";
+    return;
+  }
+
+  remote->RequestPlay();
 }
 
 void MediaSessionController::OnSeekForward(int player_id,
                                            base::TimeDelta seek_time) {
   DCHECK_EQ(player_id_, player_id);
-  web_contents_->media_web_contents_observer()
-      ->GetMediaPlayerRemote(id_)
-      ->RequestSeekForward(seek_time);
+
+  media::mojom::MediaPlayer* remote =
+      web_contents_->media_web_contents_observer()->GetMediaPlayerRemote(id_);
+  if (!remote) {
+    // TODO(https://crbug.com/1161551): Remove this when lifetime bug is fixed.
+    NOTREACHED() << "Controller should not outlive remote MediaPlayer";
+    return;
+  }
+
+  remote->RequestSeekForward(seek_time);
 }
 
 void MediaSessionController::OnSeekBackward(int player_id,
                                             base::TimeDelta seek_time) {
   DCHECK_EQ(player_id_, player_id);
-  web_contents_->media_web_contents_observer()
-      ->GetMediaPlayerRemote(id_)
-      ->RequestSeekBackward(seek_time);
+
+  media::mojom::MediaPlayer* remote =
+      web_contents_->media_web_contents_observer()->GetMediaPlayerRemote(id_);
+  if (!remote) {
+    // TODO(https://crbug.com/1161551): Remove this when lifetime bug is fixed.
+    NOTREACHED() << "Controller should not outlive remote MediaPlayer";
+    return;
+  }
+
+  remote->RequestSeekBackward(seek_time);
 }
 
 void MediaSessionController::OnSetVolumeMultiplier(int player_id,
@@ -91,17 +119,29 @@ void MediaSessionController::OnSetVolumeMultiplier(int player_id,
 void MediaSessionController::OnEnterPictureInPicture(int player_id) {
   DCHECK_EQ(player_id_, player_id);
 
-  web_contents_->media_web_contents_observer()
-      ->GetMediaPlayerRemote(id_)
-      ->RequestEnterPictureInPicture();
+  media::mojom::MediaPlayer* remote =
+      web_contents_->media_web_contents_observer()->GetMediaPlayerRemote(id_);
+  if (!remote) {
+    // TODO(https://crbug.com/1161551): Remove this when lifetime bug is fixed.
+    NOTREACHED() << "Controller should not outlive remote MediaPlayer";
+    return;
+  }
+
+  remote->RequestEnterPictureInPicture();
 }
 
 void MediaSessionController::OnExitPictureInPicture(int player_id) {
   DCHECK_EQ(player_id_, player_id);
 
-  web_contents_->media_web_contents_observer()
-      ->GetMediaPlayerRemote(id_)
-      ->RequestExitPictureInPicture();
+  media::mojom::MediaPlayer* remote =
+      web_contents_->media_web_contents_observer()->GetMediaPlayerRemote(id_);
+  if (!remote) {
+    // TODO(https://crbug.com/1161551): Remove this when lifetime bug is fixed.
+    NOTREACHED() << "Controller should not outlive remote MediaPlayer";
+    return;
+  }
+
+  remote->RequestExitPictureInPicture();
 }
 
 void MediaSessionController::OnSetAudioSinkId(