Don't disable extensions immediately if verification is out of date

chrome/browser/extensions/extension_service.cc

Index: chrome/browser/extensions/extension_service.cc
diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc
index db32e5a39c86f6bd2326edeb6d1e80bef818cc13..abb39595348c5bf0a232b910be32ae458656efe0 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -574,10 +574,7 @@ void ExtensionService::Init() {
     // rather than running immediately at startup.
     CheckForExternalUpdates();
 
-    InstallVerifier* verifier =
-        extensions::ExtensionSystem::Get(profile_)->install_verifier();
-    if (verifier->NeedsBootstrap())
-      VerifyAllExtensions(true);  // bootstrap=true.
+    MaybeBootstrapVerifier();
     base::MessageLoop::current()->PostDelayedTask(
         FROM_HERE,
         base::Bind(&ExtensionService::GarbageCollectExtensions, AsWeakPtr()),
@@ -611,6 +608,32 @@ void ExtensionService::LoadGreylistFromPrefs() {
   }
 }
 
+void ExtensionService::MaybeBootstrapVerifier() {
+  InstallVerifier* verifier =
+      extensions::ExtensionSystem::Get(profile_)->install_verifier();
+  bool do_bootstrap = false;
+
+  if (verifier->NeedsBootstrap()) {
+    do_bootstrap = true;
+  } else {
+    // If any of the installed extensions have an install time newer than the
+    // signature's timestamp, we need to bootstrap because our signature may
+    // be missing valid extensions.
+    base::Time timestamp = verifier->signature_timestamp();
+    extensions::ExtensionSet::const_iterator i;

[Finnur, 2014/02/12 13:54:57]

nit: Like you, I have a tendency to move large declarations of variables out of
the for loop for readability reasons but I've gotten pushback on that from
reviewers because then you are changing the scope of things, in this case |i|,
to live outside the scope of the for loop. I don't have a strong opinion, I'll
leave it up to you.

[asargent_no_longer_on_chrome, 2014/02/12 16:27:56]

Ah, that's actually a good argument - fixed it.

+    for (i = extensions()->begin(); i != extensions()->end(); ++i) {
+      const Extension& extension = **i;
+      if (verifier->NeedsVerification(extension) &&
+          extension_prefs_->GetInstallTime(extension.id()) >= timestamp) {

[Finnur, 2014/02/12 13:54:57]

You seem to have thought about this problem elsewhere in the code, but just to
be sure: Is this code block robust enough to handle when the system clock
changes manually? For example, if I set my system clock backward and/or
forwards, will I end up in a state where I always/never bootstrap, depending on
what time I've set? (always being better than never, I suppose).

[asargent_no_longer_on_chrome, 2014/02/12 16:27:56]

We have code elsewhere that forces a request for a new signature whenever new
extensions are installed/uninstalled, so you're right in your comment elsewhere
that this code just makes sure during the rollout period that we don't
accidentally disable things for users who have an outdated signature as
discussed in the bug and at the meeting. Eventually we can move to assuming
there's always a valid signature and get rid of this. 

+        do_bootstrap = true;
+        break;
+      }
+    }
+  }
+  if (do_bootstrap)
+    VerifyAllExtensions(true);  // bootstrap=true.

[Finnur, 2014/02/12 13:54:57]

Refresh my memory... Bootstrap means: Fetch the signatures again and verify, not
just fetch signatures and verify on next boot, right?

[asargent_no_longer_on_chrome, 2014/02/12 16:27:56]

Right - inside the callback function  FinishVerifyAllExtensions, if we
successfully got a result back from the server, down at line 683 we proceed to
call CheckManagementPolicy which, because the signature's timestamp will now be
updated, should properly enforce the results we got back from the server and
this time *not* skip any extensions based on their install time inside
InstallVerifier::MustRemainDisabled. 

[Finnur, 2014/02/12 16:56:26]

Good. I was worried that someone could force Chrome to be always in bootstrap
mode and thereby avoiding the enforcement. Seems like you got that covered.

+}
+
 void ExtensionService::VerifyAllExtensions(bool bootstrap) {
   ExtensionIdSet to_add;
   scoped_ptr<ExtensionSet> all_extensions = GenerateInstalledExtensionsSet();