From a8a881c8dbbd8b8f0fec5c59d72a9f1e66bbd09d Mon Sep 17 00:00:00 2001 From: Jeremy Apthorp Date: Thu, 22 Nov 2018 09:02:52 -0800 Subject: [PATCH] chore: restore //url dchecks (#15637) --- atom/app/atom_main_delegate.cc | 4 ++ atom/app/atom_main_delegate.h | 1 + atom/browser/atom_browser_main_parts.cc | 3 + patches/common/chromium/.patches | 1 + ...r_to_prevent_locking_scheme_registry.patch | 56 +++++++++++++++++++ patches/common/chromium/dcheck.patch | 50 ----------------- 6 files changed, 65 insertions(+), 50 deletions(-) create mode 100644 patches/common/chromium/content_allow_embedder_to_prevent_locking_scheme_registry.patch diff --git a/atom/app/atom_main_delegate.cc b/atom/app/atom_main_delegate.cc index c77edb2781a20..722d3e82db8f5 100644 --- a/atom/app/atom_main_delegate.cc +++ b/atom/app/atom_main_delegate.cc @@ -278,4 +278,8 @@ bool AtomMainDelegate::DelaySandboxInitialization( } #endif +bool AtomMainDelegate::ShouldLockSchemeRegistry() { + return false; +} + } // namespace atom diff --git a/atom/app/atom_main_delegate.h b/atom/app/atom_main_delegate.h index 7e21a0b837d9d..300e7b2df2dac 100644 --- a/atom/app/atom_main_delegate.h +++ b/atom/app/atom_main_delegate.h @@ -35,6 +35,7 @@ class AtomMainDelegate : public content::ContentMainDelegate { bool ShouldSendMachPort(const std::string& process_type) override; bool DelaySandboxInitialization(const std::string& process_type) override; #endif + bool ShouldLockSchemeRegistry() override; private: #if defined(OS_MACOSX) diff --git a/atom/browser/atom_browser_main_parts.cc b/atom/browser/atom_browser_main_parts.cc index e77a1261c6bd3..30d096063edf5 100644 --- a/atom/browser/atom_browser_main_parts.cc +++ b/atom/browser/atom_browser_main_parts.cc @@ -421,6 +421,9 @@ void AtomBrowserMainParts::PreMainMessageLoopRun() { node_bindings_->PrepareMessageLoop(); node_bindings_->RunMessageLoop(); + // url::Add*Scheme are not threadsafe, this helps prevent data races. + url::LockSchemeRegistries(); + #if defined(USE_X11) ui::TouchFactory::SetTouchDeviceListFromCommandLine(); #endif diff --git a/patches/common/chromium/.patches b/patches/common/chromium/.patches index 9738eb2ecba17..eb46624cd18c8 100644 --- a/patches/common/chromium/.patches +++ b/patches/common/chromium/.patches @@ -75,3 +75,4 @@ fix_zoom_display.patch chrome_process_finder.patch customizable_app_indicator_id_prefix.patch cross_site_document_resource_handler.patch +content_allow_embedder_to_prevent_locking_scheme_registry.patch diff --git a/patches/common/chromium/content_allow_embedder_to_prevent_locking_scheme_registry.patch b/patches/common/chromium/content_allow_embedder_to_prevent_locking_scheme_registry.patch new file mode 100644 index 0000000000000..e3b571a61c690 --- /dev/null +++ b/patches/common/chromium/content_allow_embedder_to_prevent_locking_scheme_registry.patch @@ -0,0 +1,56 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Jeremy Apthorp +Date: Wed, 21 Nov 2018 14:31:34 -0800 +Subject: content: allow embedder to prevent locking scheme registry + +The //content layer requires all schemes to be registered during startup, +because Add*Scheme aren't threadsafe. However, Electron exposes the option to +register additional schemes via JavaScript in the main process before the app +is ready, but after the //content layer has already locked the registry. + +Without this patch, calling `registerStandardSchemes` during initialization +when in debug mode will cause a DCHECK to fire. + + +diff --git a/content/app/content_main_runner_impl.cc b/content/app/content_main_runner_impl.cc +index 034fdcad958cb25780c304d184b2fbb721a13200..c2090368f6678950690463e0b90620cb9fdf5f41 100644 +--- a/content/app/content_main_runner_impl.cc ++++ b/content/app/content_main_runner_impl.cc +@@ -808,7 +808,7 @@ int ContentMainRunnerImpl::Initialize(const ContentMainParams& params) { + #endif + + RegisterPathProvider(); +- RegisterContentSchemes(true); ++ RegisterContentSchemes(delegate_->ShouldLockSchemeRegistry()); + + #if defined(OS_ANDROID) && (ICU_UTIL_DATA_IMPL == ICU_UTIL_DATA_FILE) + int icudata_fd = g_fds->MaybeGet(kAndroidICUDataDescriptor); +diff --git a/content/public/app/content_main_delegate.cc b/content/public/app/content_main_delegate.cc +index c4bdfd36ad0c34b3a91b59502414bc98c091ccee..d7ac740d0088c002bee98238faea1ef9593eee92 100644 +--- a/content/public/app/content_main_delegate.cc ++++ b/content/public/app/content_main_delegate.cc +@@ -63,6 +63,10 @@ bool ContentMainDelegate::ShouldEnableProfilerRecording() { + return false; + } + ++bool ContentMainDelegate::ShouldLockSchemeRegistry() { ++ return true; ++} ++ + service_manager::ProcessType ContentMainDelegate::OverrideProcessType() { + return service_manager::ProcessType::kDefault; + } +diff --git a/content/public/app/content_main_delegate.h b/content/public/app/content_main_delegate.h +index 979e25d1c1b9ff4b16adbab28cb44d8473f8232a..7aca113058ed111a98e396df57ebc36c06e3b896 100644 +--- a/content/public/app/content_main_delegate.h ++++ b/content/public/app/content_main_delegate.h +@@ -101,6 +101,9 @@ class CONTENT_EXPORT ContentMainDelegate { + // Returns whether or not profiler recording should be enabled. + virtual bool ShouldEnableProfilerRecording(); + ++ // Allows the embedder to prevent locking the scheme registry. ++ 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). diff --git a/patches/common/chromium/dcheck.patch b/patches/common/chromium/dcheck.patch index ebcf841917f3d..6df35c760f648 100644 --- a/patches/common/chromium/dcheck.patch +++ b/patches/common/chromium/dcheck.patch @@ -16,41 +16,6 @@ example, the checks might be disabled for a whole build target, but actually only one or two specific checks fail. Then it's better to simply comment out the failing checks and allow the rest of the target to have them enabled. -Please keep the following lists updated. - -The ELECTRON_NO_DCHECK build flag disables debug checks universally. -This patch applies the flag to the following GN targets: - - third_party/blink/renderer/core/loader:loader - url:url - -These files have debug checks explicitly commented out: - - base/memory/weak_ptr.cc - base/process/kill_win.cc - components/viz/service/display/program_binding.h - components/viz/service/display_embedder/server_shared_bitmap_manager.cc - content/browser/frame_host/navigation_controller_impl.cc - content/browser/frame_host/render_frame_host_impl.cc - content/browser/renderer_host/render_widget_host_view_mac.mm - ppapi/host/ppapi_host.cc - third_party/blink/renderer/core/dom/node.cc - third_party/blink/renderer/platform/wtf/text/string_impl.cc - ui/base/clipboard/clipboard_win.cc - -diff --git a/base/logging.h b/base/logging.h -index 08c1f0fc59672b2134c634e081f0c4df4d261b75..7a758ce3ef90145d2b68e07e17f00fc30cfb30a6 100644 ---- a/base/logging.h -+++ b/base/logging.h -@@ -874,7 +874,7 @@ const LogSeverity LOG_DCHECK = LOG_FATAL; - - #else // !(defined(_PREFAST_) && defined(OS_WIN)) - --#if DCHECK_IS_ON() -+#if DCHECK_IS_ON() && !defined(ELECTRON_NO_DCHECK) - - #define DCHECK(condition) \ - LAZY_STREAM(LOG_STREAM(DCHECK), !ANALYZER_ASSUME_TRUE(condition)) \ diff --git a/content/browser/frame_host/navigation_controller_impl.cc b/content/browser/frame_host/navigation_controller_impl.cc index d8aca9e2cbffdfd0bbb0bd67e8adfb53547132bb..86224ab7a3c5637422559da25bc8c1040a03e937 100644 --- a/content/browser/frame_host/navigation_controller_impl.cc @@ -138,18 +103,3 @@ index e49dd8c81270cdd9794ddee11bad036c2f444af5..9e61c901cd2df168520b83a8522ca8c0 FreeData(format, handle); } } -diff --git a/url/BUILD.gn b/url/BUILD.gn -index 57bbe16c15ea442963a53f89b009e2c99c7b090a..07725187c595784d9e5f49d45a8835da78144830 100644 ---- a/url/BUILD.gn -+++ b/url/BUILD.gn -@@ -98,6 +98,10 @@ component("url") { - ] - deps += [ "//third_party/icu" ] - } -+ -+ if (is_electron_build) { -+ defines += [ "ELECTRON_NO_DCHECK" ] -+ } - } - - if (is_android) {