From 51f42f9fda381f4801ada8b10524c00ed703481b Mon Sep 17 00:00:00 2001 From: Mario Sanchez Prada Date: Wed, 16 Feb 2022 13:32:41 +0100 Subject: [PATCH] Ensure upstream browser tests run with the expected set of features BraveMainDelegate::BasicStartupComplete() is where several features are enabled and disabled for Brave via command line switches, but that method is not being called when running usptream tests, which use ChromeMainDelegate instead. Because of this, all browser tests crash on DCHECK-enabled builds with a backtrace like the following one: [497557:497557:1221/135319.265636:FATAL:distiller_javascript_utils.cc(41)] Check failed: distiller_javascript_world_id != invalid_world_id. #0 0x7fd8cc5b7459 base::debug::CollectStackTrace() #1 0x7fd8cc4b12d3 base::debug::StackTrace::StackTrace() #2 0x7fd8cc4d3a34 logging::LogMessage::~LogMessage() #3 0x7fd8cc4d44ae logging::LogMessage::~LogMessage() #4 0x56550ee1de70 dom_distiller::RunIsolatedJavaScript() #5 0x56550fffc990 brave_ads::AdsTabHelper::RunIsolatedJavaScript() #6 0x7fd8c6332b3d content::WebContentsImpl::WebContentsObserverList::NotifyObservers<>() #7 0x7fd8c63531a9 content::WebContentsImpl::DocumentOnLoadCompleted() [...] #56 0x56550ccba27a _start Task trace: #0 0x7fd8c6c6ebee IPC::(anonymous namespace)::ChannelAssociatedGroupController::Accept() #1 0x7fd8cbce69df mojo::SimpleWatcher::Context::Notify() Crash keys: "ui_scheduler_async_stack" = "0x7FD8C6C6EBEE 0x7FD8CBCE69DF" "io_scheduler_async_stack" = "0x7FD8CBCE69DF 0x0" [1/1] TabStripBrowsertest.ShiftGroupLeft_OtherGroup (CRASHED) This is because the DOM Distiller feature is not enabled as expected (for Brave) on startup, and when AdsTabHelper::RunIsolatedJavaScript() (called via the WebContentsObserver machinery) calls dom_distiller::RunIsolatedJavaScript() it will fail the DCHECK that makes sure the JS World ID is already set. To fix this, we move the code of BraveMainDelegate::BasicStartupComplete() to ChromeMainDelegate::BasicStartupComplete() via a chromium_src override, so that we make sure that exactly the same initialization done for Brave and Brave-related browser tests also applies when running upstrem browser tests. This does not only fix that crash, but also makes sure that upstream browser tests are run with the same features as Brave, which is the right thing to do. --- app/brave_main_delegate.cc | 95 --------------- app/brave_main_delegate.h | 7 -- chromium_src/chrome/app/DEPS | 8 ++ .../chrome/app/chrome_main_delegate.cc | 109 +++++++++++++++++- .../chrome/app/chrome_main_delegate.h | 8 ++ 5 files changed, 124 insertions(+), 103 deletions(-) diff --git a/app/brave_main_delegate.cc b/app/brave_main_delegate.cc index ce8833e096ba..3445e8634feb 100644 --- a/app/brave_main_delegate.cc +++ b/app/brave_main_delegate.cc @@ -36,14 +36,6 @@ #include "content/public/common/content_switches.h" #include "google_apis/gaia/gaia_switches.h" -#if BUILDFLAG(IS_ANDROID) -#include "base/android/jni_android.h" -#include "brave/build/android/jni_headers/BraveQAPreferences_jni.h" -#include "components/signin/public/base/account_consistency_method.h" -#else -#include "chrome/browser/ui/profile_picker.h" -#endif - namespace { const char kBraveOriginTrialsPublicKey[] = @@ -152,90 +144,3 @@ void BraveMainDelegate::PreSandboxStartup() { brave::InitializeResourceBundle(); } } - -bool BraveMainDelegate::BasicStartupComplete(int* exit_code) { - BraveCommandLineHelper command_line(base::CommandLine::ForCurrentProcess()); - command_line.AppendSwitch(switches::kDisableClientSidePhishingDetection); - command_line.AppendSwitch(switches::kDisableDomainReliability); - command_line.AppendSwitch(switches::kEnableDomDistiller); - command_line.AppendSwitch(switches::kNoPings); - - std::string update_url = GetUpdateURLHost(); - if (!update_url.empty()) { - std::string source = "url-source=" + update_url; - command_line.AppendSwitchASCII(switches::kComponentUpdater, source.c_str()); - } - - if (!base::CommandLine::ForCurrentProcess()->HasSwitch( - embedder_support::kOriginTrialPublicKey)) { - command_line.AppendSwitchASCII(embedder_support::kOriginTrialPublicKey, - kBraveOriginTrialsPublicKey); - } - - std::string brave_sync_service_url = BRAVE_SYNC_ENDPOINT; -#if BUILDFLAG(IS_ANDROID) - AdjustSyncServiceUrlForAndroid(&brave_sync_service_url); -#endif // BUILDFLAG(IS_ANDROID)) - - // Brave's sync protocol does not use the sync service url - command_line.AppendSwitchASCII(syncer::kSyncServiceURL, - brave_sync_service_url.c_str()); - - command_line.AppendSwitchASCII(switches::kLsoUrl, kDummyUrl); - - // Brave variations - const std::string kVariationsServerURL = BRAVE_VARIATIONS_SERVER_URL; - command_line.AppendSwitchASCII(variations::switches::kVariationsServerURL, - kVariationsServerURL.c_str()); - // Insecure fall-back for variations is set to the same (secure) URL. This is - // done so that if VariationsService tries to fall back to insecure url the - // check for kHttpScheme in VariationsService::MaybeRetryOverHTTP would - // prevent it from doing so as we don't want to use an insecure fall-back. - const std::string kVariationsInsecureServerURL = BRAVE_VARIATIONS_SERVER_URL; - command_line.AppendSwitchASCII( - variations::switches::kVariationsInsecureServerURL, - kVariationsInsecureServerURL.c_str()); - - // Runtime-enabled features. To override Chromium features default state - // please see: brave/chromium_src/base/feature_override.h - std::unordered_set enabled_features = {}; - - // Runtime-disabled features. To override Chromium features default state - // please see: brave/chromium_src/base/feature_override.h - std::unordered_set disabled_features = {}; - - if (base::CommandLine::ForCurrentProcess()->HasSwitch( - switches::kDisableDnsOverHttps)) { - disabled_features.insert(features::kDnsOverHttps.name); - } - - command_line.AppendFeatures(enabled_features, disabled_features); - - bool ret = ChromeMainDelegate::BasicStartupComplete(exit_code); - - return ret; -} - -#if BUILDFLAG(IS_ANDROID) -void BraveMainDelegate::AdjustSyncServiceUrlForAndroid( - std::string* brave_sync_service_url) { - DCHECK_NE(brave_sync_service_url, nullptr); - const char kProcessTypeSwitchName[] = "type"; - - // On Android we can detect data dir only on host process, and we cannot - // for example on renderer or gpu-process, because JNI is not initialized - // And no sense to override sync service url for them in anyway - if (base::CommandLine::ForCurrentProcess()->HasSwitch( - kProcessTypeSwitchName)) { - // This is something other than browser process - return; - } - - JNIEnv* env = base::android::AttachCurrentThread(); - bool b_use_staging_sync_server = - Java_BraveQAPreferences_isSyncStagingUsed(env); - if (b_use_staging_sync_server) { - *brave_sync_service_url = kBraveSyncServiceStagingURL; - } -} -#endif // BUILDFLAG(IS_ANDROID)) diff --git a/app/brave_main_delegate.h b/app/brave_main_delegate.h index 6e275d5ec236..e7b0783dd9b6 100644 --- a/app/brave_main_delegate.h +++ b/app/brave_main_delegate.h @@ -25,17 +25,10 @@ class BraveMainDelegate : public ChromeMainDelegate { protected: // content::ContentMainDelegate implementation: - bool BasicStartupComplete(int* exit_code) override; - content::ContentBrowserClient* CreateContentBrowserClient() override; content::ContentRendererClient* CreateContentRendererClient() override; content::ContentUtilityClient* CreateContentUtilityClient() override; void PreSandboxStartup() override; - - private: -#if BUILDFLAG(IS_ANDROID) - void AdjustSyncServiceUrlForAndroid(std::string* brave_sync_service_url); -#endif // BUILDFLAG(IS_ANDROID)) }; #endif // BRAVE_APP_BRAVE_MAIN_DELEGATE_H_ diff --git a/chromium_src/chrome/app/DEPS b/chromium_src/chrome/app/DEPS index 341649b6e673..a165960014d8 100644 --- a/chromium_src/chrome/app/DEPS +++ b/chromium_src/chrome/app/DEPS @@ -5,3 +5,11 @@ include_rules = [ "+chrome/browser", "+chrome/common", ] + +specific_include_rules = { + "chrome_main_delegate.cc": [ + "+brave/build/android/jni_headers/BraveQAPreferences_jni.h", + "+components/signin/public/base/account_consistency_method.h", + "+components/sync/base/command_line_switches.h", + ], +} diff --git a/chromium_src/chrome/app/chrome_main_delegate.cc b/chromium_src/chrome/app/chrome_main_delegate.cc index 61f80da9d024..7fba1f9569ca 100644 --- a/chromium_src/chrome/app/chrome_main_delegate.cc +++ b/chromium_src/chrome/app/chrome_main_delegate.cc @@ -3,9 +3,116 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this file, * You can obtain one at http://mozilla.org/MPL/2.0/. */ -// This chormium_src override allows us to skip a lot of patching to +// This chromium_src override allows us to skip a lot of patching to // chrome/BUILD.gn #include "brave/app/brave_command_line_helper.cc" #include "brave/app/brave_main_delegate.cc" +#include "build/build_config.h" +#include "components/sync/base/command_line_switches.h" +#if BUILDFLAG(IS_ANDROID) +#include "base/android/jni_android.h" +#include "brave/build/android/jni_headers/BraveQAPreferences_jni.h" +#include "components/signin/public/base/account_consistency_method.h" +#endif + +#define BasicStartupComplete BasicStartupComplete_ChromiumImpl #include "src/chrome/app/chrome_main_delegate.cc" +#undef BasicStartupComplete + +#if BUILDFLAG(IS_ANDROID) +void AdjustSyncServiceUrlForAndroid(std::string* brave_sync_service_url) { + DCHECK_NE(brave_sync_service_url, nullptr); + const char kProcessTypeSwitchName[] = "type"; + + // On Android we can detect data dir only on host process, and we cannot + // for example on renderer or gpu-process, because JNI is not initialized + // And no sense to override sync service url for them in anyway + if (base::CommandLine::ForCurrentProcess()->HasSwitch( + kProcessTypeSwitchName)) { + // This is something other than browser process + return; + } + + JNIEnv* env = base::android::AttachCurrentThread(); + bool b_use_staging_sync_server = + Java_BraveQAPreferences_isSyncStagingUsed(env); + if (b_use_staging_sync_server) { + *brave_sync_service_url = kBraveSyncServiceStagingURL; + } +} +#endif // BUILDFLAG(IS_ANDROID) + +// We don't implement this as an overridden method in BraveMainDelegate because +// we need this to be executed also when running browser upstream tests, which +// rely on ChromeTestLauncherDelegate instead of BraveTestLauncherDelegate. +// +// Because of that, upstream tests won't get BraveMainDelegate instantiated and +// therefore we won't get any of the features below disabled/enabled when +// running those browser tests, which is not what we want. +// +// For instance, on DCHECK-enabled builds, not enabling the DOM distiller will +// get all browser tests crashing when dom_distiller::RunIsolatedJavaScript() +// gets called from AdsTabHelper::RunIsolatedJavaScript() (called via the +// WebContentsObserver machinery), since the JS World ID won't be set yet when +// dom_distiller::RunIsolatedJavaScript() gets called. +bool ChromeMainDelegate::BasicStartupComplete(int* exit_code) { + BraveCommandLineHelper command_line(base::CommandLine::ForCurrentProcess()); + command_line.AppendSwitch(switches::kDisableClientSidePhishingDetection); + command_line.AppendSwitch(switches::kDisableDomainReliability); + command_line.AppendSwitch(switches::kEnableDomDistiller); + command_line.AppendSwitch(switches::kNoPings); + + std::string update_url = GetUpdateURLHost(); + if (!update_url.empty()) { + std::string source = "url-source=" + update_url; + command_line.AppendSwitchASCII(switches::kComponentUpdater, source.c_str()); + } + + if (!base::CommandLine::ForCurrentProcess()->HasSwitch( + embedder_support::kOriginTrialPublicKey)) { + command_line.AppendSwitchASCII(embedder_support::kOriginTrialPublicKey, + kBraveOriginTrialsPublicKey); + } + + std::string brave_sync_service_url = BRAVE_SYNC_ENDPOINT; +#if BUILDFLAG(IS_ANDROID) + AdjustSyncServiceUrlForAndroid(&brave_sync_service_url); +#endif // BUILDFLAG(IS_ANDROID) + + // Brave's sync protocol does not use the sync service url + command_line.AppendSwitchASCII(syncer::kSyncServiceURL, + brave_sync_service_url.c_str()); + + command_line.AppendSwitchASCII(switches::kLsoUrl, kDummyUrl); + + // Brave variations + const std::string kVariationsServerURL = BRAVE_VARIATIONS_SERVER_URL; + command_line.AppendSwitchASCII(variations::switches::kVariationsServerURL, + kVariationsServerURL.c_str()); + // Insecure fall-back for variations is set to the same (secure) URL. This is + // done so that if VariationsService tries to fall back to insecure url the + // check for kHttpScheme in VariationsService::MaybeRetryOverHTTP would + // prevent it from doing so as we don't want to use an insecure fall-back. + const std::string kVariationsInsecureServerURL = BRAVE_VARIATIONS_SERVER_URL; + command_line.AppendSwitchASCII( + variations::switches::kVariationsInsecureServerURL, + kVariationsInsecureServerURL.c_str()); + + // Runtime-enabled features. To override Chromium features default state + // please see: brave/chromium_src/base/feature_override.h + std::unordered_set enabled_features = {}; + + // Runtime-disabled features. To override Chromium features default state + // please see: brave/chromium_src/base/feature_override.h + std::unordered_set disabled_features = {}; + + if (base::CommandLine::ForCurrentProcess()->HasSwitch( + switches::kDisableDnsOverHttps)) { + disabled_features.insert(features::kDnsOverHttps.name); + } + + command_line.AppendFeatures(enabled_features, disabled_features); + + return ChromeMainDelegate::BasicStartupComplete_ChromiumImpl(exit_code); +} diff --git a/chromium_src/chrome/app/chrome_main_delegate.h b/chromium_src/chrome/app/chrome_main_delegate.h index 71c80eccf1af..21e0a86d5a7d 100644 --- a/chromium_src/chrome/app/chrome_main_delegate.h +++ b/chromium_src/chrome/app/chrome_main_delegate.h @@ -8,9 +8,17 @@ #include "brave/common/brave_content_client.h" #include "chrome/common/chrome_content_client.h" +#include "content/public/app/content_main_delegate.h" #define ChromeContentClient BraveContentClient + +#define BasicStartupComplete \ + BasicStartupComplete_ChromiumImpl(int* exit_code); \ + bool BasicStartupComplete + #include "src/chrome/app/chrome_main_delegate.h" + +#undef BasicStartupComplete #undef ChromeContentClient #endif // BRAVE_CHROMIUM_SRC_CHROME_APP_CHROME_MAIN_DELEGATE_H_