From 0bea42bd8c4002ed28d100d9ccc06c6ac03a9b70 Mon Sep 17 00:00:00 2001 From: Andrey Lushnikov Date: Mon, 11 Sep 2017 16:21:51 -0700 Subject: [PATCH] Do not leave dangling promises when sending messages over protocol (#742) It's very bad to have 'unhandled promise rejection' that can't be handled in user code. These errors will exit node process in a near future. This patch avoids 'unhandled promise rejection' while sending protocol messages. This patch: - introduces `puppeteer:error` debug scope and starts using it for all swalloed errors. - makes sure that every `client.send` method is either awaited or its errors are handled. - starts return promises from Request.continue() and Request.abort(). - starts swallow errors from Request.contine() and Request.abort(). The last is the most important part of the patch. Since `Request.continue()` might try to continue canceled request, we should disregard the error. Fixes #627. --- docs/api.md | 2 ++ lib/Browser.js | 2 +- lib/Dialog.js | 2 +- lib/ElementHandle.js | 2 +- lib/FrameManager.js | 2 +- lib/Input.js | 2 +- lib/Launcher.js | 2 +- lib/NavigatorWatcher.js | 2 +- lib/NetworkManager.js | 20 ++++++++++++++------ lib/Page.js | 21 +++++++++++++-------- lib/Puppeteer.js | 2 +- lib/Tracing.js | 2 +- lib/helper.js | 13 ++++++++----- test/test.js | 16 +++++++++++++++- 14 files changed, 61 insertions(+), 29 deletions(-) diff --git a/docs/api.md b/docs/api.md index be03c317..a0d72d50 100644 --- a/docs/api.md +++ b/docs/api.md @@ -1314,6 +1314,7 @@ If request fails at some point, then instead of 'requestfinished' event (and pos If request gets a 'redirect' response, the request is successfully finished with the 'requestfinished' event, and a new request is issued to a redirected url. #### request.abort() +- returns: <[Promise]> Aborts request. To use this, request interception should be enabled with `page.setRequestInterceptionEnabled`. Exception is immediately thrown if the request interception is not enabled. @@ -1324,6 +1325,7 @@ Exception is immediately thrown if the request interception is not enabled. - `method` <[string]> If set changes the request method (e.g. `GET` or `POST`) - `postData` <[string]> If set changes the post data of request - `headers` <[Object]> If set changes the request HTTP headers +- returns: <[Promise]> Continues request with optional request overrides. To use this, request interception should be enabled with `page.setRequestInterceptionEnabled`. Exception is immediately thrown if the request interception is not enabled. diff --git a/lib/Browser.js b/lib/Browser.js index d36574b8..720cae22 100644 --- a/lib/Browser.js +++ b/lib/Browser.js @@ -14,7 +14,7 @@ * limitations under the License. */ -const helper = require('./helper'); +const {helper} = require('./helper'); const Page = require('./Page'); class Browser { diff --git a/lib/Dialog.js b/lib/Dialog.js index 5e618878..ebac447c 100644 --- a/lib/Dialog.js +++ b/lib/Dialog.js @@ -14,7 +14,7 @@ * limitations under the License. */ -const helper = require('./helper'); +const {helper} = require('./helper'); class Dialog { /** diff --git a/lib/ElementHandle.js b/lib/ElementHandle.js index 4c2ba64c..a3c71334 100644 --- a/lib/ElementHandle.js +++ b/lib/ElementHandle.js @@ -14,7 +14,7 @@ * limitations under the License. */ const path = require('path'); -const helper = require('./helper'); +const {helper} = require('./helper'); class ElementHandle { /** diff --git a/lib/FrameManager.js b/lib/FrameManager.js index 14882ec0..e4ae6478 100644 --- a/lib/FrameManager.js +++ b/lib/FrameManager.js @@ -16,7 +16,7 @@ const fs = require('fs'); const EventEmitter = require('events'); -const helper = require('./helper'); +const {helper} = require('./helper'); const ElementHandle = require('./ElementHandle'); class FrameManager extends EventEmitter { diff --git a/lib/Input.js b/lib/Input.js index ef6a7e43..9a2dbe95 100644 --- a/lib/Input.js +++ b/lib/Input.js @@ -14,7 +14,7 @@ * limitations under the License. */ -const helper = require('./helper'); +const {helper} = require('./helper'); class Keyboard { /** diff --git a/lib/Launcher.js b/lib/Launcher.js index bbc94b35..ecd64b77 100644 --- a/lib/Launcher.js +++ b/lib/Launcher.js @@ -22,7 +22,7 @@ const Connection = require('./Connection'); const Browser = require('./Browser'); const readline = require('readline'); const fs = require('fs'); -const helper = require('./helper'); +const {helper} = require('./helper'); const ChromiumRevision = require('../package.json').puppeteer.chromium_revision; const CHROME_PROFILE_PATH = path.join(os.tmpdir(), 'puppeteer_dev_profile-'); diff --git a/lib/NavigatorWatcher.js b/lib/NavigatorWatcher.js index cc189fe1..c607cbda 100644 --- a/lib/NavigatorWatcher.js +++ b/lib/NavigatorWatcher.js @@ -14,7 +14,7 @@ * limitations under the License. */ -const helper = require('./helper'); +const {helper} = require('./helper'); class NavigatorWatcher { /** diff --git a/lib/NetworkManager.js b/lib/NetworkManager.js index bb03ce15..34b56587 100644 --- a/lib/NetworkManager.js +++ b/lib/NetworkManager.js @@ -14,7 +14,7 @@ * limitations under the License. */ const EventEmitter = require('events'); -const helper = require('./helper'); +const {helper, debugError} = require('./helper'); const Multimap = require('./Multimap'); const url = require('url'); @@ -66,7 +66,7 @@ class NetworkManager extends EventEmitter { * @param {string} userAgent */ async setUserAgent(userAgent) { - return this._client.send('Network.setUserAgentOverride', { userAgent }); + await this._client.send('Network.setUserAgentOverride', { userAgent }); } /** @@ -240,32 +240,40 @@ class Request { /** * @param {!Object=} overrides */ - continue(overrides = {}) { + async continue(overrides = {}) { // DataURL's are not interceptable. In this case, do nothing. if (this.url.startsWith('data:')) return; console.assert(this._interceptionId, 'Request Interception is not enabled!'); console.assert(!this._interceptionHandled, 'Request is already handled!'); this._interceptionHandled = true; - this._client.send('Network.continueInterceptedRequest', { + await this._client.send('Network.continueInterceptedRequest', { interceptionId: this._interceptionId, url: overrides.url, method: overrides.method, postData: overrides.postData, headers: overrides.headers, + }).catch(error => { + // In certain cases, protocol will return error if the request was already canceled + // or the page was closed. We should tolerate these errors. + debugError(error); }); } - abort() { + async abort() { // DataURL's are not interceptable. In this case, do nothing. if (this.url.startsWith('data:')) return; console.assert(this._interceptionId, 'Request Interception is not enabled!'); console.assert(!this._interceptionHandled, 'Request is already handled!'); this._interceptionHandled = true; - this._client.send('Network.continueInterceptedRequest', { + await this._client.send('Network.continueInterceptedRequest', { interceptionId: this._interceptionId, errorReason: 'Failed' + }).catch(error => { + // In certain cases, protocol will return error if the request was already canceled + // or the page was closed. We should tolerate these errors. + debugError(error); }); } } diff --git a/lib/Page.js b/lib/Page.js index 8af47b21..92e85722 100644 --- a/lib/Page.js +++ b/lib/Page.js @@ -24,7 +24,7 @@ const EmulationManager = require('./EmulationManager'); const FrameManager = require('./FrameManager'); const {Keyboard, Mouse, Touchscreen} = require('./Input'); const Tracing = require('./Tracing'); -const helper = require('./helper'); +const {helper, debugError} = require('./helper'); class Page extends EventEmitter { /** @@ -153,7 +153,7 @@ class Page extends EventEmitter { this._client.send('Security.handleCertificateError', { eventId: event.eventId, action: 'continue' - }); + }).catch(debugError); } /** @@ -293,7 +293,7 @@ class Page extends EventEmitter { const {name, seq, args} = JSON.parse(event.args[1].value); const result = await this._pageBindings[name](...args); const expression = helper.evaluationString(deliverResult, name, seq, result); - this._client.send('Runtime.evaluate', { expression }); + this._client.send('Runtime.evaluate', { expression }).catch(debugError); function deliverResult(name, seq, result) { window[name]['callbacks'].get(seq)(result); @@ -387,8 +387,11 @@ class Page extends EventEmitter { * @return {!Promise} */ async reload(options) { - this._client.send('Page.reload'); - return this.waitForNavigation(options); + const [response] = await Promise.all([ + this.waitForNavigation(options), + this._client.send('Page.reload') + ]); + return response; } /** @@ -431,9 +434,11 @@ class Page extends EventEmitter { const entry = history.entries[history.currentIndex + delta]; if (!entry) return null; - const result = this.waitForNavigation(options); - this._client.send('Page.navigateToHistoryEntry', {entryId: entry.id}); - return result; + const [response] = await Promise.all([ + this.waitForNavigation(options), + this._client.send('Page.navigateToHistoryEntry', {entryId: entry.id}), + ]); + return response; } /** diff --git a/lib/Puppeteer.js b/lib/Puppeteer.js index fab0a980..eaeaf82b 100644 --- a/lib/Puppeteer.js +++ b/lib/Puppeteer.js @@ -13,7 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -const helper = require('./helper'); +const {helper} = require('./helper'); const Launcher = require('./Launcher'); class Puppeteer { diff --git a/lib/Tracing.js b/lib/Tracing.js index c1975d42..583fe48b 100644 --- a/lib/Tracing.js +++ b/lib/Tracing.js @@ -14,7 +14,7 @@ * limitations under the License. */ const fs = require('fs'); -const helper = require('./helper'); +const {helper} = require('./helper'); class Tracing { /** diff --git a/lib/helper.js b/lib/helper.js index 5239d2dd..192707b1 100644 --- a/lib/helper.js +++ b/lib/helper.js @@ -14,6 +14,7 @@ * limitations under the License. */ +const debugError = require('debug')(`puppeteer:error`); /** @type {?Map} */ let apiCoverage = null; class Helper { @@ -94,12 +95,11 @@ class Helper { static async releaseObject(client, remoteObject) { if (!remoteObject.objectId) return; - try { - await client.send('Runtime.releaseObject', {objectId: remoteObject.objectId}); - } catch (e) { + await client.send('Runtime.releaseObject', {objectId: remoteObject.objectId}).catch(error => { // Exceptions might happen in case of a page been navigated or closed. // Swallow these since they are harmless and we don't leak anything in this case. - } + debugError(error); + }); } /** @@ -217,4 +217,7 @@ class Helper { } } -module.exports = Helper; +module.exports = { + helper: Helper, + debugError +}; diff --git a/test/test.js b/test/test.js index b82d8b82..b6d2a23e 100644 --- a/test/test.js +++ b/test/test.js @@ -17,7 +17,7 @@ const fs = require('fs'); const rm = require('rimraf').sync; const path = require('path'); -const helper = require('../lib/helper'); +const {helper} = require('../lib/helper'); if (process.env.COVERAGE) helper.recordPublicAPICoverage(); console.log('Testing on Node', process.version); @@ -986,6 +986,20 @@ describe('Page', function() { expect(requests.length).toBe(2); expect(requests[1].response().status).toBe(404); })); + it('should not throw "Invalid Interception Id" if the request was cancelled', SX(async function() { + await page.setContent(''); + await page.setRequestInterceptionEnabled(true); + let request = null; + page.on('request', async r => request = r); + page.$eval('iframe', (frame, url) => frame.src = url, EMPTY_PAGE), + // Wait for request interception. + await waitForEvents(page, 'request'); + // Delete frame to cause request to be canceled. + await page.$eval('iframe', frame => frame.remove()); + let error = null; + await request.continue().catch(e => error = e); + expect(error).toBe(null); + })); }); describe('Page.Events.Dialog', function() {