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
This commit is contained in:
Samuel Attard 2019-05-31 15:47:18 -07:00 коммит произвёл GitHub
Родитель 26155c8a00
Коммит 87ae9324ac
Не найден ключ, соответствующий данной подписи
Идентификатор ключа GPG: 4AEE18F83AFDEB23
17 изменённых файлов: 221 добавлений и 89 удалений

2
DEPS
Просмотреть файл

@ -12,7 +12,7 @@ vars = {
'chromium_version':
'ab588d36191964c4bca8de5c320534d95606c861',
'node_version':
'a86a4a160dc520c61a602c949a32a1bc4c0fc633',
'dee0db9864a001ffc16440f725f4952a1a417069',
'nan_version':
'960dd6c70fc9eb136efdf37b4bef18fadbc3436f',

Просмотреть файл

@ -1286,6 +1286,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);
@ -1467,7 +1474,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

Просмотреть файл

@ -213,6 +213,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);

Просмотреть файл

@ -403,6 +403,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,
@ -509,6 +517,10 @@ void AtomBrowserClient::AppendExtraCommandLineSwitches(
web_preferences->AppendCommandLineSwitches(command_line);
SessionPreferences::AppendExtraCommandLineSwitches(
web_contents->GetBrowserContext(), command_line);
if (CanUseCustomSiteInstance()) {
command_line->AppendSwitch(
switches::kDisableElectronSiteInstanceOverrides);
}
}
}

Просмотреть файл

@ -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,
@ -253,6 +256,8 @@ class AtomBrowserClient : public content::ContentBrowserClient,
std::string user_agent_override_ = "";
bool disable_process_restart_tricks_ = false;
DISALLOW_COPY_AND_ASSIGN(AtomBrowserClient);
};

Просмотреть файл

@ -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";

Просмотреть файл

@ -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[];

Просмотреть файл

@ -104,6 +104,13 @@ 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();
env->WarnNonContextAwareNativeModules();
environments_.insert(env);
// Add Electron extended APIs.
@ -145,9 +152,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.

Просмотреть файл

@ -1306,3 +1306,16 @@ This is the user agent that will be used when no user agent is set at the
`webContents` or `session` level. It is useful for ensuring that your entire
app has the same user agent. Set to a custom value as early as possible
in your app's initialization to ensure that your overridden value is used.
### `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).

Просмотреть файл

@ -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
@ -58,7 +57,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
@ -79,3 +77,5 @@ disable_custom_libcxx_on_windows.patch
feat_offscreen_rendering_with_viz_compositor.patch
worker_context_will_destroy.patch
fix_breakpad_symbol_generation_on_linux_arm.patch
cross_site_document_resource_handler.patch
frame_host_manager.patch

Просмотреть файл

@ -22,31 +22,31 @@ index 2151c5b9698e9a2768875d04a2297956cc4c0d41..8a316a117ab367bcbac947894cbe1bc2
}
diff --git a/content/public/browser/content_browser_client.cc b/content/public/browser/content_browser_client.cc
index 2317ddf6a3c533f701fe44bf1b8114eb042c2189..9a823a98175c33c84b8d44a95c9d7d44568807ca 100644
index 3729dcc9ea3272c943754a92c6ed1d7a1fd8fcf3..787cd81b26508d3e04956721f0e7cec2f457aa60 100644
--- a/content/public/browser/content_browser_client.cc
+++ b/content/public/browser/content_browser_client.cc
@@ -61,6 +61,10 @@ ContentBrowserClient::SiteInstanceForNavigationType ContentBrowserClient::Should
return SiteInstanceForNavigationType::ASK_CHROMIUM;
@@ -56,6 +56,10 @@ BrowserMainParts* ContentBrowserClient::CreateBrowserMainParts(
return nullptr;
}
+bool ContentBrowserClient::ShouldBypassCORB(int render_process_id) const {
+ return false;
+}
+
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 a81f40507b2233c3bde03b940cccd6be0aaa4926..1a208d24bae80277e4a60f4180bb7f95c38561ce 100644
index 4c84fb3648b3de36015b325246559f8aefe2ebd5..bf9b3a601fc16d5070e4467a258a047f47b059f3 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 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

Просмотреть файл

@ -4,94 +4,104 @@ Date: Wed, 14 Nov 2018 20:38:46 +0530
Subject: frame_host_manager.patch
Allows embedder to intercept site instances chosen by chromium
and respond with custom instance.
and respond with custom instance. Also allows for us to at-runtime
enable or disable this patch.
diff --git a/content/browser/frame_host/render_frame_host_manager.cc b/content/browser/frame_host/render_frame_host_manager.cc
index b5301d164138f21ca8ae01abfb153efde51ec324..91efc5f94f61f7bccc2ff802851097df8eb52818 100644
index b5301d164138f21ca8ae01abfb153efde51ec324..886b6d3beb3e2d7b13a15af830bea6fb5a567cba 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -2127,6 +2127,16 @@ bool RenderFrameHostManager::InitRenderView(
@@ -2127,6 +2127,20 @@ bool RenderFrameHostManager::InitRenderView(
scoped_refptr<SiteInstance>
RenderFrameHostManager::GetSiteInstanceForNavigationRequest(
const NavigationRequest& request) {
+ BrowserContext* browser_context =
+ delegate_->GetControllerForRenderManager().GetBrowserContext();
+ // If the navigation can swap SiteInstances, compute the SiteInstance it
+ // should use.
+ // TODO(clamy): We should also consider as a candidate SiteInstance the
+ // speculative SiteInstance that was computed on redirects.
+ scoped_refptr<SiteInstance> candidate_site_instance =
+ speculative_render_frame_host_
+ ? speculative_render_frame_host_->GetSiteInstance()
+ : nullptr;
+ BrowserContext* browser_context = nullptr;
+ scoped_refptr<SiteInstance> candidate_site_instance;
+ if (!GetContentClient()->browser()->CanUseCustomSiteInstance()) {
+ browser_context =
+ delegate_->GetControllerForRenderManager().GetBrowserContext();
+ // If the navigation can swap SiteInstances, compute the SiteInstance it
+ // should use.
+ // TODO(clamy): We should also consider as a candidate SiteInstance the
+ // speculative SiteInstance that was computed on redirects.
+ candidate_site_instance =
+ speculative_render_frame_host_
+ ? speculative_render_frame_host_->GetSiteInstance()
+ : nullptr;
+ }
// First, check if the navigation can switch SiteInstances. If not, the
// navigation should use the current SiteInstance.
SiteInstance* current_site_instance = render_frame_host_->GetSiteInstance();
@@ -2159,6 +2169,51 @@ RenderFrameHostManager::GetSiteInstanceForNavigationRequest(
@@ -2159,6 +2173,53 @@ RenderFrameHostManager::GetSiteInstanceForNavigationRequest(
request.common_params().url);
no_renderer_swap_allowed |=
request.from_begin_navigation() && !can_renderer_initiate_transfer;
+
+ bool has_response_started =
+ (request.state() == NavigationRequest::RESPONSE_STARTED ||
+ request.state() == NavigationRequest::FAILED) &&
+ !speculative_render_frame_host_;
+ // Gives user a chance to choose a custom site instance.
+ SiteInstance* affinity_site_instance = nullptr;
+ scoped_refptr<SiteInstance> overriden_site_instance;
+ ContentBrowserClient::SiteInstanceForNavigationType siteInstanceType =
+ GetContentClient()->browser()->ShouldOverrideSiteInstanceForNavigation(
+ current_frame_host(), speculative_frame_host(), browser_context,
+ request.common_params().url, has_response_started,
+ &affinity_site_instance);
+ switch (siteInstanceType) {
+ case ContentBrowserClient::SiteInstanceForNavigationType::
+ FORCE_CANDIDATE_OR_NEW:
+ overriden_site_instance =
+ candidate_site_instance
+ ? candidate_site_instance
+ : SiteInstance::CreateForURL(browser_context,
+ request.common_params().url);
+ break;
+ case ContentBrowserClient::SiteInstanceForNavigationType::FORCE_CURRENT:
+ overriden_site_instance = render_frame_host_->GetSiteInstance();
+ break;
+ case ContentBrowserClient::SiteInstanceForNavigationType::FORCE_AFFINITY:
+ DCHECK(affinity_site_instance);
+ overriden_site_instance =
+ scoped_refptr<SiteInstance>(affinity_site_instance);
+ break;
+ case ContentBrowserClient::SiteInstanceForNavigationType::ASK_CHROMIUM:
+ DCHECK(!affinity_site_instance);
+ break;
+ default:
+ break;
+ }
+ if (overriden_site_instance) {
+ if (siteInstanceType ==
+ ContentBrowserClient::SiteInstanceForNavigationType::
+ FORCE_CANDIDATE_OR_NEW) {
+ GetContentClient()->browser()->RegisterPendingSiteInstance(
+ render_frame_host_.get(), overriden_site_instance.get());
+ if (!GetContentClient()->browser()->CanUseCustomSiteInstance()) {
+ bool has_response_started =
+ (request.state() == NavigationRequest::RESPONSE_STARTED ||
+ request.state() == NavigationRequest::FAILED) &&
+ !speculative_render_frame_host_;
+ // Gives user a chance to choose a custom site instance.
+ SiteInstance* affinity_site_instance = nullptr;
+ scoped_refptr<SiteInstance> overriden_site_instance;
+ ContentBrowserClient::SiteInstanceForNavigationType siteInstanceType =
+ GetContentClient()->browser()->ShouldOverrideSiteInstanceForNavigation(
+ current_frame_host(), speculative_frame_host(), browser_context,
+ request.common_params().url, has_response_started,
+ &affinity_site_instance);
+ switch (siteInstanceType) {
+ case ContentBrowserClient::SiteInstanceForNavigationType::
+ FORCE_CANDIDATE_OR_NEW:
+ overriden_site_instance =
+ candidate_site_instance
+ ? candidate_site_instance
+ : SiteInstance::CreateForURL(browser_context,
+ request.common_params().url);
+ break;
+ case ContentBrowserClient::SiteInstanceForNavigationType::FORCE_CURRENT:
+ overriden_site_instance = render_frame_host_->GetSiteInstance();
+ break;
+ case ContentBrowserClient::SiteInstanceForNavigationType::FORCE_AFFINITY:
+ DCHECK(affinity_site_instance);
+ overriden_site_instance =
+ scoped_refptr<SiteInstance>(affinity_site_instance);
+ break;
+ case ContentBrowserClient::SiteInstanceForNavigationType::ASK_CHROMIUM:
+ DCHECK(!affinity_site_instance);
+ break;
+ default:
+ break;
+ }
+ if (overriden_site_instance) {
+ if (siteInstanceType ==
+ ContentBrowserClient::SiteInstanceForNavigationType::
+ FORCE_CANDIDATE_OR_NEW) {
+ GetContentClient()->browser()->RegisterPendingSiteInstance(
+ render_frame_host_.get(), overriden_site_instance.get());
+ }
+ return overriden_site_instance;
+ }
+ return overriden_site_instance;
+ }
} else {
// Subframe navigations will use the current renderer, unless specifically
// allowed to swap processes.
@@ -2170,23 +2225,17 @@ RenderFrameHostManager::GetSiteInstanceForNavigationRequest(
@@ -2170,23 +2231,28 @@ RenderFrameHostManager::GetSiteInstanceForNavigationRequest(
if (no_renderer_swap_allowed && !should_swap_for_error_isolation)
return scoped_refptr<SiteInstance>(current_site_instance);
- // If the navigation can swap SiteInstances, compute the SiteInstance it
- // should use.
- // TODO(clamy): We should also consider as a candidate SiteInstance the
- // speculative SiteInstance that was computed on redirects.
+ if (GetContentClient()->browser()->CanUseCustomSiteInstance()) {
// If the navigation can swap SiteInstances, compute the SiteInstance it
// should use.
// TODO(clamy): We should also consider as a candidate SiteInstance the
// speculative SiteInstance that was computed on redirects.
- SiteInstance* candidate_site_instance =
- speculative_render_frame_host_
- ? speculative_render_frame_host_->GetSiteInstance()
- : nullptr;
-
+ candidate_site_instance =
speculative_render_frame_host_
? speculative_render_frame_host_->GetSiteInstance()
: nullptr;
+ }
scoped_refptr<SiteInstance> dest_site_instance = GetSiteInstanceForNavigation(
request.common_params().url, request.source_site_instance(),
- request.dest_site_instance(), candidate_site_instance,
@ -108,13 +118,17 @@ index b5301d164138f21ca8ae01abfb153efde51ec324..91efc5f94f61f7bccc2ff802851097df
}
diff --git a/content/public/browser/content_browser_client.cc b/content/public/browser/content_browser_client.cc
index 3729dcc9ea3272c943754a92c6ed1d7a1fd8fcf3..2317ddf6a3c533f701fe44bf1b8114eb042c2189 100644
index 787cd81b26508d3e04956721f0e7cec2f457aa60..8e62a5dd26595411757e03078ed0e44646c47a52 100644
--- a/content/public/browser/content_browser_client.cc
+++ b/content/public/browser/content_browser_client.cc
@@ -51,6 +51,16 @@ void OverrideOnBindInterface(const service_manager::BindSourceInfo& remote_info,
@@ -51,6 +51,20 @@ void OverrideOnBindInterface(const service_manager::BindSourceInfo& remote_info,
handle);
}
+bool ContentBrowserClient::CanUseCustomSiteInstance() {
+ return false;
+}
+
+ContentBrowserClient::SiteInstanceForNavigationType ContentBrowserClient::ShouldOverrideSiteInstanceForNavigation(
+ content::RenderFrameHost* current_rfh,
+ content::RenderFrameHost* speculative_rfh,
@ -129,10 +143,10 @@ index 3729dcc9ea3272c943754a92c6ed1d7a1fd8fcf3..2317ddf6a3c533f701fe44bf1b8114eb
const MainFunctionParams& parameters) {
return nullptr;
diff --git a/content/public/browser/content_browser_client.h b/content/public/browser/content_browser_client.h
index 4c84fb3648b3de36015b325246559f8aefe2ebd5..a81f40507b2233c3bde03b940cccd6be0aaa4926 100644
index bf9b3a601fc16d5070e4467a258a047f47b059f3..3c35eddc2498157c2b98bab55991d8aa195954f6 100644
--- a/content/public/browser/content_browser_client.h
+++ b/content/public/browser/content_browser_client.h
@@ -211,8 +211,37 @@ CONTENT_EXPORT void OverrideOnBindInterface(
@@ -211,8 +211,40 @@ CONTENT_EXPORT void OverrideOnBindInterface(
// the observer interfaces.)
class CONTENT_EXPORT ContentBrowserClient {
public:
@ -153,6 +167,9 @@ index 4c84fb3648b3de36015b325246559f8aefe2ebd5..a81f40507b2233c3bde03b940cccd6be
+ };
virtual ~ContentBrowserClient() {}
+ // Electron: Allows disabling the below ShouldOverride patch
+ virtual bool CanUseCustomSiteInstance();
+
+ // Electron: Allows overriding the SiteInstance when navigating.
+ virtual SiteInstanceForNavigationType ShouldOverrideSiteInstanceForNavigation(
+ content::RenderFrameHost* current_rfh,

Просмотреть файл

@ -1332,6 +1332,26 @@ describe('default behavior', () => {
expect(app.userAgentFallback).to.equal(initialValue)
})
})
describe('app.allowRendererProcessReuse', () => {
it('should default to false', () => {
expect(app.allowRendererProcessReuse).to.equal(false)
})
it('should cause renderer processes to get new PIDs when false', async () => {
const output = await runTestApp('site-instance-overrides', 'false')
expect(output[0]).to.be.a('number').that.is.greaterThan(0)
expect(output[1]).to.be.a('number').that.is.greaterThan(0)
expect(output[0]).to.not.equal(output[1])
})
it('should cause renderer processes to keep the same PID when true', async () => {
const output = await runTestApp('site-instance-overrides', 'true')
expect(output[0]).to.be.a('number').that.is.greaterThan(0)
expect(output[1]).to.be.a('number').that.is.greaterThan(0)
expect(output[0]).to.equal(output[1])
})
})
})
async function runTestApp (name: string, ...args: any[]) {

1
spec/fixtures/api/site-instance-overrides/index.html поставляемый Normal file
Просмотреть файл

@ -0,0 +1 @@
<html></html>

33
spec/fixtures/api/site-instance-overrides/main.js поставляемый Normal file
Просмотреть файл

@ -0,0 +1,33 @@
const { app, BrowserWindow, ipcMain } = require('electron')
const path = require('path')
process.on('uncaughtException', (e) => {
console.error(e)
process.exit(1)
})
app.allowRendererProcessReuse = JSON.parse(process.argv[2])
const pids = []
let win
ipcMain.on('pid', (event, pid) => {
pids.push(pid)
if (pids.length === 2) {
console.log(JSON.stringify(pids))
if (win) win.close()
app.quit()
} else {
if (win) win.reload()
}
})
app.whenReady().then(() => {
win = new BrowserWindow({
show: false,
webPreferences: {
preload: path.resolve(__dirname, 'preload.js')
}
})
win.loadFile('index.html')
})

4
spec/fixtures/api/site-instance-overrides/package.json поставляемый Normal file
Просмотреть файл

@ -0,0 +1,4 @@
{
"name": "site-instance-overrides",
"main": "main.js"
}

3
spec/fixtures/api/site-instance-overrides/preload.js поставляемый Normal file
Просмотреть файл

@ -0,0 +1,3 @@
const { ipcRenderer } = require('electron')
ipcRenderer.send('pid', process.pid)