Fix a bug that would cause the plugin_loader to stall.

content/browser/plugin_loader_posix.cc

Index: content/browser/plugin_loader_posix.cc
diff --git a/content/browser/plugin_loader_posix.cc b/content/browser/plugin_loader_posix.cc
index 22bc2b614c2599f8242540c471ea1a347ca16cb8..b70557fe2ad9b129c8a715e178acf80d1041b4d6 100644
--- a/content/browser/plugin_loader_posix.cc
+++ b/content/browser/plugin_loader_posix.cc
@@ -19,7 +19,7 @@
 namespace content {
 
 PluginLoaderPosix::PluginLoaderPosix()
-    : next_load_index_(0) {
+    : next_load_index_(0), loading_plugins_(false) {
 }
 
 void PluginLoaderPosix::GetPlugins(
@@ -34,9 +34,12 @@ void PluginLoaderPosix::GetPlugins(
     return;
   }
 
-  if (callbacks_.empty()) {
+  if (!loading_plugins_) {
+    loading_plugins_ = true;
     callbacks_.push_back(callback);
 
+    // When |loading_plugins_| is set to false, there must be exactly 1 call to
+    // SetPlugins()

[Avi (use Gerrit), 2014/11/11 02:38:06]

nit: end comments with a .

I don't quite follow the comment. So PrepareForPluginLoading() ends up calling
SetPlugins? (I'm not that familiar with plugin code.)

[erikchen, 2014/11/11 02:44:48]

I changed the comment to be more clear. "When |loading_plugins_| is set to
false, this instance must call SetPlugins()."

No behavioral change, just adding a much needed comment.
PluginList::Singleton()->PrepareForPluginLoading(); requires an eventual
matching call to SetPlugins(). This class does this, but doesn't explicitly
state this.

     PluginList::Singleton()->PrepareForPluginLoading();
 
     BrowserThread::PostTask(BrowserThread::FILE,
@@ -81,6 +84,10 @@ void PluginLoaderPosix::OnProcessCrashed(int exit_code) {
   LoadPluginsInternal();
 }
 
+void PluginLoaderPosix::OnProcessLaunchFailed() {
+  FinishedLoadingPlugins();
+}
+
 bool PluginLoaderPosix::Send(IPC::Message* message) {
   if (process_host_.get())
     return process_host_->Send(message);
@@ -120,15 +127,14 @@ void PluginLoaderPosix::LoadPluginsInternal() {
 
   // Check if the list is empty or all plugins have already been loaded before
   // forking.
-  if (MaybeRunPendingCallbacks())
+  if (IsFinishedLoadingPlugins()) {
+    FinishedLoadingPlugins();
     return;
+  }
 
   RecordAction(
       base::UserMetricsAction("PluginLoaderPosix.LaunchUtilityProcess"));
 
-  if (load_start_time_.is_null())
-    load_start_time_ = base::TimeTicks::Now();
-
   UtilityProcessHostImpl* host = new UtilityProcessHostImpl(
       this,
       BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO).get());
@@ -138,7 +144,13 @@ void PluginLoaderPosix::LoadPluginsInternal() {
   host->set_child_flags(ChildProcessHost::CHILD_ALLOW_HEAP_EXECUTION);
 #endif
 
-  process_host_->Send(new UtilityMsg_LoadPlugins(canonical_list_));
+  bool launched = LaunchUtilityProcess();
+  if (!launched) {
+    // The utility process either failed to start or failed to receive the IPC.
+    // This process will never receive any callbacks for OnPluginLoaded() or
+    // OnPluginLoadFailed().
+    FinishedLoadingPlugins();
+  }
 }
 
 void PluginLoaderPosix::GetPluginsWrapper(
@@ -159,12 +171,18 @@ void PluginLoaderPosix::OnPluginLoaded(uint32 index,
     return;
   }
 
-  if (!MaybeAddInternalPlugin(plugin.path))
+  std::vector<WebPluginInfo>::iterator it = FindInternalPlugin(plugin.path);

[Avi (use Gerrit), 2014/11/11 02:38:06]

auto it = ...?

[erikchen, 2014/11/11 02:44:48]

Oh yes, delightful.

+  if (it != internal_plugins_.end()) {
+    loaded_plugins_.push_back(*it);
+    internal_plugins_.erase(it);
+  } else {
     loaded_plugins_.push_back(plugin);
+  }
 
   ++next_load_index_;
 
-  MaybeRunPendingCallbacks();
+  if (IsFinishedLoadingPlugins())
+    FinishedLoadingPlugins();
 }
 
 void PluginLoaderPosix::OnPluginLoadFailed(uint32 index,
@@ -177,44 +195,45 @@ void PluginLoaderPosix::OnPluginLoadFailed(uint32 index,
 
   ++next_load_index_;
 
-  MaybeAddInternalPlugin(plugin_path);
-  MaybeRunPendingCallbacks();
+  std::vector<WebPluginInfo>::iterator it = FindInternalPlugin(plugin_path);

[Avi (use Gerrit), 2014/11/11 02:38:06]

auto it = ...?

[erikchen, 2014/11/11 02:44:48]

Done.

+  if (it != internal_plugins_.end()) {
+    loaded_plugins_.push_back(*it);
+    internal_plugins_.erase(it);
+  }
+
+  if (IsFinishedLoadingPlugins())
+    FinishedLoadingPlugins();
 }
 
-bool PluginLoaderPosix::MaybeAddInternalPlugin(
+std::vector<WebPluginInfo>::iterator PluginLoaderPosix::FindInternalPlugin(
     const base::FilePath& plugin_path) {
-  for (std::vector<WebPluginInfo>::iterator it = internal_plugins_.begin();
-       it != internal_plugins_.end();
-       ++it) {
-    if (it->path == plugin_path) {
-      loaded_plugins_.push_back(*it);
-      internal_plugins_.erase(it);
-      return true;
-    }
-  }
-  return false;
+  return std::find_if(internal_plugins_.begin(), internal_plugins_.end(),
+                      [&plugin_path](const WebPluginInfo& plugin) {
+    return plugin.path == plugin_path;

[Avi (use Gerrit), 2014/11/11 02:38:06]

Yay lambdas!

[erikchen, 2014/11/11 02:44:48]

\o/

+  });
 }
 
-bool PluginLoaderPosix::MaybeRunPendingCallbacks() {
-  if (next_load_index_ < canonical_list_.size())
-    return false;
+bool PluginLoaderPosix::IsFinishedLoadingPlugins() {
+  if (canonical_list_.empty())
+    return true;
+
+  DCHECK(next_load_index_ <= canonical_list_.size());
+  return next_load_index_ == canonical_list_.size();
+}
 
+void PluginLoaderPosix::FinishedLoadingPlugins() {
+  loading_plugins_ = false;
   PluginList::Singleton()->SetPlugins(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_));
+  for (auto& callback : callbacks_) {
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE, base::Bind(callback, loaded_plugins_));
   }
   callbacks_.clear();
+}
 
-  LOCAL_HISTOGRAM_TIMES("PluginLoaderPosix.LoadDone",
-                        (base::TimeTicks::Now() - load_start_time_) *
-                            base::Time::kMicrosecondsPerMillisecond);
-  load_start_time_ = base::TimeTicks();
-
-  return true;
+bool PluginLoaderPosix::LaunchUtilityProcess() {
+  return process_host_->Send(new UtilityMsg_LoadPlugins(canonical_list_));
 }
 
 }  // namespace content