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

content/common/plugin_list.cc

Index: content/common/plugin_list.cc
diff --git a/content/common/plugin_list.cc b/content/common/plugin_list.cc
index df1f77b3441bc52fe15986729dfbe41a4789b55f..ea74a706fe41da0848d1765d18c6a9dba10372e3 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)

[Robert Sesek, 2014/01/21 20:58:18]

I think this is kind of a weird API; it maybe sets something on an unexposed
internal condition. Since this check is only used internally, maybe this method
should just return void and just set loading_state_ instead. I defer to jam@
though.

[Bernhard Bauer, 2014/01/21 23:02:11]

Well, what happens to the state internally is not part of the API. This is
really just the initial part of LoadPlugins() extracted to a separate method, so
we can reuse it if we load plugins asynchronously.

[jam, 2014/01/23 01:43:09]

yes it's a bid odd to see this method that the caller has to call before
LoadPlugins. Can we get rid of it or hide it so that it's a private
implementation of this class?

[Bernhard Bauer, 2014/01/23 17:35:31]

LoadPlugins() does call this method internally. A caller would either call
LoadPlugins(), or call PrepareForPluginLoading(), load the plugins themselves,
then call SetPlugins().

You and Robert seem to be concerned about two different things. Robert's concern
was the internal check based on the state; I can indeed get rid of this and just
set the state unconditionally. I just wanted to extract a common method to make
out-of-process plugin loading more similar to in-process.

Your concern is that callers have to call an additional method. There doesn't
really seem to be a way around that, as we have to know when plugin loading
starts (otherwise we can't distinguish the case where the plugin list is
invalidated after we have starting loading them from the case where it is
invalidated before). One thing I could possibly do is to have the client instead
set a LoadPluginsCallback that would be called whenever the PluginList wants to
load plugins, but that would mean that PluginList would now have to queue up the
callbacks. Alternatively, I could move the state transition into the call to
GetPluginsNoRefresh() (which we call before), but that would make the contract
of that method really weird.

SetPlugins() would be easier to get rid of: PrepareForPluginLoading could return
a callback bound to it, to be called by the client with the list of plugins.

[jam, 2014/01/23 17:57:47]

We don't need to support refreshing the plugins if they get invalidated while we
were fetching them. That adds a bit of complexity for not much benefit. For the
single-renderer case, kPluginsRefreshThresholdInSeconds means that we will
really almost always have loaded the plugins before we invalidate the list. For
the multiple renderers case, if one renderer invalidated to cause a refresh, and
another invalidate, then that is a race condition and the prior invalidation is
good enough.

[Bernhard Bauer, 2014/01/23 18:17:46]

Renderers aren't the only thing that can invalidate the plugin list. This is not
just an unlikely edge case; see http://crbug.com/117561 for the bug that
prompted the initial change.

+    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) {