From 889412512b7712834bacdd6117ad2d004283f598 Mon Sep 17 00:00:00 2001 From: Jeremy Apthorp Date: Tue, 27 Nov 2018 12:58:56 -0800 Subject: [PATCH 1/7] feat: webContents.loadURL returns a promise --- docs/api/browser-window.md | 10 +++++- docs/api/web-contents.md | 9 ++++++ lib/browser/navigation-controller.js | 26 +++++++++++++++- spec/api-ipc-renderer-spec.js | 5 +-- spec/api-web-contents-spec.js | 46 ++++++++++++++++++++++++---- 5 files changed, 84 insertions(+), 12 deletions(-) diff --git a/docs/api/browser-window.md b/docs/api/browser-window.md index 4518d0f6e6c3b..2261d7ce19a47 100644 --- a/docs/api/browser-window.md +++ b/docs/api/browser-window.md @@ -1236,7 +1236,11 @@ Captures a snapshot of the page within `rect`. Omitting `rect` will capture the * `postData` ([UploadRawData[]](structures/upload-raw-data.md) | [UploadFile[]](structures/upload-file.md) | [UploadBlob[]](structures/upload-blob.md)) (optional) * `baseURLForDataURL` String (optional) - Base url (with trailing path separator) for files to be loaded by the data url. This is needed only if the specified `url` is a data url and needs to load other files. -Same as `webContents.loadURL(url[, options])`. +Returns `Promise` - the promise will resolve when the page has finished loading +(see [`did-finish-load`](web-contents.md#event-did-finish-load)), and rejects +if the page fails to load (see [`did-fail-load`](web-contents.md#event-did-fail-load)). + +Same as [`webContents.loadURL(url[, options])`](web-contents.md#contentsloadurlurl-options). The `url` can be a remote address (e.g. `http://`) or a path to a local HTML file using the `file://` protocol. @@ -1276,6 +1280,10 @@ win.loadURL('http://localhost:8000/post', { * `search` String (optional) - Passed to `url.format()`. * `hash` String (optional) - Passed to `url.format()`. +Returns `Promise` - the promise will resolve when the page has finished loading +(see [`did-finish-load`](web-contents.md#event-did-finish-load)), and rejects +if the page fails to load (see [`did-fail-load`](web-contents.md#event-did-fail-load)). + Same as `webContents.loadFile`, `filePath` should be a path to an HTML file relative to the root of your application. See the `webContents` docs for more information. diff --git a/docs/api/web-contents.md b/docs/api/web-contents.md index 0ce2f05c69181..4bd00f118b944 100644 --- a/docs/api/web-contents.md +++ b/docs/api/web-contents.md @@ -697,6 +697,11 @@ Custom value can be returned by setting `event.returnValue`. * `postData` ([UploadRawData[]](structures/upload-raw-data.md) | [UploadFile[]](structures/upload-file.md) | [UploadBlob[]](structures/upload-blob.md)) (optional) * `baseURLForDataURL` String (optional) - Base url (with trailing path separator) for files to be loaded by the data url. This is needed only if the specified `url` is a data url and needs to load other files. +Returns `Promise` - the promise will resolve when the page has finished loading +(see [`did-finish-load`](web-contents.md#event-did-finish-load)), and rejects +if the page fails to load (see +[`did-fail-load`](web-contents.md#event-did-fail-load)). + Loads the `url` in the window. The `url` must contain the protocol prefix, e.g. the `http://` or `file://`. If the load should bypass http cache then use the `pragma` header to achieve it. @@ -715,6 +720,10 @@ webContents.loadURL('https://github.com', options) * `search` String (optional) - Passed to `url.format()`. * `hash` String (optional) - Passed to `url.format()`. +Returns `Promise` - the promise will resolve when the page has finished loading +(see [`did-finish-load`](web-contents.md#event-did-finish-load)), and rejects +if the page fails to load (see [`did-fail-load`](web-contents.md#event-did-fail-load)). + Loads the given file in the window, `filePath` should be a path to an HTML file relative to the root of your application. For instance an app structure like this: diff --git a/lib/browser/navigation-controller.js b/lib/browser/navigation-controller.js index 1863961d06470..cf26db7f6ce70 100644 --- a/lib/browser/navigation-controller.js +++ b/lib/browser/navigation-controller.js @@ -55,9 +55,33 @@ const NavigationController = (function () { if (options == null) { options = {} } + const p = new Promise((resolve, reject) => { + const finishListener = () => { + this.webContents.removeListener('did-fail-load', failListener) + resolve() + } + const failListener = (event, errorCode, errorDescription, validatedURL, isMainFrame, frameProcessId, frameRoutingId) => { + if (!isMainFrame) { + return + } + this.webContents.removeListener('did-finish-load', finishListener) + const err = new Error(`${errorDescription} (${errorCode}) loading '${validatedURL}'`) + Object.assign(err, { + errno: errorCode, + code: errorDescription, + validatedURL, + frameProcessId, + frameRoutingId + }) + reject(err) + } + this.webContents.once('did-finish-load', finishListener) + this.webContents.once('did-fail-load', failListener) + }) this.pendingIndex = -1 this.webContents._loadURL(url, options) - return this.webContents.emit('load-url', url, options) + this.webContents.emit('load-url', url, options) + return p } NavigationController.prototype.getURL = function () { diff --git a/spec/api-ipc-renderer-spec.js b/spec/api-ipc-renderer-spec.js index b3a3d349756ed..97e95a0a00763 100644 --- a/spec/api-ipc-renderer-spec.js +++ b/spec/api-ipc-renderer-spec.js @@ -5,7 +5,6 @@ const dirtyChai = require('dirty-chai') const http = require('http') const path = require('path') const { closeWindow } = require('./window-helpers') -const { emittedOnce } = require('./events-helpers') const { expect } = chai chai.use(dirtyChai) @@ -229,9 +228,7 @@ describe('ipc renderer module', () => { describe('ipcRenderer.on', () => { it('is not used for internals', async () => { w = new BrowserWindow({ show: false }) - w.loadURL('about:blank') - - await emittedOnce(w.webContents, 'did-finish-load') + await w.loadURL('about:blank') const script = `require('electron').ipcRenderer.eventNames()` const result = await w.webContents.executeJavaScript(script) diff --git a/spec/api-web-contents-spec.js b/spec/api-web-contents-spec.js index a3583bf67bcc3..2a66d12578712 100644 --- a/spec/api-web-contents-spec.js +++ b/spec/api-web-contents-spec.js @@ -39,6 +39,44 @@ describe('webContents module', () => { afterEach(() => closeWindow(w).then(() => { w = null })) + describe('loadURL() promise API', () => { + it('resolves when done loading', async () => { + await w.loadURL('about:blank') + // assert: no timeout, no exception. + }) + + it('resolves when done loading a file URL', async () => { + await w.loadFile(path.join(fixtures, 'pages', 'base-page.html')) + // assert: no timeout, no exception. + }) + + it('rejects when failing to load a file URL', async () => { + let err + try { + await w.loadURL('file:non-existent') + } catch (e) { + err = e + } + expect(err).not.to.be.null() + expect(err.code).to.eql('ERR_FILE_NOT_FOUND') + expect(err.errno).to.eql(-6) + expect(err.validatedURL).to.eql('file:///non-existent') + }) + + it('rejects when loading fails due to DNS not resolved', async () => { + let err + try { + await w.loadURL('https://err.name.not.resolved') + } catch (e) { + err = e + } + expect(err).not.to.be.undefined() + expect(err.code).to.eql('ERR_NAME_NOT_RESOLVED') + expect(err.errno).to.eql(-105) + expect(err.validatedURL).to.eql('https://err.name.not.resolved/') + }) + }) + describe('getAllWebContents() API', () => { it('returns an array of web contents', (done) => { w.webContents.on('devtools-opened', () => { @@ -899,9 +937,7 @@ describe('webContents module', () => { } }) - const p = emittedOnce(w.webContents, 'did-finish-load') - w.loadURL('about:blank') - await p + await w.loadURL('about:blank') const filePath = path.join(remote.app.getPath('temp'), 'test.heapsnapshot') @@ -931,9 +967,7 @@ describe('webContents module', () => { } }) - const p = emittedOnce(w.webContents, 'did-finish-load') - w.loadURL('about:blank') - await p + await w.loadURL('about:blank') const promise = w.webContents.takeHeapSnapshot('') return expect(promise).to.be.eventually.rejectedWith(Error, 'takeHeapSnapshot failed') From 0a7a1af35ffd3d5c902f137fa882a2c52623ecc6 Mon Sep 17 00:00:00 2001 From: Jeremy Apthorp Date: Tue, 27 Nov 2018 15:40:05 -0800 Subject: [PATCH 2/7] silence unhandled rejection --- lib/browser/navigation-controller.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/browser/navigation-controller.js b/lib/browser/navigation-controller.js index cf26db7f6ce70..47249b2d5c3f6 100644 --- a/lib/browser/navigation-controller.js +++ b/lib/browser/navigation-controller.js @@ -78,6 +78,8 @@ const NavigationController = (function () { this.webContents.once('did-finish-load', finishListener) this.webContents.once('did-fail-load', failListener) }) + // Add a no-op rejection handler to silence the unhandled rejection error. + p.catch(() => {}) this.pendingIndex = -1 this.webContents._loadURL(url, options) this.webContents.emit('load-url', url, options) From 64c31c964fad3c351c2a849dbebd5b37a448f709 Mon Sep 17 00:00:00 2001 From: Jeremy Apthorp Date: Wed, 28 Nov 2018 10:32:04 -0800 Subject: [PATCH 3/7] remove unhandledrejection event after test is done --- spec/api-remote-spec.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spec/api-remote-spec.js b/spec/api-remote-spec.js index 1d0bf1a6ca845..77956cf7d18e5 100644 --- a/spec/api-remote-spec.js +++ b/spec/api-remote-spec.js @@ -429,9 +429,10 @@ describe('remote module', () => { }) it('emits unhandled rejection events in the renderer process', (done) => { - window.addEventListener('unhandledrejection', function (event) { + window.addEventListener('unhandledrejection', function handler (event) { event.preventDefault() assert.strictEqual(event.reason.message, 'rejected') + window.removeEventListener('unhandledrejection', handler) done() }) From dad43e5d7ccf1ab6dfb259e7313dd895ff9fdbc7 Mon Sep 17 00:00:00 2001 From: Jeremy Apthorp Date: Wed, 28 Nov 2018 14:04:29 -0800 Subject: [PATCH 4/7] fix validated file url on win32 --- spec/api-web-contents-spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/api-web-contents-spec.js b/spec/api-web-contents-spec.js index 2a66d12578712..184c203995d66 100644 --- a/spec/api-web-contents-spec.js +++ b/spec/api-web-contents-spec.js @@ -60,7 +60,7 @@ describe('webContents module', () => { expect(err).not.to.be.null() expect(err.code).to.eql('ERR_FILE_NOT_FOUND') expect(err.errno).to.eql(-6) - expect(err.validatedURL).to.eql('file:///non-existent') + expect(err.validatedURL).to.eql(process.platform === 'win32' ? 'file://non-existent/' : 'file:///non-existent') }) it('rejects when loading fails due to DNS not resolved', async () => { From b1cd3e018d9cfecb33059fe19629f33c9567eb55 Mon Sep 17 00:00:00 2001 From: Jeremy Apthorp Date: Wed, 28 Nov 2018 15:41:44 -0800 Subject: [PATCH 5/7] reject the promise if the navigation is aborted --- lib/browser/navigation-controller.js | 61 +++++++++++++++++++++------- spec/api-web-contents-spec.js | 4 +- 2 files changed, 48 insertions(+), 17 deletions(-) diff --git a/lib/browser/navigation-controller.js b/lib/browser/navigation-controller.js index 47249b2d5c3f6..e79062f6449c4 100644 --- a/lib/browser/navigation-controller.js +++ b/lib/browser/navigation-controller.js @@ -56,27 +56,58 @@ const NavigationController = (function () { options = {} } const p = new Promise((resolve, reject) => { - const finishListener = () => { - this.webContents.removeListener('did-fail-load', failListener) + const resolveAndCleanup = () => { + removeListeners() resolve() } + const rejectAndCleanup = (errorCode, errorDescription, url) => { + const err = new Error(`${errorDescription} (${errorCode}) loading '${url}'`) + Object.assign(err, { errno: errorCode, code: errorDescription, url }) + removeListeners() + reject(err) + } + const finishListener = () => { + resolveAndCleanup() + } const failListener = (event, errorCode, errorDescription, validatedURL, isMainFrame, frameProcessId, frameRoutingId) => { - if (!isMainFrame) { - return + if (isMainFrame) { + rejectAndCleanup(errorCode, errorDescription, validatedURL) } + } + + let navigationStarted = false + const navigationListener = (event, url, isSameDocument, isMainFrame, frameProcessId, frameRoutingId, navigationId) => { + if (isMainFrame) { + if (navigationStarted) { + // the webcontents has started another unrelated navigation in the + // main frame (probably from the app calling `loadURL` again); reject + // the promise + return rejectAndCleanup(-3, 'ERR_ABORTED', url) + } + navigationStarted = true + } + } + const stopLoadingListener = () => { + // By the time we get here, either 'finish' or 'fail' should have fired + // if the navigation occurred. However, in some situations (e.g. when + // attempting to load a page with a bad scheme), loading will stop + // without emitting finish or fail. In this case, we reject the promise + // with a generic failure. + // TODO(jeremy): enumerate all the cases in which this can happen. If + // the only one is with a bad scheme, perhaps ERR_INVALID_ARGUMENT + // would be more appropriate. + rejectAndCleanup(-2, 'ERR_FAILED', url) + } + const removeListeners = () => { this.webContents.removeListener('did-finish-load', finishListener) - const err = new Error(`${errorDescription} (${errorCode}) loading '${validatedURL}'`) - Object.assign(err, { - errno: errorCode, - code: errorDescription, - validatedURL, - frameProcessId, - frameRoutingId - }) - reject(err) + this.webContents.removeListener('did-fail-load', failListener) + this.webContents.removeListener('did-start-navigation', navigationListener) + this.webContents.removeListener('did-stop-loading', stopLoadingListener) } - this.webContents.once('did-finish-load', finishListener) - this.webContents.once('did-fail-load', failListener) + this.webContents.on('did-finish-load', finishListener) + this.webContents.on('did-fail-load', failListener) + this.webContents.on('did-start-navigation', navigationListener) + this.webContents.on('did-stop-loading', stopLoadingListener) }) // Add a no-op rejection handler to silence the unhandled rejection error. p.catch(() => {}) diff --git a/spec/api-web-contents-spec.js b/spec/api-web-contents-spec.js index 184c203995d66..ed8e61aa86314 100644 --- a/spec/api-web-contents-spec.js +++ b/spec/api-web-contents-spec.js @@ -60,7 +60,7 @@ describe('webContents module', () => { expect(err).not.to.be.null() expect(err.code).to.eql('ERR_FILE_NOT_FOUND') expect(err.errno).to.eql(-6) - expect(err.validatedURL).to.eql(process.platform === 'win32' ? 'file://non-existent/' : 'file:///non-existent') + expect(err.url).to.eql(process.platform === 'win32' ? 'file://non-existent/' : 'file:///non-existent') }) it('rejects when loading fails due to DNS not resolved', async () => { @@ -73,7 +73,7 @@ describe('webContents module', () => { expect(err).not.to.be.undefined() expect(err.code).to.eql('ERR_NAME_NOT_RESOLVED') expect(err.errno).to.eql(-105) - expect(err.validatedURL).to.eql('https://err.name.not.resolved/') + expect(err.url).to.eql('https://err.name.not.resolved/') }) }) From 3f36220e6baffbec0a03b5b40fd8e4e72fb8f587 Mon Sep 17 00:00:00 2001 From: Jeremy Apthorp Date: Wed, 28 Nov 2018 17:50:41 -0800 Subject: [PATCH 6/7] more tests --- spec/api-web-contents-spec.js | 91 +++++++++++++++++++++++++++++------ 1 file changed, 76 insertions(+), 15 deletions(-) diff --git a/spec/api-web-contents-spec.js b/spec/api-web-contents-spec.js index ed8e61aa86314..17517ddd73ec5 100644 --- a/spec/api-web-contents-spec.js +++ b/spec/api-web-contents-spec.js @@ -41,16 +41,29 @@ describe('webContents module', () => { describe('loadURL() promise API', () => { it('resolves when done loading', async () => { - await w.loadURL('about:blank') - // assert: no timeout, no exception. + await expect(w.loadURL('about:blank')).to.eventually.be.fulfilled }) it('resolves when done loading a file URL', async () => { - await w.loadFile(path.join(fixtures, 'pages', 'base-page.html')) - // assert: no timeout, no exception. + await expect(w.loadFile(path.join(fixtures, 'pages', 'base-page.html'))).to.eventually.be.fulfilled }) it('rejects when failing to load a file URL', async () => { + await expect(w.loadURL('file:non-existent')).to.eventually.be.rejected + .and.have.property('code', 'ERR_FILE_NOT_FOUND') + }) + + it('rejects when loading fails due to DNS not resolved', async () => { + await expect(w.loadURL('https://err.name.not.resolved')).to.eventually.be.rejected + .and.have.property('code', 'ERR_NAME_NOT_RESOLVED') + }) + + it('rejects when navigation is cancelled due to a bad scheme', async () => { + await expect(w.loadURL('bad-scheme://foo')).to.eventually.be.rejected + .and.have.property('code', 'ERR_FAILED') + }) + + it('sets appropriate error information on rejection', async () => { let err try { await w.loadURL('file:non-existent') @@ -63,17 +76,65 @@ describe('webContents module', () => { expect(err.url).to.eql(process.platform === 'win32' ? 'file://non-existent/' : 'file:///non-existent') }) - it('rejects when loading fails due to DNS not resolved', async () => { - let err - try { - await w.loadURL('https://err.name.not.resolved') - } catch (e) { - err = e - } - expect(err).not.to.be.undefined() - expect(err.code).to.eql('ERR_NAME_NOT_RESOLVED') - expect(err.errno).to.eql(-105) - expect(err.url).to.eql('https://err.name.not.resolved/') + it('rejects if the load is aborted', async () => { + const s = http.createServer((req, res) => { /* never complete the request */ }) + await new Promise(resolve => s.listen(0, '127.0.0.1', resolve)) + const { port } = s.address() + const p = expect(w.loadURL(`http://127.0.0.1:${port}`)).to.eventually.be.rejectedWith(Error, /ERR_ABORTED/) + // load a different file before the first load completes, causing the + // first load to be aborted. + await w.loadFile(path.join(fixtures, 'pages', 'base-page.html')) + await p + s.close() + }) + + it("doesn't reject when a subframe fails to load", async () => { + let resp = null + const s = http.createServer((req, res) => { + res.writeHead(200, { 'Content-Type': 'text/html' }) + res.write('') + resp = res + // don't end the response yet + }) + await new Promise(resolve => s.listen(0, '127.0.0.1', resolve)) + const { port } = s.address() + const p = new Promise(resolve => { + w.webContents.on('did-fail-load', (event, errorCode, errorDescription, validatedURL, isMainFrame, frameProcessId, frameRoutingId) => { + if (!isMainFrame) { + resolve() + } + }) + }) + const main = w.loadURL(`http://127.0.0.1:${port}`) + await p + resp.end() + await main + s.close() + }) + + it("doesn't resolve when a subframe loads", async () => { + let resp = null + const s = http.createServer((req, res) => { + res.writeHead(200, { 'Content-Type': 'text/html' }) + res.write('') + resp = res + // don't end the response yet + }) + await new Promise(resolve => s.listen(0, '127.0.0.1', resolve)) + const { port } = s.address() + const p = new Promise(resolve => { + w.webContents.on('did-frame-finish-load', (event, errorCode, errorDescription, validatedURL, isMainFrame, frameProcessId, frameRoutingId) => { + if (!isMainFrame) { + resolve() + } + }) + }) + const main = w.loadURL(`http://127.0.0.1:${port}`) + await p + resp.destroy() // cause the main request to fail + await expect(main).to.eventually.be.rejected + .and.have.property('errno', -355) // ERR_INCOMPLETE_CHUNKED_ENCODING + s.close() }) }) From 886aefcaf4d0c70873cfb301adadb9f2f57ebfc0 Mon Sep 17 00:00:00 2001 From: Jeremy Apthorp Date: Mon, 3 Dec 2018 14:47:18 -0800 Subject: [PATCH 7/7] narrow promise type --- docs/api/browser-window.md | 4 ++-- docs/api/web-contents.md | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/api/browser-window.md b/docs/api/browser-window.md index 2261d7ce19a47..e359a300342cd 100644 --- a/docs/api/browser-window.md +++ b/docs/api/browser-window.md @@ -1236,7 +1236,7 @@ Captures a snapshot of the page within `rect`. Omitting `rect` will capture the * `postData` ([UploadRawData[]](structures/upload-raw-data.md) | [UploadFile[]](structures/upload-file.md) | [UploadBlob[]](structures/upload-blob.md)) (optional) * `baseURLForDataURL` String (optional) - Base url (with trailing path separator) for files to be loaded by the data url. This is needed only if the specified `url` is a data url and needs to load other files. -Returns `Promise` - the promise will resolve when the page has finished loading +Returns `Promise` - the promise will resolve when the page has finished loading (see [`did-finish-load`](web-contents.md#event-did-finish-load)), and rejects if the page fails to load (see [`did-fail-load`](web-contents.md#event-did-fail-load)). @@ -1280,7 +1280,7 @@ win.loadURL('http://localhost:8000/post', { * `search` String (optional) - Passed to `url.format()`. * `hash` String (optional) - Passed to `url.format()`. -Returns `Promise` - the promise will resolve when the page has finished loading +Returns `Promise` - the promise will resolve when the page has finished loading (see [`did-finish-load`](web-contents.md#event-did-finish-load)), and rejects if the page fails to load (see [`did-fail-load`](web-contents.md#event-did-fail-load)). diff --git a/docs/api/web-contents.md b/docs/api/web-contents.md index 4bd00f118b944..9e84e40305fa7 100644 --- a/docs/api/web-contents.md +++ b/docs/api/web-contents.md @@ -697,7 +697,7 @@ Custom value can be returned by setting `event.returnValue`. * `postData` ([UploadRawData[]](structures/upload-raw-data.md) | [UploadFile[]](structures/upload-file.md) | [UploadBlob[]](structures/upload-blob.md)) (optional) * `baseURLForDataURL` String (optional) - Base url (with trailing path separator) for files to be loaded by the data url. This is needed only if the specified `url` is a data url and needs to load other files. -Returns `Promise` - the promise will resolve when the page has finished loading +Returns `Promise` - the promise will resolve when the page has finished loading (see [`did-finish-load`](web-contents.md#event-did-finish-load)), and rejects if the page fails to load (see [`did-fail-load`](web-contents.md#event-did-fail-load)). @@ -720,7 +720,7 @@ webContents.loadURL('https://github.com', options) * `search` String (optional) - Passed to `url.format()`. * `hash` String (optional) - Passed to `url.format()`. -Returns `Promise` - the promise will resolve when the page has finished loading +Returns `Promise` - the promise will resolve when the page has finished loading (see [`did-finish-load`](web-contents.md#event-did-finish-load)), and rejects if the page fails to load (see [`did-fail-load`](web-contents.md#event-did-fail-load)).