Skip to content

Commit

Permalink
Cleanup MediaPlayer-related mojo endpoints upon WebContents destruction
Browse files Browse the repository at this point in the history
Due to the recent migration from legacy IPC messages to mojo, some
crashes have been observed because of MediaSessionController and
MediaSessionImpl objects being alive after the WebContents object
they are associated to has been destroyed.

This seems to have been caused because of a race condition that
can happen if a call to a method of the MediaWebContentsObserver
mojo interface arrives in the browser process once a WebContents
is in the process of being destroyed: such messages will call
methods of MediaSessionControllersManager that will create new
MediaSessionController and MediaSessionImpl objects again, but
with an invalid WebContents object associated to them, causing
the crashes when later on such objects assume a valid WebContents
available (e.g. in MediaSessionImpl::GetMediaSessionInfoSync()).

In order to fix this problem, we need to make sure that we destroy
the objects holding the MediaWebContentsObserver mojo receivers in
the browser process as soon as we know that the either the WebContents
associated to them is about to be destroyed, but before the actual
destructor is run. Likewise, and similar to what's already done for
the map of PlayerInfo objects, we should also make sure we destroy
those objects holding the mojo receivers as soon as the associated
RFHI is getting deleted, so this CL does that as well.

Last, and even though not directly related to this crash, it's
probably a good idea to do a similar cleanup for the map of
MediaPlayerHostImpl objects (which is indexed by RFHI) and the
map of mojo::Remote<media::mojom::MediaPlayer> remotes (indexed
by MediaPlayerId), also owned by MWCO, in the two same cases, so
this CL takes care of adding that extra cleanup too.

Bug: 1157779, 1039252
Change-Id: I4932f43ce8622dbecaf20dbe417f61ee8167647d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2587045
Reviewed-by: Chrome Cunningham <chcunningham@chromium.org>
Commit-Queue: Mario Sanchez Prada <mario@igalia.com>
Cr-Commit-Position: refs/heads/master@{#836840}
  • Loading branch information
mariospr authored and Chromium LUCI CQ committed Dec 14, 2020
1 parent 0ebf0d5 commit a1e8212
Showing 1 changed file with 30 additions and 0 deletions.
30 changes: 30 additions & 0 deletions content/browser/media/media_web_contents_observer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,13 @@ void MediaWebContentsObserver::WebContentsDestroyed() {

// Remove all players so that the experiment manager is notified.
player_info_map_.clear();

// Remove all the mojo receivers and remotes associated to the media players
// handled by this WebContents to prevent from handling/sending any more
// messages after this point, plus properly cleaning things up.
media_player_hosts_.clear();
media_player_observer_hosts_.clear();
media_player_remotes_.clear();
}

void MediaWebContentsObserver::RenderFrameDeleted(
Expand All @@ -169,6 +176,29 @@ void MediaWebContentsObserver::RenderFrameDeleted(
return render_frame_host == id_and_player_info.first.render_frame_host;
});

base::EraseIf(media_player_hosts_,
[render_frame_host](const MediaPlayerHostImplMap::value_type&
media_player_hosts_value_type) {
return render_frame_host ==
media_player_hosts_value_type.first;
});

base::EraseIf(
media_player_observer_hosts_,
[render_frame_host](const MediaPlayerObserverHostImplMap::value_type&
media_player_observer_hosts_value_type) {
return render_frame_host ==
media_player_observer_hosts_value_type.first.render_frame_host;
});

base::EraseIf(
media_player_remotes_,
[render_frame_host](const MediaPlayerRemotesMap::value_type&
media_player_remotes_value_type) {
return render_frame_host ==
media_player_remotes_value_type.first.render_frame_host;
});

session_controllers_manager_.RenderFrameDeleted(render_frame_host);

if (fullscreen_player_ &&
Expand Down

0 comments on commit a1e8212

Please sign in to comment.