Gracefully handle child process death in out-of-process plugin loading.

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 da3d55afa8948b3b2933dc41b8bb3d94108a1f4c..f6a381f8e7e26a27da4a97b7b1776f3b1ef32ae4 100644
--- a/content/browser/plugin_loader_posix.cc
+++ b/content/browser/plugin_loader_posix.cc
@@ -5,73 +5,163 @@
 #include "content/browser/plugin_loader_posix.h"
 
 #include "base/bind.h"
+#include "base/message_loop.h"
 #include "base/message_loop_proxy.h"
+#include "base/metrics/histogram.h"
 #include "content/browser/browser_thread.h"
 #include "content/common/utility_messages.h"
 #include "webkit/plugins/npapi/plugin_list.h"
 
-namespace {
+using webkit::npapi::PluginList;
 
-void RunGetPluginsCallback(const PluginService::GetPluginsCallback& callback,
-                           const std::vector<webkit::WebPluginInfo> plugins) {
-  callback.Run(plugins);
+PluginLoaderPosix::PluginLoaderPosix()
+    : next_load_index_(0) {
 }
 
-}  // namespace
-
-// static
 void PluginLoaderPosix::LoadPlugins(
-    base::MessageLoopProxy* target_loop,
+    scoped_refptr<base::MessageLoopProxy> target_loop,
     const PluginService::GetPluginsCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 
-  PluginLoaderPosix* client = new PluginLoaderPosix(target_loop, callback);
-  UtilityProcessHost* process_host =
-      new UtilityProcessHost(client, BrowserThread::IO);
-  process_host->set_no_sandbox(true);
-#if defined(OS_MACOSX)
-  process_host->set_child_flags(ChildProcessHost::CHILD_ALLOW_HEAP_EXECUTION);
-#endif
+  callbacks_.push_back(PendingCallback(target_loop, callback));
 
-  std::vector<FilePath> extra_plugin_paths;
-  std::vector<FilePath> extra_plugin_dirs;
-  std::vector<webkit::WebPluginInfo> internal_plugins;
-  webkit::npapi::PluginList::Singleton()->GetPluginPathListsToLoad(
-      &extra_plugin_paths, &extra_plugin_dirs, &internal_plugins);
-
-  process_host->Send(new UtilityMsg_LoadPlugins(
-      extra_plugin_paths, extra_plugin_dirs, internal_plugins));
+  if (callbacks_.size() == 1) {
+    BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
+        base::Bind(&PluginLoaderPosix::GetPluginsToLoad, this));
+  }
 }
 
 bool PluginLoaderPosix::OnMessageReceived(const IPC::Message& message) {
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(PluginLoaderPosix, message)
-    IPC_MESSAGE_HANDLER(UtilityHostMsg_LoadedPlugins, OnGotPlugins)
+    IPC_MESSAGE_HANDLER(UtilityHostMsg_LoadedPlugin, OnPluginLoaded)
+    IPC_MESSAGE_HANDLER(UtilityHostMsg_LoadPluginFailed, OnPluginLoadFailed)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   return handled;
 }
 
 void PluginLoaderPosix::OnProcessCrashed(int exit_code) {
-  LOG(ERROR) << "Out-of-process plugin loader crashed with code " << exit_code
-             << ". You will have no plugins!";
-  // Don't leave callers hanging.
-  OnGotPlugins(std::vector<webkit::WebPluginInfo>());
+  canonical_list_.erase(canonical_list_.begin(),
+                        canonical_list_.begin() + next_load_index_ + 1);
+  next_load_index_ = 0;
+  LoadPluginsInternal();
 }
 
-PluginLoaderPosix::PluginLoaderPosix(
-    base::MessageLoopProxy* target_loop,
-    const PluginService::GetPluginsCallback& callback)
-    : target_loop_(target_loop),
-      callback_(callback) {
+bool PluginLoaderPosix::Send(IPC::Message* message) {
+  DCHECK(process_host_);

[jam, 2011/10/20 00:16:16]

nit: avoid this dcheck before variable use. in release build it does nothing. in
debug, you'll get a crash in the next line which will be just as useful

[Robert Sesek, 2011/10/20 16:00:21]

Done.

+  return process_host_->Send(message);
 }
 
 PluginLoaderPosix::~PluginLoaderPosix() {
 }
 
-void PluginLoaderPosix::OnGotPlugins(
-    const std::vector<webkit::WebPluginInfo>& plugins) {
-  webkit::npapi::PluginList::Singleton()->SetPlugins(plugins);
-  target_loop_->PostTask(FROM_HERE,
-      base::Bind(&RunGetPluginsCallback, callback_, plugins));
+void PluginLoaderPosix::GetPluginsToLoad() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+
+  base::TimeTicks start_time(base::TimeTicks::Now());
+
+  loaded_plugins_.clear();
+  next_load_index_ = 0;
+
+  canonical_list_.clear();
+  webkit::npapi::PluginList::Singleton()->GetPluginPathsToLoad(
+      &canonical_list_);
+
+  internal_plugins_.clear();
+  PluginList::Singleton()->GetInternalPlugins(&internal_plugins_);
+
+  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+      base::Bind(&PluginLoaderPosix::LoadPluginsInternal,
+                 make_scoped_refptr(this)));
+
+  HISTOGRAM_TIMES("PluginLoaderPosix.GetPluginList",
+                  (base::TimeTicks::Now() - start_time) *
+                      base::Time::kMicrosecondsPerMillisecond);
+}
+
+void PluginLoaderPosix::LoadPluginsInternal() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
+  if (load_start_time_.is_null())
+    load_start_time_ = base::TimeTicks::Now();
+
+  process_host_ = new UtilityProcessHost(this, BrowserThread::IO);
+  process_host_->set_no_sandbox(true);
+#if defined(OS_MACOSX)
+  process_host_->set_child_flags(ChildProcessHost::CHILD_ALLOW_HEAP_EXECUTION);
+#endif
+
+  process_host_->Send(new UtilityMsg_LoadPlugins(canonical_list_));
+}
+
+void PluginLoaderPosix::OnPluginLoaded(size_t index,
+                                       const webkit::WebPluginInfo& plugin) {
+  if (index >= canonical_list_.size())

[Bernhard Bauer, 2011/10/20 08:20:04]

When can that happen?

[Robert Sesek, 2011/10/20 16:00:21]

It's just being defensive against malformed/malicious messages.

[Bernhard Bauer, 2011/10/20 16:15:08]

Shouldn't we be more defensive in that case, i.e. killing the utility process?

[Robert Sesek, 2011/10/20 16:22:36]

Is there an API for that? I couldn't find one in ChildProcessHost.

[Bernhard Bauer, 2011/10/20 16:27:57]

Not sure, but we do it for renderers and extension processes, right?

[Robert Sesek, 2011/10/20 16:30:28]

Yes, but I think that may be a feature of RenderProcessHost.

+    return;
+
+  DCHECK_EQ(index, next_load_index_);
+  DCHECK_EQ(plugin.path.value(), canonical_list_[index].value());
+
+  if (!MaybeAddInternalPlugin(canonical_list_[index]))
+    loaded_plugins_.push_back(plugin);
+
+  ++next_load_index_;

[Bernhard Bauer, 2011/10/20 08:20:04]

Maybe I'm just not understanding it, but why do we need the utility process to
send us the index with every message? The DCHECK seems to imply that we don't
expect any other index, so why don't we just remove the first element from
|canonical_list_| here?

[Robert Sesek, 2011/10/20 16:00:21]

I thought about this but was hesitant because it would involve an array shift
each time (popping from the front). I measured this and it increases the
time-to-completion by 12.5ms. So, I'm keeping this as-is.

[Bernhard Bauer, 2011/10/20 16:15:08]

And if you use a linked list?

[Robert Sesek, 2011/10/20 16:22:36]

I don't think we have param traits, and even if we did, it'd still be less
efficient because it'd need to make a bunch of small heap allocations for each
node (on the IO thread).

[Bernhard Bauer, 2011/10/20 16:27:57]

Hm :-/

What if we just remove the index from the IPC message but keep counting
internally? If we're going to ignore out-of-order indices anyway, we might as
well not send them in the first place.

[Robert Sesek, 2011/10/20 16:30:28]

What's wrong with sending the index?

[Robert Sesek, 2011/10/20 16:38:05]

Actually, I now see that I don't need to do it because I can perform the sanity
check using the plugin path instead. Doing now.

+
+  RunPendingCallbacks();
+}
+
+void PluginLoaderPosix::OnPluginLoadFailed(size_t index,
+                                           const FilePath& plugin_path) {
+  if (index >= canonical_list_.size())
+    return;
+
+  DCHECK_EQ(index, next_load_index_);
+  DCHECK_EQ(plugin_path.value(), canonical_list_[index].value());
+  ++next_load_index_;
+
+  MaybeAddInternalPlugin(plugin_path);
+  RunPendingCallbacks();
+}
+
+bool PluginLoaderPosix::MaybeAddInternalPlugin(const FilePath& plugin_path) {
+  for (std::vector<webkit::WebPluginInfo>::iterator it =
+          internal_plugins_.begin();

[Bernhard Bauer, 2011/10/20 08:20:04]

Nit: one more space

[Robert Sesek, 2011/10/20 16:00:21]

Done.

+       it != internal_plugins_.end();
+       ++it) {
+    if (it->path == plugin_path) {
+      loaded_plugins_.push_back(*it);
+      internal_plugins_.erase(it);
+      return true;
+    }
+  }
+  return false;
+}
+
+void PluginLoaderPosix::RunPendingCallbacks() {
+  if (next_load_index_ >= canonical_list_.size()) {

[jam, 2011/10/20 00:16:16]

nit: better to early return instead to reduce tabbing

[Robert Sesek, 2011/10/20 16:00:21]

Done.

+    PluginList::Singleton()->SetPlugins(loaded_plugins_);
+    for (std::vector<PendingCallback>::iterator it = callbacks_.begin();
+         it != callbacks_.end();
+         ++it) {
+      it->target_loop->PostTask(FROM_HERE,
+          base::Bind(it->callback, loaded_plugins_));
+    }
+    callbacks_.clear();
+
+    HISTOGRAM_TIMES("PluginLoaderPosix.LoadDone",
+                    (base::TimeTicks::Now() - load_start_time_)
+                        * base::Time::kMicrosecondsPerMillisecond);
+    load_start_time_ = base::TimeTicks();
+  }
+}
+
+PluginLoaderPosix::PendingCallback::PendingCallback(
+    scoped_refptr<base::MessageLoopProxy> loop,
+    const PluginService::GetPluginsCallback& cb)
+    : target_loop(loop),
+      callback(cb) {
+}
+
+PluginLoaderPosix::PendingCallback::~PendingCallback() {
 }