From 7589c56cea0a115c92a64602caf8cd8a5f0585d2 Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Wed, 3 Oct 2018 17:06:40 +1000 Subject: [PATCH] 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 | 11 +-- atom/browser/atom_browser_main_parts.h | 4 - atom/browser/bridge_task_runner.cc | 65 -------------- atom/browser/bridge_task_runner.h | 48 ---------- atom/browser/javascript_environment.cc | 22 ++++- atom/browser/javascript_environment.h | 15 ++++ filenames.gni | 2 - patches/common/chromium/.patches.yaml | 7 ++ patches/common/chromium/isolate_holder.patch | 94 ++++++++++++++++++++ 9 files changed, 139 insertions(+), 129 deletions(-) delete mode 100644 atom/browser/bridge_task_runner.cc delete mode 100644 atom/browser/bridge_task_runner.h create mode 100644 patches/common/chromium/isolate_holder.patch diff --git a/atom/browser/atom_browser_main_parts.cc b/atom/browser/atom_browser_main_parts.cc index 581cd39cd3f8..05b743eee9f7 100644 --- a/atom/browser/atom_browser_main_parts.cc +++ b/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, - // so we can fool gin::PerIsolateData to use it as its task runner, instead - // of getting current message loop's task runner, which is null for now. - bridge_task_runner_ = new BridgeTaskRunner; - base::ThreadTaskRunnerHandle handle(bridge_task_runner_); + // A workaround was previously needed because there was no ThreadTaskRunner + // set. If this check is failing we may need to re-add that workaround + DCHECK(base::ThreadTaskRunnerHandle::IsSet()); // 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()); diff --git a/atom/browser/atom_browser_main_parts.h b/atom/browser/atom_browser_main_parts.h index 67334edf4f5e..115d43cc9ea4 100644 --- a/atom/browser/atom_browser_main_parts.h +++ b/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 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 bridge_task_runner_; - // Pointer to exit code. int* exit_code_ = nullptr; diff --git a/atom/browser/bridge_task_runner.cc b/atom/browser/bridge_task_runner.cc deleted file mode 100644 index b62a5bdb83d5..000000000000 --- a/atom/browser/bridge_task_runner.cc +++ /dev/null @@ -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 - -#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 diff --git a/atom/browser/bridge_task_runner.h b/atom/browser/bridge_task_runner.h deleted file mode 100644 index 7c3049b3f8c2..000000000000 --- a/atom/browser/bridge_task_runner.h +++ /dev/null @@ -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 -#include - -#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; - ~BridgeTaskRunner() override; - - std::vector tasks_; - std::vector non_nestable_tasks_; - - DISALLOW_COPY_AND_ASSIGN(BridgeTaskRunner); -}; - -} // namespace atom - -#endif // ATOM_BROWSER_BRIDGE_TASK_RUNNER_H_ diff --git a/atom/browser/javascript_environment.cc b/atom/browser/javascript_environment.cc index 5e81c001d6c6..d6418d1c3d46 100644 --- a/atom/browser/javascript_environment.cc +++ b/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::New(isolate_, context_)) {} + context_scope_(v8::Local::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 diff --git a/atom/browser/javascript_environment.h b/atom/browser/javascript_environment.h index bf05b687f8b3..da4122c54665 100644 --- a/atom/browser/javascript_environment.h +++ b/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 context() const { return v8::Local::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 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_ diff --git a/filenames.gni b/filenames.gni index 5405492f184e..6f188e033625 100644 --- a/filenames.gni +++ b/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", diff --git a/patches/common/chromium/.patches.yaml b/patches/common/chromium/.patches.yaml index 24829e789736..7ea17f2bf788 100644 --- a/patches/common/chromium/.patches.yaml +++ b/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 + 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) diff --git a/patches/common/chromium/isolate_holder.patch b/patches/common/chromium/isolate_holder.patch new file mode 100644 index 000000000000..e4ebeabb0743 --- /dev/null +++ b/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 task_runner) +- : IsolateHolder(std::move(task_runner), AccessMode::kSingleThread) {} ++ scoped_refptr task_runner, ++ IsolateAllocater* isolate_allocator) ++ : IsolateHolder(std::move(task_runner), AccessMode::kSingleThread, isolate_allocator) {} + + IsolateHolder::IsolateHolder( + scoped_refptr 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 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 task_runner); ++ scoped_refptr task_runner, ++ IsolateAllocater* isolate_allocator = nullptr); + IsolateHolder(scoped_refptr task_runner, +- AccessMode access_mode); ++ AccessMode access_mode, ++ IsolateAllocater* isolate_allocator = nullptr); + IsolateHolder( + scoped_refptr 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