Reland "Decouple Scheme Registration from ContentMain"

Reason for revert: Broke Linux MSan Tests. See https://ci.chromium.org/p/chromium/builders/ci/Linux%20MSan%20Tests/21213 for first failing build.
Reland fixes uninitialized variable in shell/common/shell_content_client.h

Original change's description:
> Decouple Scheme Registration from ContentMain
>
> Split off of
> https://chromium-review.googlesource.com/c/chromium/src/+/1901591, see
> that CL (and the bug) for additional context.
>
> The followup to this CL locks the SchemeRegistry on first use to ensure
> schemes aren't added after GURL is used (which could lead to bugs). This
> means that Scheme initialization must happen before ContentMain in order
> for Java to use GURL before running ContentMain (I set the ContentClient
> in JNI_Onload, and register schemes after the CommandLine is initialized
> in content library_loader_hooks).
>
> Because of that, ContentMainDelegate had to have a ContentClient
> available earlier, so I added an API to ContentMainDelegate for the
> embedder to create their ContentClient on-demand.
>
> Because I wanted to avoid registering schemes multiple times,
> url_schemes.cc had to be made stateful and remember whether schemes had
> already been registered (Schemes could be registered by ContentMain
> after JNI_Onload already registered them for Android).
>
> Doc: https://docs.google.com/document/d/1kDKqBaq-b6EbUm0F4ea7ARoksUcj1KUx3qxFuSXEwM4/edit
>
> Bug: 783819
> Change-Id: I0f6884b2def87dcca77e722cd3d3800017c18749
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1945926
> Reviewed-by: John Abd-El-Malek <jam@chromium.org>
> Reviewed-by: Albert J. Wong <ajwong@chromium.org>
> Commit-Queue: Michael Thiessen <mthiesse@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#725799}

TBR=ajwong@chromium.org,mthiesse@chromium.org,jam@chromium.org

Change-Id: I27a2a49dc23610350ec9f3f96ed9824e2eeed0d5
Bug: 783819
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1973198
Commit-Queue: Michael Thiessen <mthiesse@chromium.org>
Reviewed-by: Michael Thiessen <mthiesse@chromium.org>
Cr-Commit-Position: refs/heads/master@{#725981}

android_webview/lib/aw_main_delegate.cc

@@ -75,8 +75,6 @@ AwMainDelegate::AwMainDelegate() = default;
 AwMainDelegate::~AwMainDelegate() = default;
 
 bool AwMainDelegate::BasicStartupComplete(int* exit_code) {
-  content::SetContentClient(&content_client_);
-
   base::CommandLine* cl = base::CommandLine::ForCurrentProcess();
 
   // WebView uses the Android system's scrollbars and overscroll glow.
@@ -349,6 +347,10 @@ void AwMainDelegate::PostFieldTrialInitialization() {
 #endif
 }
 
+content::ContentClient* AwMainDelegate::CreateContentClient() {
+  return &content_client_;
+}
+
 content::ContentBrowserClient* AwMainDelegate::CreateContentBrowserClient() {
   DCHECK(!aw_feature_list_creator_);
   aw_feature_list_creator_ = std::make_unique<AwFeatureListCreator>();

android_webview/lib/aw_main_delegate.h

@@ -47,6 +47,7 @@ class AwMainDelegate : public content::ContentMainDelegate {
   bool ShouldCreateFeatureList() override;
   void PostEarlyInitialization(bool is_running_tests) override;
   void PostFieldTrialInitialization() override;
+  content::ContentClient* CreateContentClient() override;
   content::ContentBrowserClient* CreateContentBrowserClient() override;
   content::ContentGpuClient* CreateContentGpuClient() override;
   content::ContentRendererClient* CreateContentRendererClient() override;

ash/shell/content/client/shell_main_delegate.cc

@@ -21,16 +21,15 @@ ShellMainDelegate::ShellMainDelegate() = default;
 
 ShellMainDelegate::~ShellMainDelegate() = default;
 
-bool ShellMainDelegate::BasicStartupComplete(int* exit_code) {
-  content::SetContentClient(&content_client_);
-  return false;
-}
-
 void ShellMainDelegate::PreSandboxStartup() {
   InitializeResourceBundle();
   ui::InitializeInputMethodForTesting();
 }
 
+content::ContentClient* ShellMainDelegate::CreateContentClient() {
+  return &content_client_;
+}
+
 content::ContentBrowserClient* ShellMainDelegate::CreateContentBrowserClient() {
   browser_client_.reset(new ShellContentBrowserClient);
   return browser_client_.get();

ash/shell/content/client/shell_main_delegate.h

@@ -22,8 +22,8 @@ class ShellMainDelegate : public content::ContentMainDelegate {
   ShellMainDelegate();
   ~ShellMainDelegate() override;
 
-  bool BasicStartupComplete(int* exit_code) override;
   void PreSandboxStartup() override;
+  content::ContentClient* CreateContentClient() override;
   content::ContentBrowserClient* CreateContentBrowserClient() override;
 
  private:

chrome/app/chrome_main_delegate.cc

@@ -810,8 +810,6 @@ bool ChromeMainDelegate::BasicStartupComplete(int* exit_code) {
   }
 #endif
 
-  content::SetContentClient(&chrome_content_client_);
-
   // The TLS slot used by the memlog allocator shim needs to be initialized
   // early to ensure that it gets assigned a low slot number. If it gets
   // initialized too late, the glibc TLS system will require a malloc call in
@@ -1191,6 +1189,10 @@ void ChromeMainDelegate::ZygoteForked() {
 
 #endif  // defined(OS_LINUX)
 
+content::ContentClient* ChromeMainDelegate::CreateContentClient() {
+  return &chrome_content_client_;
+}
+
 content::ContentBrowserClient*
 ChromeMainDelegate::CreateContentBrowserClient() {
 #if defined(CHROME_MULTIPLE_DLL_CHILD)

chrome/app/chrome_main_delegate.h

@@ -69,6 +69,7 @@ class ChromeMainDelegate : public content::ContentMainDelegate {
 #endif  // !defined(CHROME_MULTIPLE_DLL_CHILD)
   void PostFieldTrialInitialization() override;
 
+  content::ContentClient* CreateContentClient() override;
   content::ContentBrowserClient* CreateContentBrowserClient() override;
   content::ContentGpuClient* CreateContentGpuClient() override;
   content::ContentRendererClient* CreateContentRendererClient() override;

chromecast/app/cast_main_delegate.cc

@@ -131,8 +131,6 @@ bool CastMainDelegate::BasicStartupComplete(int* exit_code) {
     }
   }
 #endif  // defined(OS_ANDROID)
-
-  content::SetContentClient(&content_client_);
   return false;
 }
 
@@ -282,6 +280,10 @@ void CastMainDelegate::InitializeResourceBundle() {
 #endif  // defined(OS_ANDROID)
 }
 
+content::ContentClient* CastMainDelegate::CreateContentClient() {
+  return &content_client_;
+}
+
 content::ContentBrowserClient* CastMainDelegate::CreateContentBrowserClient() {
   DCHECK(!cast_feature_list_creator_);
   cast_feature_list_creator_ = std::make_unique<CastFeatureListCreator>();

chromecast/app/cast_main_delegate.h

@@ -48,6 +48,7 @@ class CastMainDelegate : public content::ContentMainDelegate {
 #endif  // defined(OS_LINUX)
   bool ShouldCreateFeatureList() override;
   void PostEarlyInitialization(bool is_running_tests) override;
+  content::ContentClient* CreateContentClient() override;
   content::ContentBrowserClient* CreateContentBrowserClient() override;
   content::ContentGpuClient* CreateContentGpuClient() override;
   content::ContentRendererClient* CreateContentRendererClient() override;

content/app/android/content_main.cc

@@ -10,6 +10,7 @@
 #include "content/public/android/content_jni_headers/ContentMain_jni.h"
 #include "content/public/app/content_main.h"
 #include "content/public/app/content_main_delegate.h"
+#include "content/public/common/content_client.h"
 #include "services/service_manager/embedder/main.h"
 
 using base::LazyInstance;
@@ -27,6 +28,15 @@ LazyInstance<std::unique_ptr<ContentMainDelegate>>::DestructorAtExit
 
 }  // namespace
 
+class ContentClientCreator {
+ public:
+  static void Create(ContentMainDelegate* delegate) {
+    ContentClient* client = delegate->CreateContentClient();
+    DCHECK(client);
+    SetContentClient(client);
+  }
+};
+
 // TODO(qinmin/hanxi): split this function into 2 separate methods: One to
 // start the ServiceManager and one to start the remainder of the browser
 // process. The first method should always be called upon browser start, and
@@ -55,6 +65,10 @@ static jint JNI_ContentMain_Start(JNIEnv* env,
 void SetContentMainDelegate(ContentMainDelegate* delegate) {
   DCHECK(!g_content_main_delegate.Get().get());
   g_content_main_delegate.Get().reset(delegate);
+  // The ContentClient needs to be set early so that it can be used by the
+  // content library loader hooks.
+  if (!GetContentClient())
+    ContentClientCreator::Create(delegate);
 }
 
 ContentMainDelegate* GetContentMainDelegateForTesting() {

content/app/android/library_loader_hooks.cc

@@ -8,6 +8,7 @@
 #include "base/logging.h"
 #include "base/trace_event/trace_event.h"
 #include "content/common/content_constants_internal.h"
+#include "content/common/url_schemes.h"
 #include "services/tracing/public/cpp/trace_startup.h"
 
 namespace content {
@@ -54,6 +55,10 @@ bool LibraryLoaded(JNIEnv* env,
             << ", default verbosity = " << logging::GetVlogVerbosity();
   }
 
+  // Content Schemes need to be registered as early as possible after the
+  // CommandLine has been initialized to allow java and tests to use GURL before
+  // running ContentMain.
+  RegisterContentSchemes();
   return true;
 }
 

content/app/content_main_runner_impl.cc

@@ -399,6 +399,15 @@ void PreSandboxInit() {
 
 }  // namespace
 
+class ContentClientCreator {
+ public:
+  static void Create(ContentMainDelegate* delegate) {
+    ContentClient* client = delegate->CreateContentClient();
+    DCHECK(client);
+    SetContentClient(client);
+  }
+};
+
 class ContentClientInitializer {
  public:
   static void Set(const std::string& process_type,
@@ -643,6 +652,8 @@ int ContentMainRunnerImpl::Initialize(const ContentMainParams& params) {
 #endif  // !OS_ANDROID
 
   int exit_code = 0;
+  if (!GetContentClient())
+    ContentClientCreator::Create(delegate_);
   if (delegate_->BasicStartupComplete(&exit_code))
     return exit_code;
   completed_basic_startup_ = true;
@@ -662,8 +673,7 @@ int ContentMainRunnerImpl::Initialize(const ContentMainParams& params) {
   }
 #endif
 
-  if (!GetContentClient())
-    SetContentClient(&empty_content_client_);
+  RegisterContentSchemes();
   ContentClientInitializer::Set(process_type, delegate_);
 
 #if !defined(OS_ANDROID)
@@ -724,7 +734,6 @@ int ContentMainRunnerImpl::Initialize(const ContentMainParams& params) {
 #endif
 
     RegisterPathProvider();
-    RegisterContentSchemes(delegate_->ShouldLockSchemeRegistry());
 
 #if defined(OS_ANDROID) && (ICU_UTIL_DATA_IMPL == ICU_UTIL_DATA_FILE)
     // On Android, we have two ICU data files. A main one with most languages

content/app/content_main_runner_impl.h

@@ -14,7 +14,6 @@
 #include "content/browser/startup_data_impl.h"
 #include "content/public/app/content_main.h"
 #include "content/public/app/content_main_runner.h"
-#include "content/public/common/content_client.h"
 #include "content/public/common/main_function_params.h"
 #include "mojo/core/embedder/scoped_ipc_support.h"
 
@@ -73,9 +72,6 @@ class ContentMainRunnerImpl : public ContentMainRunner {
   // True if basic startup was completed.
   bool completed_basic_startup_ = false;
 
-  // Used if the embedder doesn't set one.
-  ContentClient empty_content_client_;
-
   // The delegate will outlive this object.
   ContentMainDelegate* delegate_ = nullptr;
 

content/common/url_schemes.cc

@@ -19,6 +19,8 @@
 namespace content {
 namespace {
 
+bool g_registered_url_schemes = false;
+
 const char* const kDefaultSavableSchemes[] = {
   url::kHttpScheme,
   url::kHttpsScheme,
@@ -47,7 +49,11 @@ std::vector<std::string>& GetMutableServiceWorkerSchemes() {
 
 }  // namespace
 
-void RegisterContentSchemes(bool lock_schemes) {
+void RegisterContentSchemes() {
+  // On Android, schemes may have been registered already.
+  if (g_registered_url_schemes)
+    return;
+  g_registered_url_schemes = true;
   ContentClient::Schemes schemes;
   GetContentClient()->AddAdditionalSchemes(&schemes);
 
@@ -91,14 +97,6 @@ void RegisterContentSchemes(bool lock_schemes) {
     url::EnableNonStandardSchemesForAndroidWebView();
 #endif
 
-  // Prevent future modification of the scheme lists. This is to prevent
-  // accidental creation of data races in the program. Add*Scheme aren't
-  // threadsafe so must be called when GURL isn't used on any other thread. This
-  // is really easy to mess up, so we say that all calls to Add*Scheme in Chrome
-  // must be inside this function.
-  if (lock_schemes)
-    url::LockSchemeRegistries();
-
   // Combine the default savable schemes with the additional ones given.
   GetMutableSavableSchemes().assign(std::begin(kDefaultSavableSchemes),
                                     std::end(kDefaultSavableSchemes));
@@ -109,6 +107,11 @@ void RegisterContentSchemes(bool lock_schemes) {
   GetMutableServiceWorkerSchemes() = std::move(schemes.service_worker_schemes);
 }
 
+void ReRegisterContentSchemesForTests() {
+  g_registered_url_schemes = false;
+  RegisterContentSchemes();
+}
+
 const std::vector<std::string>& GetSavableSchemes() {
   return GetMutableSavableSchemes();
 }

content/common/url_schemes.h

@@ -12,17 +12,13 @@
 
 namespace content {
 
-// Note: ContentMainRunner calls this method internally as part of main
-// initialization, so this function generally should not be called by
-// embedders. It's exported to facilitate test harnesses that do not
-// utilize ContentMainRunner and that do not wish to lock the set
-// of standard schemes at init time.
-//
 // Called near the beginning of startup to register URL schemes that should be
-// parsed as "standard" or "referrer" with the src/url/ library. Optionally, the
-// sets of schemes are locked down. The embedder can add additional schemes by
-// overriding the ContentClient::AddAdditionalSchemes method.
-CONTENT_EXPORT void RegisterContentSchemes(bool lock_schemes);
+// parsed as "standard" or "referrer" with the src/url/ library. The embedder
+// can add additional schemes by overriding the AddAdditionalSchemes method.
+CONTENT_EXPORT void RegisterContentSchemes();
+
+// Re-initializes schemes for tests.
+CONTENT_EXPORT void ReRegisterContentSchemesForTests();
 
 // See comment in ContentClient::AddAdditionalSchemes for explanations. These
 // getters can be invoked on any thread.

content/public/app/content_main_delegate.cc

@@ -6,6 +6,7 @@
 
 #include "base/logging.h"
 #include "build/build_config.h"
+#include "content/public/common/content_client.h"
 #include "content/public/gpu/content_gpu_client.h"
 #include "content/public/renderer/content_renderer_client.h"
 #include "content/public/utility/content_utility_client.h"
@@ -51,10 +52,6 @@ int ContentMainDelegate::TerminateForFatalInitializationError() {
   return 0;
 }
 
-bool ContentMainDelegate::ShouldLockSchemeRegistry() {
-  return true;
-}
-
 service_manager::ProcessType ContentMainDelegate::OverrideProcessType() {
   return service_manager::ProcessType::kDefault;
 }
@@ -71,6 +68,10 @@ bool ContentMainDelegate::ShouldCreateFeatureList() {
   return true;
 }
 
+ContentClient* ContentMainDelegate::CreateContentClient() {
+  return new ContentClient();
+}
+
 ContentBrowserClient* ContentMainDelegate::CreateContentBrowserClient() {
 #if defined(CHROME_MULTIPLE_DLL_CHILD)
   return NULL;

content/public/app/content_main_delegate.h

@@ -27,6 +27,7 @@ class ZygoteForkDelegate;
 namespace content {
 
 class ContentBrowserClient;
+class ContentClient;
 class ContentGpuClient;
 class ContentRendererClient;
 class ContentUtilityClient;
@@ -83,20 +84,6 @@ class CONTENT_EXPORT ContentMainDelegate {
   virtual void ZygoteForked() {}
 #endif  // defined(OS_LINUX)
 
-  // Allows the embedder to prevent locking the scheme registry. The scheme
-  // registry is the list of URL schemes we recognize, with some additional
-  // information about each scheme such as whether it expects a host. The
-  // scheme registry is not thread-safe, so by default it is locked before any
-  // threads are created to ensure single-threaded access. An embedder can
-  // override this to prevent the scheme registry from being locked during
-  // startup, but if they do so then they are responsible for making sure that
-  // the registry is only accessed in a thread-safe way, and for calling
-  // url::LockSchemeRegistries() when initialization is complete. If possible,
-  // prefer registering additional schemes through
-  // ContentClient::AddAdditionalSchemes over preventing the scheme registry
-  // from being locked.
-  virtual bool ShouldLockSchemeRegistry();
-
   // Fatal errors during initialization are reported by this function, so that
   // the embedder can implement graceful exit by displaying some message and
   // returning initialization error code. Default behavior is CHECK(false).
@@ -150,12 +137,14 @@ class CONTENT_EXPORT ContentMainDelegate {
   virtual void PostEarlyInitialization(bool is_running_tests) {}
 
  protected:
+  friend class ContentClientCreator;
   friend class ContentClientInitializer;
   friend class BrowserTestBase;
 
   // Called once per relevant process type to allow the embedder to customize
   // content. If an embedder wants the default (empty) implementation, don't
   // override this.
+  virtual ContentClient* CreateContentClient();
   virtual ContentBrowserClient* CreateContentBrowserClient();
   virtual ContentGpuClient* CreateContentGpuClient();
   virtual ContentRendererClient* CreateContentRendererClient();

content/public/test/browser_test_base.cc

@@ -408,7 +408,7 @@ void BrowserTestBase::SetUp() {
       SetRendererClientForTesting(delegate->CreateContentRendererClient());
 
     content::RegisterPathProvider();
-    content::RegisterContentSchemes(false);
+    content::RegisterContentSchemes();
     ui::RegisterPathProvider();
 
     delegate->PreSandboxStartup();

content/public/test/content_test_suite_base.cc

@@ -77,7 +77,7 @@ void ContentTestSuiteBase::Initialize() {
 void ContentTestSuiteBase::RegisterContentSchemes(
     ContentClient* content_client) {
   SetContentClient(content_client);
-  content::RegisterContentSchemes(false);
+  content::RegisterContentSchemes();
   SetContentClient(nullptr);
 }