Commit 65955dcb authored by liberato@chromium.org's avatar liberato@chromium.org Committed by Commit Bot

Use per-RenderFrame weak pointers for media power experiment.

Previously, we would use one weak pointer for the media
WebContentsObserver for all power experiment callbacks.  However,
since the experiment manager posts callbacks, it's possible that a
callback will run after its RenderFrameHost is deleted.  Since we
use the RenderFrameHost to notify the renderer, this would be a
problem in that case.

This CL adds per-frame weak pointers that are invalidated after the
frame is destroyed, so that any pending callbacks don't run.

Change-Id: I02835ebfd620816e385fd215472dd78ecbeae4fa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1897701Reviewed-by: default avatarChrome Cunningham <chcunningham@chromium.org>
Commit-Queue: Frank Liberato <liberato@chromium.org>
Cr-Commit-Position: refs/heads/master@{#712651}
parent ff80b841
...@@ -67,8 +67,7 @@ MediaWebContentsObserver::MediaWebContentsObserver(WebContents* web_contents) ...@@ -67,8 +67,7 @@ MediaWebContentsObserver::MediaWebContentsObserver(WebContents* web_contents)
: WebContentsObserver(web_contents), : WebContentsObserver(web_contents),
audible_metrics_(GetAudibleMetrics()), audible_metrics_(GetAudibleMetrics()),
session_controllers_manager_(this), session_controllers_manager_(this),
power_experiment_manager_(MediaPowerExperimentManager::Instance()), power_experiment_manager_(MediaPowerExperimentManager::Instance()) {}
weak_factory_(this) {}
MediaWebContentsObserver::~MediaWebContentsObserver() { MediaWebContentsObserver::~MediaWebContentsObserver() {
// Remove all players so that the experiment manager is notified. // Remove all players so that the experiment manager is notified.
...@@ -97,6 +96,9 @@ void MediaWebContentsObserver::RenderFrameDeleted( ...@@ -97,6 +96,9 @@ void MediaWebContentsObserver::RenderFrameDeleted(
picture_in_picture_allowed_in_fullscreen_.reset(); picture_in_picture_allowed_in_fullscreen_.reset();
fullscreen_player_.reset(); fullscreen_player_.reset();
} }
// Cancel any pending callbacks for players from this frame.
per_frame_factory_.erase(render_frame_host);
} }
void MediaWebContentsObserver::MaybeUpdateAudibleState() { void MediaWebContentsObserver::MaybeUpdateAudibleState() {
...@@ -364,7 +366,7 @@ void MediaWebContentsObserver::AddMediaPlayerEntry( ...@@ -364,7 +366,7 @@ void MediaWebContentsObserver::AddMediaPlayerEntry(
power_experiment_manager_->PlayerStarted( power_experiment_manager_->PlayerStarted(
id, id,
base::BindRepeating(&MediaWebContentsObserver::OnExperimentStateChanged, base::BindRepeating(&MediaWebContentsObserver::OnExperimentStateChanged,
weak_factory_.GetWeakPtr(), id)); GetWeakPtrForFrame(id.render_frame_host), id));
} }
} }
...@@ -450,4 +452,17 @@ void MediaWebContentsObserver::RemoveAllPlayers() { ...@@ -450,4 +452,17 @@ void MediaWebContentsObserver::RemoveAllPlayers() {
RemoveAllPlayers(&active_video_players_); RemoveAllPlayers(&active_video_players_);
} }
base::WeakPtr<MediaWebContentsObserver>
MediaWebContentsObserver::GetWeakPtrForFrame(
RenderFrameHost* render_frame_host) {
auto iter = per_frame_factory_.find(render_frame_host);
if (iter != per_frame_factory_.end())
return iter->second->GetWeakPtr();
auto result = per_frame_factory_.emplace(std::make_pair(
render_frame_host,
std::make_unique<base::WeakPtrFactory<MediaWebContentsObserver>>(this)));
return result.first->second->GetWeakPtr();
}
} // namespace content } // namespace content
...@@ -172,6 +172,12 @@ class CONTENT_EXPORT MediaWebContentsObserver : public WebContentsObserver { ...@@ -172,6 +172,12 @@ class CONTENT_EXPORT MediaWebContentsObserver : public WebContentsObserver {
// Remove all players. // Remove all players.
void RemoveAllPlayers(); void RemoveAllPlayers();
// Return a weak pointer to |this| that's local to |render_frame_host|, in the
// sense that we can cancel all of the ptrs to one frame without cancelling
// pointers for any of the others.
base::WeakPtr<MediaWebContentsObserver> GetWeakPtrForFrame(
RenderFrameHost* render_frame_host);
// Helper class for recording audible metrics. // Helper class for recording audible metrics.
AudibleMetrics* audible_metrics_; AudibleMetrics* audible_metrics_;
...@@ -186,7 +192,9 @@ class CONTENT_EXPORT MediaWebContentsObserver : public WebContentsObserver { ...@@ -186,7 +192,9 @@ class CONTENT_EXPORT MediaWebContentsObserver : public WebContentsObserver {
MediaSessionControllersManager session_controllers_manager_; MediaSessionControllersManager session_controllers_manager_;
MediaPowerExperimentManager* power_experiment_manager_ = nullptr; MediaPowerExperimentManager* power_experiment_manager_ = nullptr;
base::WeakPtrFactory<MediaWebContentsObserver> weak_factory_; std::map<RenderFrameHost*,
std::unique_ptr<base::WeakPtrFactory<MediaWebContentsObserver>>>
per_frame_factory_;
DISALLOW_COPY_AND_ASSIGN(MediaWebContentsObserver); DISALLOW_COPY_AND_ASSIGN(MediaWebContentsObserver);
}; };
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment