Fix problems with unloading/reloading/updating extensions that contain NPAPI

plugins, by ensuring that an extension's plugins are shut down and unloaded when the extension unloads.

BUG=34670
BUG=32806

Review URL: http://codereview.chromium.org/1596009

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@43756 0039d316-1c4b-4281-b951-d872f2087c98

14 files changed, 81 insertions, 22 deletions

diff --git a/chrome/browser/extensions/extension_browsertests_misc.cc b/chrome/browser/extensions/extension_browsertests_misc.cc
index 5b30bb0..45cabc2 100644
--- a/chrome/browser/extensions/extension_browsertests_misc.cc
+++ b/chrome/browser/extensions/extension_browsertests_misc.cc
@@ -715,18 +715,21 @@ IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, MAYBE_PluginLoadUnload) {
   UnloadExtension(service->extensions()->at(size_before)->id());
   EXPECT_EQ(size_before, service->extensions()->size());
 
-  // Now the plugin should be out of the cache again, but existing pages will
-  // still work until we reload them.
+  // Now the plugin should be unloaded, and the page should be broken.
 
   ui_test_utils::ExecuteJavaScriptAndExtractBool(
       tab->render_view_host(), L"", L"testPluginWorks()", &result);
-  EXPECT_TRUE(result);
+  EXPECT_FALSE(result);
+
+  // If we reload the extension and page, it should work again.
+
+  ASSERT_TRUE(LoadExtension(extension_dir));
+  EXPECT_EQ(size_before + 1, service->extensions()->size());
   browser()->Reload();
   ui_test_utils::WaitForNavigationInCurrentTab(browser());
-
   ui_test_utils::ExecuteJavaScriptAndExtractBool(
       tab->render_view_host(), L"", L"testPluginWorks()", &result);
-  EXPECT_FALSE(result);
+  EXPECT_TRUE(result);
 }
 
 // Used to simulate a click on the first button named 'Options'.
diff --git a/chrome/browser/plugin_process_host.cc b/chrome/browser/plugin_process_host.cc
index 2ba0c1c..c4669a5 100644
--- a/chrome/browser/plugin_process_host.cc
+++ b/chrome/browser/plugin_process_host.cc
@@ -453,6 +453,12 @@ bool PluginProcessHost::Init(const WebPluginInfo& info,
   return true;
 }
 
+void PluginProcessHost::ForceShutdown() {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
+  Send(new PluginProcessMsg_NotifyRenderersOfPendingShutdown());
+  Send(new PluginProcessMsg_Shutdown());
+}
+
 void PluginProcessHost::OnProcessLaunched() {
   FilePath gears_path;
   if (PathService::Get(chrome::FILE_GEARS_PLUGIN, &gears_path)) {
diff --git a/chrome/browser/plugin_process_host.h b/chrome/browser/plugin_process_host.h
index a98c090..e05ea2a 100644
--- a/chrome/browser/plugin_process_host.h
+++ b/chrome/browser/plugin_process_host.h
@@ -44,6 +44,9 @@ class PluginProcessHost : public ChildProcessHost,
   // be called before the object can be used.
   bool Init(const WebPluginInfo& info, const std::wstring& locale);
 
+  // Force the plugin process to shutdown (cleanly).
+  void ForceShutdown();
+
   virtual void OnMessageReceived(const IPC::Message& msg);
   virtual void OnChannelConnected(int32 peer_pid);
   virtual void OnChannelError();
diff --git a/chrome/browser/plugin_service.cc b/chrome/browser/plugin_service.cc
index 535ab31..f16108d 100644
--- a/chrome/browser/plugin_service.cc
+++ b/chrome/browser/plugin_service.cc
@@ -28,6 +28,7 @@
 #include "chrome/common/logging_chrome.h"
 #include "chrome/common/notification_type.h"
 #include "chrome/common/notification_service.h"
+#include "chrome/common/plugin_messages.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/render_messages.h"
 #ifndef DISABLE_NACL
@@ -288,25 +289,28 @@ void PluginService::OnWaitableEventSignaled(
     hklm_key_.StartWatching();
   }
 
-  NPAPI::PluginList::Singleton()->ResetPluginsLoaded();
+  NPAPI::PluginList::Singleton()->RefreshPlugins();
   PurgePluginListCache(true);
 #endif  // defined(OS_WIN)
 }
 
+static void ForceShutdownPlugin(const FilePath& plugin_path) {
+  PluginProcessHost* plugin =
+      PluginService::GetInstance()->FindPluginProcess(plugin_path);
+  if (plugin)
+    plugin->ForceShutdown();
+}
+
 void PluginService::Observe(NotificationType type,
                             const NotificationSource& source,
                             const NotificationDetails& details) {
   switch (type.value) {
     case NotificationType::EXTENSION_LOADED: {
-      // TODO(mpcomplete): We also need to force a renderer to refresh its
-      // cache of the plugin list when we inject user scripts, since it could
-      // have a stale version by the time extensions are loaded.
-      // See: http://code.google.com/p/chromium/issues/detail?id=12306
       Extension* extension = Details<Extension>(details).ptr();
       bool plugins_changed = false;
       for (size_t i = 0; i < extension->plugins().size(); ++i) {
         const Extension::PluginInfo& plugin = extension->plugins()[i];
-        NPAPI::PluginList::Singleton()->ResetPluginsLoaded();
+        NPAPI::PluginList::Singleton()->RefreshPlugins();
         NPAPI::PluginList::Singleton()->AddExtraPluginPath(plugin.path);
         plugins_changed = true;
         if (!plugin.is_public)
@@ -322,7 +326,10 @@ void PluginService::Observe(NotificationType type,
       bool plugins_changed = false;
       for (size_t i = 0; i < extension->plugins().size(); ++i) {
         const Extension::PluginInfo& plugin = extension->plugins()[i];
-        NPAPI::PluginList::Singleton()->ResetPluginsLoaded();
+        ChromeThread::PostTask(ChromeThread::IO, FROM_HERE,
+                               NewRunnableFunction(&ForceShutdownPlugin,
+                                                   plugin.path));
+        NPAPI::PluginList::Singleton()->RefreshPlugins();
         NPAPI::PluginList::Singleton()->RemoveExtraPluginPath(plugin.path);
         plugins_changed = true;
         if (!plugin.is_public)
diff --git a/chrome/common/plugin_messages_internal.h b/chrome/common/plugin_messages_internal.h
index b691f5a..4812d87 100644
--- a/chrome/common/plugin_messages_internal.h
+++ b/chrome/common/plugin_messages_internal.h
@@ -31,6 +31,10 @@ IPC_BEGIN_MESSAGES(PluginProcess)
   IPC_MESSAGE_CONTROL1(PluginProcessMsg_PluginMessage,
                        std::vector<uint8> /* opaque data */)
 
+  // Tells the plugin process to notify every connected renderer of the pending
+  // shutdown, so we don't mistake it for a crash.
+  IPC_MESSAGE_CONTROL0(PluginProcessMsg_NotifyRenderersOfPendingShutdown)
+
   // The following messages are used by all child processes, even though they
   // are listed under PluginProcess.  It seems overkill to define ChildProcess.
   // Tells the child process it should stop.
@@ -401,6 +405,8 @@ IPC_BEGIN_MESSAGES(PluginHost)
   IPC_SYNC_MESSAGE_CONTROL1_0(PluginHostMsg_SetException,
                               std::string /* message */)
 
+  IPC_MESSAGE_CONTROL0(PluginHostMsg_PluginShuttingDown)
+
 #if defined(OS_MACOSX)
   IPC_MESSAGE_ROUTED1(PluginHostMsg_UpdateGeometry_ACK,
                       int /* ack_key */)
diff --git a/chrome/plugin/plugin_channel.cc b/chrome/plugin/plugin_channel.cc
index bf14db2..c5913c3 100644
--- a/chrome/plugin/plugin_channel.cc
+++ b/chrome/plugin/plugin_channel.cc
@@ -153,6 +153,11 @@ PluginChannel* PluginChannel::GetPluginChannel(int renderer_id,
   return channel;
 }
 
+// static
+void PluginChannel::NotifyRenderersOfPendingShutdown() {
+  Broadcast(new PluginHostMsg_PluginShuttingDown());
+}
+
 PluginChannel::PluginChannel()
     : renderer_handle_(0),
       renderer_id_(-1),
diff --git a/chrome/plugin/plugin_channel.h b/chrome/plugin/plugin_channel.h
index 22bcd99..609316a6 100644
--- a/chrome/plugin/plugin_channel.h
+++ b/chrome/plugin/plugin_channel.h
@@ -26,6 +26,9 @@ class PluginChannel : public PluginChannelBase {
   static PluginChannel* GetPluginChannel(int renderer_id,
                                          MessageLoop* ipc_message_loop);
 
+  // Send a message to all renderers that the process is going to shutdown.
+  static void NotifyRenderersOfPendingShutdown();
+
   ~PluginChannel();
 
   virtual bool Send(IPC::Message* msg);
diff --git a/chrome/plugin/plugin_thread.cc b/chrome/plugin/plugin_thread.cc
index bd06bbd..688fe91 100644
--- a/chrome/plugin/plugin_thread.cc
+++ b/chrome/plugin/plugin_thread.cc
@@ -127,6 +127,8 @@ void PluginThread::OnControlMessageReceived(const IPC::Message& msg) {
   IPC_BEGIN_MESSAGE_MAP(PluginThread, msg)
     IPC_MESSAGE_HANDLER(PluginProcessMsg_CreateChannel, OnCreateChannel)
     IPC_MESSAGE_HANDLER(PluginProcessMsg_PluginMessage, OnPluginMessage)
+    IPC_MESSAGE_HANDLER(PluginProcessMsg_NotifyRenderersOfPendingShutdown,
+                        OnNotifyRenderersOfPendingShutdown)
 #if defined(OS_MACOSX)
   IPC_MESSAGE_HANDLER(PluginProcessMsg_PluginFocusNotify,
                       OnPluginFocusNotify)
@@ -165,6 +167,10 @@ void PluginThread::OnPluginMessage(const std::vector<unsigned char> &data) {
   ChildProcess::current()->ReleaseProcess();
 }
 
+void PluginThread::OnNotifyRenderersOfPendingShutdown() {
+  PluginChannel::NotifyRenderersOfPendingShutdown();
+}
+
 #if defined(OS_MACOSX)
 void PluginThread::OnPluginFocusNotify(uint32 instance_id) {
   WebPluginDelegateImpl* focused_instance =
diff --git a/chrome/plugin/plugin_thread.h b/chrome/plugin/plugin_thread.h
index b5eeefc..deca8d4 100644
--- a/chrome/plugin/plugin_thread.h
+++ b/chrome/plugin/plugin_thread.h
@@ -35,6 +35,7 @@ class PluginThread : public ChildThread {
   // Callback for when a channel has been created.
   void OnCreateChannel(int renderer_id, bool off_the_record);
   void OnPluginMessage(const std::vector<uint8> &data);
+  void OnNotifyRenderersOfPendingShutdown();
 #if defined(OS_MACOSX)
   void OnAppActivated();
   void OnPluginFocusNotify(uint32 instance_id);
diff --git a/chrome/renderer/plugin_channel_host.cc b/chrome/renderer/plugin_channel_host.cc
index bddf69f..dd14530 100644
--- a/chrome/renderer/plugin_channel_host.cc
+++ b/chrome/renderer/plugin_channel_host.cc
@@ -79,7 +79,7 @@ PluginChannelHost* PluginChannelHost::GetPluginChannelHost(
   return result;
 }
 
-PluginChannelHost::PluginChannelHost() {
+PluginChannelHost::PluginChannelHost() : expecting_shutdown_(false) {
 }
 
 PluginChannelHost::~PluginChannelHost() {
@@ -130,6 +130,7 @@ void PluginChannelHost::RemoveRoute(int route_id) {
 void PluginChannelHost::OnControlMessageReceived(const IPC::Message& message) {
   IPC_BEGIN_MESSAGE_MAP(PluginChannelHost, message)
     IPC_MESSAGE_HANDLER(PluginHostMsg_SetException, OnSetException)
+    IPC_MESSAGE_HANDLER(PluginHostMsg_PluginShuttingDown, OnPluginShuttingDown)
     IPC_MESSAGE_UNHANDLED_ERROR()
   IPC_END_MESSAGE_MAP()
 }
@@ -138,6 +139,10 @@ void PluginChannelHost::OnSetException(const std::string& message) {
   WebKit::WebBindings::setException(NULL, message.c_str());
 }
 
+void PluginChannelHost::OnPluginShuttingDown(const IPC::Message& message) {
+  expecting_shutdown_ = true;
+}
+
 void PluginChannelHost::OnChannelError() {
   PluginChannelBase::OnChannelError();
 
diff --git a/chrome/renderer/plugin_channel_host.h b/chrome/renderer/plugin_channel_host.h
index 31bf77a..668c875 100644
--- a/chrome/renderer/plugin_channel_host.h
+++ b/chrome/renderer/plugin_channel_host.h
@@ -37,6 +37,8 @@ class PluginChannelHost : public PluginChannelBase {
     PluginChannelBase::Broadcast(message);
   }
 
+  bool expecting_shutdown() { return expecting_shutdown_; }
+
  private:
   // Called on the render thread
   PluginChannelHost();
@@ -46,6 +48,7 @@ class PluginChannelHost : public PluginChannelBase {
 
   void OnControlMessageReceived(const IPC::Message& message);
   void OnSetException(const std::string& message);
+  void OnPluginShuttingDown(const IPC::Message& message);
 
   // Keep track of all the registered WebPluginDelegeProxies to
   // inform about OnChannelError
@@ -56,6 +59,10 @@ class PluginChannelHost : public PluginChannelBase {
   // used when the JS debugger is attached in order to avoid browser hangs.
   scoped_refptr<IsListeningFilter> is_listening_filter_;
 
+  // True if we are expecting the plugin process to go away - in which case,
+  // don't treat it as a crash.
+  bool expecting_shutdown_;
+
   DISALLOW_EVIL_CONSTRUCTORS(PluginChannelHost);
 };
 
diff --git a/chrome/renderer/webplugin_delegate_proxy.cc b/chrome/renderer/webplugin_delegate_proxy.cc
index 3b733ff..fbadb86 100644
--- a/chrome/renderer/webplugin_delegate_proxy.cc
+++ b/chrome/renderer/webplugin_delegate_proxy.cc
@@ -464,7 +464,8 @@ void WebPluginDelegateProxy::OnChannelError() {
     }
     plugin_->Invalidate();
   }
-  render_view_->PluginCrashed(info_.path);
+  if (!channel_host_->expecting_shutdown())
+    render_view_->PluginCrashed(info_.path);
 }
 
 void WebPluginDelegateProxy::UpdateGeometry(const gfx::Rect& window_rect,
@@ -1465,4 +1466,3 @@ bool WebPluginDelegateProxy::UseSynchronousGeometryUpdates() {
   return false;
 }
 #endif
-
diff --git a/webkit/glue/plugins/plugin_list.cc b/webkit/glue/plugins/plugin_list.cc
index f792174..7514221 100644
--- a/webkit/glue/plugins/plugin_list.cc
+++ b/webkit/glue/plugins/plugin_list.cc
@@ -32,9 +32,9 @@ bool PluginList::PluginsLoaded() {
   return plugins_loaded_;
 }
 
-void PluginList::ResetPluginsLoaded() {
+void PluginList::RefreshPlugins() {
   AutoLock lock(lock_);
-  plugins_loaded_ = false;
+  plugins_need_refresh_ = true;
 }
 
 void PluginList::AddExtraPluginPath(const FilePath& plugin_path) {
@@ -137,7 +137,8 @@ bool PluginList::CreateWebPluginInfo(const PluginVersionInfo& pvi,
   return true;
 }
 
-PluginList::PluginList() : plugins_loaded_(false) {
+PluginList::PluginList()
+    : plugins_loaded_(false), plugins_need_refresh_(false) {
   PlatformInit();
 
 #if defined(OS_WIN)
@@ -168,9 +169,12 @@ void PluginList::LoadPlugins(bool refresh) {
   std::vector<PluginVersionInfo> internal_plugins;
   {
     AutoLock lock(lock_);
-    if (plugins_loaded_ && !refresh)
+    if (plugins_loaded_ && !refresh && !plugins_need_refresh_)
       return;
 
+    // Clear the refresh bit now, because it might get set again before we
+    // reach the end of the method.
+    plugins_need_refresh_ = false;
     extra_plugin_paths = extra_plugin_paths_;
     extra_plugin_dirs = extra_plugin_dirs_;
     internal_plugins = internal_plugins_;
diff --git a/webkit/glue/plugins/plugin_list.h b/webkit/glue/plugins/plugin_list.h
index d4aa31c..cff2dd2 100644
--- a/webkit/glue/plugins/plugin_list.h
+++ b/webkit/glue/plugins/plugin_list.h
@@ -76,9 +76,9 @@ class PluginList {
   // Returns true iff the plugin list has been loaded already.
   bool PluginsLoaded();
 
-  // Clear the plugins_loaded_ bit to force a refresh next time we retrieve
-  // plugins.
-  void ResetPluginsLoaded();
+  // Cause the plugin list to refresh next time they are accessed, regardless
+  // of whether they are already loaded.
+  void RefreshPlugins();
 
   // Add/Remove an extra plugin to load when we actually do the loading.  Must
   // be called before the plugins have been loaded.
@@ -233,6 +233,9 @@ class PluginList {
 
   bool plugins_loaded_;
 
+  // If true, we reload plugins even if they've been loaded already.
+  bool plugins_need_refresh_;
+
   // Contains information about the available plugins.
   std::vector<WebPluginInfo> plugins_;