fix: intercept the isolate_holder's new isolate and register it with the node platform before initialization

Chromium Change: https://chromium-review.googlesource.com/c/chromium/src/+/1015020

atom/browser/atom_browser_main_parts.cc

@@ -10,7 +10,6 @@
 #include "atom/browser/api/trackable_object.h"
 #include "atom/browser/atom_browser_client.h"
 #include "atom/browser/atom_browser_context.h"
-#include "atom/browser/bridge_task_runner.h"
 #include "atom/browser/browser.h"
 #include "atom/browser/io_thread.h"
 #include "atom/browser/javascript_environment.h"
@@ -135,11 +134,9 @@ int AtomBrowserMainParts::PreEarlyInitialization() {
 void AtomBrowserMainParts::PostEarlyInitialization() {
   brightray::BrowserMainParts::PostEarlyInitialization();
 
-  // Temporary set the bridge_task_runner_ as current thread's task runner,
+  // A workaround was previously needed because there was no ThreadTaskRunner
-  // so we can fool gin::PerIsolateData to use it as its task runner, instead
+  // set.  If this check is failing we may need to re-add that workaround
-  // of getting current message loop's task runner, which is null for now.
+  DCHECK(base::ThreadTaskRunnerHandle::IsSet());
-  bridge_task_runner_ = new BridgeTaskRunner;
-  base::ThreadTaskRunnerHandle handle(bridge_task_runner_);
 
   // The ProxyResolverV8 has setup a complete V8 environment, in order to
   // avoid conflicts we only initialize our V8 environment after that.
@@ -243,8 +240,6 @@ void AtomBrowserMainParts::PreMainMessageLoopRun() {
 #endif  // BUILDFLAG(ENABLE_PDF_VIEWER)
 
   brightray::BrowserMainParts::PreMainMessageLoopRun();
-  bridge_task_runner_->MessageLoopIsReady();
-  bridge_task_runner_ = nullptr;
 
 #if defined(USE_X11)
   libgtkui::GtkInitFromCommandLine(*base::CommandLine::ForCurrentProcess());

atom/browser/atom_browser_main_parts.h

@@ -108,10 +108,6 @@ class AtomBrowserMainParts : public brightray::BrowserMainParts {
   // A fake BrowserProcess object that used to feed the source code from chrome.
   std::unique_ptr<BrowserProcess> fake_browser_process_;
 
-  // The gin::PerIsolateData requires a task runner to create, so we feed it
-  // with a task runner that will post all work to main loop.
-  scoped_refptr<BridgeTaskRunner> bridge_task_runner_;
-
   // Pointer to exit code.
   int* exit_code_ = nullptr;
 

atom/browser/bridge_task_runner.cc

@@ -1,65 +0,0 @@
-// Copyright (c) 2015 GitHub, Inc.
-// Use of this source code is governed by the MIT license that can be
-// found in the LICENSE file.
-
-#include "atom/browser/bridge_task_runner.h"
-
-#include <utility>
-
-#include "base/message_loop/message_loop.h"
-
-namespace atom {
-
-BridgeTaskRunner::BridgeTaskRunner() = default;
-BridgeTaskRunner::~BridgeTaskRunner() = default;
-
-void BridgeTaskRunner::MessageLoopIsReady() {
-  auto message_loop = base::MessageLoop::current();
-  CHECK(message_loop);
-  for (TaskPair& task : tasks_) {
-    message_loop->task_runner()->PostDelayedTask(
-        std::get<0>(task), std::move(std::get<1>(task)), std::get<2>(task));
-  }
-  for (TaskPair& task : non_nestable_tasks_) {
-    message_loop->task_runner()->PostNonNestableDelayedTask(
-        std::get<0>(task), std::move(std::get<1>(task)), std::get<2>(task));
-  }
-}
-
-bool BridgeTaskRunner::PostDelayedTask(const base::Location& from_here,
-                                       base::OnceClosure task,
-                                       base::TimeDelta delay) {
-  auto message_loop = base::MessageLoop::current();
-  if (!message_loop) {
-    tasks_.push_back(std::make_tuple(from_here, std::move(task), delay));
-    return true;
-  }
-
-  return message_loop->task_runner()->PostDelayedTask(from_here,
-                                                      std::move(task), delay);
-}
-
-bool BridgeTaskRunner::RunsTasksInCurrentSequence() const {
-  auto message_loop = base::MessageLoop::current();
-  if (!message_loop)
-    return true;
-
-  return message_loop->task_runner()->RunsTasksInCurrentSequence();
-}
-
-bool BridgeTaskRunner::PostNonNestableDelayedTask(
-    const base::Location& from_here,
-    base::OnceClosure task,
-    base::TimeDelta delay) {
-  auto message_loop = base::MessageLoop::current();
-  if (!message_loop) {
-    non_nestable_tasks_.push_back(
-        std::make_tuple(from_here, std::move(task), delay));
-    return true;
-  }
-
-  return message_loop->task_runner()->PostNonNestableDelayedTask(
-      from_here, std::move(task), delay);
-}
-
-}  // namespace atom

atom/browser/bridge_task_runner.h

@@ -1,48 +0,0 @@
-// Copyright (c) 2015 GitHub, Inc.
-// Use of this source code is governed by the MIT license that can be
-// found in the LICENSE file.
-
-#ifndef ATOM_BROWSER_BRIDGE_TASK_RUNNER_H_
-#define ATOM_BROWSER_BRIDGE_TASK_RUNNER_H_
-
-#include <tuple>
-#include <vector>
-
-#include "base/location.h"
-#include "base/single_thread_task_runner.h"
-#include "base/tuple.h"
-
-namespace atom {
-
-// Post all tasks to the current message loop's task runner if available,
-// otherwise delay the work until message loop is ready.
-class BridgeTaskRunner : public base::SingleThreadTaskRunner {
- public:
-  BridgeTaskRunner();
-
-  // Called when message loop is ready.
-  void MessageLoopIsReady();
-
-  // base::SingleThreadTaskRunner:
-  bool PostDelayedTask(const base::Location& from_here,
-                       base::OnceClosure task,
-                       base::TimeDelta delay) override;
-  bool RunsTasksInCurrentSequence() const override;
-  bool PostNonNestableDelayedTask(const base::Location& from_here,
-                                  base::OnceClosure task,
-                                  base::TimeDelta delay) override;
-
- private:
-  using TaskPair =
-      std::tuple<base::Location, base::OnceClosure, base::TimeDelta>;
-  ~BridgeTaskRunner() override;
-
-  std::vector<TaskPair> tasks_;
-  std::vector<TaskPair> non_nestable_tasks_;
-
-  DISALLOW_COPY_AND_ASSIGN(BridgeTaskRunner);
-};
-
-}  // namespace atom
-
-#endif  // ATOM_BROWSER_BRIDGE_TASK_RUNNER_H_

atom/browser/javascript_environment.cc

@@ -21,13 +21,19 @@ namespace atom {
 
 JavascriptEnvironment::JavascriptEnvironment()
     : initialized_(Initialize()),
-      isolate_holder_(base::ThreadTaskRunnerHandle::Get()),
+      isolate_holder_(base::ThreadTaskRunnerHandle::Get(),
+                      new AtomIsolateAllocator(this)),
       isolate_(isolate_holder_.isolate()),
       isolate_scope_(isolate_),
       locker_(isolate_),
       handle_scope_(isolate_),
       context_(isolate_, v8::Context::New(isolate_)),
-      context_scope_(v8::Local<v8::Context>::New(isolate_, context_)) {}
+      context_scope_(v8::Local<v8::Context>::New(isolate_, context_)) {
+  // The assumption is made here that the isolate_holder_ holds a single isolate
+  // if this is not the case or changes in the constructor above the logic
+  // below must be rewritten
+  node::InitializePrivatePropertiesOnIsolateData(isolate_data_);
+}
 
 JavascriptEnvironment::~JavascriptEnvironment() = default;
 
@@ -64,4 +70,16 @@ NodeEnvironment::~NodeEnvironment() {
   node::FreeEnvironment(env_);
 }
 
+AtomIsolateAllocator::AtomIsolateAllocator(JavascriptEnvironment* env)
+    : env_(env) {}
+
+v8::Isolate* AtomIsolateAllocator::Allocate() {
+  v8::Isolate* isolate = v8::Isolate::Allocate();
+  // This is a cheatsy way to add the Isolate and it's IsolateData to the node
+  // platform before it is ready
+  env_->set_isolate_data(node::CreateIsolateData(
+      isolate, uv_default_loop(), env_->platform(), true /* only_register */));
+  return isolate;
+}
+
 }  // namespace atom

atom/browser/javascript_environment.h

@@ -11,6 +11,7 @@
 namespace node {
 class Environment;
 class MultiIsolatePlatform;
+class IsolateData;
 }  // namespace node
 
 namespace atom {
@@ -29,6 +30,7 @@ class JavascriptEnvironment {
   v8::Local<v8::Context> context() const {
     return v8::Local<v8::Context>::New(isolate_, context_);
   }
+  void set_isolate_data(node::IsolateData* data) { isolate_data_ = data; }
 
  private:
   bool Initialize();
@@ -44,6 +46,7 @@ class JavascriptEnvironment {
   v8::HandleScope handle_scope_;
   v8::UniquePersistent<v8::Context> context_;
   v8::Context::Scope context_scope_;
+  node::IsolateData* isolate_data_;
 
   DISALLOW_COPY_AND_ASSIGN(JavascriptEnvironment);
 };
@@ -60,6 +63,18 @@ class NodeEnvironment {
   DISALLOW_COPY_AND_ASSIGN(NodeEnvironment);
 };
 
+class AtomIsolateAllocator : public gin::IsolateAllocater {
+ public:
+  explicit AtomIsolateAllocator(JavascriptEnvironment* env);
+
+  v8::Isolate* Allocate() override;
+
+ private:
+  JavascriptEnvironment* env_;
+
+  DISALLOW_COPY_AND_ASSIGN(AtomIsolateAllocator);
+};
+
 }  // namespace atom
 
 #endif  // ATOM_BROWSER_JAVASCRIPT_ENVIRONMENT_H_

filenames.gni

@@ -230,8 +230,6 @@ filenames = {
     "atom/browser/atom_speech_recognition_manager_delegate.h",
     "atom/browser/atom_web_ui_controller_factory.cc",
     "atom/browser/atom_web_ui_controller_factory.h",
-    "atom/browser/bridge_task_runner.cc",
-    "atom/browser/bridge_task_runner.h",
     "atom/browser/browser.cc",
     "atom/browser/browser.h",
     "atom/browser/browser_linux.cc",

patches/common/chromium/.patches.yaml

@@ -463,3 +463,10 @@ patches:
   file: gritsettings_resource_ids.patch
   description: |
     Add electron resources file to the list of resource ids generation.
+-
+  author: Samuel Attard <samuel.r.attard@gmail.com>
+  file: isolate_holder.patch
+  description: |
+    Expose a hook that allows embedders to intercept allocated isolates
+    before they are initialized or used so they can be added to the correct
+    platform.  (In our case, node_platform)

patches/common/chromium/isolate_holder.patch

@@ -0,0 +1,94 @@
+diff --git a/gin/isolate_holder.cc b/gin/isolate_holder.cc
+index e099fd3f03e5..46b5ea629d3a 100644
+--- a/gin/isolate_holder.cc
++++ b/gin/isolate_holder.cc
+@@ -30,23 +30,31 @@ v8::ArrayBuffer::Allocator* g_array_buffer_allocator = nullptr;
+ const intptr_t* g_reference_table = nullptr;
+ }  // namespace
+ 
++v8::Isolate* IsolateAllocater::Allocate() {
++    return nullptr;
++}
++
+ IsolateHolder::IsolateHolder(
+-    scoped_refptr<base::SingleThreadTaskRunner> task_runner)
+-    : IsolateHolder(std::move(task_runner), AccessMode::kSingleThread) {}
++    scoped_refptr<base::SingleThreadTaskRunner> task_runner,
++    IsolateAllocater* isolate_allocator)
++    : IsolateHolder(std::move(task_runner), AccessMode::kSingleThread, isolate_allocator) {}
+ 
+ IsolateHolder::IsolateHolder(
+     scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+-    AccessMode access_mode)
++    AccessMode access_mode,
++    IsolateAllocater* isolate_allocator)
+     : IsolateHolder(std::move(task_runner),
+                     access_mode,
+                     kAllowAtomicsWait,
+-                    IsolateCreationMode::kNormal) {}
++                    IsolateCreationMode::kNormal,
++                    isolate_allocator) {}
+ 
+ IsolateHolder::IsolateHolder(
+     scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+     AccessMode access_mode,
+     AllowAtomicsWaitMode atomics_wait_mode,
+-    IsolateCreationMode isolate_creation_mode)
++    IsolateCreationMode isolate_creation_mode,
++    IsolateAllocater* isolate_allocator)
+     : access_mode_(access_mode) {
+   DCHECK(task_runner);
+   DCHECK(task_runner->BelongsToCurrentThread());
+@@ -54,7 +62,11 @@ IsolateHolder::IsolateHolder(
+   v8::ArrayBuffer::Allocator* allocator = g_array_buffer_allocator;
+   CHECK(allocator) << "You need to invoke gin::IsolateHolder::Initialize first";
+ 
+-  isolate_ = v8::Isolate::Allocate();
++  if (isolate_allocator) {
++    isolate_ = isolate_allocator->Allocate();
++  } else {
++    isolate_ = v8::Isolate::Allocate();
++  }
+   isolate_data_.reset(
+       new PerIsolateData(isolate_, allocator, access_mode_, task_runner));
+   if (isolate_creation_mode == IsolateCreationMode::kCreateSnapshot) {
+diff --git a/gin/public/isolate_holder.h b/gin/public/isolate_holder.h
+index 84cf66e6e9cd..2d5ebb76f050 100644
+--- a/gin/public/isolate_holder.h
++++ b/gin/public/isolate_holder.h
+@@ -22,6 +22,14 @@ namespace gin {
+ class PerIsolateData;
+ class V8IsolateMemoryDumpProvider;
+ 
++class GIN_EXPORT IsolateAllocater {
++  public:
++    explicit IsolateAllocater() {};
++    virtual ~IsolateAllocater() {};
++
++    virtual v8::Isolate* Allocate();
++};
++
+ // To embed Gin, first initialize gin using IsolateHolder::Initialize and then
+ // create an instance of IsolateHolder to hold the v8::Isolate in which you
+ // will execute JavaScript. You might wish to subclass IsolateHolder if you
+@@ -59,14 +67,17 @@ class GIN_EXPORT IsolateHolder {
+   };
+ 
+   explicit IsolateHolder(
+-      scoped_refptr<base::SingleThreadTaskRunner> task_runner);
++      scoped_refptr<base::SingleThreadTaskRunner> task_runner,
++      IsolateAllocater* isolate_allocator = nullptr);
+   IsolateHolder(scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+-                AccessMode access_mode);
++                AccessMode access_mode,
++                IsolateAllocater* isolate_allocator = nullptr);
+   IsolateHolder(
+       scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+       AccessMode access_mode,
+       AllowAtomicsWaitMode atomics_wait_mode,
+-      IsolateCreationMode isolate_creation_mode = IsolateCreationMode::kNormal);
++      IsolateCreationMode isolate_creation_mode = IsolateCreationMode::kNormal,
++      IsolateAllocater* isolate_allocator = nullptr);
+   ~IsolateHolder();
+ 
+   // Should be invoked once before creating IsolateHolder instances to