From b435753c61c7201f101fa57ea49373f72595ff52 Mon Sep 17 00:00:00 2001 From: "Tommy C. Li" Date: Fri, 27 Feb 2015 10:47:27 -0800 Subject: [PATCH] Plugin Power Saver: Fix PluginPreroller outliving the PluginInstanceThrottler Merge to M42. This fixes a use-after-destroy bug leading to crashes. Object Lifetimes: blink::WebPlugin >= PluginInstanceThrottler >= PluginPreroller. The PluginPreroller is supposed to observe the Throttler and destroy itself when the Throttler gets destroyed. Previously, the WebPlugin would sometimes destroy the Throttler before the PluginPreroller was ever created. The PluginPreroller would then try to observe a non-existent throttler, and crash. This patch now attaches the PluginPreroller to the Throttler before the blink::WebPlugin has a chance to destroy the Throttler. BUG=459920, 458326, 403800 Review URL: https://codereview.chromium.org/941803003 Cr-Commit-Position: refs/heads/master@{#317837} (cherry picked from commit 260ce7214bf5e2299efd4797a7833dddbb7ebb55) Review URL: https://codereview.chromium.org/962313002 Cr-Commit-Position: refs/branch-heads/2311@{#49} Cr-Branched-From: 09b7de5dd7254947cd4306de907274fa63373d48-refs/heads/master@{#317474} --- .../chrome_content_renderer_client.cc | 9 +++---- chrome/renderer/plugins/plugin_preroller.cc | 8 +++--- chrome/renderer/plugins/plugin_preroller.h | 2 -- .../renderer/loadable_plugin_placeholder.cc | 27 +++++++------------ .../renderer/loadable_plugin_placeholder.h | 4 +-- .../renderer/plugin_instance_throttler.h | 3 +++ .../renderer/pepper/pepper_webplugin_impl.cc | 3 +++ .../pepper/plugin_instance_throttler_impl.cc | 11 ++++++++ .../pepper/plugin_instance_throttler_impl.h | 5 ++++ 9 files changed, 38 insertions(+), 34 deletions(-) diff --git a/chrome/renderer/chrome_content_renderer_client.cc b/chrome/renderer/chrome_content_renderer_client.cc index 631eafe0f3297..68676109e1cea 100644 --- a/chrome/renderer/chrome_content_renderer_client.cc +++ b/chrome/renderer/chrome_content_renderer_client.cc @@ -879,19 +879,16 @@ WebPlugin* ChromeContentRendererClient::CreatePlugin( scoped_ptr throttler = PluginInstanceThrottler::Create(power_saver_enabled); - content::PluginInstanceThrottler* throttler_raw = throttler.get(); - blink::WebPlugin* plugin = - render_frame->CreatePlugin(frame, info, params, throttler.Pass()); - if (power_saver_enabled) { // PluginPreroller manages its own lifetime. new PluginPreroller( render_frame, frame, params, info, identifier, group_name, l10n_util::GetStringFUTF16(IDS_PLUGIN_BLOCKED, group_name), - plugin, throttler_raw); + throttler.get()); } - return plugin; + return render_frame->CreatePlugin(frame, info, params, + throttler.Pass()); #else // !defined(ENABLE_PLUGINS) return render_frame->CreatePlugin(frame, info, params, nullptr); #endif // defined(ENABLE_PLUGINS) diff --git a/chrome/renderer/plugins/plugin_preroller.cc b/chrome/renderer/plugins/plugin_preroller.cc index fecfb6dad126a..618a7c340bf2f 100644 --- a/chrome/renderer/plugins/plugin_preroller.cc +++ b/chrome/renderer/plugins/plugin_preroller.cc @@ -20,7 +20,6 @@ PluginPreroller::PluginPreroller(content::RenderFrame* render_frame, const std::string& identifier, const base::string16& name, const base::string16& message, - blink::WebPlugin* plugin, content::PluginInstanceThrottler* throttler) : RenderFrameObserver(render_frame), frame_(frame), @@ -29,9 +28,7 @@ PluginPreroller::PluginPreroller(content::RenderFrame* render_frame, identifier_(identifier), name_(name), message_(message), - plugin_(plugin), throttler_(throttler) { - DCHECK(plugin); DCHECK(throttler); throttler_->AddObserver(this); } @@ -65,11 +62,12 @@ void PluginPreroller::OnThrottleStateChange() { ChromePluginPlaceholder::CreateBlockedPlugin( render_frame(), frame_, params_, info_, identifier_, name_, IDR_PLUGIN_POSTER_HTML, message_, keyframe_data_url_); - placeholder->SetPremadePlugin(plugin_, throttler_); + placeholder->SetPremadePlugin(throttler_); placeholder->set_power_saver_enabled(true); placeholder->set_allow_loading(true); - blink::WebPluginContainer* container = plugin_->container(); + blink::WebPluginContainer* container = + throttler_->GetWebPlugin()->container(); container->setPlugin(placeholder->plugin()); bool success = placeholder->plugin()->initialize(container); diff --git a/chrome/renderer/plugins/plugin_preroller.h b/chrome/renderer/plugins/plugin_preroller.h index 78e34b3a3b791..b059d77f040e7 100644 --- a/chrome/renderer/plugins/plugin_preroller.h +++ b/chrome/renderer/plugins/plugin_preroller.h @@ -34,7 +34,6 @@ class PluginPreroller : public content::PluginInstanceThrottler::Observer, const std::string& identifier, const base::string16& name, const base::string16& message, - blink::WebPlugin* plugin, content::PluginInstanceThrottler* throttler); ~PluginPreroller() override; @@ -52,7 +51,6 @@ class PluginPreroller : public content::PluginInstanceThrottler::Observer, base::string16 name_; base::string16 message_; - blink::WebPlugin* plugin_; content::PluginInstanceThrottler* throttler_; GURL keyframe_data_url_; diff --git a/components/plugins/renderer/loadable_plugin_placeholder.cc b/components/plugins/renderer/loadable_plugin_placeholder.cc index 5b33949b4c9b7..8a5a0aa653e95 100644 --- a/components/plugins/renderer/loadable_plugin_placeholder.cc +++ b/components/plugins/renderer/loadable_plugin_placeholder.cc @@ -50,13 +50,9 @@ void LoadablePluginPlaceholder::BlockForPowerSaverPoster() { #endif void LoadablePluginPlaceholder::SetPremadePlugin( - blink::WebPlugin* plugin, content::PluginInstanceThrottler* throttler) { - DCHECK(plugin); DCHECK(throttler); - DCHECK(!premade_plugin_); DCHECK(!premade_throttler_); - premade_plugin_ = plugin; premade_throttler_ = throttler; premade_throttler_->AddObserver(this); @@ -78,7 +74,6 @@ LoadablePluginPlaceholder::LoadablePluginPlaceholder( is_blocked_for_power_saver_poster_(false), power_saver_enabled_(false), plugin_marked_essential_(false), - premade_plugin_(nullptr), premade_throttler_(nullptr), allow_loading_(false), placeholder_was_replaced_(false), @@ -89,7 +84,6 @@ LoadablePluginPlaceholder::LoadablePluginPlaceholder( LoadablePluginPlaceholder::~LoadablePluginPlaceholder() { #if defined(ENABLE_PLUGINS) - DCHECK(!premade_plugin_); DCHECK(!premade_throttler_); if (!plugin_marked_essential_ && !placeholder_was_replaced_ && @@ -139,7 +133,9 @@ void LoadablePluginPlaceholder::ReplacePlugin(WebPlugin* new_plugin) { // Save the element in case the plug-in is removed from the page during // initialization. WebElement element = container->element(); - if (new_plugin != premade_plugin_ && !new_plugin->initialize(container)) { + bool plugin_needs_initialization = + !premade_throttler_ || new_plugin != premade_throttler_->GetWebPlugin(); + if (plugin_needs_initialization && !new_plugin->initialize(container)) { // We couldn't initialize the new plug-in. Restore the old one and abort. container->setPlugin(plugin()); return; @@ -232,13 +228,10 @@ void LoadablePluginPlaceholder::UpdateMessage() { void LoadablePluginPlaceholder::PluginDestroyed() { // Since the premade plugin has been detached from the container, it will not // be automatically destroyed along with the page. - if (!placeholder_was_replaced_ && premade_plugin_) { - DCHECK(premade_throttler_); + if (!placeholder_was_replaced_ && premade_throttler_) { premade_throttler_->RemoveObserver(this); + premade_throttler_->GetWebPlugin()->destroy(); premade_throttler_ = nullptr; - - premade_plugin_->destroy(); - premade_plugin_ = nullptr; } PluginPlaceholder::PluginDestroyed(); @@ -253,7 +246,6 @@ void LoadablePluginPlaceholder::WasShown() { } void LoadablePluginPlaceholder::OnThrottleStateChange() { - DCHECK(premade_plugin_); DCHECK(premade_throttler_); if (!premade_throttler_->IsThrottled()) { // Premade plugin has been unthrottled externally (by audio playback, etc.). @@ -293,13 +285,12 @@ void LoadablePluginPlaceholder::LoadPlugin() { return; } - if (premade_plugin_) { + if (premade_throttler_) { premade_throttler_->RemoveObserver(this); premade_throttler_->SetHiddenForPlaceholder(false /* hidden */); - premade_throttler_ = nullptr; - ReplacePlugin(premade_plugin_); - premade_plugin_ = nullptr; + ReplacePlugin(premade_throttler_->GetWebPlugin()); + premade_throttler_ = nullptr; } else { // TODO(mmenke): In the case of prerendering, feed into // ChromeContentRendererClient::CreatePlugin instead, to @@ -340,7 +331,7 @@ void LoadablePluginPlaceholder::DidFinishLoadingCallback() { // Wait for the placeholder to finish loading to hide the premade plugin. // This is necessary to prevent a flicker. - if (premade_plugin_ && !placeholder_was_replaced_) + if (premade_throttler_ && !placeholder_was_replaced_) premade_throttler_->SetHiddenForPlaceholder(true /* hidden */); } diff --git a/components/plugins/renderer/loadable_plugin_placeholder.h b/components/plugins/renderer/loadable_plugin_placeholder.h index ee24663b9ab63..bc09b73fdf39e 100644 --- a/components/plugins/renderer/loadable_plugin_placeholder.h +++ b/components/plugins/renderer/loadable_plugin_placeholder.h @@ -37,8 +37,7 @@ class LoadablePluginPlaceholder void set_allow_loading(bool allow_loading) { allow_loading_ = allow_loading; } // When we load the plugin, use this already-created plugin, not a new one. - void SetPremadePlugin(blink::WebPlugin* plugin, - content::PluginInstanceThrottler* throttler); + void SetPremadePlugin(content::PluginInstanceThrottler* throttler); protected: LoadablePluginPlaceholder(content::RenderFrame* render_frame, @@ -123,7 +122,6 @@ class LoadablePluginPlaceholder bool plugin_marked_essential_; // When we load, uses this premade plugin instead of creating a new one. - blink::WebPlugin* premade_plugin_; content::PluginInstanceThrottler* premade_throttler_; bool allow_loading_; diff --git a/content/public/renderer/plugin_instance_throttler.h b/content/public/renderer/plugin_instance_throttler.h index 8243a21ff8be2..d20f5257cd5c7 100644 --- a/content/public/renderer/plugin_instance_throttler.h +++ b/content/public/renderer/plugin_instance_throttler.h @@ -10,6 +10,7 @@ #include "content/common/content_export.h" namespace blink { +class WebPlugin; struct WebPluginParams; } @@ -81,6 +82,8 @@ class CONTENT_EXPORT PluginInstanceThrottler { // Called by the placeholder when the plugin should temporarily be hidden. virtual void SetHiddenForPlaceholder(bool hidden) = 0; + virtual blink::WebPlugin* GetWebPlugin() const = 0; + protected: PluginInstanceThrottler() {} diff --git a/content/renderer/pepper/pepper_webplugin_impl.cc b/content/renderer/pepper/pepper_webplugin_impl.cc index 3ed39200d3e69..a92c83cbc7abd 100644 --- a/content/renderer/pepper/pepper_webplugin_impl.cc +++ b/content/renderer/pepper/pepper_webplugin_impl.cc @@ -77,6 +77,9 @@ PepperWebPluginImpl::PepperWebPluginImpl( // Set subresource URL for crash reporting. base::debug::SetCrashKeyValue("subresource_url", init_data_->url.spec()); + + if (throttler_) + throttler_->SetWebPlugin(this); } PepperWebPluginImpl::~PepperWebPluginImpl() {} diff --git a/content/renderer/pepper/plugin_instance_throttler_impl.cc b/content/renderer/pepper/plugin_instance_throttler_impl.cc index 432dfaaea33e0..1ff6248f3b349 100644 --- a/content/renderer/pepper/plugin_instance_throttler_impl.cc +++ b/content/renderer/pepper/plugin_instance_throttler_impl.cc @@ -50,6 +50,7 @@ PluginInstanceThrottlerImpl::PluginInstanceThrottlerImpl( : state_(power_saver_enabled ? THROTTLER_STATE_AWAITING_KEYFRAME : THROTTLER_STATE_POWER_SAVER_DISABLED), is_hidden_for_placeholder_(false), + web_plugin_(nullptr), consecutive_interesting_frames_(0), frames_examined_(0), weak_factory_(this) { @@ -95,6 +96,16 @@ void PluginInstanceThrottlerImpl::SetHiddenForPlaceholder(bool hidden) { FOR_EACH_OBSERVER(Observer, observer_list_, OnHiddenForPlaceholder(hidden)); } +blink::WebPlugin* PluginInstanceThrottlerImpl::GetWebPlugin() const { + DCHECK(web_plugin_); + return web_plugin_; +} + +void PluginInstanceThrottlerImpl::SetWebPlugin(blink::WebPlugin* web_plugin) { + DCHECK(!web_plugin_); + web_plugin_ = web_plugin; +} + void PluginInstanceThrottlerImpl::Initialize( RenderFrameImpl* frame, const GURL& content_origin, diff --git a/content/renderer/pepper/plugin_instance_throttler_impl.h b/content/renderer/pepper/plugin_instance_throttler_impl.h index 226e19b048575..b223cd26b613c 100644 --- a/content/renderer/pepper/plugin_instance_throttler_impl.h +++ b/content/renderer/pepper/plugin_instance_throttler_impl.h @@ -35,6 +35,9 @@ class CONTENT_EXPORT PluginInstanceThrottlerImpl bool IsHiddenForPlaceholder() const override; void MarkPluginEssential(PowerSaverUnthrottleMethod method) override; void SetHiddenForPlaceholder(bool hidden) override; + blink::WebPlugin* GetWebPlugin() const override; + + void SetWebPlugin(blink::WebPlugin* web_plugin); bool needs_representative_keyframe() const { return state_ == THROTTLER_STATE_AWAITING_KEYFRAME; @@ -83,6 +86,8 @@ class CONTENT_EXPORT PluginInstanceThrottlerImpl bool is_hidden_for_placeholder_; + blink::WebPlugin* web_plugin_; + // Number of consecutive interesting frames we've encountered. int consecutive_interesting_frames_;