Reland "Defensive fix for crash in MediaWebContentsobserver"

This reverts commit 4a8451e4.

Reason for revert: The issue wasn't fixed by [1], so lets reinstate the defensive fix.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/2626920

Original change's description:
> Revert "Defensive fix for crash in MediaWebContentsobserver"
>
> This reverts commit 79de0d4b.
>
> Reason for revert: We expect another CL may be the correct fix. https://chromium-review.googlesource.com/c/chromium/src/+/2626920
>
> Original change's description:
> > 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}
>
> TBR=dcheng@chromium.org,mlamouri@chromium.org,chcunningham@chromium.org,liberato@chromium.org,beccahughes@chromium.org,mario@igalia.com,chromium-scoped@luci-project-accounts.iam.gserviceaccount.com
>
> # Not skipping CQ checks because original CL landed > 1 day ago.
>
> Bug: 1161551
> Bug: 1039252
> Change-Id: Id12ba170758a4479342c0eb3b2c64bb1243f34b1
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2632919
> Reviewed-by: Chrome Cunningham <chcunningham@chromium.org>
> Reviewed-by: Frank Liberato <liberato@chromium.org>
> Commit-Queue: Chrome Cunningham <chcunningham@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#844208}

TBR=dcheng@chromium.org,mlamouri@chromium.org,chcunningham@chromium.org,liberato@chromium.org,beccahughes@chromium.org,mario@igalia.com,chromium-scoped@luci-project-accounts.iam.gserviceaccount.com

# Not skipping CQ checks because original CL landed > 1 day ago.

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

content/browser/media/media_web_contents_observer.cc

@@ -528,11 +528,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(