Watchdog - Added a JOINABLE state to avoid StartupTimebomb

Watchdog - Added a JOINABLE state to avoid StartupTimebomb
blocking UI thread during Watchdog thread join.

StartupTimebomb deletes the startup_watchdog thread only if
it is joinable. This fixes the case when UI thread is blocked
until watchdog thread finishes generating the dump. StartupTimebomb
keeps calling IsJoinable on startup_watchdog thread every 10 secs
until it is joinable until it is deleted.

The delete method calls Cleanup to set the state to SHUTDOWN
for backward compatability.

BUG=110055
R=jar
TEST=base unit tests and browser ui and unit tests.

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

[ramant (doing other things), 2012-01-18 01:30:56]

Hi Jim,
  This is the change we had discussed to watchdog and threadwatcher. Would
appreciate your comments.

thanks,
raman

[jar (doing other things), 2012-01-18 17:24:44]

I think the change is a great direction.  See comments below about another
concern I had.

http://codereview.chromium.org/9173002/diff/7001/base/threading/watchdog.cc
File base/threading/watchdog.cc (right):

http://codereview.chromium.org/9173002/diff/7001/base/threading/watchdog.cc#n...
base/threading/watchdog.cc:48: init_successful_ = PlatformThread::Create(0,  //
Default stack size.
Perhaps we should save this return status into a temp variable, then grab the
lock, and assign it into the init_successful_ slot.  Similarly, perhaps we
should hold the lock when examining this slot.  It seems like a critical part of
the state, an intimately tied to the correctness of the slot |handle_|.

It could even be that we should mix this value into the state_ enum, adding
another state:

UNINITIALIZED
DISARMED
ARMED
SHUTDOWN
JOINABLE

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

http://codereview.chromium.org/9173002/diff/7001/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher.cc:855:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
FYI: This is an assertion that is failing on the try bot currently under the
guise of the sync test failing.  I can't see the call stack, and can only see
the CHECK.

[ramant (doing other things), 2012-01-18 20:45:30]

Hi Jim,
  Thanks very much for catching the sync tests failure. Made the changes we had
discussed.

thanks,
raman

http://codereview.chromium.org/9173002/diff/7001/base/threading/watchdog.cc
File base/threading/watchdog.cc (right):

http://codereview.chromium.org/9173002/diff/7001/base/threading/watchdog.cc#n...
base/threading/watchdog.cc:48: init_successful_ = PlatformThread::Create(0,  //
Default stack size.
On 2012/01/18 17:24:44, jar wrote:
> Perhaps we should save this return status into a temp variable, then grab the
> lock, and assign it into the init_successful_ slot.  Similarly, perhaps we
> should hold the lock when examining this slot.  It seems like a critical part
of
> the state, an intimately tied to the correctness of the slot |handle_|.
> 
> It could even be that we should mix this value into the state_ enum, adding
> another state:
> 
> UNINITIALIZED
> DISARMED
> ARMED
> SHUTDOWN
> JOINABLE

jar and I had talked and we have decided to change init_successful_ to enabled_.
enabled_ is initialized to the enabled value that is passed in. If Watchdog is
enabled, we will create/start the thread and we set the enabled_ to true/false
based on the value returned by Thread's creation.

Done.

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

http://codereview.chromium.org/9173002/diff/7001/chrome/browser/metrics/threa...
chrome/browser/metrics/thread_watcher.cc:855:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2012/01/18 17:24:44, jar wrote:
> FYI: This is an assertion that is failing on the try bot currently under the
> guise of the sync test failing.  I can't see the call stack, and can only see
> the CHECK.

It was being run on Test thread. The DCHECK was wrong. We already check that it
is being Disarm'ed on the same thread as the thread on which it was
created/Armed.

Done.

[jar (doing other things), 2012-01-18 22:27:01]

lgtm

[commit-bot: I haz the power, 2012-01-18 22:56:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/9173002/15001

[commit-bot: I haz the power, 2012-01-19 00:07:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/9173002/15001

[commit-bot: I haz the power, 2012-01-19 01:46:52]

Change committed as 118209