From b942c54bea59336726e45032d628ccd02dc7a215 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 4 Jan 2017 14:24:49 -0800 Subject: [PATCH 1/5] Use closeWindow helper --- spec/chromium-spec.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/chromium-spec.js b/spec/chromium-spec.js index 5d30181c2e74..88088e089b26 100644 --- a/spec/chromium-spec.js +++ b/spec/chromium-spec.js @@ -46,7 +46,7 @@ describe('chromium feature', function () { var w = null afterEach(function () { - w != null ? w.destroy() : void 0 + return closeWindow(w).then(() => w = null) }) it('is set correctly when window is not shown', function (done) { @@ -157,7 +157,7 @@ describe('chromium feature', function () { var w = null afterEach(function () { - w != null ? w.destroy() : void 0 + return closeWindow(w).then(() => w = null) }) it('should register for file scheme', function (done) { @@ -312,7 +312,7 @@ describe('chromium feature', function () { let w = null afterEach(function () { - if (w) w.destroy() + return closeWindow(w).then(() => w = null) }) it('is null for main window', function (done) { From 12382f064bcb17fdd229418a90014c5e11ea1085 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 4 Jan 2017 14:44:14 -0800 Subject: [PATCH 2/5] Add failing spec for cycle in options --- spec/chromium-spec.js | 22 +++++++++++++++++++++- spec/fixtures/pages/window-open.html | 2 +- spec/static/main.js | 10 ++++++++++ 3 files changed, 32 insertions(+), 2 deletions(-) diff --git a/spec/chromium-spec.js b/spec/chromium-spec.js index 88088e089b26..039fc1e9371b 100644 --- a/spec/chromium-spec.js +++ b/spec/chromium-spec.js @@ -3,7 +3,7 @@ const http = require('http') const path = require('path') const ws = require('ws') const url = require('url') -const remote = require('electron').remote +const {ipcRenderer, remote} = require('electron') const {closeWindow} = require('./window-helpers') const {BrowserWindow, ipcMain, protocol, session, webContents} = remote @@ -187,6 +187,12 @@ describe('chromium feature', function () { return } + let w = null + + afterEach(() => { + return closeWindow(w).then(() => w = null) + }) + it('returns a BrowserWindowProxy object', function () { var b = window.open('about:blank', '', 'show=no') assert.equal(b.closed, false) @@ -260,6 +266,20 @@ describe('chromium feature', function () { b = window.open('file://' + fixtures + '/pages/window-open-size.html', '', 'show=no,width=' + size.width + ',height=' + size.height) }) + it('handles cycles when merging the parent options into the child options', (done) => { + w = BrowserWindow.fromId(ipcRenderer.sendSync('create-window-with-options-cycle')) + w.loadURL('file://' + fixtures + '/pages/window-open.html') + w.webContents.once('new-window', (event, url, frameName, disposition, options) => { + assert.deepEqual(options, { + show: false, + foo: { + bar: null + } + }) + done() + }) + }) + it('defines a window.location getter', function (done) { var b, targetURL if (process.platform === 'win32') { diff --git a/spec/fixtures/pages/window-open.html b/spec/fixtures/pages/window-open.html index b7af009cd2b7..06bd393c0a03 100644 --- a/spec/fixtures/pages/window-open.html +++ b/spec/fixtures/pages/window-open.html @@ -1,7 +1,7 @@ diff --git a/spec/static/main.js b/spec/static/main.js index 259350f9598f..aad8a64d80ed 100644 --- a/spec/static/main.js +++ b/spec/static/main.js @@ -230,3 +230,13 @@ ipcMain.on('close-on-will-navigate', (event, id) => { contents.send('closed-on-will-navigate') }) }) + + +ipcMain.on('create-window-with-options-cycle', (event) => { + // This can't be done over remote since cycles are already + // nulled out at the IPC layer + const foo = {} + foo.bar = foo + const window = new BrowserWindow({show: false, foo: foo}) + event.returnValue = window.id +}) From 1944fdc962c58b3bb8d0cd0cceff8996b416c8a2 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 4 Jan 2017 14:50:05 -0800 Subject: [PATCH 3/5] Track visited parents and null out cycles --- lib/browser/guest-window-manager.js | 26 ++++++++++++++++---------- spec/chromium-spec.js | 11 +++++++---- spec/static/main.js | 5 +++++ 3 files changed, 28 insertions(+), 14 deletions(-) diff --git a/lib/browser/guest-window-manager.js b/lib/browser/guest-window-manager.js index 8212e5094aad..c0a5efb262a0 100644 --- a/lib/browser/guest-window-manager.js +++ b/lib/browser/guest-window-manager.js @@ -7,19 +7,25 @@ const hasProp = {}.hasOwnProperty const frameToGuest = {} // Copy attribute of |parent| to |child| if it is not defined in |child|. -const mergeOptions = function (child, parent) { - let key, value - for (key in parent) { +const mergeOptions = function (child, parent, visited) { + // Check for circular reference. + if (visited == null) visited = new Set() + if (visited.has(parent)) return + + visited.add(parent) + for (const key in parent) { if (!hasProp.call(parent, key)) continue - value = parent[key] - if (!(key in child)) { - if (typeof value === 'object') { - child[key] = mergeOptions({}, value) - } else { - child[key] = value - } + if (key in child) continue + + const value = parent[key] + if (typeof value === 'object') { + child[key] = mergeOptions({}, value, visited) + } else { + child[key] = value } } + visited.delete(parent) + return child } diff --git a/spec/chromium-spec.js b/spec/chromium-spec.js index 039fc1e9371b..e663dcccfeca 100644 --- a/spec/chromium-spec.js +++ b/spec/chromium-spec.js @@ -270,10 +270,13 @@ describe('chromium feature', function () { w = BrowserWindow.fromId(ipcRenderer.sendSync('create-window-with-options-cycle')) w.loadURL('file://' + fixtures + '/pages/window-open.html') w.webContents.once('new-window', (event, url, frameName, disposition, options) => { - assert.deepEqual(options, { - show: false, - foo: { - bar: null + assert.equal(options.show, false) + assert.deepEqual(options.foo, { + bar: null, + baz: { + hello: { + world: true + } } }) done() diff --git a/spec/static/main.js b/spec/static/main.js index aad8a64d80ed..9b9f6c08275a 100644 --- a/spec/static/main.js +++ b/spec/static/main.js @@ -237,6 +237,11 @@ ipcMain.on('create-window-with-options-cycle', (event) => { // nulled out at the IPC layer const foo = {} foo.bar = foo + foo.baz = { + hello: { + world: true + } + } const window = new BrowserWindow({show: false, foo: foo}) event.returnValue = window.id }) From fd23c7bf766ade7ff4173071fc662308d3a36af1 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 4 Jan 2017 14:54:18 -0800 Subject: [PATCH 4/5] Assert duplicate objects are supported --- spec/chromium-spec.js | 5 +++++ spec/static/main.js | 1 + 2 files changed, 6 insertions(+) diff --git a/spec/chromium-spec.js b/spec/chromium-spec.js index e663dcccfeca..8b672a62484e 100644 --- a/spec/chromium-spec.js +++ b/spec/chromium-spec.js @@ -277,6 +277,11 @@ describe('chromium feature', function () { hello: { world: true } + }, + baz2: { + hello: { + world: true + } } }) done() diff --git a/spec/static/main.js b/spec/static/main.js index 9b9f6c08275a..5713783679a3 100644 --- a/spec/static/main.js +++ b/spec/static/main.js @@ -242,6 +242,7 @@ ipcMain.on('create-window-with-options-cycle', (event) => { world: true } } + foo.baz2 = foo.baz const window = new BrowserWindow({show: false, foo: foo}) event.returnValue = window.id }) From 1f07977f091ef9b23e75d333ac141a78364884d9 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 4 Jan 2017 14:57:51 -0800 Subject: [PATCH 5/5] Remove lint errors --- spec/chromium-spec.js | 10 +++++----- spec/static/main.js | 3 +-- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/spec/chromium-spec.js b/spec/chromium-spec.js index 8b672a62484e..58efdc745c39 100644 --- a/spec/chromium-spec.js +++ b/spec/chromium-spec.js @@ -46,7 +46,7 @@ describe('chromium feature', function () { var w = null afterEach(function () { - return closeWindow(w).then(() => w = null) + return closeWindow(w).then(function () { w = null }) }) it('is set correctly when window is not shown', function (done) { @@ -157,7 +157,7 @@ describe('chromium feature', function () { var w = null afterEach(function () { - return closeWindow(w).then(() => w = null) + return closeWindow(w).then(function () { w = null }) }) it('should register for file scheme', function (done) { @@ -190,7 +190,7 @@ describe('chromium feature', function () { let w = null afterEach(() => { - return closeWindow(w).then(() => w = null) + return closeWindow(w).then(function () { w = null }) }) it('returns a BrowserWindowProxy object', function () { @@ -269,7 +269,7 @@ describe('chromium feature', function () { it('handles cycles when merging the parent options into the child options', (done) => { w = BrowserWindow.fromId(ipcRenderer.sendSync('create-window-with-options-cycle')) w.loadURL('file://' + fixtures + '/pages/window-open.html') - w.webContents.once('new-window', (event, url, frameName, disposition, options) => { + w.webContents.once('new-window', (event, url, frameName, disposition, options) => { assert.equal(options.show, false) assert.deepEqual(options.foo, { bar: null, @@ -340,7 +340,7 @@ describe('chromium feature', function () { let w = null afterEach(function () { - return closeWindow(w).then(() => w = null) + return closeWindow(w).then(function () { w = null }) }) it('is null for main window', function (done) { diff --git a/spec/static/main.js b/spec/static/main.js index 5713783679a3..b9d7d9473c31 100644 --- a/spec/static/main.js +++ b/spec/static/main.js @@ -231,12 +231,11 @@ ipcMain.on('close-on-will-navigate', (event, id) => { }) }) - ipcMain.on('create-window-with-options-cycle', (event) => { // This can't be done over remote since cycles are already // nulled out at the IPC layer const foo = {} - foo.bar = foo + foo.bar = foo foo.baz = { hello: { world: true