Skip to content

Commit

Permalink
feat: Add option to conditionally disable site instance patches (#18554)
Browse files Browse the repository at this point in the history
* feat: Add option to conditionally disable site instance patches (#18396)

* chore: allow conditional disable of the site instance override patches at runtime

* feat: add app.allowRendererProcessReuse property to allow runtime disable of site instance overrides

spec: add tests for the new allowRendererProcessReuse property

feat: add console warnings / errors for loading non context-aware native modules
  * Only error if the patch is disabled
  * Warn all the time, this will ship in Electron 7

* chore: do not warn in about context aware in v6

* chore: update patches
  • Loading branch information
MarshallOfSound committed Jun 13, 2019
1 parent f7042c8 commit 18ec438
Show file tree
Hide file tree
Showing 20 changed files with 226 additions and 95 deletions.
2 changes: 1 addition & 1 deletion DEPS
Expand Up @@ -12,7 +12,7 @@ vars = {
'chromium_version':
'76.0.3809.23',
'node_version':
'a86a4a160dc520c61a602c949a32a1bc4c0fc633',
'dee0db9864a001ffc16440f725f4952a1a417069',

'boto_version': 'f7574aa6cc2c819430c1f05e9a1a1a666ef8169b',
'pyyaml_version': '3.12',
Expand Down
12 changes: 11 additions & 1 deletion atom/browser/api/atom_api_app.cc
Expand Up @@ -1279,6 +1279,13 @@ std::string App::GetUserAgentFallback() {
return AtomBrowserClient::Get()->GetUserAgent();
}

void App::SetBrowserClientCanUseCustomSiteInstance(bool should_disable) {
AtomBrowserClient::Get()->SetCanUseCustomSiteInstance(should_disable);
}
bool App::CanBrowserClientUseCustomSiteInstance() {
return AtomBrowserClient::Get()->CanUseCustomSiteInstance();
}

#if defined(OS_MACOSX)
bool App::MoveToApplicationsFolder(mate::Arguments* args) {
return ui::cocoa::AtomBundleMover::Move(args);
Expand Down Expand Up @@ -1441,7 +1448,10 @@ void App::BuildPrototype(v8::Isolate* isolate,
#endif
.SetProperty("userAgentFallback", &App::GetUserAgentFallback,
&App::SetUserAgentFallback)
.SetMethod("enableSandbox", &App::EnableSandbox);
.SetMethod("enableSandbox", &App::EnableSandbox)
.SetProperty("allowRendererProcessReuse",
&App::CanBrowserClientUseCustomSiteInstance,
&App::SetBrowserClientCanUseCustomSiteInstance);
}

} // namespace api
Expand Down
2 changes: 2 additions & 0 deletions atom/browser/api/atom_api_app.h
Expand Up @@ -212,6 +212,8 @@ class App : public AtomBrowserClient::Delegate,
void EnableSandbox(mate::Arguments* args);
void SetUserAgentFallback(const std::string& user_agent);
std::string GetUserAgentFallback();
void SetBrowserClientCanUseCustomSiteInstance(bool should_disable);
bool CanBrowserClientUseCustomSiteInstance();

#if defined(OS_MACOSX)
bool MoveToApplicationsFolder(mate::Arguments* args);
Expand Down
12 changes: 12 additions & 0 deletions atom/browser/atom_browser_client.cc
Expand Up @@ -397,6 +397,14 @@ void AtomBrowserClient::OverrideWebkitPrefs(content::RenderViewHost* host,
web_preferences->OverrideWebkitPrefs(prefs);
}

void AtomBrowserClient::SetCanUseCustomSiteInstance(bool should_disable) {
disable_process_restart_tricks_ = should_disable;
}

bool AtomBrowserClient::CanUseCustomSiteInstance() {
return disable_process_restart_tricks_;
}

content::ContentBrowserClient::SiteInstanceForNavigationType
AtomBrowserClient::ShouldOverrideSiteInstanceForNavigation(
content::RenderFrameHost* current_rfh,
Expand Down Expand Up @@ -503,6 +511,10 @@ void AtomBrowserClient::AppendExtraCommandLineSwitches(
web_preferences->AppendCommandLineSwitches(command_line);
SessionPreferences::AppendExtraCommandLineSwitches(
web_contents->GetBrowserContext(), command_line);
if (CanUseCustomSiteInstance()) {
command_line->AppendSwitch(
switches::kDisableElectronSiteInstanceOverrides);
}
}
}

Expand Down
5 changes: 5 additions & 0 deletions atom/browser/atom_browser_client.h
Expand Up @@ -67,6 +67,9 @@ class AtomBrowserClient : public content::ContentBrowserClient,
std::string GetUserAgent() const override;
void SetUserAgent(const std::string& user_agent);

void SetCanUseCustomSiteInstance(bool should_disable);
bool CanUseCustomSiteInstance() override;

protected:
void RenderProcessWillLaunch(
content::RenderProcessHost* host,
Expand Down Expand Up @@ -234,6 +237,8 @@ class AtomBrowserClient : public content::ContentBrowserClient,

std::string user_agent_override_ = "";

bool disable_process_restart_tricks_ = false;

DISALLOW_COPY_AND_ASSIGN(AtomBrowserClient);
};

Expand Down
2 changes: 2 additions & 0 deletions atom/common/options_switches.cc
Expand Up @@ -232,6 +232,8 @@ const char kScrollBounce[] = "scroll-bounce";
const char kHiddenPage[] = "hidden-page";
const char kNativeWindowOpen[] = "native-window-open";
const char kWebviewTag[] = "webview-tag";
const char kDisableElectronSiteInstanceOverrides[] =
"disable-electron-site-instance-overrides";

// Command switch passed to renderer process to control nodeIntegration.
const char kNodeIntegrationInWorker[] = "node-integration-in-worker";
Expand Down
1 change: 1 addition & 0 deletions atom/common/options_switches.h
Expand Up @@ -118,6 +118,7 @@ extern const char kNodeIntegrationInWorker[];
extern const char kWebviewTag[];
extern const char kNodeIntegrationInSubFrames[];
extern const char kDisableHtmlFullscreenWindowResize[];
extern const char kDisableElectronSiteInstanceOverrides[];

extern const char kWidevineCdmPath[];
extern const char kWidevineCdmVersion[];
Expand Down
14 changes: 11 additions & 3 deletions atom/renderer/atom_renderer_client.cc
Expand Up @@ -103,6 +103,12 @@ void AtomRendererClient::DidCreateScriptContext(

// Setup node environment for each window.
node::Environment* env = node_bindings_->CreateEnvironment(context);
auto* command_line = base::CommandLine::ForCurrentProcess();
// If we have disabled the site instance overrides we should prevent loading
// any non-context aware native module
if (command_line->HasSwitch(switches::kDisableElectronSiteInstanceOverrides))
env->ForceOnlyContextAwareNativeModules();

environments_.insert(env);

// Add Electron extended APIs.
Expand Down Expand Up @@ -144,9 +150,11 @@ void AtomRendererClient::WillReleaseScriptContext(
// Destroy the node environment. We only do this if node support has been
// enabled for sub-frames to avoid a change-of-behavior / introduce crashes
// for existing users.
// TODO(MarshallOfSOund): Free the environment regardless of this switch
if (base::CommandLine::ForCurrentProcess()->HasSwitch(
switches::kNodeIntegrationInSubFrames))
// We also do this if we have disable electron site instance overrides to
// avoid memory leaks
auto* command_line = base::CommandLine::ForCurrentProcess();
if (command_line->HasSwitch(switches::kNodeIntegrationInSubFrames) ||
command_line->HasSwitch(switches::kDisableElectronSiteInstanceOverrides))
node::FreeEnvironment(env);

// ElectronBindings is tracking node environments.
Expand Down
13 changes: 13 additions & 0 deletions docs/api/app.md
Expand Up @@ -1386,3 +1386,16 @@ A `Boolean` property that returns `true` if the app is packaged, `false` otherw
[Squirrel-Windows]: https://github.com/Squirrel/Squirrel.Windows
[JumpListBeginListMSDN]: https://msdn.microsoft.com/en-us/library/windows/desktop/dd378398(v=vs.85).aspx
[about-panel-options]: https://developer.apple.com/reference/appkit/nsapplication/1428479-orderfrontstandardaboutpanelwith?language=objc

### `app.allowRendererProcessReuse`

A `Boolean` which when `true` disables the overrides that Electron has in place
to ensure renderer processes are restarted on every navigation. The current
default value for this property is `false`.

The intention is for these overrides to become disabled by default and then at
some point in the future this property will be removed. This property impacts
which native modules you can use in the renderer process. For more information
on the direction Electron is going with renderer process restarts and usage of
native modules in the renderer process please check out this
[Tracking Issue](https://github.com/electron/electron/issues/18397).
4 changes: 2 additions & 2 deletions patches/common/chromium/.patches
Expand Up @@ -9,7 +9,6 @@ browser_compositor_mac.patch
can_create_window.patch
disable_hidden.patch
dom_storage_limits.patch
frame_host_manager.patch
out_of_process_instance.patch
render_widget_host_view_base.patch
render_widget_host_view_mac.patch
Expand Down Expand Up @@ -59,7 +58,6 @@ command-ismediakey.patch
tts.patch
printing.patch
verbose_generate_breakpad_symbols.patch
cross_site_document_resource_handler.patch
content_allow_embedder_to_prevent_locking_scheme_registry.patch
support_mixed_sandbox_with_zygote.patch
disable_color_correct_rendering.patch
Expand All @@ -80,3 +78,5 @@ partially_revert_fb4ab3be3fc0024d00358d5b7816d3215a8ff20c.patch
disable_custom_libcxx_on_windows.patch
fix_breakpad_symbol_generation_on_linux_arm.patch
woa_compiler_workaround.patch
cross_site_document_resource_handler.patch
frame_host_manager.patch
2 changes: 1 addition & 1 deletion patches/common/chromium/blink_local_frame.patch
Expand Up @@ -14,7 +14,7 @@ when there is code doing that.
This patch reverts the change to fix the crash in Electron.

diff --git a/third_party/blink/renderer/core/frame/local_frame.cc b/third_party/blink/renderer/core/frame/local_frame.cc
index 4851cb4a227b8cbccfb0775d46f1ea001e5fce74..8f24199a056eac92ba7f2173fb1c7f173d50d6ff 100644
index 06f9c9f62dc94bd093942d9ee523a4279203a177..3ee1caa9ad92cfedd326b364817ce9b5e8db800e 100644
--- a/third_party/blink/renderer/core/frame/local_frame.cc
+++ b/third_party/blink/renderer/core/frame/local_frame.cc
@@ -393,10 +393,6 @@ void LocalFrame::DetachImpl(FrameDetachType type) {
Expand Down
8 changes: 4 additions & 4 deletions patches/common/chromium/can_create_window.patch
Expand Up @@ -5,10 +5,10 @@ Subject: can_create_window.patch


diff --git a/content/browser/frame_host/render_frame_host_impl.cc b/content/browser/frame_host/render_frame_host_impl.cc
index ae459238a1bd31e219f1b1a2cb539d4c071dcf33..63e11474acb2af5406015b7a3d39dc51cf160506 100644
index b07b5f0b6eddfb9c754bed039c061de98a361f0e..4d1f8c2c5499291f55bf17cd7a0f45c3655c2985 100644
--- a/content/browser/frame_host/render_frame_host_impl.cc
+++ b/content/browser/frame_host/render_frame_host_impl.cc
@@ -3709,6 +3709,7 @@ void RenderFrameHostImpl::CreateNewWindow(
@@ -3712,6 +3712,7 @@ void RenderFrameHostImpl::CreateNewWindow(
last_committed_origin_, params->window_container_type,
params->target_url, params->referrer.To<Referrer>(),
params->frame_name, params->disposition, *params->features,
Expand All @@ -17,10 +17,10 @@ index ae459238a1bd31e219f1b1a2cb539d4c071dcf33..63e11474acb2af5406015b7a3d39dc51
&no_javascript_access);

diff --git a/content/common/frame.mojom b/content/common/frame.mojom
index 82882159b0bac6d47d678c485de0aacc7db06c2d..dd2299094b79d82da7ec1cd8f559050b6f0e2af5 100644
index 577fd09ef05584962b5357028702d54f36719c6a..6dd3dcb250a5744d62b13b0ee5544d8270d8d882 100644
--- a/content/common/frame.mojom
+++ b/content/common/frame.mojom
@@ -291,6 +291,10 @@ struct CreateNewWindowParams {
@@ -298,6 +298,10 @@ struct CreateNewWindowParams {

// The window features to use for the new window.
blink.mojom.WindowFeatures features;
Expand Down
26 changes: 13 additions & 13 deletions patches/common/chromium/cross_site_document_resource_handler.patch
Expand Up @@ -22,31 +22,31 @@ index d514c10160dd12f225c42e927977660cacbc9c43..49345f1d4d75c8b96efe485202d89774
}

diff --git a/content/public/browser/content_browser_client.cc b/content/public/browser/content_browser_client.cc
index 671d2259d50bfbb5b56a79907c1c86218ee04469..373eee0faf29f2d4be98156f4abd476c61dff713 100644
index d15061de5254fd4f248fed92f47a1b1fcf34cd68..ac0151af07e440b7e4b7a291530abe108b5cd7d9 100644
--- a/content/public/browser/content_browser_client.cc
+++ b/content/public/browser/content_browser_client.cc
@@ -62,6 +62,10 @@ ContentBrowserClient::SiteInstanceForNavigationType ContentBrowserClient::Should
return SiteInstanceForNavigationType::ASK_CHROMIUM;
@@ -57,6 +57,10 @@ std::unique_ptr<BrowserMainParts> ContentBrowserClient::CreateBrowserMainParts(
return nullptr;
}

+bool ContentBrowserClient::ShouldBypassCORB(int render_process_id) const {
+ return false;
+}
+
std::unique_ptr<BrowserMainParts> ContentBrowserClient::CreateBrowserMainParts(
const MainFunctionParams& parameters) {
return nullptr;
void ContentBrowserClient::PostAfterStartupTask(
const base::Location& from_here,
const scoped_refptr<base::TaskRunner>& task_runner,
diff --git a/content/public/browser/content_browser_client.h b/content/public/browser/content_browser_client.h
index fcc5a0e5cd830afe459c1defe968b01e3f05f764..3067cf61e02f6abd048dc888237ef9e2d4909a78 100644
index e5cddbc3a28e0f90bd1d1ae6ebe28d5f2d0299c7..93fd647a472466633d608e05c0b66653c696073d 100644
--- a/content/public/browser/content_browser_client.h
+++ b/content/public/browser/content_browser_client.h
@@ -242,6 +242,9 @@ class CONTENT_EXPORT ContentBrowserClient {
content::RenderFrameHost* rfh,
content::SiteInstance* pending_site_instance) {}
@@ -219,6 +219,9 @@ class CONTENT_EXPORT ContentBrowserClient {
virtual std::unique_ptr<BrowserMainParts> CreateBrowserMainParts(
const MainFunctionParams& parameters);

+ // Electron: Allows bypassing CORB checks for a renderer process.
+ virtual bool ShouldBypassCORB(int render_process_id) const;
+
// Allows the embedder to set any number of custom BrowserMainParts
// implementations for the browser startup code. See comments in
// browser_main_parts.h.
// Allows the embedder to change the default behavior of
// BrowserThread::PostAfterStartupTask to better match whatever
// definition of "startup" the embedder has in mind. This may be

0 comments on commit 18ec438

Please sign in to comment.