STH Set: Delay loading until after start-up.

chrome/browser/component_updater/sth_set_component_installer.cc

 // Copyright 2016 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/component_updater/sth_set_component_installer.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/files/file_enumerator.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/logging.h"
 #include "base/path_service.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/values.h"
 #include "base/version.h"
 #include "chrome/browser/net/sth_distributor_provider.h"
+#include "chrome/common/chrome_features.h"
 #include "components/component_updater/component_updater_paths.h"
 #include "components/safe_json/safe_json_parser.h"
+#include "components/variations/variations_associated_data.h"
 #include "content/public/browser/browser_thread.h"
 #include "crypto/sha2.h"
 #include "net/cert/ct_log_response_parser.h"
 #include "net/cert/signed_tree_head.h"
 #include "net/cert/sth_distributor.h"
 #include "net/cert/sth_observer.h"
 
 using component_updater::ComponentUpdateService;
 
 namespace {
@@ skipping 36 matching lines @@
 bool STHSetComponentInstallerTraits::OnCustomInstall(
     const base::DictionaryValue& manifest,
     const base::FilePath& install_dir) {
   return true;  // Nothing custom here.
 }
 
 void STHSetComponentInstallerTraits::ComponentReady(
     const base::Version& version,
     const base::FilePath& install_dir,
     std::unique_ptr<base::DictionaryValue> manifest) {
-  if (!content::BrowserThread::PostBlockingPoolTask(
-          FROM_HERE,
-          base::Bind(&STHSetComponentInstallerTraits::LoadSTHsFromDisk,
-                     base::Unretained(this), GetInstalledPath(install_dir),
-                     version))) {
-    NOTREACHED();
+  if (variations::GetVariationParamValueByFeature(features::kSTHSetComponent,
+                                                  "delayed_load") == "yes") {

[gab, 2016/07/12 20:09:25]

Flip this condition around (i.e. make the default on trunk in the absence of a
value for the feature to be a delayed load), this will make it run on the
waterfall (and hence a cleaner merge to Stable when we confirm it works).

[Eran Messeri, 2016/07/13 10:51:55]

Done.

+    content::BrowserThread::PostAfterStartupTask(

[gab, 2016/07/12 20:09:24]

Actually, IIUC from https://codereview.chromium.org/2140093002/#msg10 this is
*really* not critical (i.e. it could even be delayed much further after
startup?) Can we make it a 5 or even 10 minutes delay instead?

PostAfterStartupTask is meant for non-startup critical work which should still
run very shortly after startup (i.e. earlier than a fixed delay on fast machines
but later on slow machines).

PS: In the upcoming TaskScheduler world this will be a perfect candidate for
BACKGROUND work, but until then we have to play with hackish delays...

[Eran Messeri, 2016/07/13 10:51:55]

Yes, loading the STHs can be delayed by 5-10 minutes.

[gab, 2016/07/13 17:29:08]

Ok then please use:

// STHs perform an async sanity verification, it is fine to delay them much
after startup.
constexpr base::TimeDelta sth_load_delay = base::TimeDelta::FromMinutes(10);
content::BrowserThread::GetBlockingPool()->PostDelayedTask(..., sth_load_delay);

[gab, 2016/07/14 14:46:03]

ping (shall we do a mega delay? that won't result in a better metric but
potential jank right after recording first paint is not great either if really
not needed right away)

[Eran Messeri, 2016/07/18 09:36:06]

To follow up, since we came to the conclusion we can parse the JSON simply using
the JSONReader rather than the SafeJsonParser, it seems unnecessary to do such a
long delay. 
There's now another variation parameter in place that'll allow us to experiment
both types of parsing methods, so if post-startup read using the JSONReader is
still too slow we can re-visit the option of a long delay.

+        FROM_HERE, content::BrowserThread::GetBlockingPool(),
+        base::Bind(&STHSetComponentInstallerTraits::LoadSTHsFromDisk,
+                   base::Unretained(this), GetInstalledPath(install_dir),

[Alexei Svitkine (slow), 2016/07/12 19:59:27]

As an aside, why is this used base::Unretained()? Is there a guarantee that this
class won't be destroyed (e.g. shutting down while it's fetching the component)
before the task runs? If there's no obvious such guarantee, please add a
WeakPointerFactory as a a member and pass a weak pointer.

(If it is guaranteed, add a comment.)

[Eran Messeri, 2016/07/13 10:51:55]

There are no guarantees, so I've switched to a WeakPointerFactory.

+                   version));
+  } else {
+    if (!content::BrowserThread::PostBlockingPoolTask(
+            FROM_HERE,
+            base::Bind(&STHSetComponentInstallerTraits::LoadSTHsFromDisk,

[gab, 2016/07/12 20:09:24]

Extract:

const base::Closure load_sths_closure = base::Bind(...);

outside of the conditionals and re-use |load_sths_closure| in both places.

[Eran Messeri, 2016/07/13 10:51:55]

Done.

+                       base::Unretained(this), GetInstalledPath(install_dir),
+                       version))) {
+      NOTREACHED();

[gab, 2016/07/12 20:09:24]

This NOTREACHED() is not necessary. PostTask() will never fail unless during
late shutdown in which case we don't care to load STHs I assume.

(callers of PostTask() pretty much never check the return value unless it is
critical to run before shutdown -- and even then the API states that "true"
means that it "may run"...)

[Eran Messeri, 2016/07/13 10:51:55]

Thanks for the explanation, removed. Does it make sense to change the API? the
if (...PostTask()) { NOTREACHED(); } pattern is something I saw scattered around
 the code quite a few times.

[gab, 2016/07/13 17:29:08]

There are far more places that don't have NOTREACHED() then places that do.

TBH, I would like for the API to just return void but it's *way* too spread for
this by now.

It's not harmful to NOTREACHED(), but it's not really ever correct either
(except in a few low-level corner cases which it's not worth going into here --
but are the reason why this can't be trivially turned into "void").

+    }
   }
 }
 
 // Called during startup and installation before ComponentReady().
 bool STHSetComponentInstallerTraits::VerifyInstallation(
     const base::DictionaryValue& manifest,
     const base::FilePath& install_dir) const {
   return base::PathExists(GetInstalledPath(install_dir));
 }
 
@@ skipping 98 matching lines @@
 
   std::unique_ptr<ComponentInstallerTraits> traits(
       new STHSetComponentInstallerTraits(distributor));
   // |cus| will take ownership of |installer| during installer->Register(cus).
   DefaultComponentInstaller* installer =
       new DefaultComponentInstaller(std::move(traits));
   installer->Register(cus, base::Closure());
 }
 
 }  // namespace component_updater