From 6b5bd3b6ceb9457b091171bbde40700be71c66dd Mon Sep 17 00:00:00 2001 From: Thiago de Arruda Date: Mon, 15 May 2017 08:57:23 -0300 Subject: [PATCH 1/2] Fix how rpc-server releases references after page reload In addition to listening for "render-view-deleted", listen for "ELECTRON_BROWSER_CONTEXT_RELEASE" synchronous message, which is sent by the remote module when the page is about to be navigated. This is required to allow child windows running in the same renderer to correctly manage remote object references, since `render-view-deleted` is only called when the renderer exits. Close #9387 --- lib/browser/rpc-server.js | 5 +++++ lib/renderer/api/remote.js | 4 ++++ 2 files changed, 9 insertions(+) diff --git a/lib/browser/rpc-server.js b/lib/browser/rpc-server.js index 8d543f2d7d..815298d3d9 100644 --- a/lib/browser/rpc-server.js +++ b/lib/browser/rpc-server.js @@ -394,6 +394,11 @@ ipcMain.on('ELECTRON_BROWSER_DEREFERENCE', function (event, id) { objectsRegistry.remove(event.sender.getId(), id) }) +ipcMain.on('ELECTRON_BROWSER_CONTEXT_RELEASE', (e) => { + objectsRegistry.clear(e.sender.getId()) + e.returnValue = null +}) + ipcMain.on('ELECTRON_BROWSER_GUEST_WEB_CONTENTS', function (event, guestInstanceId) { try { let guestViewManager = require('./guest-view-manager') diff --git a/lib/renderer/api/remote.js b/lib/renderer/api/remote.js index 5e790133d3..fdc95cdb5c 100644 --- a/lib/renderer/api/remote.js +++ b/lib/renderer/api/remote.js @@ -304,6 +304,10 @@ ipcRenderer.on('ELECTRON_RENDERER_RELEASE_CALLBACK', function (event, id) { callbacksRegistry.remove(id) }) +process.on('exit', () => { + ipcRenderer.sendSync('ELECTRON_BROWSER_CONTEXT_RELEASE') +}) + // Get remote module. exports.require = function (module) { return metaToValue(ipcRenderer.sendSync('ELECTRON_BROWSER_REQUIRE', module)) From 5654ff0d430b2902aedef4607094e92d4898a39c Mon Sep 17 00:00:00 2001 From: Thiago de Arruda Date: Mon, 15 May 2017 09:17:07 -0300 Subject: [PATCH 2/2] Add test for #9387 --- spec/api-browser-window-spec.js | 68 ++++++++++++++++++++++++- spec/fixtures/api/sandbox.html | 20 ++++++-- spec/fixtures/module/hello-child.js | 6 +++ spec/fixtures/module/hello.js | 6 +++ spec/fixtures/module/preload-sandbox.js | 2 +- 5 files changed, 97 insertions(+), 5 deletions(-) create mode 100644 spec/fixtures/module/hello-child.js create mode 100644 spec/fixtures/module/hello.js diff --git a/spec/api-browser-window-spec.js b/spec/api-browser-window-spec.js index 8d5f329a14..be8dc30e25 100644 --- a/spec/api-browser-window-spec.js +++ b/spec/api-browser-window-spec.js @@ -1161,7 +1161,6 @@ describe('BrowserWindow module', function () { } }) w.loadURL('file://' + path.join(fixtures, 'api', 'sandbox.html?allocate-memory')) - w.webContents.openDevTools({mode: 'detach'}) ipcMain.once('answer', function (event, {bytesBeforeOpen, bytesAfterOpen, bytesAfterClose}) { const memoryIncreaseByOpen = bytesAfterOpen - bytesBeforeOpen const memoryDecreaseByClose = bytesAfterOpen - bytesAfterClose @@ -1173,6 +1172,73 @@ describe('BrowserWindow module', function () { done() }) }) + + // see #9387 + it('properly manages remote object references after page reload', (done) => { + w.destroy() + w = new BrowserWindow({ + show: false, + webPreferences: { + preload: preload, + sandbox: true + } + }) + w.loadURL('file://' + path.join(fixtures, 'api', 'sandbox.html?reload-remote')) + + ipcMain.on('get-remote-module-path', (event) => { + event.returnValue = path.join(fixtures, 'module', 'hello.js') + }) + + let reload = false + ipcMain.on('reloaded', (event) => { + event.returnValue = reload + reload = !reload + }) + + ipcMain.once('reload', (event) => { + event.sender.reload() + }) + + ipcMain.once('answer', (event, arg) => { + ipcMain.removeAllListeners('reloaded') + ipcMain.removeAllListeners('get-remote-module-path') + assert.equal(arg, 'hi') + done() + }) + }) + + it('properly manages remote object references after page reload in child window', (done) => { + w.destroy() + w = new BrowserWindow({ + show: false, + webPreferences: { + preload: preload, + sandbox: true + } + }) + w.loadURL('file://' + path.join(fixtures, 'api', 'sandbox.html?reload-remote-child')) + + ipcMain.on('get-remote-module-path', (event) => { + event.returnValue = path.join(fixtures, 'module', 'hello-child.js') + }) + + let reload = false + ipcMain.on('reloaded', (event) => { + event.returnValue = reload + reload = !reload + }) + + ipcMain.once('reload', (event) => { + event.sender.reload() + }) + + ipcMain.once('answer', (event, arg) => { + ipcMain.removeAllListeners('reloaded') + ipcMain.removeAllListeners('get-remote-module-path') + assert.equal(arg, 'hi child window') + done() + }) + }) }) describe('nativeWindowOpen option', () => { diff --git a/spec/fixtures/api/sandbox.html b/spec/fixtures/api/sandbox.html index f8d7aa7921..5353d8815a 100644 --- a/spec/fixtures/api/sandbox.html +++ b/spec/fixtures/api/sandbox.html @@ -13,13 +13,28 @@ await timeout(100) } } - if (window.opener) { + + const [,test] = window.location.href.split('?') + if (window.opener && test !== 'reload-remote') { window.callback = () => { opener.require('electron').ipcRenderer.send('answer', document.body.innerHTML) } } else { - const {ipcRenderer} = require('electron') + const {ipcRenderer, remote} = require('electron') const tests = { + 'reload-remote-child': () => { + open(`${location.protocol}//${location.pathname}?reload-remote`) + }, + 'reload-remote': async () => { + const p = ipcRenderer.sendSync('get-remote-module-path') + const Hello = remote.require(p) + if (!ipcRenderer.sendSync('reloaded')) { + ipcRenderer.send('reload') + return + } + await invokeGc() + ipcRenderer.send('answer', new Hello().say()) + }, 'allocate-memory': async () => { await invokeGc() const {privateBytes: bytesBeforeOpen} = process.getProcessMemoryInfo() @@ -95,7 +110,6 @@ popup.close() }, false) - let [,test] = window.location.href.split('?') if (tests.hasOwnProperty(test)) tests[test]() } diff --git a/spec/fixtures/module/hello-child.js b/spec/fixtures/module/hello-child.js new file mode 100644 index 0000000000..09ac18900e --- /dev/null +++ b/spec/fixtures/module/hello-child.js @@ -0,0 +1,6 @@ +class Hello { + say () { + return 'hi child window' + } +} +module.exports = Hello diff --git a/spec/fixtures/module/hello.js b/spec/fixtures/module/hello.js new file mode 100644 index 0000000000..9debd60618 --- /dev/null +++ b/spec/fixtures/module/hello.js @@ -0,0 +1,6 @@ +class Hello { + say () { + return 'hi' + } +} +module.exports = Hello diff --git a/spec/fixtures/module/preload-sandbox.js b/spec/fixtures/module/preload-sandbox.js index 15d6e06a86..f8f8833985 100644 --- a/spec/fixtures/module/preload-sandbox.js +++ b/spec/fixtures/module/preload-sandbox.js @@ -3,9 +3,9 @@ const {ipcRenderer} = require('electron') window.ipcRenderer = ipcRenderer window.setImmediate = setImmediate + window.require = require if (location.protocol === 'file:') { window.test = 'preload' - window.require = require window.process = process } else if (location.href !== 'about:blank') { addEventListener('DOMContentLoaded', () => {