Skip to content

Commit

Permalink
Ensure upstream browser tests run with the expected set of features
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@mariospr
mariospr committed Mar 21, 2022
1 parent 50e3dc8 commit 3e735ac
Show file tree
Hide file tree
Showing 5 changed files with 122 additions and 103 deletions.
95 changes: 0 additions & 95 deletions app/brave_main_delegate.cc
View file
Expand Up @@ -36,14 +36,6 @@
#include "content/public/common/content_switches.h"
#include "google_apis/gaia/gaia_switches.h"

#if defined(OS_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[] =
Expand Down Expand Up @@ -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 defined(OS_ANDROID)
AdjustSyncServiceUrlForAndroid(&brave_sync_service_url);
#endif // defined(OS_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<const char*> enabled_features = {};

// Runtime-disabled features. To override Chromium features default state
// please see: brave/chromium_src/base/feature_override.h
std::unordered_set<const char*> 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 defined(OS_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 // defined(OS_ANDROID)
7 changes: 0 additions & 7 deletions app/brave_main_delegate.h
View file
Expand Up @@ -24,17 +24,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 defined(OS_ANDROID)
void AdjustSyncServiceUrlForAndroid(std::string* brave_sync_service_url);
#endif // defined(OS_ANDROID)
};

#endif // BRAVE_APP_BRAVE_MAIN_DELEGATE_H_
7 changes: 7 additions & 0 deletions chromium_src/chrome/app/DEPS
View file
Expand Up @@ -5,3 +5,10 @@ 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",
],
}
108 changes: 107 additions & 1 deletion chromium_src/chrome/app/chrome_main_delegate.cc
View file
Expand Up @@ -3,9 +3,115 @@
* 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 "components/sync/base/command_line_switches.h"

#if defined(OS_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 defined(OS_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 // defined(OS_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 defined(OS_ANDROID)
AdjustSyncServiceUrlForAndroid(&brave_sync_service_url);
#endif // defined(OS_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<const char*> enabled_features = {};

// Runtime-disabled features. To override Chromium features default state
// please see: brave/chromium_src/base/feature_override.h
std::unordered_set<const char*> 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);
}
8 changes: 8 additions & 0 deletions chromium_src/chrome/app/chrome_main_delegate.h
View file
Expand Up @@ -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_

0 comments on commit 3e735ac

Please sign in to comment.