Fix a bug that would cause the plugin_loader to stall.

content/browser/plugin_loader_posix.cc

 // Copyright (c) 2012 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 "content/browser/plugin_loader_posix.h"
 
 #include "base/bind.h"
 #include "base/message_loop/message_loop.h"
 #include "base/message_loop/message_loop_proxy.h"
 #include "base/metrics/histogram.h"
 #include "content/browser/utility_process_host_impl.h"
 #include "content/common/child_process_host_impl.h"
 #include "content/common/plugin_list.h"
 #include "content/common/utility_messages.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/plugin_service.h"
 #include "content/public/browser/user_metrics.h"
 
 namespace content {
 
 PluginLoaderPosix::PluginLoaderPosix()
-    : next_load_index_(0) {
+    : next_load_index_(0), loading_plugins_(false) {
 }
 
 void PluginLoaderPosix::GetPlugins(
     const PluginService::GetPluginsCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 
   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()) {
+  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,
                             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
@@ skipping 24 matching lines @@
   } else {
     canonical_list_.erase(canonical_list_.begin(),
                           canonical_list_.begin() + next_load_index_ + 1);
   }
 
   next_load_index_ = 0;
 
   LoadPluginsInternal();
 }
 
+void PluginLoaderPosix::OnProcessLaunchFailed() {
+  FinishedLoadingPlugins();
+}
+
 bool PluginLoaderPosix::Send(IPC::Message* message) {
   if (process_host_.get())
     return process_host_->Send(message);
   return false;
 }
 
 PluginLoaderPosix::~PluginLoaderPosix() {
 }
 
 void PluginLoaderPosix::GetPluginsToLoad() {
@@ skipping 19 matching lines @@
   LOCAL_HISTOGRAM_TIMES("PluginLoaderPosix.GetPluginList",
                         (base::TimeTicks::Now() - start_time) *
                             base::Time::kMicrosecondsPerMillisecond);
 }
 
 void PluginLoaderPosix::LoadPluginsInternal() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 
   // 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());
   process_host_ = host->AsWeakPtr();
   process_host_->DisableSandbox();
 #if defined(OS_MACOSX)
   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(
     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_) {
     LOG(ERROR) << "Received unexpected plugin load message for "
                << plugin.path.value() << "; index=" << 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,
                                            const base::FilePath& plugin_path) {
   if (index != next_load_index_) {
     LOG(ERROR) << "Received unexpected plugin load failure message for "
                << plugin_path.value() << "; index=" << index;
     return;
   }
 
   ++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