Fix GetChannel interface misuse in ThreadWatcher and ShutdownWatcher.

chrome/browser/metrics/thread_watcher.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/metrics/thread_watcher.h"
 
 #include <math.h>  // ceil
 
 #include "base/debug/alias.h"
 #include "base/string_tokenizer.h"
@@ skipping 405 matching lines @@
 
 // static
 void ThreadWatcherList::ParseCommandLine(
     const CommandLine& command_line,
     uint32* unresponsive_threshold,
     std::set<std::string>* crash_on_hang_thread_names,
     uint32* live_threads_threshold) {
   // Determine |unresponsive_threshold| based on switches::kCrashOnHangSeconds.
   *unresponsive_threshold = kUnresponsiveCount;
 
-  if (chrome::VersionInfo::GetChannel() == chrome::VersionInfo::CHANNEL_BETA) {
-    // Increase the unresponsive_threshold in Beta channel to reduce the number
-    // of crashes due to ThreadWatcher.
+  chrome::VersionInfo::Channel channel = chrome::VersionInfo::GetChannel();
+  if (channel == chrome::VersionInfo::CHANNEL_STABLE ||

[jar (doing other things), 2011/09/10 19:20:31]

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.

+      channel == chrome::VersionInfo::CHANNEL_BETA) {
+    // Increase the unresponsive_threshold in Stable and Beta channels to
+    // reduce the number of crashes due to ThreadWatcher.
     *unresponsive_threshold *= 2;

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

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.

-  } else {
+  } else if (channel == chrome::VersionInfo::CHANNEL_DEV ||
+             channel == chrome::VersionInfo::CHANNEL_CANARY) {
     // In Canary and Dev channels, for Windows XP (old systems), double the
     // unresponsive_threshold to give OS a chance to schedule UI/IO threads a
     // time slice to respond with a pong message (to get around limitations with
     // the OS).
 #if defined(OS_WIN)
     if (base::win::GetVersion() <= base::win::VERSION_XP)
       *unresponsive_threshold *= 2;

[jar (doing other things), 2011/09/10 19:20:31]

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).

 #endif
   }
 
   std::string crash_on_hang_seconds =
       command_line.GetSwitchValueASCII(switches::kCrashOnHangSeconds);
   if (!crash_on_hang_seconds.empty()) {
     int crash_seconds = atoi(crash_on_hang_seconds.c_str());
     if (crash_seconds > 0) {
       *unresponsive_threshold = static_cast<uint32>(
           ceil(static_cast<float>(crash_seconds) / kUnresponsiveSeconds));
     }
   }
 
   std::string crash_on_hang_threads;
 
   // Default to crashing the browser if UI or IO threads are not responsive
   // except in stable channel.
-  if (chrome::VersionInfo::GetChannel() == chrome::VersionInfo::CHANNEL_STABLE)
+  if (channel == chrome::VersionInfo::CHANNEL_STABLE)
     crash_on_hang_threads = "";
   else
     crash_on_hang_threads = "UI,IO";
 
   if (command_line.HasSwitch(switches::kCrashOnHangThreads)) {
     crash_on_hang_threads =
         command_line.GetSwitchValueASCII(switches::kCrashOnHangThreads);
   }
   StringTokenizer tokens(crash_on_hang_threads, ",");
   while (tokens.GetNext())
@@ skipping 261 matching lines @@
 
 void ShutdownWatcherHelper::Arm(const base::TimeDelta& duration) {
   DCHECK(!shutdown_watchdog_);
   base::TimeDelta actual_duration = duration;
   chrome::VersionInfo::Channel channel = chrome::VersionInfo::GetChannel();
   if (channel == chrome::VersionInfo::CHANNEL_STABLE) {
     actual_duration *= 50;
   } else if (channel == chrome::VersionInfo::CHANNEL_BETA ||
              channel == chrome::VersionInfo::CHANNEL_DEV) {
     actual_duration *= 25;
-  } else {
+  } else if (channel == chrome::VersionInfo::CHANNEL_CANARY) {
     // In Canary, for Windows XP, give twice the time for shutdown.
 #if defined(OS_WIN)
     if (base::win::GetVersion() <= base::win::VERSION_XP)
       actual_duration *= 2;

[jar (doing other things), 2011/09/10 19:20:31]

Here again, I think the XP test should provide the mulitplier for all channels.

 #endif
   }
   shutdown_watchdog_ = new ShutdownWatchDogThread(actual_duration);
   shutdown_watchdog_->Arm();
 }