Merge pull request #13734 from electron/backport-13727-3-0-x

fix: use context counter as contextId (backport 3-0-x)

atom/common/api/atom_api_v8_util.cc

@@ -12,20 +12,10 @@
 #include "atom/common/native_mate_converters/gurl_converter.h"
 #include "atom/common/node_includes.h"
 #include "base/hash.h"
-#include "base/process/process_handle.h"
-#include "base/strings/stringprintf.h"
 #include "native_mate/dictionary.h"
 #include "url/origin.h"
 #include "v8/include/v8-profiler.h"
 
-// This is defined in later versions of Chromium, remove this if you see
-// compiler complaining duplicate defines.
-#if defined(OS_WIN) || defined(OS_FUCHSIA)
-#define CrPRIdPid "ld"
-#else
-#define CrPRIdPid "d"
-#endif
-
 namespace std {
 
 // The hash function used by DoubleIDWeakMap.
@@ -100,16 +90,6 @@ int32_t GetObjectHash(v8::Local<v8::Object> object) {
   return object->GetIdentityHash();
 }
 
-std::string GetContextID(v8::Isolate* isolate) {
-  // When a page is reloaded, V8 and blink may have optimizations that do not
-  // free blink::WebLocalFrame and v8::Context and reuse them for the new page,
-  // while we always recreate node::Environment when a page is loaded.
-  // So the only reliable way to return an identity for a page, is to return the
-  // address of the node::Environment instance.
-  node::Environment* env = node::Environment::GetCurrent(isolate);
-  return base::StringPrintf("%" CrPRIdPid "-%p", base::GetCurrentProcId(), env);
-}
-
 void TakeHeapSnapshot(v8::Isolate* isolate) {
   isolate->GetHeapProfiler()->TakeHeapSnapshot();
 }
@@ -132,7 +112,6 @@ void Initialize(v8::Local<v8::Object> exports,
   dict.SetMethod("setHiddenValue", &SetHiddenValue);
   dict.SetMethod("deleteHiddenValue", &DeleteHiddenValue);
   dict.SetMethod("getObjectHash", &GetObjectHash);
-  dict.SetMethod("getContextId", &GetContextID);
   dict.SetMethod("takeHeapSnapshot", &TakeHeapSnapshot);
   dict.SetMethod("setRemoteCallbackFreer", &atom::RemoteCallbackFreer::BindTo);
   dict.SetMethod("setRemoteObjectFreer", &atom::RemoteObjectFreer::BindTo);

atom/renderer/atom_renderer_client.cc

@@ -79,6 +79,8 @@ void AtomRendererClient::RunScriptsAtDocumentEnd(
 void AtomRendererClient::DidCreateScriptContext(
     v8::Handle<v8::Context> context,
     content::RenderFrame* render_frame) {
+  RendererClientBase::DidCreateScriptContext(context, render_frame);
+
   // Only allow node integration for the main frame, unless it is a devtools
   // extension page.
   if (!render_frame->IsMainFrame() && !IsDevToolsExtension(render_frame))

atom/renderer/atom_sandboxed_renderer_client.cc

@@ -97,7 +97,7 @@ void InitializeBindings(v8::Local<v8::Object> binding,
   base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
   if (command_line->HasSwitch(switches::kGuestInstanceID))
     b.Set(options::kGuestInstanceID,
-        command_line->GetSwitchValueASCII(switches::kGuestInstanceID));
+          command_line->GetSwitchValueASCII(switches::kGuestInstanceID));
 }
 
 class AtomSandboxedRenderFrameObserver : public AtomRenderFrameObserver {
@@ -153,6 +153,8 @@ void AtomSandboxedRendererClient::RenderViewCreated(
 void AtomSandboxedRendererClient::DidCreateScriptContext(
     v8::Handle<v8::Context> context,
     content::RenderFrame* render_frame) {
+  RendererClientBase::DidCreateScriptContext(context, render_frame);
+
   // Only allow preload for the main frame or
   // For devtools we still want to run the preload_bundle script
   if (!render_frame->IsMainFrame() && !IsDevTools(render_frame))

atom/renderer/renderer_client_base.cc

@@ -17,7 +17,9 @@
 #include "atom/renderer/guest_view_container.h"
 #include "atom/renderer/preferences_manager.h"
 #include "base/command_line.h"
+#include "base/process/process_handle.h"
 #include "base/strings/string_split.h"
+#include "base/strings/stringprintf.h"
 #include "chrome/renderer/media/chrome_key_systems.h"
 #include "chrome/renderer/pepper/pepper_helper.h"
 #include "chrome/renderer/printing/print_web_view_helper.h"
@@ -46,6 +48,14 @@
 #include "atom/common/atom_constants.h"
 #endif  // defined(ENABLE_PDF_VIEWER)
 
+// This is defined in later versions of Chromium, remove this if you see
+// compiler complaining duplicate defines.
+#if defined(OS_WIN) || defined(OS_FUCHSIA)
+#define CrPRIdPid "ld"
+#else
+#define CrPRIdPid "d"
+#endif
+
 namespace atom {
 
 namespace {
@@ -80,6 +90,19 @@ RendererClientBase::RendererClientBase() {
 
 RendererClientBase::~RendererClientBase() {}
 
+void RendererClientBase::DidCreateScriptContext(
+    v8::Handle<v8::Context> context,
+    content::RenderFrame* render_frame) {
+  // global.setHidden("contextId", `${processId}-${++nextContextId}`)
+  std::string context_id = base::StringPrintf(
+      "%" CrPRIdPid "-%d", base::GetCurrentProcId(), ++next_context_id_);
+  v8::Isolate* isolate = context->GetIsolate();
+  v8::Local<v8::String> key = mate::StringToSymbol(isolate, "contextId");
+  v8::Local<v8::Private> private_key = v8::Private::ForApi(isolate, key);
+  v8::Local<v8::Value> value = mate::ConvertToV8(isolate, context_id);
+  context->Global()->SetPrivate(context, private_key, value);
+}
+
 void RendererClientBase::AddRenderBindings(
     v8::Isolate* isolate,
     v8::Local<v8::Object> binding_object) {

atom/renderer/renderer_client_base.h

@@ -21,7 +21,7 @@ class RendererClientBase : public content::ContentRendererClient {
   ~RendererClientBase() override;
 
   virtual void DidCreateScriptContext(v8::Handle<v8::Context> context,
-                                      content::RenderFrame* render_frame) = 0;
+                                      content::RenderFrame* render_frame);
   virtual void WillReleaseScriptContext(v8::Handle<v8::Context> context,
                                         content::RenderFrame* render_frame) = 0;
   virtual void DidClearWindowObject(content::RenderFrame* render_frame);
@@ -57,6 +57,9 @@ class RendererClientBase : public content::ContentRendererClient {
  private:
   std::unique_ptr<PreferencesManager> preferences_manager_;
   bool isolated_world_;
+
+  // An increasing ID used for indentifying an V8 context in this process.
+  int next_context_id_ = 0;
 };
 
 }  // namespace atom

lib/browser/objects-registry.js

@@ -11,7 +11,7 @@ class ObjectsRegistry {
     this.storage = {}
 
     // Stores the IDs of objects referenced by WebContents.
-    // (webContentsId) => [id]
+    // (webContentsContextId) => [id]
     this.owners = {}
   }
 
@@ -22,9 +22,10 @@ class ObjectsRegistry {
     const id = this.saveToStorage(obj)
 
     // Add object to the set of referenced objects.
-    let owner = this.owners[contextId]
+    const webContentsContextId = `${webContents.id}-${contextId}`
+    let owner = this.owners[webContentsContextId]
     if (!owner) {
-      owner = this.owners[contextId] = new Set()
+      owner = this.owners[webContentsContextId] = new Set()
       this.registerDeleteListener(webContents, contextId)
     }
     if (!owner.has(id)) {
@@ -44,8 +45,9 @@ class ObjectsRegistry {
   // Dereference an object according to its ID.
   // Note that an object may be double-freed (cleared when page is reloaded, and
   // then garbage collected in old page).
-  remove (contextId, id) {
-    let owner = this.owners[contextId]
+  remove (webContents, contextId, id) {
+    const webContentsContextId = `${webContents.id}-${contextId}`
+    let owner = this.owners[webContentsContextId]
     if (owner) {
       // Remove the reference in owner.
       owner.delete(id)
@@ -55,13 +57,14 @@ class ObjectsRegistry {
   }
 
   // Clear all references to objects refrenced by the WebContents.
-  clear (contextId) {
-    let owner = this.owners[contextId]
+  clear (webContents, contextId) {
+    const webContentsContextId = `${webContents.id}-${contextId}`
+    let owner = this.owners[webContentsContextId]
     if (!owner) return
 
     for (let id of owner) this.dereference(id)
 
-    delete this.owners[contextId]
+    delete this.owners[webContentsContextId]
   }
 
   // Private: Saves the object into storage and assigns an ID for it.
@@ -91,13 +94,13 @@ class ObjectsRegistry {
     }
   }
 
-  // Private: Clear the storage when webContents is reloaded/navigated.
+  // Private: Clear the storage when renderer process is destoryed.
   registerDeleteListener (webContents, contextId) {
     const processId = webContents.getProcessId()
     const listener = (event, deletedProcessId) => {
       if (deletedProcessId === processId) {
         webContents.removeListener('render-view-deleted', listener)
-        this.clear(contextId)
+        this.clear(webContents, contextId)
       }
     }
     webContents.on('render-view-deleted', listener)

lib/browser/rpc-server.js

@@ -394,12 +394,12 @@ ipcMain.on('ELECTRON_BROWSER_MEMBER_GET', function (event, contextId, id, name)
 })
 
 ipcMain.on('ELECTRON_BROWSER_DEREFERENCE', function (event, contextId, id) {
-  objectsRegistry.remove(contextId, id)
+  objectsRegistry.remove(event.sender, contextId, id)
 })
 
-ipcMain.on('ELECTRON_BROWSER_CONTEXT_RELEASE', (e, contextId) => {
-  objectsRegistry.clear(contextId)
-  e.returnValue = null
+ipcMain.on('ELECTRON_BROWSER_CONTEXT_RELEASE', (event, contextId) => {
+  objectsRegistry.clear(event.sender, contextId)
+  event.returnValue = null
 })
 
 ipcMain.on('ELECTRON_BROWSER_GUEST_WEB_CONTENTS', function (event, contextId, guestInstanceId) {

lib/renderer/api/remote.js

@@ -10,7 +10,7 @@ const callbacksRegistry = new CallbacksRegistry()
 const remoteObjectCache = v8Util.createIDWeakMap()
 
 // An unique ID that can represent current context.
-const contextId = v8Util.getContextId()
+const contextId = v8Util.getHiddenValue(global, 'contextId')
 
 // Notify the main process when current context is going to be released.
 // Note that when the renderer process is destroyed, the message may not be

lib/renderer/web-view/web-view.js

@@ -9,7 +9,7 @@ const webViewConstants = require('./web-view-constants')
 const hasProp = {}.hasOwnProperty
 
 // An unique ID that can represent current context.
-const contextId = v8Util.getContextId()
+const contextId = v8Util.getHiddenValue(global, 'contextId')
 
 // ID generator.
 let nextId = 0