Add a hang monitor for Pepper plugins

This adds a hang monitor for Pepper plugins. It monitors sync messages on the I/O thread of the renderer and sends a message to the browser if it's blocked for too long.

The browser will show an infobar allowing you to terminate the plugin.

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

[brettw, 2012-04-10 18:07:56]

http://codereview.chromium.org/10014013/diff/4001/content/common/view_messages.h
File content/common/view_messages.h (right):

http://codereview.chromium.org/10014013/diff/4001/content/common/view_message...
content/common/view_messages.h:1179: // Enable or disable inverting of web
content pixels, for users who prefer
I moved this from below, it was in the wrong place.

http://codereview.chromium.org/10014013/diff/4001/content/public/browser/rend...
File content/public/browser/render_process_host.h (left):

http://codereview.chromium.org/10014013/diff/4001/content/public/browser/rend...
content/public/browser/render_process_host.h:134: // etc. It is generated by
ChildProcessInfo.
Noticed some out-of-date comments when figuring out what to do here.

http://codereview.chromium.org/10014013/diff/4001/ppapi/example/example.cc
File ppapi/example/example.cc (right):

http://codereview.chromium.org/10014013/diff/4001/ppapi/example/example.cc#ne...
ppapi/example/example.cc:9: #include <unistd.h>
You can ignore the changes in this file, but I want to keep it around for
testing, I'll revert this before I commit.

[brettw, 2012-04-10 19:52:34]

Ben: owners review for browser/ui

[Ben Goodger (Google), 2012-04-10 19:58:42]

LGTM for ui

[brettw, 2012-04-10 19:59:19]

substituting Yuzhu for Trung who already has a patch out from me.

[yzshen1, 2012-04-11 18:30:46]

Hi, Brett.

Only some nits. Thanks!

http://codereview.chromium.org/10014013/diff/6002/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10014013/diff/6002/chrome/app/generated_resour...
chrome/app/generated_resources.grd:6114: +        A plug-in (<ph
name="URL">$1<ex>Flash</ex></ph>) isn't responding.
nit: It is better to name it something like "plugin_name" instead of "URL".

http://codereview.chromium.org/10014013/diff/6002/chrome/browser/ui/hung_plug...
File chrome/browser/ui/hung_plugin_tab_helper.cc (right):

http://codereview.chromium.org/10014013/diff/6002/chrome/browser/ui/hung_plug...
chrome/browser/ui/hung_plugin_tab_helper.cc:43: // TODO(brettw) it might be nice
to do some stuff to capture a stack.
Please do so soon. :)
Besides, please record some stats so that we can measure the number of hangs in
the field.

http://codereview.chromium.org/10014013/diff/6002/chrome/browser/ui/hung_plug...
chrome/browser/ui/hung_plugin_tab_helper.cc:155: TabContentsWrapper* tcw =
line 155-159 can be extracted as a separate method in order t o avoid
duplicating code.

http://codereview.chromium.org/10014013/diff/6002/chrome/browser/ui/hung_plug...
File chrome/browser/ui/hung_plugin_tab_helper.h (right):

http://codereview.chromium.org/10014013/diff/6002/chrome/browser/ui/hung_plug...
chrome/browser/ui/hung_plugin_tab_helper.h:36:
HungPluginTabHelper(content::WebContents* contents);
explicit, please.

http://codereview.chromium.org/10014013/diff/6002/chrome/browser/ui/hung_plug...
chrome/browser/ui/hung_plugin_tab_helper.h:59: bool is_hung;
This member is not used.

http://codereview.chromium.org/10014013/diff/6002/chrome/browser/ui/hung_plug...
chrome/browser/ui/hung_plugin_tab_helper.h:64: base::TimeTicks bar_closed_time;
This member is not useful, either.

http://codereview.chromium.org/10014013/diff/6002/chrome/browser/ui/hung_plug...
chrome/browser/ui/hung_plugin_tab_helper.h:93: // Called on a timer for a hung
plugin if the bar is closed to re-show the
nit: please consider removing the "if the bar is closed" part.

http://codereview.chromium.org/10014013/diff/6002/content/browser/ppapi_plugi...
File content/browser/ppapi_plugin_process_host.cc (right):

http://codereview.chromium.org/10014013/diff/6002/content/browser/ppapi_plugi...
content/browser/ppapi_plugin_process_host.cc:212: process_->GetData().id);
Should it be 0?

http://codereview.chromium.org/10014013/diff/6002/content/public/browser/web_...
File content/public/browser/web_contents_delegate.h (right):

http://codereview.chromium.org/10014013/diff/6002/content/public/browser/web_...
content/public/browser/web_contents_delegate.h:378: // The child_id is the
unique child process ID from the plugin. Note that this
nit: child_id -> plugin_child_id.

http://codereview.chromium.org/10014013/diff/6002/content/public/browser/web_...
content/public/browser/web_contents_delegate.h:380: // for anything in case
there's an exploited renderer. The name will be
"name" is not presented as a parameter?

http://codereview.chromium.org/10014013/diff/6002/content/public/browser/web_...
content/public/browser/web_contents_delegate.h:382: // implementation should use
the patch to extract the plugin name.
patch -> path.

http://codereview.chromium.org/10014013/diff/6002/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_hung_plugin_filter.h (right):

http://codereview.chromium.org/10014013/diff/6002/content/renderer/pepper/pep...
content/renderer/pepper/pepper_hung_plugin_filter.h:89: // Time that the last
message was received from the plugin. This will be 0
Please remove one of the "This will be xxx ... " sentences.

http://codereview.chromium.org/10014013/diff/6002/ppapi/example/example.cc
File ppapi/example/example.cc (right):

http://codereview.chromium.org/10014013/diff/6002/ppapi/example/example.cc#ne...
ppapi/example/example.cc:194: sleep(15);
This is not going to be checked in, right?
Besides, I think this doesn't work on Windows. You probably want to use
::Sleep(15*1000)

[brettw, 2012-04-11 19:56:08]

Thanks, I should have fixed everything.

http://codereview.chromium.org/10014013/diff/6002/chrome/browser/ui/hung_plug...
File chrome/browser/ui/hung_plugin_tab_helper.cc (right):

http://codereview.chromium.org/10014013/diff/6002/chrome/browser/ui/hung_plug...
chrome/browser/ui/hung_plugin_tab_helper.cc:43: // TODO(brettw) it might be nice
to do some stuff to capture a stack.
I filed a bug for this and updated the comment.

[yzshen1, 2012-04-11 20:08:09]

LGTM

with two more nits.

http://codereview.chromium.org/10014013/diff/6002/content/public/browser/web_...
File content/public/browser/web_contents_delegate.h (right):

http://codereview.chromium.org/10014013/diff/6002/content/public/browser/web_...
content/public/browser/web_contents_delegate.h:380: // for anything in case
there's an exploited renderer. The name will be
On 2012/04/11 18:30:46, yzshen1 wrote:
> "name" is not presented as a parameter?
This is not addressed. (Have I misunderstood it?)

http://codereview.chromium.org/10014013/diff/16001/chrome/browser/ui/hung_plu...
File chrome/browser/ui/hung_plugin_tab_helper.cc (right):

http://codereview.chromium.org/10014013/diff/16001/chrome/browser/ui/hung_plu...
chrome/browser/ui/hung_plugin_tab_helper.cc:154: InfoBarTabHelper*
infobar_helper = tcw->infobar_tab_helper();
GetInfoBarHelper()? (And also other places in the file.)

[brettw, 2012-04-11 20:30:25]

http://codereview.chromium.org/10014013/diff/6002/content/public/browser/web_...
File content/public/browser/web_contents_delegate.h (right):

http://codereview.chromium.org/10014013/diff/6002/content/public/browser/web_...
content/public/browser/web_contents_delegate.h:380: // for anything in case
there's an exploited renderer. The name will be
Sorry! This was from a previous version of the patch but I never ended up using
the name so I removed it.

http://codereview.chromium.org/10014013/diff/16001/chrome/browser/ui/hung_plu...
File chrome/browser/ui/hung_plugin_tab_helper.cc (right):

http://codereview.chromium.org/10014013/diff/16001/chrome/browser/ui/hung_plu...
chrome/browser/ui/hung_plugin_tab_helper.cc:154: InfoBarTabHelper*
infobar_helper = tcw->infobar_tab_helper();
On 2012/04/11 20:08:10, yzshen1 wrote:
> GetInfoBarHelper()? (And also other places in the file.)

Thanks, this time I actually waited for the compile to finish :P

[brettw, 2012-04-11 20:43:14]

Actually uploaded the patch now.

[yzshen1, 2012-04-11 20:45:09]

lgtm

[commit-bot: I haz the power, 2012-04-12 18:24:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brettw@chromium.org/10014013/11009

[commit-bot: I haz the power, 2012-04-12 21:52:24]

Try job failure for 10014013-11009 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...