From b5fd491c2dd60a3d5f8c527891d7fdaa72f06cdc Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Mon, 28 Dec 2015 22:31:14 +0800 Subject: [PATCH 1/6] Fix circular reference caused by RemoteMemberFunction --- atom/renderer/api/lib/remote.coffee | 38 ++++++++++++++++------------- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/atom/renderer/api/lib/remote.coffee b/atom/renderer/api/lib/remote.coffee index 2828586849f2..88e598902c1c 100644 --- a/atom/renderer/api/lib/remote.coffee +++ b/atom/renderer/api/lib/remote.coffee @@ -72,20 +72,10 @@ metaToValue = (meta) -> # Polulate delegate members. for member in meta.members - do (member) -> - if member.type is 'function' - ret[member.name] = - class RemoteMemberFunction - constructor: -> - if @constructor is RemoteMemberFunction - # Constructor call. - obj = ipcRenderer.sendSync 'ATOM_BROWSER_MEMBER_CONSTRUCTOR', meta.id, member.name, wrapArgs(arguments) - return metaToValue obj - else - # Call member function. - ret = ipcRenderer.sendSync 'ATOM_BROWSER_MEMBER_CALL', meta.id, member.name, wrapArgs(arguments) - return metaToValue ret - else + if member.type is 'function' + ret[member.name] = createRemoteMemberFunction meta.id, member.name + else + do (member) -> Object.defineProperty ret, member.name, enumerable: true, configurable: false, @@ -93,11 +83,10 @@ metaToValue = (meta) -> # Set member data. ipcRenderer.sendSync 'ATOM_BROWSER_MEMBER_SET', meta.id, member.name, value value - get: -> # Get member data. - ret = ipcRenderer.sendSync 'ATOM_BROWSER_MEMBER_GET', meta.id, member.name - metaToValue ret + val = ipcRenderer.sendSync 'ATOM_BROWSER_MEMBER_GET', meta.id, member.name + metaToValue val # Track delegate object's life time, and tell the browser to clean up # when the object is GCed. @@ -117,6 +106,21 @@ metaToPlainObject = (meta) -> obj[name] = value for {name, value} in meta.members obj +# Create a RemoteMemberFunction instance. +# This function's content should not be inlined into metaToValue, otherwise V8 +# may consider it circular reference. +createRemoteMemberFunction = (metaId, name) -> + class RemoteMemberFunction + constructor: -> + if @constructor is RemoteMemberFunction + # Constructor call. + ret = ipcRenderer.sendSync 'ATOM_BROWSER_MEMBER_CONSTRUCTOR', metaId, name, wrapArgs(arguments) + return metaToValue ret + else + # Call member function. + ret = ipcRenderer.sendSync 'ATOM_BROWSER_MEMBER_CALL', metaId, name, wrapArgs(arguments) + return metaToValue ret + # Browser calls a callback in renderer. ipcRenderer.on 'ATOM_RENDERER_CALLBACK', (event, id, args) -> callbacksRegistry.apply id, metaToValue(args) From 6785870ddef7e364fb166239acf1b0a721afe5e9 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Mon, 28 Dec 2015 22:32:07 +0800 Subject: [PATCH 2/6] Variables are not shadowed in inline class --- atom/renderer/api/lib/remote.coffee | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/atom/renderer/api/lib/remote.coffee b/atom/renderer/api/lib/remote.coffee index 88e598902c1c..716d3be5fdb6 100644 --- a/atom/renderer/api/lib/remote.coffee +++ b/atom/renderer/api/lib/remote.coffee @@ -65,8 +65,8 @@ metaToValue = (meta) -> return metaToValue obj else # Function call. - ret = ipcRenderer.sendSync 'ATOM_BROWSER_FUNCTION_CALL', meta.id, wrapArgs(arguments) - return metaToValue ret + obj = ipcRenderer.sendSync 'ATOM_BROWSER_FUNCTION_CALL', meta.id, wrapArgs(arguments) + return metaToValue obj else ret = v8Util.createObjectWithName meta.name From 3d2163230b4b43b420d72b4073fd2ab8b35389d5 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Mon, 28 Dec 2015 22:51:01 +0800 Subject: [PATCH 3/6] Optimize the case when creating plain object --- atom/common/api/atom_api_v8_util.cc | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/atom/common/api/atom_api_v8_util.cc b/atom/common/api/atom_api_v8_util.cc index bba3399a8dbd..dfa65ab8ecae 100644 --- a/atom/common/api/atom_api_v8_util.cc +++ b/atom/common/api/atom_api_v8_util.cc @@ -10,14 +10,16 @@ namespace { v8::Local CreateObjectWithName(v8::Isolate* isolate, - v8::Local name) { + const std::string& name) { + if (name == "Object") + return v8::Object::New(isolate); v8::Local t = v8::FunctionTemplate::New(isolate); - t->SetClassName(name); + t->SetClassName(mate::StringToV8(isolate, name)); return t->GetFunction()->NewInstance(); } v8::Local GetHiddenValue(v8::Local object, - v8::Local key) { + v8::Local key) { return object->GetHiddenValue(key); } From ffc2870ccb6767378f0eb188a642d23bf00c2939 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Tue, 29 Dec 2015 10:17:35 +0800 Subject: [PATCH 4/6] Fix circular reference caused by Object.defineProperty --- atom/renderer/api/lib/remote.coffee | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/atom/renderer/api/lib/remote.coffee b/atom/renderer/api/lib/remote.coffee index 716d3be5fdb6..3ed44278bd71 100644 --- a/atom/renderer/api/lib/remote.coffee +++ b/atom/renderer/api/lib/remote.coffee @@ -75,18 +75,7 @@ metaToValue = (meta) -> if member.type is 'function' ret[member.name] = createRemoteMemberFunction meta.id, member.name else - do (member) -> - Object.defineProperty ret, member.name, - enumerable: true, - configurable: false, - set: (value) -> - # Set member data. - ipcRenderer.sendSync 'ATOM_BROWSER_MEMBER_SET', meta.id, member.name, value - value - get: -> - # Get member data. - val = ipcRenderer.sendSync 'ATOM_BROWSER_MEMBER_GET', meta.id, member.name - metaToValue val + Object.defineProperty ret, member.name, createRemoteMemberProperty(meta.id, member.name) # Track delegate object's life time, and tell the browser to clean up # when the object is GCed. @@ -121,6 +110,20 @@ createRemoteMemberFunction = (metaId, name) -> ret = ipcRenderer.sendSync 'ATOM_BROWSER_MEMBER_CALL', metaId, name, wrapArgs(arguments) return metaToValue ret +# Create configuration for defineProperty. +# This function's content should not be inlined into metaToValue, otherwise V8 +# may consider it circular reference. +createRemoteMemberProperty = (metaId, name) -> + enumerable: true, + configurable: false, + set: (value) -> + # Set member data. + ipcRenderer.sendSync 'ATOM_BROWSER_MEMBER_SET', metaId, name, value + value + get: -> + # Get member data. + metaToValue ipcRenderer.sendSync('ATOM_BROWSER_MEMBER_GET', metaId, name) + # Browser calls a callback in renderer. ipcRenderer.on 'ATOM_RENDERER_CALLBACK', (event, id, args) -> callbacksRegistry.apply id, metaToValue(args) From d8d963b780c3f1dd3e5f01f189014650f61be0c2 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Tue, 29 Dec 2015 10:29:48 +0800 Subject: [PATCH 5/6] Cache function templates created by CreateObjectWithName --- atom/common/api/atom_api_v8_util.cc | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/atom/common/api/atom_api_v8_util.cc b/atom/common/api/atom_api_v8_util.cc index dfa65ab8ecae..7b9490df6a8b 100644 --- a/atom/common/api/atom_api_v8_util.cc +++ b/atom/common/api/atom_api_v8_util.cc @@ -2,19 +2,35 @@ // Use of this source code is governed by the MIT license that can be // found in the LICENSE file. +#include +#include + #include "atom/common/api/object_life_monitor.h" #include "atom/common/node_includes.h" +#include "base/stl_util.h" #include "native_mate/dictionary.h" #include "v8/include/v8-profiler.h" namespace { +using FunctionTemplateHandle = + v8::CopyablePersistentTraits::CopyablePersistent; + +// The handles are leaked on purpose. +std::map function_templates_; + v8::Local CreateObjectWithName(v8::Isolate* isolate, const std::string& name) { if (name == "Object") return v8::Object::New(isolate); + + if (ContainsKey(function_templates_, name)) + return v8::Local::New( + isolate, function_templates_[name])->GetFunction()->NewInstance(); + v8::Local t = v8::FunctionTemplate::New(isolate); t->SetClassName(mate::StringToV8(isolate, name)); + function_templates_[name] = FunctionTemplateHandle(isolate, t); return t->GetFunction()->NewInstance(); } From 2294a5ce693d01453bdb2424ac5d0238a24f235f Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Tue, 29 Dec 2015 10:40:10 +0800 Subject: [PATCH 6/6] Leak FunctionTemplateHandle They are cached through the app's lifetime, and freeing them at the right time is complicate, so just leak them. --- atom/common/api/atom_api_v8_util.cc | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/atom/common/api/atom_api_v8_util.cc b/atom/common/api/atom_api_v8_util.cc index 7b9490df6a8b..c86335adb15e 100644 --- a/atom/common/api/atom_api_v8_util.cc +++ b/atom/common/api/atom_api_v8_util.cc @@ -13,10 +13,21 @@ namespace { -using FunctionTemplateHandle = - v8::CopyablePersistentTraits::CopyablePersistent; +// A Persistent that can be copied and will not free itself. +template +struct LeakedPersistentTraits { + typedef v8::Persistent > LeakedPersistent; + static const bool kResetInDestructor = false; + template + static V8_INLINE void Copy(const v8::Persistent& source, + LeakedPersistent* dest) { + // do nothing, just allow copy + } +}; // The handles are leaked on purpose. +using FunctionTemplateHandle = + LeakedPersistentTraits::LeakedPersistent; std::map function_templates_; v8::Local CreateObjectWithName(v8::Isolate* isolate,