Reland: Fix WebViewPlugin::scheduleAnimation crash

The crash was probably due to accessing a dangling pointer to the plugin
container during a small time frame between PepperWebPluginImpl::destroy()
and the destructor being called. (Speculated from source since no reliable
repro is found.)

This CL clears eveything in the destroy() function as if the destructor has
been called, only delaying memory release.

In additional to the original CL https://codereview.chromium.org/1137663006/
This CL also switched the destruction order of pepper instance and the
throttler. Engaging throttle during destruction cause crash and doesn't make
sense anyway.

R=tommycli,raymes
BUG=483068,487607

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

Cr-Commit-Position: refs/heads/master@{#330638}

chrome/browser/plugins/plugin_power_saver_browsertest.cc

@@ -99,6 +99,8 @@ class PluginPowerSaverBrowserTest : virtual public InProcessBrowserTest {
     command_line->AppendSwitch(switches::kEnablePluginPowerSaver);
     command_line->AppendSwitch(switches::kEnablePepperTesting);
     command_line->AppendSwitch(switches::kEnablePluginPlaceholderTesting);
+    command_line->AppendSwitchASCII(
+        switches::kOverridePluginPowerSaverForTesting, "ignore-list");
 
     ASSERT_TRUE(ppapi::RegisterPowerSaverTestPlugin(command_line));
   }

chrome/browser/prerender/prerender_browsertest.cc

@@ -1119,6 +1119,8 @@ class PrerenderBrowserTest : virtual public InProcessBrowserTest {
     command_line->AppendSwitchASCII(switches::kPrerenderMode,
                                     switches::kPrerenderModeSwitchValueEnabled);
     command_line->AppendSwitch(switches::kEnablePepperTesting);
+    command_line->AppendSwitchASCII(
+        switches::kOverridePluginPowerSaverForTesting, "ignore-list");
 
     ASSERT_TRUE(ppapi::RegisterPowerSaverTestPlugin(command_line));
   }

content/browser/renderer_host/render_process_host_impl.cc

@@ -1295,6 +1295,7 @@ void RenderProcessHostImpl::PropagateBrowserCommandLineToRenderer(
     switches::kMemoryMetrics,
     switches::kNoReferrers,
     switches::kNoSandbox,
+    switches::kOverridePluginPowerSaverForTesting,
     switches::kPpapiInProcess,
     switches::kProfilerTiming,
     switches::kReducedReferrerGranularity,

content/content_tests.gypi

@@ -290,6 +290,7 @@
       'renderer/pepper/pepper_file_chooser_host_unittest.cc',
       'renderer/pepper/pepper_graphics_2d_host_unittest.cc',
       'renderer/pepper/pepper_url_request_unittest.cc',
+      'renderer/pepper/pepper_webplugin_impl_browsertest.cc',
       'renderer/pepper/plugin_power_saver_helper_browsertest.cc',
       'test/ppapi/ppapi_browsertest.cc',
       'test/ppapi/ppapi_test.cc',

content/public/common/content_switches.cc

@@ -630,6 +630,14 @@ const char kDisableAppContainer[]           = "disable-appcontainer";
 // Number of worker threads used to rasterize content.
 const char kNumRasterThreads[]              = "num-raster-threads";
 
+// Override the behavior of plugin throttling for testing.
+// By default the throttler is only enabled for a hard-coded list of plugins.
+// Set the value to 'never' to disable throttling.
+// Set the value to 'ignore-list' to ignore the hard-coded list.
+// Set the value to 'always' to always throttle every plugin instance.
+const char kOverridePluginPowerSaverForTesting[] =
+    "override-plugin-power-saver-for-testing";
+
 // Controls the behavior of history navigation in response to horizontal
 // overscroll.
 // Set the value to '0' to disable.

content/public/common/content_switches.h

@@ -182,6 +182,7 @@ CONTENT_EXPORT extern const char kNoReferrers[];
 CONTENT_EXPORT extern const char kNoSandbox[];
 CONTENT_EXPORT extern const char kDisableAppContainer[];
 CONTENT_EXPORT extern const char kNumRasterThreads[];
+CONTENT_EXPORT extern const char kOverridePluginPowerSaverForTesting[];
 CONTENT_EXPORT extern const char kOverscrollHistoryNavigation[];
 extern const char kPluginLauncher[];
 CONTENT_EXPORT extern const char kPluginPath[];

content/renderer/pepper/pepper_plugin_instance_impl.cc

@@ -670,6 +670,13 @@ void PepperPluginInstanceImpl::Delete() {
 
   // Keep a reference on the stack. See NOTE above.
   scoped_refptr<PepperPluginInstanceImpl> ref(this);
+
+  // It is important to destroy the throttler before anything else.
+  // The plugin instance may flush its graphics pipeline during its postmortem
+  // spasm, causing the throttler to engage and obtain new dangling reference
+  // to the plugin container being destroyed.
+  throttler_.reset();
+
   // Force the MessageChannel to release its "passthrough object" which should
   // release our last reference to the "InstanceObject" and will probably
   // destroy it. We want to do this prior to calling DidDestroy in case the

content/renderer/pepper/pepper_webplugin_impl_browsertest.cc

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/command_line.h"
+#include "content/public/common/content_client.h"
+#include "content/public/common/content_constants.h"
+#include "content/public/common/content_switches.h"
+#include "content/public/common/pepper_plugin_info.h"
+#include "content/public/renderer/content_renderer_client.h"
+#include "content/public/test/render_view_test.h"
+#include "content/renderer/pepper/pepper_webplugin_impl.h"
+#include "content/renderer/pepper/plugin_instance_throttler_impl.h"
+#include "content/renderer/pepper/plugin_module.h"
+#include "content/renderer/pepper/renderer_ppapi_host_impl.h"
+#include "content/renderer/render_frame_impl.h"
+#include "content/test/test_content_client.h"
+#include "ppapi/c/pp_errors.h"
+#include "ppapi/c/ppb_core.h"
+#include "ppapi/c/ppb_graphics_2d.h"
+#include "ppapi/c/ppb_image_data.h"
+#include "ppapi/c/ppb_instance.h"
+#include "ppapi/c/ppp_instance.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/WebKit/public/web/WebLocalFrame.h"
+
+namespace content {
+namespace {
+
+class PepperWebPluginImplBrowserTest
+    : public RenderViewTest,
+      public PluginInstanceThrottler::Observer {
+ public:
+  PepperWebPluginImplBrowserTest()
+      : throttler_(nullptr),
+        throttle_engaged_(false),
+        pp_module_(0),
+        pp_instance_(0),
+        graphics2d_(0) {}
+
+  void SetUp() override {
+    base::CommandLine& command_line = *base::CommandLine::ForCurrentProcess();
+    command_line.AppendSwitchASCII(
+        switches::kOverridePluginPowerSaverForTesting, "always");
+
+    current_test_ = this;
+    RenderViewTest::SetUp();
+  }
+  void TearDown() override {
+    RenderViewTest::TearDown();
+    current_test_ = nullptr;
+  }
+  ContentClient* CreateContentClient() override {
+    return new MockContentClient;
+  }
+  ContentRendererClient* CreateContentRendererClient() override {
+    return new MockContentRendererClient;
+  }
+
+  // PluginInstanceThrottler::Observer implementation
+  void OnThrottleStateChange() override {
+    if (throttler_->IsThrottled())
+      throttle_engaged_ = true;
+  }
+
+ protected:
+  // PPP implementation
+  static const void* GetInterface(const char* name) {
+    static PPP_Instance ppp_instance = {
+        &PepperWebPluginImplBrowserTest::DidCreate,
+        &PepperWebPluginImplBrowserTest::DidDestroy,
+        &PepperWebPluginImplBrowserTest::DidChangeView,
+        &PepperWebPluginImplBrowserTest::DidChangeFocus,
+        &PepperWebPluginImplBrowserTest::HandleDocumentLoad};
+    if (!strcmp(name, PPP_INSTANCE_INTERFACE))
+      return &ppp_instance;
+    return nullptr;
+  }
+  static int InitializeModule(PP_Module module,
+                              PPB_GetInterface get_interface) {
+    EXPECT_EQ(0, current_test_->pp_module_);
+    current_test_->pp_module_ = module;
+    ppb_core_ = static_cast<const PPB_Core*>(get_interface(PPB_CORE_INTERFACE));
+    ppb_graphics2d_ = static_cast<const PPB_Graphics2D*>(
+        get_interface(PPB_GRAPHICS_2D_INTERFACE));
+    ppb_image_data_ = static_cast<const PPB_ImageData*>(
+        get_interface(PPB_IMAGEDATA_INTERFACE));
+    ppb_instance_ =
+        static_cast<const PPB_Instance*>(get_interface(PPB_INSTANCE_INTERFACE));
+    return PP_OK;
+  }
+  static void ShutdownModule() {
+    EXPECT_NE(0, current_test_->pp_module_);
+    current_test_->pp_module_ = 0;
+  }
+
+  static void DummyCallback(void*, int32_t) {}
+
+  void PaintSomething() {
+    PP_Size size = {2, 1};
+    PP_Resource image = ppb_image_data_->Create(
+        pp_instance_, ppb_image_data_->GetNativeImageDataFormat(), &size,
+        PP_TRUE);
+    int32_t* pixels = static_cast<int32_t*>(ppb_image_data_->Map(image));
+    pixels[0] = 0xff000000;
+    pixels[1] = 0xffffffff;
+    ppb_image_data_->Unmap(image);
+    ppb_graphics2d_->ReplaceContents(graphics2d_, image);
+    PP_CompletionCallback callback = {
+        &PepperWebPluginImplBrowserTest::DummyCallback, nullptr, 0};
+    ppb_graphics2d_->Flush(graphics2d_, callback);
+    ppb_core_->ReleaseResource(image);
+  }
+
+  // PPP_Instance implementation
+  static PP_Bool DidCreate(PP_Instance instance,
+                           uint32_t,
+                           const char* [],
+                           const char* []) {
+    EXPECT_EQ(0, current_test_->pp_instance_);
+    current_test_->pp_instance_ = instance;
+    PP_Size size = {2, 1};
+    current_test_->graphics2d_ =
+        ppb_graphics2d_->Create(instance, &size, PP_TRUE);
+    ppb_instance_->BindGraphics(instance, current_test_->graphics2d_);
+    return PP_TRUE;
+  }
+  static void DidDestroy(PP_Instance instance) {
+    EXPECT_NE(0, current_test_->pp_instance_);
+    current_test_->PaintSomething();
+    ppb_core_->ReleaseResource(current_test_->graphics2d_);
+    current_test_->pp_instance_ = 0;
+  }
+  static void DidChangeView(PP_Instance, PP_Resource) {}
+  static void DidChangeFocus(PP_Instance, PP_Bool) {}
+  static PP_Bool HandleDocumentLoad(PP_Instance, PP_Resource) {
+    return PP_FALSE;
+  }
+
+  static PepperPluginInfo GetPluginInfo() {
+    PepperPluginInfo info;
+    info.is_internal = true;
+    info.path = base::FilePath(FILE_PATH_LITERAL("internal-always-throttle"));
+    info.name = "Always Throttle";
+    info.mime_types.push_back(
+        WebPluginMimeType("test/always-throttle", "", ""));
+    info.internal_entry_points.get_interface =
+        &PepperWebPluginImplBrowserTest::GetInterface;
+    info.internal_entry_points.initialize_module =
+        &PepperWebPluginImplBrowserTest::InitializeModule;
+    info.internal_entry_points.shutdown_module =
+        &PepperWebPluginImplBrowserTest::ShutdownModule;
+    return info;
+  }
+
+  class MockContentClient : public TestContentClient {
+   public:
+    void AddPepperPlugins(std::vector<PepperPluginInfo>* plugins) override {
+      plugins->push_back(GetPluginInfo());
+    }
+  };
+  class MockContentRendererClient : public ContentRendererClient {
+   public:
+    bool OverrideCreatePlugin(RenderFrame* render_frame,
+                              blink::WebLocalFrame* frame,
+                              const blink::WebPluginParams& params,
+                              blink::WebPlugin** plugin) override {
+      current_test_->throttler_ = new PluginInstanceThrottlerImpl;
+      current_test_->throttler_->AddObserver(current_test_);
+      *plugin = render_frame->CreatePlugin(frame,
+          GetPluginInfo().ToWebPluginInfo(), params,
+          make_scoped_ptr(current_test_->throttler_));
+      return *plugin;
+    }
+  };
+
+  PluginInstanceThrottlerImpl* throttler_;
+  bool throttle_engaged_;
+  PP_Module pp_module_;
+  PP_Instance pp_instance_;
+  PP_Resource graphics2d_;
+  static PepperWebPluginImplBrowserTest* current_test_;
+  static const PPB_Core* ppb_core_;
+  static const PPB_Graphics2D* ppb_graphics2d_;
+  static const PPB_ImageData* ppb_image_data_;
+  static const PPB_Instance* ppb_instance_;
+};
+PepperWebPluginImplBrowserTest* PepperWebPluginImplBrowserTest::current_test_;
+const PPB_Core* PepperWebPluginImplBrowserTest::ppb_core_;
+const PPB_Graphics2D* PepperWebPluginImplBrowserTest::ppb_graphics2d_;
+const PPB_ImageData* PepperWebPluginImplBrowserTest::ppb_image_data_;
+const PPB_Instance* PepperWebPluginImplBrowserTest::ppb_instance_;
+
+// This test simulates the behavior of a plugin that emits new frames during
+// destruction. The throttler shouldn't engage and create a placeholder for
+// a to-be destroyed plugin in such case. See crbug.com/483068
+TEST_F(PepperWebPluginImplBrowserTest, NotEngageThrottleDuringDestroy) {
+  LoadHTML("<!DOCTYPE html><object type='test/always-throttle'></object>");
+  EXPECT_NE(0, pp_instance_);
+  LoadHTML("");
+  EXPECT_EQ(0, pp_instance_);
+  EXPECT_FALSE(throttle_engaged_);
+}
+
+}  // unnamed namespace
+
+}  // namespace content

content/renderer/pepper/plugin_power_saver_helper.cc

@@ -4,10 +4,14 @@
 
 #include "content/renderer/pepper/plugin_power_saver_helper.h"
 
+#include <string>
+
+#include "base/command_line.h"
 #include "base/metrics/histogram.h"
 #include "base/strings/string_number_conversions.h"
 #include "content/common/frame_messages.h"
 #include "content/public/common/content_constants.h"
+#include "content/public/common/content_switches.h"
 #include "content/public/renderer/render_frame.h"
 #include "ppapi/shared_impl/ppapi_constants.h"
 #include "third_party/WebKit/public/web/WebDocument.h"
@@ -60,7 +64,17 @@ PluginPowerSaverHelper::PeripheralPlugin::~PeripheralPlugin() {
 }
 
 PluginPowerSaverHelper::PluginPowerSaverHelper(RenderFrame* render_frame)
-    : RenderFrameObserver(render_frame) {
+    : RenderFrameObserver(render_frame)
+    , override_for_testing_(Normal) {
+  base::CommandLine& command_line = *base::CommandLine::ForCurrentProcess();
+  std::string override_for_testing = command_line.GetSwitchValueASCII(
+      switches::kOverridePluginPowerSaverForTesting);
+  if (override_for_testing == "never")
+    override_for_testing_ = Never;
+  else if (override_for_testing == "ignore-list")
+    override_for_testing_ = IgnoreList;
+  else if (override_for_testing == "always")
+    override_for_testing_ = Always;
 }
 
 PluginPowerSaverHelper::~PluginPowerSaverHelper() {
@@ -123,8 +137,12 @@ bool PluginPowerSaverHelper::ShouldThrottleContent(
 
   // This feature has only been tested throughly with Flash thus far.
   // It is also enabled for the Power Saver test plugin for browser tests.
-  if (plugin_module_name != content::kFlashPluginName &&
-      plugin_module_name != ppapi::kPowerSaverTestPluginName) {
+  if (override_for_testing_ == Always) {
+    return true;
+  } else if (override_for_testing_ == Never) {
+    return false;
+  } else if (override_for_testing_ == Normal &&
+             plugin_module_name != content::kFlashPluginName) {
     return false;
   }
 

content/renderer/pepper/plugin_power_saver_helper.h

@@ -68,6 +68,13 @@ class CONTENT_EXPORT PluginPowerSaverHelper : public RenderFrameObserver {
     base::Closure unthrottle_callback;
   };
 
+  enum OverrideForTesting {
+    Normal,
+    Never,
+    IgnoreList,
+    Always
+  };
+
   // RenderFrameObserver implementation.
   void DidCommitProvisionalLoad(bool is_new_navigation,
                                 bool is_same_page_navigation) override;
@@ -76,6 +83,8 @@ class CONTENT_EXPORT PluginPowerSaverHelper : public RenderFrameObserver {
   void OnUpdatePluginContentOriginWhitelist(
       const std::set<GURL>& origin_whitelist);
 
+  OverrideForTesting override_for_testing_;
+
   // Local copy of the whitelist for the entire tab.
   std::set<GURL> origin_whitelist_;