Move out load of bundled assistant proto to the component updater

This CL is moving lifting out the loading from the resource bundle to
the corresponding 'ssl_error_assistant_component'.

The previous implementation was loading on-demand the bundled proto
when needed. But, when the 'ssl_error_assistant_component' is ready, it's
setting the received proto. As no SSL error occured, the bundled proto is
not loaded and must be loaded on the UI thread.

This is causing jank on the UI thread. Which can be seen on the bug 888555.

The current proposition is to keep the bundle loading on-demand since
error paths are not the common ones. But, for the
'ssl_error_assistant_component' path we proposed to also read the bundled
proto and choose the most recent one before sending it to the error
assistant on the UI thread.

That way, most of the job is done before getting on the UI thread.

NOTE: ReadErrorAssistantProtoFromResourceBundle is thread safe and can be
executed on any thread. It can also be executed twice.

R=fdoray@chromium.org, meacer@chromium.org
CC=carlosil@chromium.org, agl@chromium.org

Bug: 888555
Change-Id: Icd994f93c849b3c4d8b01d7921dff69f571397a8
Reviewed-on: https://chromium-review.googlesource.com/c/1247025
Commit-Queue: Etienne Bergeron <etienneb@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Reviewed-by: Sorin Jianu <sorin@chromium.org>
Reviewed-by: Mustafa Emre Acer <meacer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597109}

chrome/browser/component_updater/ssl_error_assistant_component_installer.cc

@@ -12,6 +12,7 @@
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
 #include "base/task/post_task.h"
+#include "chrome/browser/ssl/ssl_error_assistant.h"
 #include "chrome/browser/ssl/ssl_error_handler.h"
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/browser_thread.h"
@@ -46,6 +47,16 @@ void LoadProtoFromDisk(const base::FilePath& pb_path) {
     DVLOG(1) << "Failed parsing proto " << pb_path.value();
     return;
   }
+
+  // Retrieve the default proto from the resource bundle and keep the most
+  // recent version. This is required since the component updater may still have
+  // an older version.
+  std::unique_ptr<chrome_browser_ssl::SSLErrorAssistantConfig> default_proto =
+      SSLErrorAssistant::GetErrorAssistantProtoFromResourceBundle();
+  if (default_proto && default_proto->version_id() > proto->version_id()) {
+    proto = std::move(default_proto);
+  }
+
   base::CreateSingleThreadTaskRunnerWithTraits({content::BrowserThread::UI})
       ->PostTask(FROM_HERE,
                  base::BindOnce(&SSLErrorHandler::SetErrorAssistantProto,

chrome/browser/ssl/captive_portal_helper_android.cc

@@ -14,6 +14,7 @@
 #include "base/android/jni_string.h"
 #include "base/logging.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "chrome/browser/ssl/ssl_error_assistant.h"
 #include "chrome/browser/ssl/ssl_error_handler.h"
 #include "content/public/browser/browser_thread.h"
 #include "jni/CaptivePortalHelper_jni.h"
@@ -26,6 +27,13 @@ void JNI_CaptivePortalHelper_SetCaptivePortalCertificateForTesting(
     JNIEnv* env,
     const base::android::JavaParamRef<jclass>& jcaller,
     const base::android::JavaParamRef<jstring>& jhash) {
+  auto default_proto =
+      SSLErrorAssistant::GetErrorAssistantProtoFromResourceBundle();
+  base::PostTaskWithTraits(
+      FROM_HERE, {content::BrowserThread::UI},
+      base::BindOnce(SSLErrorHandler::SetErrorAssistantProto,
+                     std::move(default_proto)));
+
   const std::string hash = ConvertJavaStringToUTF8(env, jhash);
   auto config_proto =
       std::make_unique<chrome_browser_ssl::SSLErrorAssistantConfig>();

chrome/browser/ssl/ssl_error_assistant.cc

@@ -123,19 +123,6 @@ LoadDynamicInterstitialList(
   return dynamic_interstitial_list;
 }
 
-// Reads the SSL error assistant configuration from the resource bundle.
-std::unique_ptr<chrome_browser_ssl::SSLErrorAssistantConfig>
-ReadErrorAssistantProtoFromResourceBundle() {
-  auto proto = std::make_unique<chrome_browser_ssl::SSLErrorAssistantConfig>();
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  DCHECK(proto);
-  ui::ResourceBundle& bundle = ui::ResourceBundle::GetSharedInstance();
-  base::StringPiece data =
-      bundle.GetRawDataResource(IDR_SSL_ERROR_ASSISTANT_PB);
-  google::protobuf::io::ArrayInputStream stream(data.data(), data.size());
-  return proto->ParseFromZeroCopyStream(&stream) ? std::move(proto) : nullptr;
-}
-
 bool RegexMatchesAny(const std::vector<std::string>& organization_names,
                      const std::string& pattern) {
   const re2::RE2 regex(pattern);
@@ -203,9 +190,10 @@ SSLErrorAssistant::~SSLErrorAssistant() {}
 bool SSLErrorAssistant::IsKnownCaptivePortalCertificate(
     const net::SSLInfo& ssl_info) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  EnsureInitialized();
+  CHECK(error_assistant_proto_);
+
   if (!captive_portal_spki_hashes_) {
-    error_assistant_proto_ = ReadErrorAssistantProtoFromResourceBundle();
-    CHECK(error_assistant_proto_);
     captive_portal_spki_hashes_ =
         LoadCaptivePortalCertHashes(*error_assistant_proto_);
   }
@@ -219,10 +207,10 @@ SSLErrorAssistant::MatchDynamicInterstitial(const net::SSLInfo& ssl_info,
   // Load the dynamic interstitial data from SSL error assistant proto if it's
   // not already loaded.
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  if (!dynamic_interstitial_list_) {
-    if (!error_assistant_proto_)
-      error_assistant_proto_ = ReadErrorAssistantProtoFromResourceBundle();
+  EnsureInitialized();
+  CHECK(error_assistant_proto_);
 
+  if (!dynamic_interstitial_list_) {
     DCHECK(error_assistant_proto_);
     dynamic_interstitial_list_ =
         LoadDynamicInterstitialList(*error_assistant_proto_);
@@ -270,11 +258,12 @@ const std::string SSLErrorAssistant::MatchKnownMITMSoftware(
     return std::string();
   }
 
+  EnsureInitialized();
+  CHECK(error_assistant_proto_);
+
   // Load MITM software data from the SSL error assistant proto.
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   if (!mitm_software_list_) {
-    if (!error_assistant_proto_)
-      error_assistant_proto_ = ReadErrorAssistantProtoFromResourceBundle();
     DCHECK(error_assistant_proto_);
     mitm_software_list_ = LoadMITMSoftwareList(*error_assistant_proto_);
   }
@@ -322,21 +311,23 @@ const std::string SSLErrorAssistant::MatchKnownMITMSoftware(
   return std::string();
 }
 
+// static
+std::unique_ptr<chrome_browser_ssl::SSLErrorAssistantConfig>
+SSLErrorAssistant::GetErrorAssistantProtoFromResourceBundle() {
+  // Reads the SSL error assistant configuration from the resource bundle.
+  auto proto = std::make_unique<chrome_browser_ssl::SSLErrorAssistantConfig>();
+  DCHECK(proto);
+  ui::ResourceBundle& bundle = ui::ResourceBundle::GetSharedInstance();
+  base::StringPiece data =
+      bundle.GetRawDataResource(IDR_SSL_ERROR_ASSISTANT_PB);
+  google::protobuf::io::ArrayInputStream stream(data.data(), data.size());
+  return proto->ParseFromZeroCopyStream(&stream) ? std::move(proto) : nullptr;
+}
+
 void SSLErrorAssistant::SetErrorAssistantProto(
     std::unique_ptr<chrome_browser_ssl::SSLErrorAssistantConfig> proto) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   CHECK(proto);
-  if (!error_assistant_proto_) {
-    // If the user hasn't seen an SSL error and a component update is available,
-    // the local resource bundle won't have been read and error_assistant_proto_
-    // will be null. It's possible that the local resource bundle has a higher
-    // version_id than the component updater component, so load the local
-    // resource bundle once to compare versions.
-    // TODO(meacer): Ideally, ReadErrorAssistantProtoFromResourceBundle should
-    // only be called once and not on the UI thread. Move the call to the
-    // component updater component.
-    error_assistant_proto_ = ReadErrorAssistantProtoFromResourceBundle();
-  }
 
   // Ignore versions that are not new. INT_MAX is used by tests, so always allow
   // it.
@@ -356,6 +347,11 @@ void SSLErrorAssistant::SetErrorAssistantProto(
       LoadDynamicInterstitialList(*error_assistant_proto_);
 }
 
+void SSLErrorAssistant::EnsureInitialized() {
+  if (!error_assistant_proto_)
+    error_assistant_proto_ = GetErrorAssistantProtoFromResourceBundle();
+}
+
 void SSLErrorAssistant::ResetForTesting() {
   error_assistant_proto_.reset();
   mitm_software_list_.reset();

chrome/browser/ssl/ssl_error_assistant.h

@@ -88,11 +88,18 @@ class SSLErrorAssistant {
   void SetErrorAssistantProto(
       std::unique_ptr<chrome_browser_ssl::SSLErrorAssistantConfig> proto);
 
+  // Returns the SSL error assistant config stored in the resource bundle. This
+  // function is thread-safe and can safely be called multiple times.
+  static std::unique_ptr<chrome_browser_ssl::SSLErrorAssistantConfig>
+  GetErrorAssistantProtoFromResourceBundle();
+
   // Testing methods:
   void ResetForTesting();
   int GetErrorAssistantProtoVersionIdForTesting() const;
 
  private:
+  void EnsureInitialized();
+
   // SPKI hashes belonging to certs treated as captive portals. Null until the
   // first time ShouldDisplayCaptiveProtalInterstitial() or
   // SetErrorAssistantProto() is called.

chrome/browser/ssl/ssl_error_handler.cc

@@ -233,6 +233,7 @@ class ConfigSingleton {
   void SetErrorAssistantProto(
       std::unique_ptr<chrome_browser_ssl::SSLErrorAssistantConfig>
           error_assistant_proto);
+
   void SetEnterpriseManagedForTesting(bool enterprise_managed);
   bool IsEnterpriseManagedFlagSetForTesting() const;
   int GetErrorAssistantProtoVersionIdForTesting() const;
@@ -726,6 +727,7 @@ bool SSLErrorHandler::IsTimerRunningForTesting() const {
   return timer_.IsRunning();
 }
 
+// static
 void SSLErrorHandler::SetErrorAssistantProto(
     std::unique_ptr<chrome_browser_ssl::SSLErrorAssistantConfig> config_proto) {
   g_config.Pointer()->SetErrorAssistantProto(std::move(config_proto));

chrome/browser/ssl/ssl_error_handler_unittest.cc

@@ -20,6 +20,7 @@
 #include "chrome/browser/captive_portal/captive_portal_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ssl/common_name_mismatch_handler.h"
+#include "chrome/browser/ssl/ssl_error_assistant.h"
 #include "chrome/browser/ssl/ssl_error_assistant.pb.h"
 #include "chrome/common/buildflags.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
@@ -368,6 +369,9 @@ class SSLErrorAssistantProtoTest : public ChromeRenderViewHostTestHarness {
   void SetUp() override {
     ChromeRenderViewHostTestHarness::SetUp();
     SSLErrorHandler::ResetConfigForTesting();
+    SSLErrorHandler::SetErrorAssistantProto(
+        SSLErrorAssistant::GetErrorAssistantProtoFromResourceBundle());
+
     SSLErrorHandler::SetInterstitialDelayForTesting(base::TimeDelta());
     ResetErrorHandlerFromFile(kOkayCertName,
                               net::CERT_STATUS_COMMON_NAME_INVALID);