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

zcbenz · nornagon · commit 2af6d9ced713 · 2018-07-19T11:35:49.000-07:00
For sandboxed renderer it may not have a node::Environment in the context,
using a increasing counter as contextId works for all cases.
diff --git a/atom/common/api/atom_api_v8_util.cc b/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();
 }
@@ -130,7 +110,6 @@ void Initialize(v8::Local<v8::Object> exports, v8::Local<v8::Value> unused,
   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);
diff --git a/atom/common/context_counter.cc b/atom/common/context_counter.cc
@@ -0,0 +1,19 @@
+// Copyright (c) 2018 GitHub, Inc.
+// Use of this source code is governed by the MIT license that can be
+// found in the LICENSE file.
+
+#include "atom/common/context_counter.h"
+
+namespace atom {
+
+namespace {
+
+int g_context_id = 0;
+
+}  // namespace
+
+int GetNextContextId() {
+  return ++g_context_id;
+}
+
+}  // namespace atom
diff --git a/atom/common/context_counter.h b/atom/common/context_counter.h
@@ -0,0 +1,15 @@
+// Copyright (c) 2018 GitHub, Inc.
+// Use of this source code is governed by the MIT license that can be
+// found in the LICENSE file.
+
+#ifndef ATOM_COMMON_CONTEXT_COUNTER_H_
+#define ATOM_COMMON_CONTEXT_COUNTER_H_
+
+namespace atom {
+
+// Increase the context counter, and return current count.
+int GetNextContextId();
+
+}  // namespace atom
+
+#endif  // ATOM_COMMON_CONTEXT_COUNTER_H_
diff --git a/atom/renderer/atom_renderer_client.cc b/atom/renderer/atom_renderer_client.cc
@@ -82,6 +82,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))
diff --git a/atom/renderer/atom_sandboxed_renderer_client.cc b/atom/renderer/atom_sandboxed_renderer_client.cc
@@ -153,6 +153,7 @@ 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
diff --git a/atom/renderer/renderer_client_base.cc b/atom/renderer/renderer_client_base.cc
@@ -9,6 +9,7 @@
 
 #include "atom/common/atom_constants.h"
 #include "atom/common/color_util.h"
+#include "atom/common/context_counter.h"
 #include "atom/common/native_mate_converters/value_converter.h"
 #include "atom/common/options_switches.h"
 #include "atom/renderer/atom_autofill_agent.h"
@@ -19,7 +20,9 @@
 #include "atom/renderer/preferences_manager.h"
 #include "base/command_line.h"
 #include "base/memory/ptr_util.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"
@@ -44,6 +47,14 @@
 #include <shlobj.h>
 #endif
 
+// 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 {
@@ -78,6 +89,19 @@ RendererClientBase::RendererClientBase() {
 RendererClientBase::~RendererClientBase() {
 }
 
+void RendererClientBase::DidCreateScriptContext(
+    v8::Handle<v8::Context> context,
+    content::RenderFrame* render_frame) {
+  // global.setHidden("contextId", `${processId}-${GetNextContextId()}`)
+  std::string context_id = base::StringPrintf(
+      "%" CrPRIdPid "-%d", base::GetCurrentProcId(), GetNextContextId());
+  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) {
diff --git a/atom/renderer/renderer_client_base.h b/atom/renderer/renderer_client_base.h
@@ -21,7 +21,7 @@ class RendererClientBase : public content::ContentRendererClient {
   virtual ~RendererClientBase();
 
   virtual void DidCreateScriptContext(
-      v8::Handle<v8::Context> context, content::RenderFrame* render_frame) = 0;
+      v8::Handle<v8::Context> context, content::RenderFrame* render_frame);
   virtual void WillReleaseScriptContext(
       v8::Handle<v8::Context> context, content::RenderFrame* render_frame) = 0;
   virtual void DidClearWindowObject(content::RenderFrame* render_frame);
diff --git a/filenames.gypi b/filenames.gypi
@@ -437,6 +437,8 @@
       'atom/common/color_util.h',
       'atom/common/common_message_generator.cc',
       'atom/common/common_message_generator.h',
+      'atom/common/context_counter.cc',
+      'atom/common/context_counter.h',
       'atom/common/crash_reporter/crash_reporter.cc',
       'atom/common/crash_reporter/crash_reporter.h',
       'atom/common/crash_reporter/crash_reporter_linux.cc',
diff --git a/lib/renderer/api/remote.js b/lib/renderer/api/remote.js
@@ -12,7 +12,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
