Merge pull request #6442 from electron/circular-references

Maintain stack of visited objects for cycle detection

lib/renderer/api/remote.js

@@ -5,34 +5,31 @@ 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)) {
-      return true
-    }
-    visited.push(field)
-  }
-  return false
-}
+const remoteObjectCache = v8Util.createIDWeakMap()
 
 // 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) {
+    // Check for circular reference.
+    if (visited.has(value)) {
       return {
+        type: 'value',
+        value: null
+      }
+    }
+
+    if (Array.isArray(value)) {
+      visited.add(value)
+      let meta = {
         type: 'array',
         value: wrapArgs(value, visited)
       }
+      visited.delete(value)
+      return meta
     } else if (Buffer.isBuffer(value)) {
       return {
         type: 'buffer',
@@ -58,19 +55,20 @@ 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({
+      visited.add(value)
+      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',
@@ -95,7 +93,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
 
@@ -142,7 +140,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)
@@ -151,7 +149,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':
@@ -220,7 +218,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) {
@@ -292,8 +290,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)
@@ -302,7 +299,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)
 }

spec/api-ipc-spec.js

@@ -44,6 +44,58 @@ 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 arrayA = ['foo']
+      var arrayB = [arrayA, 'bar']
+      arrayA.push(arrayB)
+      assert.deepEqual(a.returnArgs(arrayA, arrayB), [
+        ['foo', [null, 'bar']],
+        [['foo', null], 'bar']
+      ])
+
+      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]}
+      ])
+
+      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), [
+        [null]
+      ])
+
+      objectA = {}
+      objectA.foo = objectA
+      objectA.bar = 'baz'
+      assert.deepEqual(a.returnArgs(objectA), [
+        {foo: null, bar: 'baz'}
+      ])
+
+      objectA = {}
+      objectA.foo = {bar: objectA}
+      objectA.bar = 'baz'
+      assert.deepEqual(a.returnArgs(objectA), [
+        {foo: {bar: null}, bar: 'baz'}
+      ])
+    })
   })
 
   describe('remote.createFunctionWithReturnValue', function () {

spec/fixtures/module/circular.js

@@ -0,0 +1,3 @@
+exports.returnArgs = function (...args) {
+  return args
+}