Restart plugin loading only if the plugin list has actually become stale.

BUG=171404

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

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

8 files changed, 148 insertions, 95 deletions

diff --git a/chrome/browser/plugins/plugin_prefs_unittest.cc b/chrome/browser/plugins/plugin_prefs_unittest.cc
index 3ea617a..8ee7aa9 100644
--- a/chrome/browser/plugins/plugin_prefs_unittest.cc
+++ b/chrome/browser/plugins/plugin_prefs_unittest.cc
@@ -6,7 +6,6 @@
 
 #include "base/at_exit.h"
 #include "base/bind.h"
-#include "base/message_loop/message_loop.h"
 #include "base/path_service.h"
 #include "base/run_loop.h"
 #include "base/strings/utf_string_conversions.h"
@@ -15,7 +14,7 @@
 #include "content/public/browser/plugin_service.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/common/webplugininfo.h"
-#include "content/public/test/test_browser_thread.h"
+#include "content/public/test/test_browser_thread_bundle.h"
 #include "content/public/test/test_utils.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -206,10 +205,9 @@ TEST_F(PluginPrefsTest, EnabledAndDisabledByPolicy) {
 #if !(defined(OS_LINUX) && defined(USE_AURA))
 
 TEST_F(PluginPrefsTest, UnifiedPepperFlashState) {
+  content::TestBrowserThreadBundle browser_threads;
   base::ShadowingAtExitManager at_exit_manager_;  // Destroys the PluginService.
 
-  base::MessageLoop message_loop;
-  content::TestBrowserThread ui_thread(BrowserThread::UI, &message_loop);
   PluginService::GetInstance()->Init();
   PluginService::GetInstance()->DisablePluginsDiscoveryForTesting();
 
diff --git a/content/browser/plugin_loader_posix.cc b/content/browser/plugin_loader_posix.cc
index 611b147..8ab2798 100644
--- a/content/browser/plugin_loader_posix.cc
+++ b/content/browser/plugin_loader_posix.cc
@@ -22,16 +22,35 @@ PluginLoaderPosix::PluginLoaderPosix()
     : next_load_index_(0) {
 }
 
-void PluginLoaderPosix::LoadPlugins(
-    scoped_refptr<base::MessageLoopProxy> target_loop,
+void PluginLoaderPosix::GetPlugins(
     const PluginService::GetPluginsCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 
-  callbacks_.push_back(PendingCallback(target_loop, callback));
+  std::vector<WebPluginInfo> cached_plugins;
+  if (PluginList::Singleton()->GetPluginsNoRefresh(&cached_plugins)) {
+    // Can't assume the caller is reentrant.
+    base::MessageLoop::current()->PostTask(FROM_HERE,
+        base::Bind(callback, cached_plugins));
+    return;
+  }
+
+  if (callbacks_.empty()) {
+    callbacks_.push_back(callback);
+
+    PluginList::Singleton()->PrepareForPluginLoading();
 
-  if (callbacks_.size() == 1) {
-    BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
-        base::Bind(&PluginLoaderPosix::GetPluginsToLoad, this));
+    BrowserThread::PostTask(BrowserThread::FILE,
+                            FROM_HERE,
+                            base::Bind(&PluginLoaderPosix::GetPluginsToLoad,
+                                       make_scoped_refptr(this)));
+  } else {
+    // If we are currently loading plugins, the plugin list might have been
+    // invalidated in the mean time, or might get invalidated before we finish.
+    // We'll wait until we have finished the current run, then try to get them
+    // again from the plugin list. If it has indeed been invalidated, it will
+    // restart plugin loading, otherwise it will immediately run the callback.
+    callbacks_.push_back(base::Bind(&PluginLoaderPosix::GetPluginsWrapper,
+                                    make_scoped_refptr(this), callback));
   }
 }
 
@@ -122,6 +141,16 @@ void PluginLoaderPosix::LoadPluginsInternal() {
   process_host_->Send(new UtilityMsg_LoadPlugins(canonical_list_));
 }
 
+void PluginLoaderPosix::GetPluginsWrapper(
+    const PluginService::GetPluginsCallback& callback,
+    const std::vector<WebPluginInfo>& plugins_unused) {
+  // We are being called after plugin loading has finished, but we don't know
+  // whether the plugin list has been invalidated in the mean time
+  // (and therefore |plugins| might already be stale). So we simply ignore it
+  // and call regular GetPlugins() instead.
+  GetPlugins(callback);
+}
+
 void PluginLoaderPosix::OnPluginLoaded(uint32 index,
                                        const WebPluginInfo& plugin) {
   if (index != next_load_index_) {
@@ -172,37 +201,20 @@ bool PluginLoaderPosix::MaybeRunPendingCallbacks() {
 
   PluginList::Singleton()->SetPlugins(loaded_plugins_);
 
-  // Only call the first callback with loaded plugins because there may be
-  // some extra plugin paths added since the first callback is added.
-  if (!callbacks_.empty()) {
-    PendingCallback callback = callbacks_.front();
-    callbacks_.pop_front();
-    callback.target_loop->PostTask(
-        FROM_HERE,
-        base::Bind(callback.callback, loaded_plugins_));
+  for (std::vector<PluginService::GetPluginsCallback>::iterator it =
+           callbacks_.begin();
+       it != callbacks_.end(); ++it) {
+    base::MessageLoop::current()->PostTask(FROM_HERE,
+                                           base::Bind(*it, loaded_plugins_));
   }
+  callbacks_.clear();
 
   HISTOGRAM_TIMES("PluginLoaderPosix.LoadDone",
                   (base::TimeTicks::Now() - load_start_time_)
                       * base::Time::kMicrosecondsPerMillisecond);
   load_start_time_ = base::TimeTicks();
 
-  if (!callbacks_.empty()) {
-    BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
-        base::Bind(&PluginLoaderPosix::GetPluginsToLoad, this));
-    return false;
-  }
   return true;
 }
 
-PluginLoaderPosix::PendingCallback::PendingCallback(
-    scoped_refptr<base::MessageLoopProxy> loop,
-    const PluginService::GetPluginsCallback& cb)
-    : target_loop(loop),
-      callback(cb) {
-}
-
-PluginLoaderPosix::PendingCallback::~PendingCallback() {
-}
-
 }  // namespace content
diff --git a/content/browser/plugin_loader_posix.h b/content/browser/plugin_loader_posix.h
index 60cd5c6..42c6437 100644
--- a/content/browser/plugin_loader_posix.h
+++ b/content/browser/plugin_loader_posix.h
@@ -5,7 +5,6 @@
 #ifndef CONTENT_BROWSER_PLUGIN_LOADER_POSIX_H_
 #define CONTENT_BROWSER_PLUGIN_LOADER_POSIX_H_
 
-#include <deque>
 #include <vector>
 
 #include "base/basictypes.h"
@@ -51,10 +50,9 @@ class CONTENT_EXPORT PluginLoaderPosix
  public:
   PluginLoaderPosix();
 
-  // Must be called from the IO thread.
-  void LoadPlugins(
-      scoped_refptr<base::MessageLoopProxy> target_loop,
-      const PluginService::GetPluginsCallback& callback);
+  // Must be called from the IO thread. The |callback| will be called on the IO
+  // thread too.
+  void GetPlugins(const PluginService::GetPluginsCallback& callback);
 
   // UtilityProcessHostClient:
   virtual void OnProcessCrashed(int exit_code) OVERRIDE;
@@ -64,15 +62,6 @@ class CONTENT_EXPORT PluginLoaderPosix
   virtual bool Send(IPC::Message* msg) OVERRIDE;
 
  private:
-  struct PendingCallback {
-    PendingCallback(scoped_refptr<base::MessageLoopProxy> target_loop,
-                    const PluginService::GetPluginsCallback& callback);
-    ~PendingCallback();
-
-    scoped_refptr<base::MessageLoopProxy> target_loop;
-    PluginService::GetPluginsCallback callback;
-  };
-
   virtual ~PluginLoaderPosix();
 
   // Called on the FILE thread to get the list of plugin paths to probe.
@@ -81,6 +70,12 @@ class CONTENT_EXPORT PluginLoaderPosix
   // Must be called on the IO thread.
   virtual void LoadPluginsInternal();
 
+  // Called after plugin loading has finished, if we don't know whether the
+  // plugin list has been invalidated in the mean time.
+  void GetPluginsWrapper(
+      const PluginService::GetPluginsCallback& callback,
+      const std::vector<WebPluginInfo>& plugins_unused);
+
   // Message handlers.
   void OnPluginLoaded(uint32 index, const WebPluginInfo& plugin);
   void OnPluginLoadFailed(uint32 index, const base::FilePath& plugin_path);
@@ -113,7 +108,7 @@ class CONTENT_EXPORT PluginLoaderPosix
 
   // The callback and message loop on which the callback will be run when the
   // plugin loading process has been completed.
-  std::deque<PendingCallback> callbacks_;
+  std::vector<PluginService::GetPluginsCallback> callbacks_;
 
   // The time at which plugin loading started.
   base::TimeTicks load_start_time_;
diff --git a/content/browser/plugin_loader_posix_unittest.cc b/content/browser/plugin_loader_posix_unittest.cc
index 4690768..542b2c6 100644
--- a/content/browser/plugin_loader_posix_unittest.cc
+++ b/content/browser/plugin_loader_posix_unittest.cc
@@ -11,6 +11,7 @@
 #include "base/message_loop/message_loop.h"
 #include "base/strings/utf_string_conversions.h"
 #include "content/browser/browser_thread_impl.h"
+#include "content/common/plugin_list.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -96,7 +97,8 @@ class PluginLoaderPosixTest : public testing::Test {
   WebPluginInfo plugin3_;
 
  private:
-  base::ShadowingAtExitManager at_exit_manager_;  // Destroys PluginService.
+  // Destroys PluginService and PluginList.
+  base::ShadowingAtExitManager at_exit_manager_;
 
   base::MessageLoopForIO message_loop_;
   BrowserThreadImpl file_thread_;
@@ -110,25 +112,48 @@ TEST_F(PluginLoaderPosixTest, QueueRequests) {
   PluginService::GetPluginsCallback callback =
       base::Bind(&VerifyCallback, base::Unretained(&did_callback));
 
-  EXPECT_EQ(0u, plugin_loader()->number_of_pending_callbacks());
-  plugin_loader()->LoadPlugins(message_loop()->message_loop_proxy(), callback);
-  EXPECT_EQ(1u, plugin_loader()->number_of_pending_callbacks());
+  plugin_loader()->GetPlugins(callback);
+  plugin_loader()->GetPlugins(callback);
 
-  plugin_loader()->LoadPlugins(message_loop()->message_loop_proxy(), callback);
-  EXPECT_EQ(2u, plugin_loader()->number_of_pending_callbacks());
+  EXPECT_CALL(*plugin_loader(), LoadPluginsInternal()).Times(1);
+  message_loop()->RunUntilIdle();
 
-  EXPECT_CALL(*plugin_loader(), LoadPluginsInternal()).Times(2);
+  EXPECT_EQ(0, did_callback);
+
+  plugin_loader()->canonical_list()->clear();
+  plugin_loader()->canonical_list()->push_back(plugin1_.path);
+  plugin_loader()->TestOnPluginLoaded(0, plugin1_);
+  message_loop()->RunUntilIdle();
+
+  EXPECT_EQ(2, did_callback);
+}
+
+TEST_F(PluginLoaderPosixTest, QueueRequestsAndInvalidate) {
+  int did_callback = 0;
+  PluginService::GetPluginsCallback callback =
+      base::Bind(&VerifyCallback, base::Unretained(&did_callback));
+
+  plugin_loader()->GetPlugins(callback);
+
+  EXPECT_CALL(*plugin_loader(), LoadPluginsInternal()).Times(1);
   message_loop()->RunUntilIdle();
 
   EXPECT_EQ(0, did_callback);
+  ::testing::Mock::VerifyAndClearExpectations(plugin_loader());
+
+  // Invalidate the plugin list, then queue up another request.
+  PluginList::Singleton()->RefreshPlugins();
+  plugin_loader()->GetPlugins(callback);
+
+  EXPECT_CALL(*plugin_loader(), LoadPluginsInternal()).Times(1);
 
   plugin_loader()->canonical_list()->clear();
   plugin_loader()->canonical_list()->push_back(plugin1_.path);
   plugin_loader()->TestOnPluginLoaded(0, plugin1_);
   message_loop()->RunUntilIdle();
 
+  // Only the first request should have been fulfilled.
   EXPECT_EQ(1, did_callback);
-  EXPECT_EQ(1u, plugin_loader()->number_of_pending_callbacks());
 
   plugin_loader()->canonical_list()->clear();
   plugin_loader()->canonical_list()->push_back(plugin1_.path);
@@ -136,7 +161,6 @@ TEST_F(PluginLoaderPosixTest, QueueRequests) {
   message_loop()->RunUntilIdle();
 
   EXPECT_EQ(2, did_callback);
-  EXPECT_EQ(0u, plugin_loader()->number_of_pending_callbacks());
 }
 
 TEST_F(PluginLoaderPosixTest, ThreeSuccessfulLoads) {
@@ -144,7 +168,7 @@ TEST_F(PluginLoaderPosixTest, ThreeSuccessfulLoads) {
   PluginService::GetPluginsCallback callback =
       base::Bind(&VerifyCallback, base::Unretained(&did_callback));
 
-  plugin_loader()->LoadPlugins(message_loop()->message_loop_proxy(), callback);
+  plugin_loader()->GetPlugins(callback);
 
   EXPECT_CALL(*plugin_loader(), LoadPluginsInternal()).Times(1);
   message_loop()->RunUntilIdle();
@@ -184,7 +208,7 @@ TEST_F(PluginLoaderPosixTest, ThreeSuccessfulLoadsThenCrash) {
   PluginService::GetPluginsCallback callback =
       base::Bind(&VerifyCallback, base::Unretained(&did_callback));
 
-  plugin_loader()->LoadPlugins(message_loop()->message_loop_proxy(), callback);
+  plugin_loader()->GetPlugins(callback);
 
   EXPECT_CALL(*plugin_loader(), LoadPluginsInternal()).Times(2);
   message_loop()->RunUntilIdle();
@@ -226,7 +250,7 @@ TEST_F(PluginLoaderPosixTest, TwoFailures) {
   PluginService::GetPluginsCallback callback =
       base::Bind(&VerifyCallback, base::Unretained(&did_callback));
 
-  plugin_loader()->LoadPlugins(message_loop()->message_loop_proxy(), callback);
+  plugin_loader()->GetPlugins(callback);
 
   EXPECT_CALL(*plugin_loader(), LoadPluginsInternal()).Times(1);
   message_loop()->RunUntilIdle();
@@ -264,7 +288,7 @@ TEST_F(PluginLoaderPosixTest, CrashedProcess) {
   PluginService::GetPluginsCallback callback =
       base::Bind(&VerifyCallback, base::Unretained(&did_callback));
 
-  plugin_loader()->LoadPlugins(message_loop()->message_loop_proxy(), callback);
+  plugin_loader()->GetPlugins(callback);
 
   EXPECT_CALL(*plugin_loader(), LoadPluginsInternal()).Times(1);
   message_loop()->RunUntilIdle();
@@ -296,7 +320,7 @@ TEST_F(PluginLoaderPosixTest, InternalPlugin) {
   PluginService::GetPluginsCallback callback =
       base::Bind(&VerifyCallback, base::Unretained(&did_callback));
 
-  plugin_loader()->LoadPlugins(message_loop()->message_loop_proxy(), callback);
+  plugin_loader()->GetPlugins(callback);
 
   EXPECT_CALL(*plugin_loader(), LoadPluginsInternal()).Times(1);
   message_loop()->RunUntilIdle();
@@ -344,7 +368,7 @@ TEST_F(PluginLoaderPosixTest, AllCrashed) {
   PluginService::GetPluginsCallback callback =
       base::Bind(&VerifyCallback, base::Unretained(&did_callback));
 
-  plugin_loader()->LoadPlugins(message_loop()->message_loop_proxy(), callback);
+  plugin_loader()->GetPlugins(callback);
 
   // Spin the loop so that the canonical list of plugins can be set.
   EXPECT_CALL(*plugin_loader(), LoadPluginsInternal()).Times(1);
diff --git a/content/browser/plugin_service_impl.cc b/content/browser/plugin_service_impl.cc
index b65704b..047859a 100644
--- a/content/browser/plugin_service_impl.cc
+++ b/content/browser/plugin_service_impl.cc
@@ -115,6 +115,12 @@ void NotifyPluginDirChanged(const base::FilePath& path, bool error) {
 }
 #endif
 
+void ForwardCallback(base::MessageLoopProxy* target_loop,
+                     const PluginService::GetPluginsCallback& callback,
+                     const std::vector<WebPluginInfo>& plugins) {
+  target_loop->PostTask(FROM_HERE, base::Bind(callback, plugins));
+}
+
 }  // namespace
 
 // static
@@ -597,20 +603,9 @@ void PluginServiceImpl::GetPlugins(const GetPluginsCallback& callback) {
     return;
   }
 #if defined(OS_POSIX)
-  std::vector<WebPluginInfo> cached_plugins;
-  if (PluginList::Singleton()->GetPluginsNoRefresh(&cached_plugins)) {
-    // Can't assume the caller is reentrant.
-    target_loop->PostTask(FROM_HERE,
-        base::Bind(callback, cached_plugins));
-  } else {
-    // If we switch back to loading plugins in process, then we need to make
-    // sure g_thread_init() gets called since plugins may call glib at load.
-    if (!plugin_loader_.get())
-      plugin_loader_ = new PluginLoaderPosix;
-    BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
-        base::Bind(&PluginLoaderPosix::LoadPlugins, plugin_loader_,
-                   target_loop, callback));
-  }
+  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+      base::Bind(&PluginServiceImpl::GetPluginsOnIOThread,
+                 base::Unretained(this), target_loop, callback));
 #else
   NOTREACHED();
 #endif
@@ -629,6 +624,23 @@ void PluginServiceImpl::GetPluginsInternal(
       base::Bind(callback, plugins));
 }
 
+#if defined(OS_POSIX)
+void PluginServiceImpl::GetPluginsOnIOThread(
+    base::MessageLoopProxy* target_loop,
+    const GetPluginsCallback& callback) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
+  // If we switch back to loading plugins in process, then we need to make
+  // sure g_thread_init() gets called since plugins may call glib at load.
+
+  if (!plugin_loader_)
+    plugin_loader_ = new PluginLoaderPosix;
+
+  plugin_loader_->GetPlugins(
+      base::Bind(&ForwardCallback, make_scoped_refptr(target_loop), callback));
+}
+#endif
+
 void PluginServiceImpl::OnWaitableEventSignaled(
     base::WaitableEvent* waitable_event) {
 #if defined(OS_WIN)
diff --git a/content/browser/plugin_service_impl.h b/content/browser/plugin_service_impl.h
index 6fa3b4a..55788fc 100644
--- a/content/browser/plugin_service_impl.h
+++ b/content/browser/plugin_service_impl.h
@@ -179,6 +179,12 @@ class CONTENT_EXPORT PluginServiceImpl
   void GetPluginsInternal(base::MessageLoopProxy* target_loop,
                           const GetPluginsCallback& callback);
 
+#if defined(OS_POSIX)
+  void GetPluginsOnIOThread(
+      base::MessageLoopProxy* target_loop,
+      const GetPluginsCallback& callback);
+#endif
+
   // Binding directly to GetAllowedPluginForOpenChannelToPlugin() isn't possible
   // because more arity is needed <http://crbug.com/98542>. This just forwards.
   void ForwardGetAllowedPluginForOpenChannelToPlugin(
diff --git a/content/common/plugin_list.cc b/content/common/plugin_list.cc
index df1f77b..ea74a70 100644
--- a/content/common/plugin_list.cc
+++ b/content/common/plugin_list.cc
@@ -173,14 +173,18 @@ PluginList::PluginList()
       plugins_discovery_disabled_(false) {
 }
 
-void PluginList::LoadPlugins(bool include_npapi) {
-  {
-    base::AutoLock lock(lock_);
-    if (loading_state_ == LOADING_STATE_UP_TO_DATE)
-      return;
+bool PluginList::PrepareForPluginLoading() {
+  base::AutoLock lock(lock_);
+  if (loading_state_ == LOADING_STATE_UP_TO_DATE)
+    return false;
 
-    loading_state_ = LOADING_STATE_REFRESHING;
-  }
+  loading_state_ = LOADING_STATE_REFRESHING;
+  return true;
+}
+
+void PluginList::LoadPlugins(bool include_npapi) {
+  if (!PrepareForPluginLoading())
+    return;
 
   std::vector<WebPluginInfo> new_plugins;
   base::Closure will_load_callback;
@@ -201,13 +205,7 @@ void PluginList::LoadPlugins(bool include_npapi) {
     LoadPluginIntoPluginList(*it, &new_plugins, &plugin_info);
   }
 
-  base::AutoLock lock(lock_);
-  plugins_list_.swap(new_plugins);
-
-  // If we haven't been invalidated in the mean time, mark the plug-in list as
-  // up-to-date.
-  if (loading_state_ != LOADING_STATE_NEEDS_REFRESH)
-    loading_state_ = LOADING_STATE_UP_TO_DATE;
+  SetPlugins(new_plugins);
 }
 
 bool PluginList::LoadPluginIntoPluginList(
@@ -280,11 +278,12 @@ void PluginList::GetPluginPathsToLoad(std::vector<base::FilePath>* plugin_paths,
 void PluginList::SetPlugins(const std::vector<WebPluginInfo>& plugins) {
   base::AutoLock lock(lock_);
 
-  DCHECK_NE(LOADING_STATE_REFRESHING, loading_state_);
-  loading_state_ = LOADING_STATE_UP_TO_DATE;
+  // If we haven't been invalidated in the mean time, mark the plug-in list as
+  // up-to-date.
+  if (loading_state_ != LOADING_STATE_NEEDS_REFRESH)
+    loading_state_ = LOADING_STATE_UP_TO_DATE;
 
-  plugins_list_.clear();
-  plugins_list_.insert(plugins_list_.end(), plugins.begin(), plugins.end());
+  plugins_list_ = plugins;
 }
 
 void PluginList::set_will_load_plugins_callback(const base::Closure& callback) {
diff --git a/content/common/plugin_list.h b/content/common/plugin_list.h
index 1205238..87319da 100644
--- a/content/common/plugin_list.h
+++ b/content/common/plugin_list.h
@@ -139,6 +139,13 @@ class CONTENT_EXPORT PluginList {
   void GetPluginPathsToLoad(std::vector<base::FilePath>* plugin_paths,
                             bool include_npapi);
 
+  // Signals that plugin loading will start. This method should be called before
+  // loading plugins with a different instance of this class. Returns false if
+  // the plugin list is up to date.
+  // When loading has finished, SetPlugins() should be called with the list of
+  // plugins.
+  bool PrepareForPluginLoading();
+
   // Clears the internal list of Plugins and copies them from the vector.
   void SetPlugins(const std::vector<WebPluginInfo>& plugins);