From 0ac883c6d4d268d300f2ceba13946ce70e17e138 Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Tue, 20 Mar 2018 15:54:47 +1100 Subject: [PATCH] Remove the race condition between new process creation and context release (#12342) * Remove the race condition between new process creation and old process releasing remote context Previously there was a race condition where the getId() method would return the new context ID even though the release was for the old context. This changes it to send the "initial" context ID with the release message to ensure there is no race. * fetch context ID from remote in sandbox mode --- atom/browser/api/atom_api_web_contents.cc | 10 +++++++--- atom/browser/api/atom_api_web_contents.h | 2 ++ atom/browser/atom_browser_client.cc | 6 ++++++ atom/common/options_switches.cc | 3 +++ atom/common/options_switches.h | 1 + lib/browser/rpc-server.js | 4 ++-- lib/renderer/api/remote.js | 11 ++++++++++- 7 files changed, 31 insertions(+), 6 deletions(-) diff --git a/atom/browser/api/atom_api_web_contents.cc b/atom/browser/api/atom_api_web_contents.cc index 081a88485540..38740abd0452 100644 --- a/atom/browser/api/atom_api_web_contents.cc +++ b/atom/browser/api/atom_api_web_contents.cc @@ -1033,13 +1033,17 @@ void WebContents::NavigationEntryCommitted( details.is_same_document, details.did_replace_entry); } -int64_t WebContents::GetID() const { - int64_t process_id = web_contents()->GetRenderProcessHost()->GetID(); - int64_t routing_id = web_contents()->GetRenderViewHost()->GetRoutingID(); +int64_t WebContents::GetIDForContents(content::WebContents* web_contents) { + int64_t process_id = web_contents->GetRenderProcessHost()->GetID(); + int64_t routing_id = web_contents->GetMainFrame()->GetRoutingID(); int64_t rv = (process_id << 32) + routing_id; return rv; } +int64_t WebContents::GetID() const { + return WebContents::GetIDForContents(web_contents()); +} + int WebContents::GetProcessID() const { return web_contents()->GetRenderProcessHost()->GetID(); } diff --git a/atom/browser/api/atom_api_web_contents.h b/atom/browser/api/atom_api_web_contents.h index d94902894c57..5cd710149190 100644 --- a/atom/browser/api/atom_api_web_contents.h +++ b/atom/browser/api/atom_api_web_contents.h @@ -89,6 +89,8 @@ class WebContents : public mate::TrackableObject, static void BuildPrototype(v8::Isolate* isolate, v8::Local prototype); + static int64_t GetIDForContents(content::WebContents* web_contents); + // Notifies to destroy any guest web contents before destroying self. void DestroyWebContents(bool async); diff --git a/atom/browser/atom_browser_client.cc b/atom/browser/atom_browser_client.cc index 8b7915bae9c9..b8a844a6886d 100644 --- a/atom/browser/atom_browser_client.cc +++ b/atom/browser/atom_browser_client.cc @@ -10,6 +10,7 @@ #include "atom/browser/api/atom_api_app.h" #include "atom/browser/api/atom_api_protocol.h" +#include "atom/browser/api/atom_api_web_contents.h" #include "atom/browser/atom_browser_context.h" #include "atom/browser/atom_browser_main_parts.h" #include "atom/browser/atom_quota_permission_context.h" @@ -322,6 +323,11 @@ void AtomBrowserClient::AppendExtraCommandLineSwitches( web_contents, command_line); SessionPreferences::AppendExtraCommandLineSwitches( web_contents->GetBrowserContext(), command_line); + + auto context_id = atom::api::WebContents::GetIDForContents( + web_contents); + command_line->AppendSwitchASCII(switches::kContextId, + base::IntToString(context_id)); } } diff --git a/atom/common/options_switches.cc b/atom/common/options_switches.cc index eed6e7bc092f..c1cd185443c0 100644 --- a/atom/common/options_switches.cc +++ b/atom/common/options_switches.cc @@ -178,6 +178,9 @@ const char kAppUserModelId[] = "app-user-model-id"; // The application path const char kAppPath[] = "app-path"; +// The context ID for this process +const char kContextId[] = "context-id"; + // The command line switch versions of the options. const char kBackgroundColor[] = "background-color"; const char kPreloadScript[] = "preload"; diff --git a/atom/common/options_switches.h b/atom/common/options_switches.h index 525301971c8a..9d65bc376925 100644 --- a/atom/common/options_switches.h +++ b/atom/common/options_switches.h @@ -88,6 +88,7 @@ extern const char kRegisterServiceWorkerSchemes[]; extern const char kSecureSchemes[]; extern const char kAppUserModelId[]; extern const char kAppPath[]; +extern const char kContextId[]; extern const char kBackgroundColor[]; extern const char kPreloadScript[]; diff --git a/lib/browser/rpc-server.js b/lib/browser/rpc-server.js index 633360db1e27..20f62980c955 100644 --- a/lib/browser/rpc-server.js +++ b/lib/browser/rpc-server.js @@ -394,8 +394,8 @@ ipcMain.on('ELECTRON_BROWSER_DEREFERENCE', function (event, id) { objectsRegistry.remove(event.sender.getId(), id) }) -ipcMain.on('ELECTRON_BROWSER_CONTEXT_RELEASE', (e) => { - objectsRegistry.clear(e.sender.getId()) +ipcMain.on('ELECTRON_BROWSER_CONTEXT_RELEASE', (e, contextId) => { + objectsRegistry.clear(contextId) e.returnValue = null }) diff --git a/lib/renderer/api/remote.js b/lib/renderer/api/remote.js index a4f8072ed576..3de99ecab255 100644 --- a/lib/renderer/api/remote.js +++ b/lib/renderer/api/remote.js @@ -268,7 +268,7 @@ ipcRenderer.on('ELECTRON_RENDERER_RELEASE_CALLBACK', (event, id) => { process.on('exit', () => { const command = 'ELECTRON_BROWSER_CONTEXT_RELEASE' - ipcRenderer.sendSync(command) + ipcRenderer.sendSync(command, initialContext) }) exports.require = (module) => { @@ -295,6 +295,15 @@ exports.getCurrentWebContents = () => { return metaToValue(ipcRenderer.sendSync('ELECTRON_BROWSER_CURRENT_WEB_CONTENTS')) } +const CONTEXT_ARG = '--context-id=' +let initialContext = process.argv.find(arg => arg.startsWith(CONTEXT_ARG)) +if (initialContext) { + initialContext = parseInt(initialContext.substr(CONTEXT_ARG.length), 10) +} else { + // In sandbox we need to pull this from remote + initialContext = exports.getCurrentWebContents().getId() +} + // Get a global object in browser. exports.getGlobal = (name) => { const command = 'ELECTRON_BROWSER_GLOBAL'