From 10d39f673a30dc5499f76fcf29132764b3c992cd Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Thu, 25 Aug 2016 09:10:37 -0700 Subject: [PATCH 1/2] Add failing spec for duplicate references over IPC --- spec/api-ipc-spec.js | 16 ++++++++++++++++ spec/static/main.js | 4 ++-- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/spec/api-ipc-spec.js b/spec/api-ipc-spec.js index ff91e70b2136..1af3ce6634ea 100644 --- a/spec/api-ipc-spec.js +++ b/spec/api-ipc-spec.js @@ -325,6 +325,22 @@ describe('ipc module', function () { }) ipcRenderer.send('message', document.location) }) + + it('can send objects that both reference the same object', function (done) { + const child = {hello: 'world'} + const foo = {name: 'foo', child: child} + const bar = {name: 'bar', child: child} + const array = [foo, bar] + + ipcRenderer.once('message', function (event, arrayValue, fooValue, barValue, childValue) { + assert.deepEqual(arrayValue, array) + assert.deepEqual(fooValue, foo) + assert.deepEqual(barValue, bar) + assert.deepEqual(childValue, child) + done() + }) + ipcRenderer.send('message', array, foo, bar, child) + }) }) describe('ipc.sendSync', function () { diff --git a/spec/static/main.js b/spec/static/main.js index cefb79f6c2ab..967cd02b7310 100644 --- a/spec/static/main.js +++ b/spec/static/main.js @@ -35,8 +35,8 @@ process.stdout // Access console to reproduce #3482. console -ipcMain.on('message', function (event, arg) { - event.sender.send('message', arg) +ipcMain.on('message', function (event, ...args) { + event.sender.send('message', ...args) }) // Write output to file if OUTPUT_TO_FILE is defined. From 50e2e26e4f2bd8878298d76da435ad84cea5c1fa Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Thu, 25 Aug 2016 09:26:07 -0700 Subject: [PATCH 2/2] Improve cycle detection in V8ValueConverter --- .../v8_value_converter.cc | 82 +++++++++++++++---- .../v8_value_converter.h | 1 + 2 files changed, 68 insertions(+), 15 deletions(-) diff --git a/atom/common/native_mate_converters/v8_value_converter.cc b/atom/common/native_mate_converters/v8_value_converter.cc index eb3ff3c8c32b..1f4ff7ccd3a1 100644 --- a/atom/common/native_mate_converters/v8_value_converter.cc +++ b/atom/common/native_mate_converters/v8_value_converter.cc @@ -49,33 +49,83 @@ class V8ValueConverter::FromV8ValueState { // other handle B in the map points to the same object as A. Note that A can // be unique even if there already is another handle with the same identity // hash (key) in the map, because two objects can have the same hash. - bool UpdateAndCheckUniqueness(v8::Local handle) { - typedef HashToHandleMap::const_iterator Iterator; - int hash = handle->GetIdentityHash(); - // We only compare using == with handles to objects with the same identity - // hash. Different hash obviously means different objects, but two objects - // in a couple of thousands could have the same identity hash. - std::pair range = unique_map_.equal_range(hash); - for (auto it = range.first; it != range.second; ++it) { - // Operator == for handles actually compares the underlying objects. - if (it->second == handle) - return false; - } + bool AddToUniquenessCheck(v8::Local handle) { + int hash; + auto iter = GetIteratorInMap(handle, &hash); + if (iter != unique_map_.end()) + return false; + unique_map_.insert(std::make_pair(hash, handle)); return true; } + bool RemoveFromUniquenessCheck(v8::Local handle) { + int unused_hash; + auto iter = GetIteratorInMap(handle, &unused_hash); + if (iter == unique_map_.end()) + return false; + unique_map_.erase(iter); + return true; + } + bool HasReachedMaxRecursionDepth() { return max_recursion_depth_ < 0; } private: - typedef std::multimap > HashToHandleMap; + using HashToHandleMap = std::multimap>; + using Iterator = HashToHandleMap::const_iterator; + + Iterator GetIteratorInMap(v8::Local handle, int* hash) { + *hash = handle->GetIdentityHash(); + // We only compare using == with handles to objects with the same identity + // hash. Different hash obviously means different objects, but two objects + // in a couple of thousands could have the same identity hash. + std::pair range = unique_map_.equal_range(*hash); + for (auto it = range.first; it != range.second; ++it) { + // Operator == for handles actually compares the underlying objects. + if (it->second == handle) + return it; + } + // Not found. + return unique_map_.end(); + } + HashToHandleMap unique_map_; int max_recursion_depth_; }; +// A class to ensure that objects/arrays that are being converted by +// this V8ValueConverterImpl do not have cycles. +// +// An example of cycle: var v = {}; v = {key: v}; +// Not an example of cycle: var v = {}; a = [v, v]; or w = {a: v, b: v}; +class V8ValueConverter::ScopedUniquenessGuard { + public: + ScopedUniquenessGuard(V8ValueConverter::FromV8ValueState* state, + v8::Local value) + : state_(state), + value_(value), + is_valid_(state_->AddToUniquenessCheck(value_)) {} + ~ScopedUniquenessGuard() { + if (is_valid_) { + bool removed = state_->RemoveFromUniquenessCheck(value_); + DCHECK(removed); + } + } + + bool is_valid() const { return is_valid_; } + + private: + typedef std::multimap > HashToHandleMap; + V8ValueConverter::FromV8ValueState* state_; + v8::Local value_; + bool is_valid_; + + DISALLOW_COPY_AND_ASSIGN(ScopedUniquenessGuard); +}; + V8ValueConverter::V8ValueConverter() : reg_exp_allowed_(false), function_allowed_(false), @@ -281,7 +331,8 @@ base::Value* V8ValueConverter::FromV8Array( v8::Local val, FromV8ValueState* state, v8::Isolate* isolate) const { - if (!state->UpdateAndCheckUniqueness(val)) + ScopedUniquenessGuard uniqueness_guard(state, val); + if (!uniqueness_guard.is_valid()) return base::Value::CreateNullValue().release(); std::unique_ptr scope; @@ -328,7 +379,8 @@ base::Value* V8ValueConverter::FromV8Object( v8::Local val, FromV8ValueState* state, v8::Isolate* isolate) const { - if (!state->UpdateAndCheckUniqueness(val)) + ScopedUniquenessGuard uniqueness_guard(state, val); + if (!uniqueness_guard.is_valid()) return base::Value::CreateNullValue().release(); std::unique_ptr scope; diff --git a/atom/common/native_mate_converters/v8_value_converter.h b/atom/common/native_mate_converters/v8_value_converter.h index 632587022d14..0e5c4d9f5a41 100644 --- a/atom/common/native_mate_converters/v8_value_converter.h +++ b/atom/common/native_mate_converters/v8_value_converter.h @@ -32,6 +32,7 @@ class V8ValueConverter { private: class FromV8ValueState; + class ScopedUniquenessGuard; v8::Local ToV8ValueImpl(v8::Isolate* isolate, const base::Value* value) const;