Distinguish plugin disconnections from plugin crashes.

Distinguish plugin disconnections from plugin crashes.

If the channel between a renderer and a plugin encounters an error, currently we display an infobar saying "<plugin_name> has crashed.".
This may cause confusion, because the plugin process may still be alive and no crash dump is generated in that case.

With this CL, we display a different prompt if the plugin process is still alive:
"<plugin_name> has encountered an error."

This CL only affects Windows.

TEST=None
BUG=None


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=176060

[yzshen1, 2013-01-09 00:25:10]

Hi, reviewers.

Would you please take a look? Thanks!

Jeff: please look at generated_resources.grd.
Justin: please look at view_messages.h.
Brett: everything else.

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

https://codereview.chromium.org/11820009/diff/1/chrome/browser/plugins/plugin...
File chrome/browser/plugins/plugin_observer.cc (right):

https://codereview.chromium.org/11820009/diff/1/chrome/browser/plugins/plugin...
chrome/browser/plugins/plugin_observer.cc:201: is_running =
base::GetTerminationStatus(plugin_handle, NULL) ==
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.

[jeffreyc, 2013-01-09 00:41:44]

So that we don't cause any users to mistakenly think the word "connection"
means something is wrong with their network connection, how about we
simplify it to:

"<plugin_name> has encountered an error."

thanks,
Jeff



On Tue, Jan 8, 2013 at 4:36 PM, <jschuh@chromium.org> wrote:

>
> https://codereview.chromium.**org/11820009/diff/1/chrome/**
>
browser/plugins/plugin_**observer.cc<https://codereview.chromium.org/11820009/diff/1/chrome/browser/plugins/plugin_observer.cc>
> File chrome/browser/plugins/plugin_**observer.cc (right):
>
> https://codereview.chromium.**org/11820009/diff/1/chrome/**
>
browser/plugins/plugin_**observer.cc#newcode201<https://codereview.chromium.org/11820009/diff/1/chrome/browser/plugins/plugin_observer.cc#newcode201>
> chrome/browser/plugins/plugin_**observer.cc:201: is_running =
> base::GetTerminationStatus(**plugin_handle, NULL) ==
> 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.
>
>
https://codereview.chromium.**org/11820009/<https://codereview.chromium.org/1...
>

[yzshen1, 2013-01-09 00:45:49]

https://codereview.chromium.org/11820009/diff/1/chrome/browser/plugins/plugin...
File chrome/browser/plugins/plugin_observer.cc (right):

https://codereview.chromium.org/11820009/diff/1/chrome/browser/plugins/plugin...
chrome/browser/plugins/plugin_observer.cc:201: is_running =
base::GetTerminationStatus(plugin_handle, NULL) ==
On 2013/01/09 00:36:17, Justin Schuh wrote:
> 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.

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!

[yzshen1, 2013-01-09 01:21:29]

Good point. Thanks!
Fixed.
On 2013/01/09 00:41:44, jeffreyc wrote:
> So that we don't cause any users to mistakenly think the word "connection"
> means something is wrong with their network connection, how about we
> simplify it to:
> 
> "<plugin_name> has encountered an error."
> 
> thanks,
> Jeff
> 
> 
> 
> On Tue, Jan 8, 2013 at 4:36 PM, <mailto:jschuh@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.**org/11820009/diff/1/chrome/**
> >
>
browser/plugins/plugin_**observer.cc<https://codereview.chromium.org/11820009/diff/1/chrome/browser/plugins/plugin_observer.cc>
> > File chrome/browser/plugins/plugin_**observer.cc (right):
> >
> > https://codereview.chromium.**org/11820009/diff/1/chrome/**
> >
>
browser/plugins/plugin_**observer.cc#newcode201<https://codereview.chromium.org/11820009/diff/1/chrome/browser/plugins/plugin_observer.cc#newcode201>
> > chrome/browser/plugins/plugin_**observer.cc:201: is_running =
> > base::GetTerminationStatus(**plugin_handle, NULL) ==
> > 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.
> >
> >
>
https://codereview.chromium.**org/11820009/%3Chttps://codereview.chromium.org...>
> >

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

Lgtm on the security side. I'll let someone else weigh in on if the termination
status is a reasonable compromise.

https://codereview.chromium.org/11820009/diff/1/chrome/browser/plugins/plugin...
File chrome/browser/plugins/plugin_observer.cc (right):

https://codereview.chromium.org/11820009/diff/1/chrome/browser/plugins/plugin...
chrome/browser/plugins/plugin_observer.cc:201: is_running =
base::GetTerminationStatus(plugin_handle, NULL) ==
On 2013/01/09 00:45:50, yzshen1 wrote:
> On 2013/01/09 00:36:17, Justin Schuh wrote:
> > 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.
> 
> 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!

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.

[yzshen1, 2013-01-09 06:17:56]

Thanks, Justin.

I have added more comments about the possibility of pid being reused.

On 2013/01/09 01:29:16, Justin Schuh wrote:
> Lgtm on the security side. I'll let someone else weigh in on if the
termination
> status is a reasonable compromise.
> 
>
https://codereview.chromium.org/11820009/diff/1/chrome/browser/plugins/plugin...
> File chrome/browser/plugins/plugin_observer.cc (right):
> 
>
https://codereview.chromium.org/11820009/diff/1/chrome/browser/plugins/plugin...
> chrome/browser/plugins/plugin_observer.cc:201: is_running =
> base::GetTerminationStatus(plugin_handle, NULL) ==
> On 2013/01/09 00:45:50, yzshen1 wrote:
> > On 2013/01/09 00:36:17, Justin Schuh wrote:
> > > 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.
> > 
> > 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!
> 
> 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.

[jochen (gone - plz use gerrit), 2013-01-09 08:29:12]

https://codereview.chromium.org/11820009/diff/14005/content/shell/webkit_test...
File content/shell/webkit_test_controller.cc (right):

https://codereview.chromium.org/11820009/diff/14005/content/shell/webkit_test...
content/shell/webkit_test_controller.cc:263: printer_->AddErrorMessage("#CRASHED
- plugin");
can you change the message to "#CRASHED - plugin (pid %d)" and fill in the
plugin_pid?

[yzshen1, 2013-01-09 18:48:11]

https://codereview.chromium.org/11820009/diff/14005/content/shell/webkit_test...
File content/shell/webkit_test_controller.cc (right):

https://codereview.chromium.org/11820009/diff/14005/content/shell/webkit_test...
content/shell/webkit_test_controller.cc:263: printer_->AddErrorMessage("#CRASHED
- plugin");
On 2013/01/09 08:29:12, jochen wrote:
> can you change the message to "#CRASHED - plugin (pid %d)" and fill in the
> plugin_pid?

Done.

[jochen (gone - plz use gerrit), 2013-01-09 18:49:48]

content/shell lgtm

[brettw, 2013-01-09 19:06:05]

lgtm

[commit-bot: I haz the power, 2013-01-09 21:06:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/11820009/6005

[commit-bot: I haz the power, 2013-01-09 21:06:28]

Failed to apply patch for content/shell/webkit_test_controller.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/shell/webkit_test_controller.cc
  Hunk #1 FAILED at 10.
  Hunk #2 succeeded at 266 (offset 9 lines).
  1 out of 2 hunks FAILED -- saving rejects to file
content/shell/webkit_test_controller.cc.rej

Patch:       content/shell/webkit_test_controller.cc
Index: content/shell/webkit_test_controller.cc
diff --git a/content/shell/webkit_test_controller.cc
b/content/shell/webkit_test_controller.cc
index
7b282765d8500fae63f59b2aee2f807b86fcc367..cd4d4920a9bab466551937ce0fa4e0b127bef337
100644
--- a/content/shell/webkit_test_controller.cc
+++ b/content/shell/webkit_test_controller.cc
@@ -10,6 +10,7 @@
 #include "base/file_util.h"
 #include "base/message_loop.h"
 #include "base/run_loop.h"
+#include "base/stringprintf.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
@@ -257,9 +258,11 @@ bool WebKitTestController::OnMessageReceived(const
IPC::Message& message) {
   return handled;
 }
 
-void WebKitTestController::PluginCrashed(const FilePath& plugin_path) {
+void WebKitTestController::PluginCrashed(const FilePath& plugin_path,
+                                         base::ProcessId plugin_pid) {
   DCHECK(CalledOnValidThread());
-  printer_->AddErrorMessage("#CRASHED - plugin");
+  printer_->AddErrorMessage(
+      base::StringPrintf("#CRASHED - plugin (pid %d)", plugin_pid));
 }
 
 void WebKitTestController::RenderViewCreated(RenderViewHost* render_view_host)
{

[commit-bot: I haz the power, 2013-01-09 21:43:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/11820009/5019

[commit-bot: I haz the power, 2013-01-10 02:47:02]

Retried try job too often on mac_rel for step(s) browser_tests

[commit-bot: I haz the power, 2013-01-10 06:24:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/11820009/5019

[commit-bot: I haz the power, 2013-01-10 14:06:44]

Change committed as 176060