From ab1786c17d4a6958f16337131f5bdc5d56c7c553 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Mon, 11 Jul 2016 09:52:34 -0700 Subject: [PATCH 1/8] Add spec for circular array/object references --- spec/api-ipc-spec.js | 19 +++++++++++++++++++ spec/fixtures/module/circular.js | 3 +++ 2 files changed, 22 insertions(+) create mode 100644 spec/fixtures/module/circular.js diff --git a/spec/api-ipc-spec.js b/spec/api-ipc-spec.js index 0d9ba0ddeefd..8f994fed81a9 100644 --- a/spec/api-ipc-spec.js +++ b/spec/api-ipc-spec.js @@ -44,6 +44,25 @@ describe('ipc module', function () { comparePaths(path.normalize(remote.process.mainModule.filename), path.resolve(__dirname, 'static', 'main.js')) comparePaths(path.normalize(remote.process.mainModule.paths[0]), path.resolve(__dirname, 'static', 'node_modules')) }) + + it('handles circular references in arrays and objects', function () { + var a = remote.require(path.join(fixtures, 'module', 'circular.js')) + var array1 = ['foo'] + var array2 = [array1, 'bar'] + array1.push(array2) + assert.deepEqual(a.returnArgs(array1, array2), [ + ['foo', [[], 'bar']], + [] + ]) + + var object1 = {foo: 'bar'} + var object2 = {baz: object1} + object1.object2 = object2 + assert.deepEqual(a.returnArgs(object1, object2), [ + {foo: 'bar', object2: {baz: {foo: 'bar', object2: null}}}, + {baz: null} + ]) + }) }) describe('remote.createFunctionWithReturnValue', function () { diff --git a/spec/fixtures/module/circular.js b/spec/fixtures/module/circular.js new file mode 100644 index 000000000000..e8629c424acd --- /dev/null +++ b/spec/fixtures/module/circular.js @@ -0,0 +1,3 @@ +exports.returnArgs = function (...args) { + return args +} From 2cc2b8a14665f9543082f42f1e07a0cddb40f0d0 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Mon, 11 Jul 2016 09:53:06 -0700 Subject: [PATCH 2/8] Return empty array for circular references --- lib/renderer/api/remote.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/renderer/api/remote.js b/lib/renderer/api/remote.js index 79fe41ed566a..d38eff719d7f 100644 --- a/lib/renderer/api/remote.js +++ b/lib/renderer/api/remote.js @@ -31,7 +31,7 @@ var wrapArgs = function (args, visited) { if (Array.isArray(value)) { return { type: 'array', - value: wrapArgs(value, visited) + value: isCircular(value, visited) ? [] : wrapArgs(value, visited) } } else if (Buffer.isBuffer(value)) { return { From 5da0b856f9618df59830e7cc9c7d562cb5aa60d6 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Mon, 11 Jul 2016 09:54:18 -0700 Subject: [PATCH 3/8] Call includes directly on visited array --- lib/renderer/api/remote.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/renderer/api/remote.js b/lib/renderer/api/remote.js index d38eff719d7f..db3b19c8480f 100644 --- a/lib/renderer/api/remote.js +++ b/lib/renderer/api/remote.js @@ -5,14 +5,12 @@ const {ipcRenderer, isPromise, CallbacksRegistry} = require('electron') const callbacksRegistry = new CallbacksRegistry() -var includes = [].includes - var remoteObjectCache = v8Util.createIDWeakMap() // Check for circular reference. var isCircular = function (field, visited) { if (typeof field === 'object') { - if (includes.call(visited, field)) { + if (visited.includes(field)) { return true } visited.push(field) From 71a8bac12aa2347a4c479d9998b3056212b9a65c Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Mon, 11 Jul 2016 10:27:43 -0700 Subject: [PATCH 4/8] Add more failing circular reference specs --- spec/api-ipc-spec.js | 49 +++++++++++++++++++++++++++++++++----------- 1 file changed, 37 insertions(+), 12 deletions(-) diff --git a/spec/api-ipc-spec.js b/spec/api-ipc-spec.js index 8f994fed81a9..7a5f31f5f1f9 100644 --- a/spec/api-ipc-spec.js +++ b/spec/api-ipc-spec.js @@ -47,20 +47,45 @@ describe('ipc module', function () { it('handles circular references in arrays and objects', function () { var a = remote.require(path.join(fixtures, 'module', 'circular.js')) - var array1 = ['foo'] - var array2 = [array1, 'bar'] - array1.push(array2) - assert.deepEqual(a.returnArgs(array1, array2), [ - ['foo', [[], 'bar']], - [] + + var arrayA = ['foo'] + var arrayB = [arrayA, 'bar'] + arrayA.push(arrayB) + assert.deepEqual(a.returnArgs(arrayA, arrayB), [ + ['foo', [null, 'bar']], + [['foo', null], 'bar'] ]) - var object1 = {foo: 'bar'} - var object2 = {baz: object1} - object1.object2 = object2 - assert.deepEqual(a.returnArgs(object1, object2), [ - {foo: 'bar', object2: {baz: {foo: 'bar', object2: null}}}, - {baz: null} + var objectA = {foo: 'bar'} + var objectB = {baz: objectA} + objectA.objectB = objectB + assert.deepEqual(a.returnArgs(objectA, objectB), [ + {foo: 'bar', objectB: {baz: null}}, + {baz: {foo: 'bar', objectB: null}} + ]) + + arrayA = [1, 2, 3] + assert.deepEqual(a.returnArgs({foo: arrayA}, {bar: arrayA}), [ + {foo: [1, 2, 3]}, + {bar: [1, 2, 3]} + ]) + + arrayA = [] + arrayA.push(arrayA) + assert.deepEqual(a.returnArgs(arrayA), [ + [null] + ]) + + var objectA = {} + objectA.foo = objectA + assert.deepEqual(a.returnArgs(objectA), [ + {foo: null} + ]) + + objectA = {} + objectA.foo = {bar: objectA} + assert.deepEqual(a.returnArgs(objectA), [ + {foo: {bar: null}} ]) }) }) From 564b0cace5f298fd307f805a20f0886e46b09e42 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Mon, 11 Jul 2016 10:28:01 -0700 Subject: [PATCH 5/8] Maintain visted objects as a stack for cycle detection --- lib/renderer/api/remote.js | 63 +++++++++++++++++++++----------------- 1 file changed, 35 insertions(+), 28 deletions(-) diff --git a/lib/renderer/api/remote.js b/lib/renderer/api/remote.js index db3b19c8480f..22326b7842f0 100644 --- a/lib/renderer/api/remote.js +++ b/lib/renderer/api/remote.js @@ -5,32 +5,41 @@ const {ipcRenderer, isPromise, CallbacksRegistry} = require('electron') const callbacksRegistry = new CallbacksRegistry() -var remoteObjectCache = v8Util.createIDWeakMap() +const remoteObjectCache = v8Util.createIDWeakMap() // Check for circular reference. -var isCircular = function (field, visited) { +const isCircular = function (field, visited) { + if (visited.has(field)) { + return true + } + if (typeof field === 'object') { - if (visited.includes(field)) { - return true - } - visited.push(field) + visited.add(field) } return false } // Convert the arguments object into an array of meta data. -var wrapArgs = function (args, visited) { - var valueToMeta +const wrapArgs = function (args, visited) { if (visited == null) { - visited = [] + visited = new Set() } - valueToMeta = function (value) { - var field, prop, ret - if (Array.isArray(value)) { + + const valueToMeta = function (value) { + if (isCircular(value, visited)) { return { - type: 'array', - value: isCircular(value, visited) ? [] : wrapArgs(value, visited) + type: 'value', + value: null } + } + + if (Array.isArray(value)) { + let meta = { + type: 'array', + value: wrapArgs(value, visited) + } + visited.delete(value) + return meta } else if (Buffer.isBuffer(value)) { return { type: 'buffer', @@ -56,19 +65,19 @@ var wrapArgs = function (args, visited) { } } - ret = { + let meta = { type: 'object', name: value.constructor != null ? value.constructor.name : '', members: [] } - for (prop in value) { - field = value[prop] - ret.members.push({ + for (let prop in value) { + meta.members.push({ name: prop, - value: valueToMeta(isCircular(field, visited) ? null : field) + value: valueToMeta(value[prop]) }) } - return ret + visited.delete(value) + return meta } else if (typeof value === 'function' && v8Util.getHiddenValue(value, 'returnValue')) { return { type: 'function-with-return-value', @@ -93,7 +102,7 @@ var wrapArgs = function (args, visited) { // Populate object's members from descriptors. // The |ref| will be kept referenced by |members|. // This matches |getObjectMemebers| in rpc-server. -let setObjectMembers = function (ref, object, metaId, members) { +const setObjectMembers = function (ref, object, metaId, members) { for (let member of members) { if (object.hasOwnProperty(member.name)) continue @@ -140,7 +149,7 @@ let setObjectMembers = function (ref, object, metaId, members) { // Populate object's prototype from descriptor. // This matches |getObjectPrototype| in rpc-server. -let setObjectPrototype = function (ref, object, metaId, descriptor) { +const setObjectPrototype = function (ref, object, metaId, descriptor) { if (descriptor === null) return let proto = {} setObjectMembers(ref, proto, metaId, descriptor.members) @@ -149,7 +158,7 @@ let setObjectPrototype = function (ref, object, metaId, descriptor) { } // Convert meta data from browser into real value. -let metaToValue = function (meta) { +const metaToValue = function (meta) { var el, i, len, ref1, results, ret switch (meta.type) { case 'value': @@ -218,7 +227,7 @@ let metaToValue = function (meta) { } // Construct a plain object from the meta. -var metaToPlainObject = function (meta) { +const metaToPlainObject = function (meta) { var i, len, obj, ref1 obj = (function () { switch (meta.type) { @@ -290,8 +299,7 @@ exports.__defineGetter__('process', function () { // Create a funtion that will return the specifed value when called in browser. exports.createFunctionWithReturnValue = function (returnValue) { - var func - func = function () { + const func = function () { return returnValue } v8Util.setHiddenValue(func, 'returnValue', true) @@ -300,7 +308,6 @@ exports.createFunctionWithReturnValue = function (returnValue) { // Get the guest WebContents from guestInstanceId. exports.getGuestWebContents = function (guestInstanceId) { - var meta - meta = ipcRenderer.sendSync('ELECTRON_BROWSER_GUEST_WEB_CONTENTS', guestInstanceId) + const meta = ipcRenderer.sendSync('ELECTRON_BROWSER_GUEST_WEB_CONTENTS', guestInstanceId) return metaToValue(meta) } From bd58e1b2c3a0db51149cb7eb32e60f9a6dc2f028 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Mon, 11 Jul 2016 10:30:18 -0700 Subject: [PATCH 6/8] Remove linter errors --- spec/api-ipc-spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/api-ipc-spec.js b/spec/api-ipc-spec.js index 7a5f31f5f1f9..9644a3dc4eed 100644 --- a/spec/api-ipc-spec.js +++ b/spec/api-ipc-spec.js @@ -76,7 +76,7 @@ describe('ipc module', function () { [null] ]) - var objectA = {} + objectA = {} objectA.foo = objectA assert.deepEqual(a.returnArgs(objectA), [ {foo: null} From 00f82aaffe11c6ad2788eadd5cf0a299dd5ea3c7 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Mon, 11 Jul 2016 10:39:21 -0700 Subject: [PATCH 7/8] Only check arrays and objects for cycles --- lib/renderer/api/remote.js | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/lib/renderer/api/remote.js b/lib/renderer/api/remote.js index 22326b7842f0..40f067f3401b 100644 --- a/lib/renderer/api/remote.js +++ b/lib/renderer/api/remote.js @@ -7,18 +7,6 @@ const callbacksRegistry = new CallbacksRegistry() const remoteObjectCache = v8Util.createIDWeakMap() -// Check for circular reference. -const isCircular = function (field, visited) { - if (visited.has(field)) { - return true - } - - if (typeof field === 'object') { - visited.add(field) - } - return false -} - // Convert the arguments object into an array of meta data. const wrapArgs = function (args, visited) { if (visited == null) { @@ -26,7 +14,8 @@ const wrapArgs = function (args, visited) { } const valueToMeta = function (value) { - if (isCircular(value, visited)) { + // Check for circular reference. + if (visited.has(value)) { return { type: 'value', value: null @@ -34,6 +23,7 @@ const wrapArgs = function (args, visited) { } if (Array.isArray(value)) { + visited.add(value) let meta = { type: 'array', value: wrapArgs(value, visited) @@ -70,6 +60,7 @@ const wrapArgs = function (args, visited) { name: value.constructor != null ? value.constructor.name : '', members: [] } + visited.add(value) for (let prop in value) { meta.members.push({ name: prop, From 2d9391f7d76d207cc5a63b9995e8e522616f2a61 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Mon, 11 Jul 2016 10:49:23 -0700 Subject: [PATCH 8/8] Add more cycle tests --- spec/api-ipc-spec.js | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/spec/api-ipc-spec.js b/spec/api-ipc-spec.js index 9644a3dc4eed..bd1a7e3d86b7 100644 --- a/spec/api-ipc-spec.js +++ b/spec/api-ipc-spec.js @@ -70,6 +70,12 @@ describe('ipc module', function () { {bar: [1, 2, 3]} ]) + objectA = {foo: 'bar'} + assert.deepEqual(a.returnArgs({foo: objectA}, {bar: objectA}), [ + {foo: {foo: 'bar'}}, + {bar: {foo: 'bar'}} + ]) + arrayA = [] arrayA.push(arrayA) assert.deepEqual(a.returnArgs(arrayA), [ @@ -78,14 +84,16 @@ describe('ipc module', function () { objectA = {} objectA.foo = objectA + objectA.bar = 'baz' assert.deepEqual(a.returnArgs(objectA), [ - {foo: null} + {foo: null, bar: 'baz'} ]) objectA = {} objectA.foo = {bar: objectA} + objectA.bar = 'baz' assert.deepEqual(a.returnArgs(objectA), [ - {foo: {bar: null}} + {foo: {bar: null}, bar: 'baz'} ]) }) })