Distinguish plugin disconnections from plugin crashes.

chrome/browser/plugins/plugin_observer.cc

Index: chrome/browser/plugins/plugin_observer.cc
diff --git a/chrome/browser/plugins/plugin_observer.cc b/chrome/browser/plugins/plugin_observer.cc
index 5205a5d6c5e4119c4837d99493a90ce4396858f5..237cb9cf95764efc001fffd5e6d8c17428dc40ae 100644
--- a/chrome/browser/plugins/plugin_observer.cc
+++ b/chrome/browser/plugins/plugin_observer.cc
@@ -6,6 +6,8 @@
 
 #include "base/auto_reset.h"
 #include "base/bind.h"
+#include "base/metrics/histogram.h"
+#include "base/process_util.h"
 #include "base/stl_util.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/api/infobars/confirm_infobar_delegate.h"
@@ -182,11 +184,44 @@ PluginObserver::~PluginObserver() {
 #endif
 }
 
-void PluginObserver::PluginCrashed(const FilePath& plugin_path) {
+void PluginObserver::PluginCrashed(const FilePath& plugin_path,
+                                   base::ProcessId plugin_pid) {
   DCHECK(!plugin_path.value().empty());
 
   string16 plugin_name =
       PluginService::GetInstance()->GetPluginDisplayNameByPath(plugin_path);
+  string16 infobar_text;
+#if defined(OS_WIN)
+  // Find out whether the plugin process is still alive.
+  base::ProcessHandle plugin_handle = base::kNullProcessHandle;
+  bool open_result = base::OpenProcessHandleWithAccess(
+      plugin_pid, PROCESS_QUERY_INFORMATION | SYNCHRONIZE, &plugin_handle);
+  bool is_running = false;
+  if (open_result) {
+    is_running = base::GetTerminationStatus(plugin_handle, NULL) ==

[jschuh, 2013/01/09 00:36:17]

This will work only if a handle to the dead process remains open. Otherwise the
process id will get recycled. If this works in your testing, it's only because
you're currently winning a race condition. The correct way to do this is to keep
a handle to the process that maps to the ID.

[yzshen1, 2013/01/09 00:45:50]

I tried to figure out how to hold a handle instead of an ID, but it seems hard.

I understand that ID will be reused. I thought usually it won't be reused
immediately. (No guarantee, I know. :)) And the worst outcome is that we display
a wrong prompt.

I will think harder. Thanks!

[jschuh, 2013/01/09 01:29:16]

Well, the odds of it getting reused are very slim. I just dislike the ambiguity,
but I'll let someone else decide if it's acceptable.

+        base::TERMINATION_STATUS_STILL_RUNNING;
+    base::CloseProcessHandle(plugin_handle);
+  }
+
+  if (is_running) {
+    infobar_text = l10n_util::GetStringFUTF16(IDS_PLUGIN_DISCONNECTED_PROMPT,
+                                              plugin_name);
+    UMA_HISTOGRAM_COUNTS("Plugin.ShowDisconnectedInfobar", 1);
+  } else {
+    infobar_text = l10n_util::GetStringFUTF16(IDS_PLUGIN_CRASHED_PROMPT,
+                                              plugin_name);
+    UMA_HISTOGRAM_COUNTS("Plugin.ShowCrashedInfobar", 1);
+  }
+#else
+  // Calling the POSIX version of base::GetTerminationStatus() may affect other
+  // code which is interested in the process termination status. (Please see the
+  // comment of the function.) Therefore, it shouldn't be used to distinguish
+  // disconnections from crashes.
+  infobar_text = l10n_util::GetStringFUTF16(IDS_PLUGIN_CRASHED_PROMPT,
+                                            plugin_name);
+  UMA_HISTOGRAM_COUNTS("Plugin.ShowCrashedInfobar", 1);
+#endif
+
   gfx::Image* icon = &ResourceBundle::GetSharedInstance().GetNativeImageNamed(
       IDR_INFOBAR_PLUGIN_CRASHED);
   InfoBarService* infobar_service =
@@ -195,7 +230,7 @@ void PluginObserver::PluginCrashed(const FilePath& plugin_path) {
       new SimpleAlertInfoBarDelegate(
           infobar_service,
           icon,
-          l10n_util::GetStringFUTF16(IDS_PLUGIN_CRASHED_PROMPT, plugin_name),
+          infobar_text,
           true));
 }