Plugin Power Saver: Fix PluginPreroller outliving the PluginInstanceT…

…hrottler

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 260ce72)

Review URL: https://codereview.chromium.org/962313002

Cr-Commit-Position: refs/branch-heads/2311@{#49}
Cr-Branched-From: 09b7de5-refs/heads/master@{#317474}

chrome/renderer/chrome_content_renderer_client.cc

@@ -879,19 +879,16 @@ WebPlugin* ChromeContentRendererClient::CreatePlugin(
 
         scoped_ptr<content::PluginInstanceThrottler> 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)

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);

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_;

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 */);
 }
 

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_;

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() {}
 

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() {}

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,

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_;