Implements a monitor to watch for the browser hanging.

Implements a monitor to watch for the browser hanging.

BUG=

Committed: https://crrev.com/c34049f14ab8d830ccd1b4ad5b7a508b1ad9de27
Cr-Commit-Position: refs/heads/master@{#322395}

[Sigurður Ásgeirsson, 2015-03-25 17:55:12]

siggi@chromium.org changed reviewers:
+ erikwright@chromium.org

[Sigurður Ásgeirsson, 2015-03-25 17:55:13]

PTAL - this should allow polling the browser process singleton window once a
minute, with a 20 second timeout.

[erikwright (departed), 2015-03-25 18:57:22]

https://codereview.chromium.org/1036623002/diff/20001/components/browser_watc...
File components/browser_watcher/window_hang_monitor_win.cc (right):

https://codereview.chromium.org/1036623002/diff/20001/components/browser_watc...
components/browser_watcher/window_hang_monitor_win.cc:91: // Outstanding, the
window is not yet hung.
har har.

https://codereview.chromium.org/1036623002/diff/20001/components/browser_watc...
components/browser_watcher/window_hang_monitor_win.cc:112:
outstanding->monitor->OnPongReceived(lresult);
why pass the lresult?

https://codereview.chromium.org/1036623002/diff/20001/components/browser_watc...
components/browser_watcher/window_hang_monitor_win.cc:112:
outstanding->monitor->OnPongReceived(lresult);
Why the indirection? This is a (static) member method, it can set
outstanding->monitor->outstanding_ping_ to nullptr just fine.

https://codereview.chromium.org/1036623002/diff/20001/components/browser_watc...
components/browser_watcher/window_hang_monitor_win.cc:115: delete outstanding;
I'd maybe put this in OnPongReceived

https://codereview.chromium.org/1036623002/diff/20001/components/browser_watc...
components/browser_watcher/window_hang_monitor_win.cc:119:
DCHECK(window_process_.IsValid());
include logging.h

https://codereview.chromium.org/1036623002/diff/20001/components/browser_watc...
components/browser_watcher/window_hang_monitor_win.cc:125:
callback_.Run(WINDOW_VANISHED);
include callback.h

https://codereview.chromium.org/1036623002/diff/20001/components/browser_watc...
components/browser_watcher/window_hang_monitor_win.cc:139: // Message sending
failed, assume he window is no longer valid,
he -> the

https://codereview.chromium.org/1036623002/diff/20001/components/browser_watc...
components/browser_watcher/window_hang_monitor_win.cc:183: MaybeSendPing();
why the indirection?

https://codereview.chromium.org/1036623002/diff/20001/components/browser_watc...
File components/browser_watcher/window_hang_monitor_win.h (right):

https://codereview.chromium.org/1036623002/diff/20001/components/browser_watc...
components/browser_watcher/window_hang_monitor_win.h:27: // Called precisely
zero or once.
zero or one time(s).

https://codereview.chromium.org/1036623002/diff/20001/components/browser_watc...
components/browser_watcher/window_hang_monitor_win.h:50: // Checks that
|window_| is still valid, and sends it a ping.
Comments for the other methods?

https://codereview.chromium.org/1036623002/diff/20001/components/browser_watc...
components/browser_watcher/window_hang_monitor_win.h:61: WindowHangMonitor*
monitor;
I believe the ordering rules would have this go above all method declaration in
the private section.

https://codereview.chromium.org/1036623002/diff/20001/components/browser_watc...
components/browser_watcher/window_hang_monitor_win.h:67: // The registry path
where a hang event will be recorded.
comment invalid

https://codereview.chromium.org/1036623002/diff/20001/components/browser_watc...
File components/browser_watcher/window_hang_monitor_win_unittest.cc (right):

https://codereview.chromium.org/1036623002/diff/20001/components/browser_watc...
components/browser_watcher/window_hang_monitor_win_unittest.cc:109: const
base::ScopedTempDir& temp_dir() const { return temp_dir_; }
this is just used to get a unique string, right?

How about just concatenating the current process PID with a prefix like
"window_hang_monitor_test_"?

Less obscure, and leaves less crud around if the test dies somehow.

[Sigurður Ásgeirsson, 2015-03-25 19:56:53]

Thanks, PTanotherL?

https://codereview.chromium.org/1036623002/diff/20001/components/browser_watc...
File components/browser_watcher/window_hang_monitor_win.cc (right):

https://codereview.chromium.org/1036623002/diff/20001/components/browser_watc...
components/browser_watcher/window_hang_monitor_win.cc:91: // Outstanding, the
window is not yet hung.
On 2015/03/25 18:57:21, erikwright wrote:
> har har.

Acknowledged.

https://codereview.chromium.org/1036623002/diff/20001/components/browser_watc...
components/browser_watcher/window_hang_monitor_win.cc:112:
outstanding->monitor->OnPongReceived(lresult);
On 2015/03/25 18:57:21, erikwright wrote:
> Why the indirection? This is a (static) member method, it can set
> outstanding->monitor->outstanding_ping_ to nullptr just fine.

Done.

https://codereview.chromium.org/1036623002/diff/20001/components/browser_watc...
components/browser_watcher/window_hang_monitor_win.cc:112:
outstanding->monitor->OnPongReceived(lresult);
On 2015/03/25 18:57:21, erikwright wrote:
> why pass the lresult?

Yeah - indeed. I thought I'd have a use for it.

https://codereview.chromium.org/1036623002/diff/20001/components/browser_watc...
components/browser_watcher/window_hang_monitor_win.cc:115: delete outstanding;
On 2015/03/25 18:57:21, erikwright wrote:
> I'd maybe put this in OnPongReceived

This wants to be in the callback, as that way a late callback will clean up the
instance.

https://codereview.chromium.org/1036623002/diff/20001/components/browser_watc...
components/browser_watcher/window_hang_monitor_win.cc:119:
DCHECK(window_process_.IsValid());
On 2015/03/25 18:57:21, erikwright wrote:
> include logging.h

Done.

https://codereview.chromium.org/1036623002/diff/20001/components/browser_watc...
components/browser_watcher/window_hang_monitor_win.cc:125:
callback_.Run(WINDOW_VANISHED);
On 2015/03/25 18:57:21, erikwright wrote:
> include callback.h

Done.

https://codereview.chromium.org/1036623002/diff/20001/components/browser_watc...
components/browser_watcher/window_hang_monitor_win.cc:139: // Message sending
failed, assume he window is no longer valid,
On 2015/03/25 18:57:21, erikwright wrote:
> he -> the

Done.

https://codereview.chromium.org/1036623002/diff/20001/components/browser_watc...
components/browser_watcher/window_hang_monitor_win.cc:183: MaybeSendPing();
On 2015/03/25 18:57:21, erikwright wrote:
> why the indirection?

For readability - IMHO marginally clearer than to delegate straight to a
function whose only role in life is to reuse some implementation.

https://codereview.chromium.org/1036623002/diff/20001/components/browser_watc...
File components/browser_watcher/window_hang_monitor_win.h (right):

https://codereview.chromium.org/1036623002/diff/20001/components/browser_watc...
components/browser_watcher/window_hang_monitor_win.h:27: // Called precisely
zero or once.
On 2015/03/25 18:57:22, erikwright wrote:
> zero or one time(s).

Done.

https://codereview.chromium.org/1036623002/diff/20001/components/browser_watc...
components/browser_watcher/window_hang_monitor_win.h:50: // Checks that
|window_| is still valid, and sends it a ping.
On 2015/03/25 18:57:21, erikwright wrote:
> Comments for the other methods?

Done.

https://codereview.chromium.org/1036623002/diff/20001/components/browser_watc...
components/browser_watcher/window_hang_monitor_win.h:61: WindowHangMonitor*
monitor;
On 2015/03/25 18:57:21, erikwright wrote:
> I believe the ordering rules would have this go above all method declaration
in
> the private section.

Done.

https://codereview.chromium.org/1036623002/diff/20001/components/browser_watc...
components/browser_watcher/window_hang_monitor_win.h:67: // The registry path
where a hang event will be recorded.
On 2015/03/25 18:57:21, erikwright wrote:
> comment invalid

Done.

https://codereview.chromium.org/1036623002/diff/20001/components/browser_watc...
File components/browser_watcher/window_hang_monitor_win_unittest.cc (right):

https://codereview.chromium.org/1036623002/diff/20001/components/browser_watc...
components/browser_watcher/window_hang_monitor_win_unittest.cc:109: const
base::ScopedTempDir& temp_dir() const { return temp_dir_; }
On 2015/03/25 18:57:22, erikwright wrote:
> this is just used to get a unique string, right?
> 
> How about just concatenating the current process PID with a prefix like
> "window_hang_monitor_test_"?
> 
> Less obscure, and leaves less crud around if the test dies somehow.

Done.

[erikwright (departed), 2015-03-25 20:06:59]

LGTM

https://codereview.chromium.org/1036623002/diff/40001/components/browser_watc...
File components/browser_watcher/window_hang_monitor_win.h (right):

https://codereview.chromium.org/1036623002/diff/40001/components/browser_watc...
components/browser_watcher/window_hang_monitor_win.h:86: // The time after which
|window_| is assumed hung.
blank line preceding, for consistency

https://codereview.chromium.org/1036623002/diff/40001/components/browser_watc...
File components/browser_watcher/window_hang_monitor_win_unittest.cc (right):

https://codereview.chromium.org/1036623002/diff/40001/components/browser_watc...
components/browser_watcher/window_hang_monitor_win_unittest.cc:26: const
base::char16 kRegistryPath[] = L"Software\\HangMonitorTest";
not used

https://codereview.chromium.org/1036623002/diff/40001/components/browser_watc...
components/browser_watcher/window_hang_monitor_win_unittest.cc:81:
base::Unretained(this), &deleted));
deleted is optional and not used, so make it NULL?

Actually no caller ever sends it. So remove the param?

[Sigurður Ásgeirsson, 2015-03-25 20:41:17]

Thanks - it dawned on me that the tests were flaky as written, as ping()
accesses a member that's updated on receipt of an asynchronous message.
Added synchronization or removed the tests where they don't make sense one moar
look?

https://codereview.chromium.org/1036623002/diff/40001/components/browser_watc...
File components/browser_watcher/window_hang_monitor_win.h (right):

https://codereview.chromium.org/1036623002/diff/40001/components/browser_watc...
components/browser_watcher/window_hang_monitor_win.h:86: // The time after which
|window_| is assumed hung.
On 2015/03/25 20:06:58, erikwright wrote:
> blank line preceding, for consistency

Done.

https://codereview.chromium.org/1036623002/diff/40001/components/browser_watc...
File components/browser_watcher/window_hang_monitor_win_unittest.cc (right):

https://codereview.chromium.org/1036623002/diff/40001/components/browser_watc...
components/browser_watcher/window_hang_monitor_win_unittest.cc:26: const
base::char16 kRegistryPath[] = L"Software\\HangMonitorTest";
On 2015/03/25 20:06:59, erikwright wrote:
> not used

Done.

https://codereview.chromium.org/1036623002/diff/40001/components/browser_watc...
components/browser_watcher/window_hang_monitor_win_unittest.cc:81:
base::Unretained(this), &deleted));
On 2015/03/25 20:06:58, erikwright wrote:
> deleted is optional and not used, so make it NULL?
> 
> Actually no caller ever sends it. So remove the param?

Ah - that's why the test flaked - the Wait is missing!. This has to be
synchronous for the DetectsWindowDisappearance test not to flake

[Sigurður Ásgeirsson, 2015-03-25 20:41:19]

Thanks - it dawned on me that the tests were flaky as written, as ping()
accesses a member that's updated on receipt of an asynchronous message.
Added synchronization or removed the tests where they don't make sense one moar
look?

[Sigurður Ásgeirsson, 2015-03-26 15:33:27]

The CQ bit was checked by siggi@chromium.org

[Sigurður Ásgeirsson, 2015-03-26 15:33:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
erikwright@chromium.org
Link to the patchset: https://codereview.chromium.org/1036623002/#ps60001
(title: "Address Erik's comments. Synchronize for ping() or remove the tests.")

[commit-bot: I haz the power, 2015-03-26 15:33:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1036623002/60001

[commit-bot: I haz the power, 2015-03-26 16:09:36]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-26 16:10:57]

Patchset 4 (id:??) landed as
https://crrev.com/c34049f14ab8d830ccd1b4ad5b7a508b1ad9de27
Cr-Commit-Position: refs/heads/master@{#322395}