Fix GetChannel interface misuse in ThreadWatcher and ShutdownWatcher.

Fix GetChannel interface misuse and Windows version logic in ThreadWatcher and
ShutdownWatcher.

GetChannel can return CHANNEL_STABLE, CHANNEL_BETA, CHANNEL_DEV,
CHANNEL_CANARY, or CHANNEL_UNKNOWN. CHANNEL_UNKNOWN generally means "not
Google Chrome", that is, Chromium. The existing code ignored the possibility
of CHANNEL_UNKNOWN. Its mishandling of CHANNEL_STABLE resulted in fragile
code.

The existing code also doubled the timeouts on Windows XP only on certain
channels, although the intent was to double them on Windows XP regardless of
channel.

BUG=96155, 95506, 90095
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=100601

[Mark Mentovai, 2011-09-10 18:50:38]

http://codereview.chromium.org/7780022/diff/3/chrome/browser/metrics/thread_w...
File chrome/browser/metrics/thread_watcher.cc (right):

http://codereview.chromium.org/7780022/diff/3/chrome/browser/metrics/thread_w...
chrome/browser/metrics/thread_watcher.cc:431: *unresponsive_threshold *= 2;
I couldn’t figure out why you would want to do this on the beta channel and then
stop doing it when beta is promoted to stable. Checking explicitly for
CHANNEL_STABLE here means that this will behave as I expect was intended if the
stable channel’s default for crash_on_hang_threads changes, or if someone uses
--crash-on-hang-threads without --crash-on-hang-seconds in a stable-channel
build. As it was written, the logic was prone to providing surprising results
under future maintenance.

[Mark Mentovai, 2011-09-10 18:51:33]

Meant to include you too, Jim.

[Mark Mentovai, 2011-09-10 19:09:58]

Patch set 3 does what I think Jim means on bug 96155 comment 2: adjusts the
timeouts based on channel, and then independently gives twice as much time on
XP, regardless of channel.

Jim, is this what you meant? Because another way to read your comment would be
to adjust the timeouts based on channel, and then double it on XP if no
adjustments were made based on channel.

(The difference between the two is just an |else|.)

[jar (doing other things), 2011-09-10 19:20:30]

Thanks for looking at this.  Raman and I have been working at a frantic pace
this week, and I for one was clearly not reviewing as well as I should have.

Since you're in this code, perhaps you can make the other changes mentioned
below.

IF you don't have the time, then even the changes listed so far LGTM.

http://codereview.chromium.org/7780022/diff/3/chrome/browser/metrics/thread_w...
File chrome/browser/metrics/thread_watcher.cc (right):

http://codereview.chromium.org/7780022/diff/3/chrome/browser/metrics/thread_w...
chrome/browser/metrics/thread_watcher.cc:427: if (channel ==
chrome::VersionInfo::CHANNEL_STABLE ||
I agree. This addition makes the code less fragile.

If you want to break this out, I'd suggest that the stable channel should have a
higher multiplier (by default) than the beta channel.  Perhaps at least a factor
of 2 yet again larger.

http://codereview.chromium.org/7780022/diff/3/chrome/browser/metrics/thread_w...
chrome/browser/metrics/thread_watcher.cc:440: *unresponsive_threshold *= 2;
IMO, this multiplier should not be within an if block (line 432) at all.  I
think all channels should get this multiply IF we are using XP (old OS ==>
probably old box ==> need more time now and then).

http://codereview.chromium.org/7780022/diff/3/chrome/browser/metrics/thread_w...
chrome/browser/metrics/thread_watcher.cc:744: actual_duration *= 2;
Here again, I think the XP test should provide the mulitplier for all channels.

[Mark Mentovai, 2011-09-10 19:32:38]

Patch set 4 makes all of the suggested changes. The intervals are always doubled
on XP regardless of channel and regardless of whether the interval was already
adjusted for another reason. The stable channel’s intervals are twice as long as
the beta channel’s.

My remaining question: for channel-based adjustments, ThreadWatcher increases
the time for the stable and beta channels, while ShutdownWatcher increases the
time for the stable, beta, and dev channels. Is this mismatch intentional?
Should ThreadWatcher start increasing time on the dev channel, or should
ShutdownWatcher stop?

[ramant (doing other things), 2011-09-10 22:26:07]

LGTM. Thanks for looking at this code and making the changes.

[ramant (doing other things), 2011-09-10 22:43:11]

On 2011/09/10 19:32:38, Mark Mentovai wrote:
> Patch set 4 makes all of the suggested changes. The intervals are always
doubled
> on XP regardless of channel and regardless of whether the interval was already
> adjusted for another reason. The stable channel’s intervals are twice as long
as
> the beta channel’s.
> 
> My remaining question: for channel-based adjustments, ThreadWatcher increases
> the time for the stable and beta channels, while ShutdownWatcher increases the
> time for the stable, beta, and dev channels. Is this mismatch intentional?
> Should ThreadWatcher start increasing time on the dev channel, or should
> ShutdownWatcher stop?

Haven't found how much time is the correct time. During Shutdown (if some one
has lot of disk cache, lot of bookmarks, lot of history, it seems to take long
time). 

In Dev channel, we were getting 1000+ crashes. I became nervous and asked Jim if
we could increase the time (we thought we will give 10 mins for beta/dev and 20
mins for stable). I wasn't able to handle all the crashes to file bugs. Now we
have increased shutdown time to 90 secs, please remove channel == DEV (we should
do whatever Jim thinks is right). May be we should hard code the the time for
Beta and Stable 
 in the Arm method.

We were trying to pick the top hangs (with shutdown, some tasks on some machines
are taking very long time and we are crashing them unnecessarily - false
positives). 

We have the same issue with ThreadWatcher sometimes (false positives), but in
general we are getting very few hangs where either UI or IO threads are busy and
other browser threads are also busy.

If you want me to make the changes, I can submit a CL.

thanks much.

[Mark Mentovai, 2011-09-10 23:17:59]

I checked this in as-is, with all of the suggested improvements, but without
changing any of the timings as there’s no consensus on what they should change
to. I’d be happy to make further adjustments to the timings if there’s agreement
on what those adjustments should be.