Added command line switches "crash-on-hang-threads" and "crash-on-hang-seconds"

Added command line switches "crash-on-hang-threads" and"crash-on-hang-seconds" for crashing the browser whenthreads specified in "crash-on-hang-threads" don't respondfor "crash-on-hang-seconds".TEST=thread watcher unit testsBUG=85282
R=jar
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=89630

[ramant (doing other things), 2011-06-09 20:15:49]

Hi Jim,
  Implemented what we had talked about the "crash-on-hang-threads" and
"crash-on-hang-seconds" command line switches to give users flexibility to crash
on any hung browser thread and the ability to specify how long the watched
thread needs to be unresponsive before we crash the browser.

  Would appreciate your comments.

thanks much,
raman

[jar (doing other things), 2011-06-10 00:38:33]

A few nits, and a few comments about tiny refactoring.

http://codereview.chromium.org/7134007/diff/1011/chrome/browser/metrics/threa...
File chrome/browser/metrics/thread_watcher.cc (right):

http://codereview.chromium.org/7134007/diff/1011/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher.cc:249: if
(ThreadWatcherList::IsResponsive(this))
This interface surprises me.  We are asking a question about ourself, but yet to
do so, we must ask a second class.  I would expect that questions about |this|
can best be answered in our class (i.e., by methods in our class).

You might have to ask some other class if our thread name is on the list of
crashable items... but a question like IsResponsive() sure seems like a local
question.

http://codereview.chromium.org/7134007/diff/1011/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher.cc:271: // Crash the browser if watched
thread is in "--crash-on-hang-threads" command
This sentence seems to focus on the command line switch... but I think we should
just refer to the list of crashable threads (since that list might or might not
come from the command line).

http://codereview.chromium.org/7134007/diff/1011/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher.cc:274: if (no_of_responding_threads == 1
&& ThreadWatcherList::CrashOnHang(this)) {
I'm not sure which is better.... but I suspect we should keep this "presence in
the crash list" as a private attribute, perhaps loading it during construction? 
It always seems nice to remember data that is all about us, inside the instance.
 For example, if this is loaded as a boolean member, we could just check it
before bothering with the count.  I could be wrong with this... and maybe this
is just an (unnecessary) "optimization" to hold that state locally.   Again, I
just don't like having to always ask other classes about what we should do based
on our internal state (our thread id).

http://codereview.chromium.org/7134007/diff/1011/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher.cc:309: crash_on_unresponsive_count_ =
crash_seconds / kUnresponsiveSeconds;
I don't think you meant to scale it down.  If you did, you probably need to
round up.

http://codereview.chromium.org/7134007/diff/1011/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher.cc:316: crash_on_hang_threads = "UI,IO";
You probably could use an early return here.  Perhaps the code would work, but I
think the early return is what was intended (and would read more cleanly).

http://codereview.chromium.org/7134007/diff/1011/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher.cc:321: // We will ignore empty and
duplicate thread_names.
You probably don't need to worry about dups (which are indeed handled by the
map), or blanks (which won't be queried for when you run).  I'd suggest killing
lines 321 and 322.

http://codereview.chromium.org/7134007/diff/1011/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher.cc:330:
global_->crash_on_hang_thread_names_.clear();
I don't think you need to waste time clear()ing.  We're about to delete it
anyway.  Is this just an effort to be cleaner?

http://codereview.chromium.org/7134007/diff/1011/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher.cc:413: bool
ThreadWatcherList::IsResponsive(ThreadWatcher* watcher) {
I'd rather see just the unresponsive_count() passed in, than the whole watcher.

...but then again...

Given that all this method does is compare it to a member of a global, perhaps
we should have the value stored as a value in a static slot of ThreadWatcher
(rather than here in ThreadWatcherList).  This really seems to be associated
with (act as a limit for) a member of ThreadWatcher.

I suspect this should just be a method on ThreadWatcher, comparing a slot to a
static slot.

http://codereview.chromium.org/7134007/diff/1011/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher.cc:419: return false;
Except for unusual circumstances (where we plan to make the logic much hairier
RSN), this should just be:

return watcher->unresponsive_count() < global_->crash_on_unresponsive_count_;

http://codereview.chromium.org/7134007/diff/1011/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher.cc:423: bool
ThreadWatcherList::CrashOnHang(ThreadWatcher* watcher) {
I'd rather see this take a std::string thread_name argument.

I'm also tempted to see this called once during ThreadWatcher construction, to
set a boolean slot in ThreadWatcher.

http://codereview.chromium.org/7134007/diff/1011/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher.cc:433: return false;
return it != global_->crash_on_hang_thread_names_.end();

http://codereview.chromium.org/7134007/diff/1011/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher.cc:450:
global_->crash_on_unresponsive_count_)
FWIW: IF you restructured this (pushing the threshold over into ThreadWatcher),
this would be:
if (it->second->IsUnresponsive())

http://codereview.chromium.org/7134007/diff/1011/chrome/browser/metrics/threa...
File chrome/browser/metrics/thread_watcher.h (right):

http://codereview.chromium.org/7134007/diff/1011/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher.h:331: // is responsive or not.
nit: Don't bother explaining the variable... since it has its own comment.  All
you need is:

// Default values for |crash_on_unresponsive_count_|.

http://codereview.chromium.org/7134007/diff/1011/chrome/browser/metrics/threa...
File chrome/browser/metrics/thread_watcher_unittest.cc (right):

http://codereview.chromium.org/7134007/diff/1011/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher_unittest.cc:247: CommandLine
command_line(CommandLine::NO_PROGRAM);
Is there a nice way we can test this with some command line specified?  List a
thread?  See that the name was picked up?

[ramant (doing other things), 2011-06-13 03:26:02]

Hi Jim,
  Made the changes we had discussed. Would appreciate if you could take a look
at this CL one more time.

thanks much,
raman

http://codereview.chromium.org/7134007/diff/1011/chrome/browser/metrics/threa...
File chrome/browser/metrics/thread_watcher.cc (right):

http://codereview.chromium.org/7134007/diff/1011/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher.cc:249: if
(ThreadWatcherList::IsResponsive(this))
On 2011/06/10 00:38:33, jar wrote:
> This interface surprises me.  We are asking a question about ourself, but yet
to
> do so, we must ask a second class.  I would expect that questions about |this|
> can best be answered in our class (i.e., by methods in our class).
> 
> You might have to ask some other class if our thread name is on the list of
> crashable items... but a question like IsResponsive() sure seems like a local
> question.

Done.

http://codereview.chromium.org/7134007/diff/1011/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher.cc:271: // Crash the browser if watched
thread is in "--crash-on-hang-threads" command
On 2011/06/10 00:38:33, jar wrote:
> This sentence seems to focus on the command line switch... but I think we
should
> just refer to the list of crashable threads (since that list might or might
not
> come from the command line).

Done.

http://codereview.chromium.org/7134007/diff/1011/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher.cc:274: if (no_of_responding_threads == 1
&& ThreadWatcherList::CrashOnHang(this)) {
On 2011/06/10 00:38:33, jar wrote:
> I'm not sure which is better.... but I suspect we should keep this "presence
in
> the crash list" as a private attribute, perhaps loading it during
construction? 
> It always seems nice to remember data that is all about us, inside the
instance.
>  For example, if this is loaded as a boolean member, we could just check it
> before bothering with the count.  I could be wrong with this... and maybe this
> is just an (unnecessary) "optimization" to hold that state locally.   Again, I
> just don't like having to always ask other classes about what we should do
based
> on our internal state (our thread id).

Done.

http://codereview.chromium.org/7134007/diff/1011/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher.cc:309: crash_on_unresponsive_count_ =
crash_seconds / kUnresponsiveSeconds;
On 2011/06/10 00:38:33, jar wrote:
> I don't think you meant to scale it down.  If you did, you probably need to
> round up.

Done.

http://codereview.chromium.org/7134007/diff/1011/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher.cc:316: crash_on_hang_threads = "UI,IO";
On 2011/06/10 00:38:33, jar wrote:
> You probably could use an early return here.  Perhaps the code would work, but
I
> think the early return is what was intended (and would read more cleanly).

Wanted to add UI and IO threads (which we wanted to crash if they are not
responding by default) to crash_on_hang_thread_names_, thus couldn't return
early.

Done.

http://codereview.chromium.org/7134007/diff/1011/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher.cc:321: // We will ignore empty and
duplicate thread_names.
On 2011/06/10 00:38:33, jar wrote:
> You probably don't need to worry about dups (which are indeed handled by the
> map), or blanks (which won't be queried for when you run).  I'd suggest
killing
> lines 321 and 322.

Done.

http://codereview.chromium.org/7134007/diff/1011/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher.cc:330:
global_->crash_on_hang_thread_names_.clear();
On 2011/06/10 00:38:33, jar wrote:
> I don't think you need to waste time clear()ing.  We're about to delete it
> anyway.  Is this just an effort to be cleaner?

Done.

http://codereview.chromium.org/7134007/diff/1011/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher.cc:413: bool
ThreadWatcherList::IsResponsive(ThreadWatcher* watcher) {
On 2011/06/10 00:38:33, jar wrote:
> I'd rather see just the unresponsive_count() passed in, than the whole
watcher.
> 
> ...but then again...
> 
> Given that all this method does is compare it to a member of a global, perhaps
> we should have the value stored as a value in a static slot of ThreadWatcher
> (rather than here in ThreadWatcherList).  This really seems to be associated
> with (act as a limit for) a member of ThreadWatcher.
> 
> I suspect this should just be a method on ThreadWatcher, comparing a slot to a
> static slot.

Moved this method into ThreadWatcher.

Done.

http://codereview.chromium.org/7134007/diff/1011/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher.cc:419: return false;
On 2011/06/10 00:38:33, jar wrote:
> Except for unusual circumstances (where we plan to make the logic much hairier
> RSN), this should just be:
> 
> return watcher->unresponsive_count() < global_->crash_on_unresponsive_count_;

Moved this method into ThreadWatcher.

Done.

http://codereview.chromium.org/7134007/diff/1011/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher.cc:423: bool
ThreadWatcherList::CrashOnHang(ThreadWatcher* watcher) {
On 2011/06/10 00:38:33, jar wrote:
> I'd rather see this take a std::string thread_name argument.
> 
> I'm also tempted to see this called once during ThreadWatcher construction, to
> set a boolean slot in ThreadWatcher.

Done.

http://codereview.chromium.org/7134007/diff/1011/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher.cc:433: return false;
On 2011/06/10 00:38:33, jar wrote:
> return it != global_->crash_on_hang_thread_names_.end();

Done.

http://codereview.chromium.org/7134007/diff/1011/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher.cc:450:
global_->crash_on_unresponsive_count_)
On 2011/06/10 00:38:33, jar wrote:
> FWIW: IF you restructured this (pushing the threshold over into
ThreadWatcher),
> this would be:
> if (it->second->IsUnresponsive())

Done.

http://codereview.chromium.org/7134007/diff/1011/chrome/browser/metrics/threa...
File chrome/browser/metrics/thread_watcher.h (right):

http://codereview.chromium.org/7134007/diff/1011/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher.h:331: // is responsive or not.
On 2011/06/10 00:38:33, jar wrote:
> nit: Don't bother explaining the variable... since it has its own comment. 
All
> you need is:
> 
> // Default values for |crash_on_unresponsive_count_|.

Done.

http://codereview.chromium.org/7134007/diff/1011/chrome/browser/metrics/threa...
File chrome/browser/metrics/thread_watcher_unittest.cc (right):

http://codereview.chromium.org/7134007/diff/1011/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher_unittest.cc:247: CommandLine
command_line(CommandLine::NO_PROGRAM);
On 2011/06/10 00:38:33, jar wrote:
> Is there a nice way we can test this with some command line specified?  List a
> thread?  See that the name was picked up?

Done.

[jar (doing other things), 2011-06-14 00:56:27]

http://codereview.chromium.org/7134007/diff/4019/chrome/browser/metrics/threa...
File chrome/browser/metrics/thread_watcher.cc (right):

http://codereview.chromium.org/7134007/diff/4019/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher.cc:287: // number of other threads
responding is equal to 1. We picked 1 to reduce the
We probably need to parameterize this value "1" as well (i.e., it needs to be a
command line arg, and controlled by a default value).

http://codereview.chromium.org/7134007/diff/4019/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher.cc:317:
DCHECK(!WatchDogThread::CurrentlyOnWatchDogThread());
Is this is the only reason we have a lock, perhaps we can just post a task to
the WatchDogThread to construct this thread watcher list.  If we're willing to
leak it, then we'd have no problem.  If we don't want to leak it, we can post
another task to the WatchDogThread to delete this when we are "done".  It would
be nice if all the code (of significance) ran on the one thread.

http://codereview.chromium.org/7134007/diff/4019/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher.cc:349: StringTokenizer
t(crash_on_hang_threads, ",");
Unless this is really existing practice, it is better to use whole words than
one-letter varible names.  Perhaps "tokens"

http://codereview.chromium.org/7134007/diff/4019/chrome/browser/metrics/threa...
File chrome/browser/metrics/thread_watcher.h (right):

http://codereview.chromium.org/7134007/diff/4019/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher.h:252: uint32
crash_on_unresponsive_count_;
nit: Consider changing the name.  The count is about a threshold, but it may or
may not involve crashing.  How about:

unresponsive_threshold_

http://codereview.chromium.org/7134007/diff/4019/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher.h:256: // |crash_on_unresponsive_count_|
number of ping messages.
Some of the comment are repetetive.  It is nicer when possible to refer to other
items for details.  For instance, this could be:

This is set to true if we want to crash the browser when the watch thread has
become sufficiently unresponsive, while other threads are sufficiently
responsive.


The advantage of this is that IF we change something, we don't need to change a
ton of comments.  It best, these little micro-comments could duplicate a header
comment, which describes the whole picture.  You shouldn't have to keep
mentioning the count in each of these comments though.  Also note that it is
more than count, as we want a couple of things to to have happened.

http://codereview.chromium.org/7134007/diff/4019/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher.h:304: static void GetStatusOfThreads(int*
no_of_responding_threads,
nit: abreviations should not be used in names in this case "no" means "number." 
Suggest:
responding_thread_count
unresponding_thread_count

[ramant (doing other things), 2011-06-16 22:26:45]

Hi Jim,
  Removed the locking in ThreadWatcherList by making all the code that accesses
the globals run on one thread. Reorganized code in ThreadWatcherList object.
Added ThreadWatcherObserver class to watch for UI activity.

  Could you please look at these changes one more time?

thanks very much,
raman

http://codereview.chromium.org/7134007/diff/4019/chrome/browser/metrics/threa...
File chrome/browser/metrics/thread_watcher.cc (right):

http://codereview.chromium.org/7134007/diff/4019/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher.cc:287: // number of other threads
responding is equal to 1. We picked 1 to reduce the
On 2011/06/14 00:56:27, jar wrote:
> We probably need to parameterize this value "1" as well (i.e., it needs to be
a
> command line arg, and controlled by a default value).  

Done.

http://codereview.chromium.org/7134007/diff/4019/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher.cc:317:
DCHECK(!WatchDogThread::CurrentlyOnWatchDogThread());
On 2011/06/14 00:56:27, jar wrote:
> Is this is the only reason we have a lock, perhaps we can just post a task to
> the WatchDogThread to construct this thread watcher list.  If we're willing to
> leak it, then we'd have no problem.  If we don't want to leak it, we can post
> another task to the WatchDogThread to delete this when we are "done".  It
would
> be nice if all the code (of significance) ran on the one thread.

Done.

http://codereview.chromium.org/7134007/diff/4019/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher.cc:349: StringTokenizer
t(crash_on_hang_threads, ",");
On 2011/06/14 00:56:27, jar wrote:
> Unless this is really existing practice, it is better to use whole words than
> one-letter varible names.  Perhaps "tokens"

Done.

http://codereview.chromium.org/7134007/diff/4019/chrome/browser/metrics/threa...
File chrome/browser/metrics/thread_watcher.h (right):

http://codereview.chromium.org/7134007/diff/4019/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher.h:252: uint32
crash_on_unresponsive_count_;
On 2011/06/14 00:56:27, jar wrote:
> nit: Consider changing the name.  The count is about a threshold, but it may
or
> may not involve crashing.  How about:
> 
> unresponsive_threshold_

Done.

http://codereview.chromium.org/7134007/diff/4019/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher.h:256: // |crash_on_unresponsive_count_|
number of ping messages.
On 2011/06/14 00:56:27, jar wrote:
> Some of the comment are repetetive.  It is nicer when possible to refer to
other
> items for details.  For instance, this could be:
> 
> This is set to true if we want to crash the browser when the watch thread has
> become sufficiently unresponsive, while other threads are sufficiently
> responsive.
> 
> 
> The advantage of this is that IF we change something, we don't need to change
a
> ton of comments.  It best, these little micro-comments could duplicate a
header
> comment, which describes the whole picture.  You shouldn't have to keep
> mentioning the count in each of these comments though.  Also note that it is
> more than count, as we want a couple of things to to have happened.

Done.

http://codereview.chromium.org/7134007/diff/4019/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher.h:304: static void GetStatusOfThreads(int*
no_of_responding_threads,
On 2011/06/14 00:56:27, jar wrote:
> nit: abreviations should not be used in names in this case "no" means
"number." 
> Suggest:
> responding_thread_count
> unresponding_thread_count

Done.

[jar (doing other things), 2011-06-18 15:04:16]

LGTM

Just a few style nits.

http://codereview.chromium.org/7134007/diff/17026/chrome/browser/metrics/thre...
File chrome/browser/metrics/thread_watcher.cc (right):

http://codereview.chromium.org/7134007/diff/17026/chrome/browser/metrics/thre...
chrome/browser/metrics/thread_watcher.cc:297: CHECK(crash+thread_id_);
style nit: spaces around binary operator "+"

http://codereview.chromium.org/7134007/diff/17026/chrome/browser/metrics/thre...
chrome/browser/metrics/thread_watcher.cc:303: bool
ThreadWatcher::CrashOnUnresponsiveness() {
nit: The function name almost suggests this function does the crashing.  Better
may be:

IsVeryUnresponsive()

The "Very" will hint at "beyond a threshold"

http://codereview.chromium.org/7134007/diff/17026/chrome/browser/metrics/thre...
chrome/browser/metrics/thread_watcher.cc:522:
g_thread_watcher_list_->registered_.erase(it->first);
nit: Not a big deal for perf here.... but you might be able to just do:
erase(it)
rather than
erase(it->first)

http://codereview.chromium.org/7134007/diff/17026/chrome/browser/metrics/thre...
chrome/browser/metrics/thread_watcher.cc:582: need_to_awaken = true;
nit: Early return would make this cleaner.  You won't need the bool.
if (now - last_wakeup_time_ < wakeup_interval)
  return;

http://codereview.chromium.org/7134007/diff/17026/chrome/browser/metrics/thre...
File chrome/browser/metrics/thread_watcher.h (right):

http://codereview.chromium.org/7134007/diff/17026/chrome/browser/metrics/thre...
chrome/browser/metrics/thread_watcher.h:30: //   want to crash the browser
because of hung watched thread.
nit: You used "watched thread" on line 30 and 27, but used IO thread on lines 22
and 26.  I'm a little wary that this can be confusing.  Perhaps you should
instead consistently (lines 22-30) use the phrase:

watched (IO) thread

http://codereview.chromium.org/7134007/diff/17026/chrome/browser/metrics/thre...
File chrome/browser/metrics/thread_watcher_unittest.cc (right):

http://codereview.chromium.org/7134007/diff/17026/chrome/browser/metrics/thre...
chrome/browser/metrics/thread_watcher_unittest.cc:5: #include <math.h>  // ceil
nit: I'm not used to seeing mention of the function you used.  I'd either
suggest removing the comment, or making it style compliant using a short
sentence with a period at the end.

http://codereview.chromium.org/7134007/diff/17026/chrome/browser/metrics/thre...
chrome/browser/metrics/thread_watcher_unittest.cc:249: ThreadWatcherTest() :
setup_complete_(&lock_),
style nit: I think you have to put the initializers on one line, or else wrap
the : and indent it 4 spaces, and then align initializers undre that.

[ramant (doing other things), 2011-06-19 21:18:16]

Hi Jim,
  Made all the changes you had suggested.

thanks,
raman

http://codereview.chromium.org/7134007/diff/17026/chrome/browser/metrics/thre...
File chrome/browser/metrics/thread_watcher.cc (right):

http://codereview.chromium.org/7134007/diff/17026/chrome/browser/metrics/thre...
chrome/browser/metrics/thread_watcher.cc:297: CHECK(crash+thread_id_);
On 2011/06/18 15:04:16, jar wrote:
> style nit: spaces around binary operator "+"

Done.

http://codereview.chromium.org/7134007/diff/17026/chrome/browser/metrics/thre...
chrome/browser/metrics/thread_watcher.cc:303: bool
ThreadWatcher::CrashOnUnresponsiveness() {
On 2011/06/18 15:04:16, jar wrote:
> nit: The function name almost suggests this function does the crashing. 
Better
> may be:
> 
> IsVeryUnresponsive()
> 
> The "Very" will hint at "beyond a threshold"

Done.

http://codereview.chromium.org/7134007/diff/17026/chrome/browser/metrics/thre...
chrome/browser/metrics/thread_watcher.cc:522:
g_thread_watcher_list_->registered_.erase(it->first);
On 2011/06/18 15:04:16, jar wrote:
> nit: Not a big deal for perf here.... but you might be able to just do:
> erase(it)
> rather than
> erase(it->first)

Done.

http://codereview.chromium.org/7134007/diff/17026/chrome/browser/metrics/thre...
chrome/browser/metrics/thread_watcher.cc:582: need_to_awaken = true;
On 2011/06/18 15:04:16, jar wrote:
> nit: Early return would make this cleaner.  You won't need the bool.
> if (now - last_wakeup_time_ < wakeup_interval)
>   return;

Done.

http://codereview.chromium.org/7134007/diff/17026/chrome/browser/metrics/thre...
File chrome/browser/metrics/thread_watcher.h (right):

http://codereview.chromium.org/7134007/diff/17026/chrome/browser/metrics/thre...
chrome/browser/metrics/thread_watcher.h:30: //   want to crash the browser
because of hung watched thread.
On 2011/06/18 15:04:16, jar wrote:
> nit: You used "watched thread" on line 30 and 27, but used IO thread on lines
22
> and 26.  I'm a little wary that this can be confusing.  Perhaps you should
> instead consistently (lines 22-30) use the phrase:
> 
> watched (IO) thread
> 

Done.

http://codereview.chromium.org/7134007/diff/17026/chrome/browser/metrics/thre...
File chrome/browser/metrics/thread_watcher_unittest.cc (right):

http://codereview.chromium.org/7134007/diff/17026/chrome/browser/metrics/thre...
chrome/browser/metrics/thread_watcher_unittest.cc:5: #include <math.h>  // ceil
On 2011/06/18 15:04:16, jar wrote:
> nit: I'm not used to seeing mention of the function you used.  I'd either
> suggest removing the comment, or making it style compliant using a short
> sentence with a period at the end.

Done.

http://codereview.chromium.org/7134007/diff/17026/chrome/browser/metrics/thre...
chrome/browser/metrics/thread_watcher_unittest.cc:249: ThreadWatcherTest() :
setup_complete_(&lock_),
On 2011/06/18 15:04:16, jar wrote:
> style nit: I think you have to put the initializers on one line, or else wrap
> the : and indent it 4 spaces, and then align initializers undre that.

Done.