From 06cf0406fe8ddcef3a500a689952c77fdfb9c9a8 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Tue, 26 Apr 2016 16:10:27 +0900 Subject: [PATCH] Dereference remote objects with native code Previously we rely on the v8util.setDestructor to dereference the remote objects in JavaScript, however as documented in V8, it is forbidden to call V8 APIs in object's destructor (e.g. the weak callback), and doing so would result in crashs. This commit removes the JavaScript setDestructor method, and avoids doing the dereference work with V8. --- atom/common/api/atom_api_v8_util.cc | 13 ++-- atom/common/api/object_life_monitor.cc | 29 +++----- atom/common/api/object_life_monitor.h | 16 ++--- atom/common/api/remote_callback_freer.cc | 70 +++++++++++++++++++ atom/common/api/remote_callback_freer.h | 44 ++++++++++++ atom/common/api/remote_object_freer.cc | 63 +++++++++++++++++ atom/common/api/remote_object_freer.h | 32 +++++++++ .../content_converter.cc | 13 ++++ .../content_converter.h | 2 + filenames.gypi | 4 ++ lib/browser/rpc-server.js | 43 +++++++----- lib/renderer/api/remote.js | 4 +- 12 files changed, 272 insertions(+), 61 deletions(-) create mode 100644 atom/common/api/remote_callback_freer.cc create mode 100644 atom/common/api/remote_callback_freer.h create mode 100644 atom/common/api/remote_object_freer.cc create mode 100644 atom/common/api/remote_object_freer.h diff --git a/atom/common/api/atom_api_v8_util.cc b/atom/common/api/atom_api_v8_util.cc index 0ebd939398f..109f9f0f36c 100644 --- a/atom/common/api/atom_api_v8_util.cc +++ b/atom/common/api/atom_api_v8_util.cc @@ -4,7 +4,9 @@ #include -#include "atom/common/api/object_life_monitor.h" +#include "atom/common/api/remote_callback_freer.h" +#include "atom/common/api/remote_object_freer.h" +#include "atom/common/native_mate_converters/content_converter.h" #include "atom/common/node_includes.h" #include "native_mate/dictionary.h" #include "v8/include/v8-profiler.h" @@ -51,12 +53,6 @@ int32_t GetObjectHash(v8::Local object) { return object->GetIdentityHash(); } -void SetDestructor(v8::Isolate* isolate, - v8::Local object, - v8::Local callback) { - atom::ObjectLifeMonitor::BindTo(isolate, object, callback); -} - void TakeHeapSnapshot(v8::Isolate* isolate) { isolate->GetHeapProfiler()->TakeHeapSnapshot(); } @@ -68,8 +64,9 @@ void Initialize(v8::Local exports, v8::Local unused, dict.SetMethod("setHiddenValue", &SetHiddenValue); dict.SetMethod("deleteHiddenValue", &DeleteHiddenValue); dict.SetMethod("getObjectHash", &GetObjectHash); - dict.SetMethod("setDestructor", &SetDestructor); dict.SetMethod("takeHeapSnapshot", &TakeHeapSnapshot); + dict.SetMethod("setRemoteCallbackFreer", &atom::RemoteCallbackFreer::BindTo); + dict.SetMethod("setRemoteObjectFreer", &atom::RemoteObjectFreer::BindTo); } } // namespace diff --git a/atom/common/api/object_life_monitor.cc b/atom/common/api/object_life_monitor.cc index 916ad8a5177..ffcc0d71842 100644 --- a/atom/common/api/object_life_monitor.cc +++ b/atom/common/api/object_life_monitor.cc @@ -10,30 +10,28 @@ namespace atom { -// static -void ObjectLifeMonitor::BindTo(v8::Isolate* isolate, - v8::Local target, - v8::Local destructor) { - new ObjectLifeMonitor(isolate, target, destructor); -} - ObjectLifeMonitor::ObjectLifeMonitor(v8::Isolate* isolate, - v8::Local target, - v8::Local destructor) + v8::Local target) : isolate_(isolate), context_(isolate, isolate->GetCurrentContext()), target_(isolate, target), - destructor_(isolate, destructor), weak_ptr_factory_(this) { target_.SetWeak(this, OnObjectGC, v8::WeakCallbackType::kParameter); } +ObjectLifeMonitor::~ObjectLifeMonitor() { + if (target_.IsEmpty()) + return; + target_.ClearWeak(); + target_.Reset(); +} + // static void ObjectLifeMonitor::OnObjectGC( const v8::WeakCallbackInfo& data) { ObjectLifeMonitor* self = data.GetParameter(); self->target_.Reset(); - self->RunCallback(); + self->RunDestructor(); data.SetSecondPassCallback(Free); } @@ -43,13 +41,4 @@ void ObjectLifeMonitor::Free( delete data.GetParameter(); } -void ObjectLifeMonitor::RunCallback() { - v8::HandleScope handle_scope(isolate_); - v8::Local context = v8::Local::New( - isolate_, context_); - v8::Context::Scope context_scope(context); - v8::Local::New(isolate_, destructor_)->Call( - context->Global(), 0, nullptr); -} - } // namespace atom diff --git a/atom/common/api/object_life_monitor.h b/atom/common/api/object_life_monitor.h index 82d923fcedb..59d5fdb5cff 100644 --- a/atom/common/api/object_life_monitor.h +++ b/atom/common/api/object_life_monitor.h @@ -12,25 +12,19 @@ namespace atom { class ObjectLifeMonitor { - public: - static void BindTo(v8::Isolate* isolate, - v8::Local target, - v8::Local destructor); + protected: + ObjectLifeMonitor(v8::Isolate* isolate, v8::Local target); + virtual ~ObjectLifeMonitor(); + + virtual void RunDestructor() = 0; private: - ObjectLifeMonitor(v8::Isolate* isolate, - v8::Local target, - v8::Local destructor); - static void OnObjectGC(const v8::WeakCallbackInfo& data); static void Free(const v8::WeakCallbackInfo& data); - void RunCallback(); - v8::Isolate* isolate_; v8::Global context_; v8::Global target_; - v8::Global destructor_; base::WeakPtrFactory weak_ptr_factory_; diff --git a/atom/common/api/remote_callback_freer.cc b/atom/common/api/remote_callback_freer.cc new file mode 100644 index 00000000000..24d2897ae75 --- /dev/null +++ b/atom/common/api/remote_callback_freer.cc @@ -0,0 +1,70 @@ +// Copyright (c) 2016 GitHub, Inc. +// Use of this source code is governed by the MIT license that can be +// found in the LICENSE file. + +#include "atom/common/api/remote_callback_freer.h" + +#include "atom/common/api/api_messages.h" +#include "base/strings/utf_string_conversions.h" +#include "base/values.h" +#include "content/public/browser/render_process_host.h" +#include "content/public/browser/web_contents.h" + +namespace atom { + +// static +void RemoteCallbackFreer::BindTo(v8::Isolate* isolate, + v8::Local target, + int object_id, + content::WebContents* web_contents) { + new RemoteCallbackFreer(isolate, target, object_id, web_contents); +} + +RemoteCallbackFreer::RemoteCallbackFreer(v8::Isolate* isolate, + v8::Local target, + int object_id, + content::WebContents* web_contents) + : ObjectLifeMonitor(isolate, target), + content::WebContentsObserver(web_contents), + web_contents_(web_contents), + renderer_process_id_(GetRendererProcessID()), + object_id_(object_id) { +} + +RemoteCallbackFreer::~RemoteCallbackFreer() { +} + +void RemoteCallbackFreer::RunDestructor() { + if (!web_contents_) + return; + + if (renderer_process_id_ == GetRendererProcessID()) { + base::string16 channel = + base::ASCIIToUTF16("ELECTRON_RENDERER_RELEASE_CALLBACK"); + base::ListValue args; + args.AppendInteger(object_id_); + Send(new AtomViewMsg_Message(routing_id(), channel, args)); + } + web_contents_ = nullptr; +} + +void RemoteCallbackFreer::WebContentsDestroyed() { + if (!web_contents_) + return; + + web_contents_ = nullptr; + delete this; +} + +int RemoteCallbackFreer::GetRendererProcessID() { + if (!web_contents_) + return -1; + + auto process = web_contents()->GetRenderProcessHost(); + if (!process) + return -1; + + return process->GetID(); +} + +} // namespace atom diff --git a/atom/common/api/remote_callback_freer.h b/atom/common/api/remote_callback_freer.h new file mode 100644 index 00000000000..5c160c2d8ec --- /dev/null +++ b/atom/common/api/remote_callback_freer.h @@ -0,0 +1,44 @@ +// Copyright (c) 2016 GitHub, Inc. +// Use of this source code is governed by the MIT license that can be +// found in the LICENSE file. + +#ifndef ATOM_COMMON_API_REMOTE_CALLBACK_FREER_H_ +#define ATOM_COMMON_API_REMOTE_CALLBACK_FREER_H_ +#include "atom/common/api/object_life_monitor.h" +#include "content/public/browser/web_contents_observer.h" + +namespace atom { + +class RemoteCallbackFreer : public ObjectLifeMonitor, + public content::WebContentsObserver { + public: + static void BindTo(v8::Isolate* isolate, + v8::Local target, + int object_id, + content::WebContents* web_conents); + + protected: + RemoteCallbackFreer(v8::Isolate* isolate, + v8::Local target, + int object_id, + content::WebContents* web_conents); + ~RemoteCallbackFreer() override; + + void RunDestructor() override; + + // content::WebContentsObserver: + void WebContentsDestroyed() override; + + private: + int GetRendererProcessID(); + + content::WebContents* web_contents_; + int renderer_process_id_; + int object_id_; + + DISALLOW_COPY_AND_ASSIGN(RemoteCallbackFreer); +}; + +} // namespace atom + +#endif // ATOM_COMMON_API_REMOTE_CALLBACK_FREER_H_ diff --git a/atom/common/api/remote_object_freer.cc b/atom/common/api/remote_object_freer.cc new file mode 100644 index 00000000000..1762f1d1e06 --- /dev/null +++ b/atom/common/api/remote_object_freer.cc @@ -0,0 +1,63 @@ +// Copyright (c) 2016 GitHub, Inc. +// Use of this source code is governed by the MIT license that can be +// found in the LICENSE file. + +#include "atom/common/api/remote_object_freer.h" + +#include "atom/common/api/api_messages.h" +#include "base/strings/utf_string_conversions.h" +#include "base/values.h" +#include "content/public/renderer/render_view.h" +#include "third_party/WebKit/public/web/WebLocalFrame.h" +#include "third_party/WebKit/public/web/WebView.h" + +using blink::WebLocalFrame; +using blink::WebView; + +namespace atom { + +namespace { + +content::RenderView* GetCurrentRenderView() { + WebLocalFrame* frame = WebLocalFrame::frameForCurrentContext(); + if (!frame) + return nullptr; + + WebView* view = frame->view(); + if (!view) + return nullptr; // can happen during closing. + + return content::RenderView::FromWebView(view); +} + +} // namespace + +// static +void RemoteObjectFreer::BindTo( + v8::Isolate* isolate, v8::Local target, int object_id) { + new RemoteObjectFreer(isolate, target, object_id); +} + +RemoteObjectFreer::RemoteObjectFreer( + v8::Isolate* isolate, v8::Local target, int object_id) + : ObjectLifeMonitor(isolate, target), + object_id_(object_id) { +} + +RemoteObjectFreer::~RemoteObjectFreer() { +} + +void RemoteObjectFreer::RunDestructor() { + content::RenderView* render_view = GetCurrentRenderView(); + if (!render_view) + return; + + base::string16 channel = base::ASCIIToUTF16("ipc-message"); + base::ListValue args; + args.AppendString("ELECTRON_BROWSER_DEREFERENCE"); + args.AppendInteger(object_id_); + render_view->Send( + new AtomViewHostMsg_Message(render_view->GetRoutingID(), channel, args)); +} + +} // namespace atom diff --git a/atom/common/api/remote_object_freer.h b/atom/common/api/remote_object_freer.h new file mode 100644 index 00000000000..c2b5d8b7d30 --- /dev/null +++ b/atom/common/api/remote_object_freer.h @@ -0,0 +1,32 @@ +// Copyright (c) 2016 GitHub, Inc. +// Use of this source code is governed by the MIT license that can be +// found in the LICENSE file. + +#ifndef ATOM_COMMON_API_REMOTE_OBJECT_FREER_H_ +#define ATOM_COMMON_API_REMOTE_OBJECT_FREER_H_ + +#include "atom/common/api/object_life_monitor.h" + +namespace atom { + +class RemoteObjectFreer : public ObjectLifeMonitor { + public: + static void BindTo( + v8::Isolate* isolate, v8::Local target, int object_id); + + protected: + RemoteObjectFreer( + v8::Isolate* isolate, v8::Local target, int object_id); + ~RemoteObjectFreer() override; + + void RunDestructor() override; + + private: + int object_id_; + + DISALLOW_COPY_AND_ASSIGN(RemoteObjectFreer); +}; + +} // namespace atom + +#endif // ATOM_COMMON_API_REMOTE_OBJECT_FREER_H_ diff --git a/atom/common/native_mate_converters/content_converter.cc b/atom/common/native_mate_converters/content_converter.cc index 7e7cd9bd1ff..154dcf88ce3 100644 --- a/atom/common/native_mate_converters/content_converter.cc +++ b/atom/common/native_mate_converters/content_converter.cc @@ -180,4 +180,17 @@ v8::Local Converter::ToV8( return atom::api::WebContents::CreateFrom(isolate, val).ToV8(); } +// static +bool Converter::FromV8( + v8::Isolate* isolate, + v8::Local val, + content::WebContents** out) { + atom::api::WebContents* web_contents = nullptr; + if (!ConvertFromV8(isolate, val, &web_contents) || !web_contents) + return false; + + *out = web_contents->web_contents(); + return true; +} + } // namespace mate diff --git a/atom/common/native_mate_converters/content_converter.h b/atom/common/native_mate_converters/content_converter.h index b1a42b6897c..f2e7211ce5d 100644 --- a/atom/common/native_mate_converters/content_converter.h +++ b/atom/common/native_mate_converters/content_converter.h @@ -57,6 +57,8 @@ template<> struct Converter { static v8::Local ToV8(v8::Isolate* isolate, content::WebContents* val); + static bool FromV8(v8::Isolate* isolate, v8::Local val, + content::WebContents** out); }; } // namespace mate diff --git a/filenames.gypi b/filenames.gypi index c3eca34528d..1c213949756 100644 --- a/filenames.gypi +++ b/filenames.gypi @@ -303,6 +303,10 @@ 'atom/common/api/locker.h', 'atom/common/api/object_life_monitor.cc', 'atom/common/api/object_life_monitor.h', + 'atom/common/api/remote_callback_freer.cc', + 'atom/common/api/remote_callback_freer.h', + 'atom/common/api/remote_object_freer.cc', + 'atom/common/api/remote_object_freer.h', 'atom/common/asar/archive.cc', 'atom/common/asar/archive.h', 'atom/common/asar/asar_util.cc', diff --git a/lib/browser/rpc-server.js b/lib/browser/rpc-server.js index ca193b4115e..1dff5fdb8ad 100644 --- a/lib/browser/rpc-server.js +++ b/lib/browser/rpc-server.js @@ -12,8 +12,19 @@ const FUNCTION_PROPERTIES = [ ] // The remote functions in renderer processes. -// (webContentsId) => {id: Function} -let rendererFunctions = {} +// id => Function +let rendererFunctions = new IDWeakMap() + +// Merge two IDs together. +let mergeIds = function (webContentsId, metaId) { + const PADDING_BITS = 20 + if ((webContentsId << PADDING_BITS) < 0) { + throw new Error(`webContents ID is too large: ${webContentsId}`) + } else if (metaId > (1 << PADDING_BITS)) { + throw new Error(`Object ID is too large: ${metaId}`) + } + return (webContentsId << PADDING_BITS) + metaId +} // Return the description of object's members: let getObjectMembers = function (object) { @@ -165,32 +176,26 @@ var unwrapArgs = function (sender, args) { return returnValue } case 'function': { + // Merge webContentsId and meta.id, since meta.id can be the same in + // different webContents. + const webContentsId = sender.getId() + const objectId = mergeIds(webContentsId, meta.id) + // Cache the callbacks in renderer. - let webContentsId = sender.getId() - let callbacks = rendererFunctions[webContentsId] - if (!callbacks) { - callbacks = rendererFunctions[webContentsId] = new IDWeakMap() - sender.once('render-view-deleted', function (event, id) { - callbacks.clear() - delete rendererFunctions[id] - }) + if (rendererFunctions.has(objectId)) { + return rendererFunctions.get(objectId) } - if (callbacks.has(meta.id)) return callbacks.get(meta.id) - let callIntoRenderer = function (...args) { - if ((webContentsId in rendererFunctions) && !sender.isDestroyed()) { + if (!sender.isDestroyed() && webContentsId === sender.getId()) { sender.send('ELECTRON_RENDERER_CALLBACK', meta.id, valueToMeta(sender, args)) } else { throw new Error(`Attempting to call a function in a renderer window that has been closed or released. Function provided here: ${meta.location}.`) } } - v8Util.setDestructor(callIntoRenderer, function () { - if ((webContentsId in rendererFunctions) && !sender.isDestroyed()) { - sender.send('ELECTRON_RENDERER_RELEASE_CALLBACK', meta.id) - } - }) - callbacks.set(meta.id, callIntoRenderer) + + v8Util.setRemoteCallbackFreer(callIntoRenderer, meta.id, sender) + rendererFunctions.set(objectId, callIntoRenderer) return callIntoRenderer } default: diff --git a/lib/renderer/api/remote.js b/lib/renderer/api/remote.js index d15ebd717ec..6631ea22575 100644 --- a/lib/renderer/api/remote.js +++ b/lib/renderer/api/remote.js @@ -210,9 +210,7 @@ let metaToValue = function (meta) { // Track delegate object's life time, and tell the browser to clean up // when the object is GCed. - v8Util.setDestructor(ret, function () { - ipcRenderer.send('ELECTRON_BROWSER_DEREFERENCE', meta.id) - }) + v8Util.setRemoteObjectFreer(ret, meta.id) // Remember object's id. v8Util.setHiddenValue(ret, 'atomId', meta.id)