Add an outdated upgrade bubble view.

chrome/browser/upgrade_detector_impl.cc

Index: chrome/browser/upgrade_detector_impl.cc
diff --git a/chrome/browser/upgrade_detector_impl.cc b/chrome/browser/upgrade_detector_impl.cc
index 570275e391ba59c8452c3de6d6861f7f4ead8e04..6cd508afc482e70b0bf6f6c246c130be31fe68f3 100644
--- a/chrome/browser/upgrade_detector_impl.cc
+++ b/chrome/browser/upgrade_detector_impl.cc
@@ -7,28 +7,33 @@
 #include <string>
 
 #include "base/bind.h"
+#include "base/build_time.h"
 #include "base/command_line.h"
 #include "base/file_path.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/singleton.h"
+#include "base/metrics/field_trial.h"
 #include "base/path_service.h"
 #include "base/string_number_conversions.h"
 #include "base/string_util.h"
 #include "base/time.h"
 #include "base/utf_string_conversions.h"
+#include "base/version.h"
+#include "chrome/browser/browser_process.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/chrome_version_info.h"
-#include "chrome/installer/util/browser_distribution.h"
 #include "content/public/browser/browser_thread.h"
 #include "ui/base/resource/resource_bundle.h"
 
 #if defined(OS_WIN)
+#include "chrome/installer/util/browser_distribution.h"
+#include "chrome/installer/util/google_update_settings.h"
+#include "chrome/installer/util/helper.h"
 #include "chrome/installer/util/install_util.h"
 #elif defined(OS_MACOSX)
 #include "chrome/browser/mac/keystone_glue.h"
 #elif defined(OS_POSIX)
 #include "base/process_util.h"
-#include "base/version.h"
 #endif
 
 using content::BrowserThread;
@@ -46,6 +51,13 @@ const int kNotifyCycleTimeMs = 20 * 60 * 1000;  // 20 minutes.
 // Same as kNotifyCycleTimeMs but only used during testing.
 const int kNotifyCycleTimeForTestingMs = 500;  // Half a second.
 
+// The number of days after which we identify a build/install as outdated.
+const uint64 kOutdatedBuildAgeInDays = 12 * 7;
+
+// Finch Experiment strings to identify if we should check for outdated install.
+const char kOutdatedInstallCheckTrialName[] = "OutdatedInstallCheck";
+const char kOutdatedInstallCheck12WeeksGroupName[] = "12WeeksOutdatedIntalls";
+
 std::string CmdLineInterval() {
   const CommandLine& cmd_line = *CommandLine::ForCurrentProcess();
   return cmd_line.GetSwitchValueASCII(switches::kCheckForUpdateIntervalSec);
@@ -54,7 +66,9 @@ std::string CmdLineInterval() {
 bool IsTesting() {
   const CommandLine& cmd_line = *CommandLine::ForCurrentProcess();
   return cmd_line.HasSwitch(switches::kSimulateUpgrade) ||
-      cmd_line.HasSwitch(switches::kCheckForUpdateIntervalSec);
+      cmd_line.HasSwitch(switches::kCheckForUpdateIntervalSec) ||
+      cmd_line.HasSwitch(switches::kSimulateCriticalUpdate) ||
+      cmd_line.HasSwitch(switches::kSimulateOutdated);
 }
 
 // How often to check for an upgrade.
@@ -68,12 +82,120 @@ int GetCheckForUpgradeEveryMs() {
   return kCheckForUpgradeMs;
 }
 
+#if defined(OS_WIN)
+bool IsSystemInstall() {
+  FilePath exe_path;
+  if (!PathService::Get(base::DIR_EXE, &exe_path)) {
+    NOTREACHED() << "Failed to find executable path";
+    return false;
+  }
+
+  return !InstallUtil::IsPerUserInstall(exe_path.value().c_str());
+}
+
+// This task checks the update policy and calls back the task only if automatic
+// updates are allowed.
+void DetectUpdatability(bool* is_auto_update_allowed) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+
+  string16 app_guid = installer::GetAppGuidForUpdates(IsSystemInstall());
+  DCHECK(!app_guid.empty());
+  *is_auto_update_allowed = GoogleUpdateSettings::AUTOMATIC_UPDATES ==
+      GoogleUpdateSettings::GetAppUpdatePolicy(app_guid, NULL);
+}
+#endif  // defined(OS_WIN)
+
+}  // namespace
+
+UpgradeDetectorImpl::UpgradeDetectorImpl()
+    : ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)),
+      is_unstable_channel_(false),
+      is_outdated_reinstall_allowed_(false),
+      build_date_(base::GetBuildTime()) {
+  CommandLine command_line(*CommandLine::ForCurrentProcess());
+  // The different command line switches that affect testing can't be used
+  // simultaneously, if they do, here's the precedence order, based on the order
+  // of the if statements below:
+  // - kDisableBackgroundNetworking prevents any of the other tests to work.

[Finnur, 2013/02/01 17:09:15]

nit: You prevent something _from_ working, but you enable something _to_ work
(if I'm not mistaken).

But how do the tests factor into this? Should you replace "test" with "command
line flag"?

[MAD, 2013/02/02 03:55:34]

Done.

+  // - kSimulateUpgrade will not test critical or outdated upgrade tests.
+  // - kSimulateCriticalUpdate has precedence over kSimulateOutdated.
+  // - kSimulateOutdated can work on it's own, or with a specified date.
+  if (command_line.HasSwitch(switches::kDisableBackgroundNetworking))
+    return;
+  if (command_line.HasSwitch(switches::kSimulateUpgrade)) {
+    UpgradeDetected(false, is_unstable_channel_);
+    return;
+  }
+  if (command_line.HasSwitch(switches::kSimulateCriticalUpdate)) {
+    UpgradeDetected(true, is_unstable_channel_);
+    return;
+  }
+  if (command_line.HasSwitch(switches::kSimulateOutdated)) {
+    // The outdated simulation can work without a value, which means outdated
+    // now, or with a value that must be a well formed date/time string that
+    // overrides the build date.
+    is_outdated_reinstall_allowed_ = true;
+    std::string build_date = command_line.GetSwitchValueASCII(
+        switches::kSimulateOutdated);
+    base::Time maybe_build_time;
+    bool result = base::Time::FromString(build_date.c_str(), &maybe_build_time);
+    if (result && !maybe_build_time.is_null()) {
+      // We got a valid build date simulation so use it and check for upgrades.
+      build_date_ = maybe_build_time;
+      detect_upgrade_timer_.Start(FROM_HERE,
+          base::TimeDelta::FromMilliseconds(GetCheckForUpgradeEveryMs()),
+          this, &UpgradeDetectorImpl::CheckForUpgrade);
+    } else {
+      // Without a valid date, we simulate that we are already outdated...
+      upgrade_required_ = UPGRADE_REQUIRED_OUTDATED;
+      UpgradeDetected(false, is_unstable_channel_);
+    }
+    return;
+  }
+  // Windows: only enable upgrade notifications for official builds.
+  // Mac: only enable them if the updater (Keystone) is present.
+  // Linux (and other POSIX): always enable regardless of branding.
+#if (defined(OS_WIN) && defined(GOOGLE_CHROME_BUILD)) || defined(OS_POSIX)
+#if defined(OS_MACOSX)
+  is_outdated_reinstall_allowed_ = keystone_glue::KeystoneEnabled();
+  if (is_outdated_reinstall_allowed_)
+#endif
+  {
+    detect_upgrade_timer_.Start(FROM_HERE,
+        base::TimeDelta::FromMilliseconds(GetCheckForUpgradeEveryMs()),
+        this, &UpgradeDetectorImpl::CheckForUpgrade);
+  }
+#endif
+
+  // Only enable the outdated install check if we are running the trial for it.
+  if (base::FieldTrialList::FindFullName(kOutdatedInstallCheckTrialName) !=
+      kOutdatedInstallCheck12WeeksGroupName) {
+    is_outdated_reinstall_allowed_ = false;
+    return;
+  }
+
+// On Windows, there might be a policy preventing updates, so validate
+// updatability, and then set |is_outdated_reinstall_allowed_| appropriately.
+#if defined(OS_WIN) && defined(GOOGLE_CHROME_BUILD)
+  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
+                          base::Bind(&DetectUpdatability,

[Finnur, 2013/02/01 17:09:15]

I believe we have a race condition here for the Windows Release channel and I
don't think it is the one you mentioned. We fire off two tasks, first
CheckForUpgrade and then DetectUpdatability, but the one that is posted *first*
depends on a variable (is_outdated_reinstall_allowed_) that the latter sets.
That seems like ripe for a race condition... Also, I think that variable isn't
really needed because this whole class shouldn't do anything if that variable
becomes false. Right? If you can't update you can't update. Outdated update is
just one of the things it can't do.

However, I think this CL necessitates a restructure of the ctor a bit anyway,
because it is getting too difficult to understand, especially with the new
ifdefs and the new outdated variable. My thinking is that restructuring will not
only simplify all this logic but also gets rid of the
is_outdated_reinstall_allowed_ variable altogether. 

Here's what I have in mind: I suspect I'd keep everything in the ctor down to
line 155 as is, but at the end just fire off a CheckForUpgradability async task
on the FILE thread (then the ctor is done and does nothing more). That new task,
however, would look something like this:

void CheckForUpgradability() {
#if defined(OS_WINDOW) && defined(GOOGLE_CHROME_BUILD)
  if (!DetectUpdatability())
    return;  // Upgrade is blocked by enterprise policy.
#elif defined(OS_WINDOW) && !defined(GOOGLE_CHROME_BUILD)
  return;  // Chromium has no upgrade channel.
#elif defined(OS_MACOSX)
  if (!keystone_glue::KeystoneEnabled())
    return;  // Keystone updater not enabled.
#elif !defined(OS_POSIX)
  return;
#endif

  [Jump jubilantly to the UI thread to perform CheckForUpgrade()]
}

Now, I'm assuming KeystoneEnabled can be called on the FILE thread, as that
would be the simplest and cleanest implementation. If that doesn't work then,
just call this function on the UI thread directly from the ctor (then there is
no need to jump jubilantly, just call it directly). Then the top becomes:

#if defined(OS_WINDOW) && defined(GOOGLE_CHROME_BUILD)
  [Jump to DetectUpdatability on the FILE thread, which will result
   in a jump to the UI thread for CheckforUpgrade if Chrome is 
   upgradable]
   return;
#elif ... etc

There might be other options too if you take a step back and think about this
from a higher level. Also note that I think we don't need to check for upgrade
if we find that Chrome is outdated, so we can skip that part and early return on
outdate builds (after notifying the user). Maybe that helps us simplify the
restructuring a bit.

I can't really comment on the change to static. Maybe my brain is fried but on
the face of it I don't think it is necessary. However, this is enough for you to
digest for now. We can talk about that later (unless it becomes a moot point
because of restructuring, which I'm kind of hoping).

[MAD, 2013/02/02 03:55:34]

OK, I agree that things need to be cleaned up, but for the cases where there is
nothing to do on the FILE thread, we might as well stay on the UI thread. No?

Let see what you think of the refactoring I did.

+                                     &is_outdated_reinstall_allowed_));
+#elif defined(OS_POSIX) && !(defined(OS_MACOSX) || defined(OS_CHROME))
+  // All other POSIX devices are always allowed, except OS_MACOSX which sets it
+  // above, and Chrome OS which doesn't support the reinstall bubble.
+  is_outdated_reinstall_allowed_ = true;
+#endif
+}
+
+UpgradeDetectorImpl::~UpgradeDetectorImpl() {
+}
+
+// Static
 // This task checks the currently running version of Chrome against the
-// installed version. If the installed version is newer, it runs the passed
-// callback task. Otherwise it just deletes the task.
-void DetectUpgradeTask(const base::Closure& upgrade_detected_task,
-                       bool* is_unstable_channel,
-                       bool* is_critical_upgrade) {
+// installed version. If the installed version is newer, it calls back
+// UpgradeDetectorImpl::UpgradeDetected.
+void UpgradeDetectorImpl::DetectUpgradeTask(
+    base::WeakPtr<UpgradeDetectorImpl> upgrade_detector) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
   Version installed_version;
@@ -83,14 +205,7 @@ void DetectUpgradeTask(const base::Closure& upgrade_detected_task,
   // Get the version of the currently *installed* instance of Chrome,
   // which might be newer than the *running* instance if we have been
   // upgraded in the background.
-  FilePath exe_path;
-  if (!PathService::Get(base::DIR_EXE, &exe_path)) {
-    NOTREACHED() << "Failed to find executable path";
-    return;
-  }
-
-  bool system_install =
-      !InstallUtil::IsPerUserInstall(exe_path.value().c_str());
+  bool system_install = IsSystemInstall();
 
   // TODO(tommi): Check if using the default distribution is always the right
   // thing to do.
@@ -118,8 +233,8 @@ void DetectUpgradeTask(const base::Closure& upgrade_detected_task,
 #endif
 
   chrome::VersionInfo::Channel channel = chrome::VersionInfo::GetChannel();
-  *is_unstable_channel = channel == chrome::VersionInfo::CHANNEL_DEV ||
-                         channel == chrome::VersionInfo::CHANNEL_CANARY;
+  bool is_unstable_channel = channel == chrome::VersionInfo::CHANNEL_DEV ||
+                             channel == chrome::VersionInfo::CHANNEL_CANARY;
 
   // Get the version of the currently *running* instance of Chrome.
   chrome::VersionInfo version_info;
@@ -140,64 +255,71 @@ void DetectUpgradeTask(const base::Closure& upgrade_detected_task,
       (installed_version.CompareTo(running_version) > 0)) {
     // If a more recent version is available, it might be that we are lacking
     // a critical update, such as a zero-day fix.
-    *is_critical_upgrade =
-        critical_update.IsValid() &&
-        (critical_update.CompareTo(running_version) > 0);
+    bool critical = critical_update.IsValid() &&
+                    critical_update.CompareTo(running_version) > 0;
 
     // Fire off the upgrade detected task.
     BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-                            upgrade_detected_task);
+                            base::Bind(&UpgradeDetectorImpl::UpgradeDetected,
+                            upgrade_detector, critical, is_unstable_channel));
   }
 }
 
-}  // namespace
-
-UpgradeDetectorImpl::UpgradeDetectorImpl()
-    : ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)),
-      is_unstable_channel_(false) {
-  CommandLine command_line(*CommandLine::ForCurrentProcess());
-  if (command_line.HasSwitch(switches::kDisableBackgroundNetworking))
-    return;
-  if (command_line.HasSwitch(switches::kSimulateUpgrade)) {
-    UpgradeDetected();
-    return;
-  }
-  // Windows: only enable upgrade notifications for official builds.
-  // Mac: only enable them if the updater (Keystone) is present.
-  // Linux (and other POSIX): always enable regardless of branding.
-#if (defined(OS_WIN) && defined(GOOGLE_CHROME_BUILD)) || defined(OS_POSIX)
-#if defined(OS_MACOSX)
-  if (keystone_glue::KeystoneEnabled())
-#endif
-  {
-    detect_upgrade_timer_.Start(FROM_HERE,
-        base::TimeDelta::FromMilliseconds(GetCheckForUpgradeEveryMs()),
-        this, &UpgradeDetectorImpl::CheckForUpgrade);
+void UpgradeDetectorImpl::CheckForUpgrade() {
+  // Only check for outdated install when allowed and when no upgrade has been
+  // detected.
+  if (is_outdated_reinstall_allowed_ &&

[Finnur, 2013/02/01 17:09:15]

This would be a field trial check instead (according to my proposal).

[MAD, 2013/02/02 03:55:34]

Done.

+      upgrade_required_ == UPGRADE_REQUIRED_NONE) {
+    CompareBuildTimeToSaneTime();
   }
-#endif
-}
 
-UpgradeDetectorImpl::~UpgradeDetectorImpl() {
-}
+  // If we are simulating an outdated install, no need to detect an upgrade.
+  // Since upgrade has precedence over outdated installs, it would prevent
+  // testing. And [critical] upgrade simulations should never get here, they go
+  // straight to UpgtradeDetected.
+  const CommandLine& cmd_line = *CommandLine::ForCurrentProcess();
+  DCHECK(!cmd_line.HasSwitch(switches::kSimulateCriticalUpdate));
+  DCHECK(!cmd_line.HasSwitch(switches::kSimulateUpgrade));
+  if (cmd_line.HasSwitch(switches::kSimulateOutdated))
+    return;
 
-void UpgradeDetectorImpl::CheckForUpgrade() {
   weak_factory_.InvalidateWeakPtrs();
-  base::Closure callback_task =
-      base::Bind(&UpgradeDetectorImpl::UpgradeDetected,
-                 weak_factory_.GetWeakPtr());
   // We use FILE as the thread to run the upgrade detection code on all
   // platforms. For Linux, this is because we don't want to block the UI thread
   // while launching a background process and reading its output; on the Mac and
   // on Windows checking for an upgrade requires reading a file.
   BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
-                          base::Bind(&DetectUpgradeTask,
-                                     callback_task,
-                                     &is_unstable_channel_,
-                                     &is_critical_upgrade_));
+                          base::Bind(&UpgradeDetectorImpl::DetectUpgradeTask,
+                                     weak_factory_.GetWeakPtr()));
+}
+
+void UpgradeDetectorImpl::CompareBuildTimeToSaneTime() {
+  DCHECK(is_outdated_reinstall_allowed_);
+  // Check again, in case we got posted more than once by CheckForUpgrade.
+  if (upgrade_required_ != UPGRADE_REQUIRED_NONE)
+    return;
+
+  base::Time network_time;
+// TODO(mad): Uncomment when variations service NetworkTime CL get committed.
+//  if (!g_browser_process->variations_service()->GetNetworkTime(&network_time))
+//    return;

[Finnur, 2013/02/01 17:09:15]

I likey.

[MAD, 2013/02/02 03:55:34]

B-)

+

[Finnur, 2013/02/01 17:09:15]

I think you should add a check, just to be sure. Something like...

if (build_date_ > network_time) {
  NOTREACHED();
  return;
}

[MAD, 2013/02/02 03:55:34]

Done.

+  DCHECK(!network_time.is_null());

[Finnur, 2013/02/01 17:09:15]

You could pull that into the if above and check that build_date_ is not null,
perhaps? At least early return if network_time is null.

[MAD, 2013/02/02 03:55:34]

Done.

+  if (network_time - build_date_ >
+      base::TimeDelta::FromDays(kOutdatedBuildAgeInDays)) {
+    upgrade_required_ = UPGRADE_REQUIRED_OUTDATED;
+    UpgradeDetected(false, is_unstable_channel_);
+  }
 }
 
-void UpgradeDetectorImpl::UpgradeDetected() {
+void UpgradeDetectorImpl::UpgradeDetected(bool critical,
+                                          bool is_unstable_channel) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  is_unstable_channel_ = is_unstable_channel;
+  if (critical)
+    upgrade_required_ = UPGRADE_REQUIRED_CRITICAL;
+  else if (upgrade_required_ == UPGRADE_REQUIRED_NONE)

[Finnur, 2013/02/01 17:09:15]

I'm sure that a few months down the line we're going to look at this and go...
"wait, why are we checking for NONE here"? Maybe you should document that you do
this because of the outdated codepath?

[MAD, 2013/02/02 03:55:34]

Refactoring took care of this.

+    upgrade_required_ = UPGRADE_REQUIRED_REGULAR;
 
   // Stop the recurring timer (that is checking for changes).
   detect_upgrade_timer_.Stop();
@@ -222,13 +344,14 @@ void UpgradeDetectorImpl::NotifyOnUpgrade() {
   bool is_testing = IsTesting();
   int64 time_passed = is_testing ? delta.InSeconds() : delta.InHours();
 
+  bool is_critical_or_outdated = upgrade_required_ > UPGRADE_REQUIRED_REGULAR;
   if (is_unstable_channel_) {
     // There's only one threat level for unstable channels like dev and
     // canary, and it hits after one hour. During testing, it hits after one
     // minute.
     const int kUnstableThreshold = 1;
 
-    if (is_critical_upgrade_)
+    if (is_critical_or_outdated)
       set_upgrade_notification_stage(UPGRADE_ANNOYANCE_CRITICAL);
     else if (time_passed >= kUnstableThreshold) {
       set_upgrade_notification_stage(UPGRADE_ANNOYANCE_LOW);
@@ -247,10 +370,10 @@ void UpgradeDetectorImpl::NotifyOnUpgrade() {
     const int kLowThreshold = 2 * kMultiplier;
 
     // These if statements must be sorted (highest interval first).
-    if (time_passed >= kSevereThreshold || is_critical_upgrade_) {
+    if (time_passed >= kSevereThreshold || is_critical_or_outdated) {
       set_upgrade_notification_stage(
-          is_critical_upgrade_ ? UPGRADE_ANNOYANCE_CRITICAL :
-                                 UPGRADE_ANNOYANCE_SEVERE);
+          is_critical_or_outdated ? UPGRADE_ANNOYANCE_CRITICAL :
+                                    UPGRADE_ANNOYANCE_SEVERE);
 
       // We can't get any higher, baby.
       upgrade_notification_timer_.Stop();