Generate dumps for relevant renderers when users kill a hung pepper plugin.

Generate dumps for relevant renderers when users kill a hung pepper plugin.

This CL collects information in order to diagnose inter-process pepper deadlocks.
We don't crash those renderers. And we only do this on the Windows Canary channel, for 20% of pepper plugin hangs.

BUG=147593
TEST=None


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=157643

[yzshen1, 2012-09-14 18:47:42]

Hi, Brett and Eric.

Would you please take a look? Thanks!

[brettw, 2012-09-14 19:44:37]

lgtm

http://codereview.chromium.org/10909241/diff/7001/chrome/app/breakpad_win.cc
File chrome/app/breakpad_win.cc (right):

http://codereview.chromium.org/10909241/diff/7001/chrome/app/breakpad_win.cc#...
chrome/app/breakpad_win.cc:134: DWORD WINAPI DumpProcessHandlingPepper(void*) {
It would make more sense to me if this was called something like
DumpIfHandlingPepper or something. From the current name, I originally thought
this would find the process handling Pepper calls and dump it.

http://codereview.chromium.org/10909241/diff/7001/chrome/browser/hang_monitor...
File chrome/browser/hang_monitor/hang_crash_dump_win.h (right):

http://codereview.chromium.org/10909241/diff/7001/chrome/browser/hang_monitor...
chrome/browser/hang_monitor/hang_crash_dump_win.h:16: void
CrashDumpProcessHandlingPepper(HANDLE hprocess);
Can this be called CrashDumpIfProcessHandlingPepper instead?

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

http://codereview.chromium.org/10909241/diff/7001/chrome/browser/ui/hung_plug...
chrome/browser/ui/hung_plugin_tab_helper.cc:84:
OwnedHandleVector::~OwnedHandleVector() {
I'd put this inline above. It's very short and I was originally thrown by the
fact that two of the functions are inline and one isn't.

[yzshen1, 2012-09-14 20:07:19]

Thanks Brett!

http://codereview.chromium.org/10909241/diff/7001/chrome/app/breakpad_win.cc
File chrome/app/breakpad_win.cc (right):

http://codereview.chromium.org/10909241/diff/7001/chrome/app/breakpad_win.cc#...
chrome/app/breakpad_win.cc:134: DWORD WINAPI DumpProcessHandlingPepper(void*) {
On 2012/09/14 19:44:37, brettw wrote:
> It would make more sense to me if this was called something like
> DumpIfHandlingPepper or something. From the current name, I originally thought
> this would find the process handling Pepper calls and dump it.

Done.

http://codereview.chromium.org/10909241/diff/7001/chrome/browser/hang_monitor...
File chrome/browser/hang_monitor/hang_crash_dump_win.h (right):

http://codereview.chromium.org/10909241/diff/7001/chrome/browser/hang_monitor...
chrome/browser/hang_monitor/hang_crash_dump_win.h:16: void
CrashDumpProcessHandlingPepper(HANDLE hprocess);
On 2012/09/14 19:44:37, brettw wrote:
> Can this be called CrashDumpIfProcessHandlingPepper instead?

Done.

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

http://codereview.chromium.org/10909241/diff/7001/chrome/browser/ui/hung_plug...
chrome/browser/ui/hung_plugin_tab_helper.cc:84:
OwnedHandleVector::~OwnedHandleVector() {
On 2012/09/14 19:44:37, brettw wrote:
> I'd put this inline above. It's very short and I was originally thrown by the
> fact that two of the functions are inline and one isn't.

Done.

[eroman, 2012-09-14 21:02:26]

+cpu for breakpad_win.cc

I don't think this will work as intended:

AFAICT InjectDumpIfHandlingPepper() will run inside chrome.exe. However the
calls to ScopedTrackPpapiMessage are inside of chrome.dll. So they will actually
be reading/writing different copies of the singleton PpapiMessageTracker (one
for chrome.exe one for chrome.dll).

[yzshen1, 2012-09-14 21:07:43]

On 2012/09/14 21:02:26, eroman wrote:
> +cpu for breakpad_win.cc
> 
> I don't think this will work as intended:
> 
> AFAICT InjectDumpIfHandlingPepper() will run inside chrome.exe. However the
> calls to ScopedTrackPpapiMessage are inside of chrome.dll. So they will
actually
> be reading/writing different copies of the singleton PpapiMessageTracker (one
> for chrome.exe one for chrome.dll).

Thanks, Eric.

I thought we only have one 'singleton' instance in a process?  And I have tested
the code, which worked as intended.

[eroman, 2012-09-14 22:38:27]

My guess is you are testing with the component build, which means the singletons
are going to live in the pepper dll.

Be sure to test using a regular build, where you have just chrome.exe and
chrome.dll.

[yzshen1, 2012-09-16 01:14:24]

On 2012/09/14 22:38:27, eroman wrote:
> My guess is you are testing with the component build, which means the
singletons
> are going to live in the pepper dll.
> 
> Be sure to test using a regular build, where you have just chrome.exe and
> chrome.dll.

Thank you so much, Eric!
I have updated the CL and please take another look.

[eroman, 2012-09-17 19:23:48]

LGTM after answering my comment
(http://codereview.chromium.org/10909241/diff/9003/chrome/browser/ui/hung_plug...)

http://codereview.chromium.org/10909241/diff/9003/chrome/browser/hang_monitor...
File chrome/browser/hang_monitor/hang_crash_dump_win.cc (right):

http://codereview.chromium.org/10909241/diff/9003/chrome/browser/hang_monitor...
chrome/browser/hang_monitor/hang_crash_dump_win.cc:65: // thread using function
pointer relative to chrome.exe, here we creates a
creates --> create

http://codereview.chromium.org/10909241/diff/9003/chrome/browser/hang_monitor...
chrome/browser/hang_monitor/hang_crash_dump_win.cc:74: // Canary channel.
Good description!

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

http://codereview.chromium.org/10909241/diff/9003/chrome/browser/ui/hung_plug...
chrome/browser/ui/hung_plugin_tab_helper.cc:299:
content::BrowserThread::PostTask(
Why is it necessary to duplicate the handles, and then post the task to the IO
thread?

Is your concern that dumping the child processes might block the UI thread for
too long?

At a maximum, it could be kGenerateDumpTimeoutMS * 7 = 1 minute 10 seconds.

If that is the concern though, blocking the IO thread isn't much better --
should post to a worker thread like FILE.

[yzshen1, 2012-09-17 20:11:03]

http://codereview.chromium.org/10909241/diff/9003/chrome/browser/hang_monitor...
File chrome/browser/hang_monitor/hang_crash_dump_win.cc (right):

http://codereview.chromium.org/10909241/diff/9003/chrome/browser/hang_monitor...
chrome/browser/hang_monitor/hang_crash_dump_win.cc:65: // thread using function
pointer relative to chrome.exe, here we creates a
On 2012/09/17 19:23:48, eroman wrote:
> creates --> create

Done.

http://codereview.chromium.org/10909241/diff/9003/chrome/browser/hang_monitor...
chrome/browser/hang_monitor/hang_crash_dump_win.cc:74: // Canary channel.
Thanks!
On 2012/09/17 19:23:48, eroman wrote:
> Good description!

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

http://codereview.chromium.org/10909241/diff/9003/chrome/browser/ui/hung_plug...
chrome/browser/ui/hung_plugin_tab_helper.cc:299:
content::BrowserThread::PostTask(
On 2012/09/17 19:23:48, eroman wrote:
> Why is it necessary to duplicate the handles, and then post the task to the IO
> thread?
> 
> Is your concern that dumping the child processes might block the UI thread for
> too long?
> 
> At a maximum, it could be kGenerateDumpTimeoutMS * 7 = 1 minute 10 seconds.
> 
> If that is the concern though, blocking the IO thread isn't much better --
> should post to a worker thread like FILE.

The reasons why I did it this way:
- I know that we forbid any blocking operations on the UI thread. Letting UI
stop responding for a minute is not okay. I didn't know that we strictly
disallow that on the IO thread as well.

- KillPluginOnIOThread() is called on the IO thread, since
BrowserChildProcessHostIterator should be used on that thread. I would like to
do renderer dumping on the same thread (before killing plugin). Otherwise, we
might kill the plugin first and change all the call stacks of the renderers.

- We could duplicate handle of the plugin as well and move all dumping/killing
to the FILE thread. Just more trouble. :)

Does it make sense? Thanks!

[eroman, 2012-09-17 20:54:31]

i agree that you should do the dumping of child processes before killing the
plugin process.

note that blocking the IO thread is almost as bad as blocking the UI thread,
since it is also our IPC thread. (So renderers will become unresponsive if it is
hung).

[yzshen1, 2012-09-17 23:00:50]

On 2012/09/17 20:54:31, eroman wrote:
> i agree that you should do the dumping of child processes before killing the
> plugin process.
> 
> note that blocking the IO thread is almost as bad as blocking the UI thread,
> since it is also our IPC thread. (So renderers will become unresponsive if it
is
> hung).

Problem addressed! Please take another look. Thanks!

[eroman, 2012-09-18 19:36:34]

lgtm

[commit-bot: I haz the power, 2012-09-19 21:05:42]

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

[commit-bot: I haz the power, 2012-09-19 23:04:04]

Change committed as 157643