From e0c8d46af1ce2c063778b66aa29c4c00ab8aeba5 Mon Sep 17 00:00:00 2001 From: Yury Semikhatsky Date: Wed, 21 Aug 2019 10:26:48 -0700 Subject: [PATCH] fix: abort page.waitForRequest/Response when page closes (#4865) We'd like to pass an abortion signal inside Helper.waitForEvent in order to interrupt it when browser/page closes. Several approaches have been considered: 1. Pass CDPSession instance as a another parameter to the helper method and listen to Disconnected event on it. It would introduce undesired dependency on the session object. 2. Listen to the CDPSession closure at the call sites (e.g. waitForRequest) and pass an abortion promise which would be fulfilled when such event is fired. The listeners would have to be removed from the session on successful completion of waitForEvent so we'd have to pass some kind of DisposablePromise which would be disposed during cleanup. Such parameter looked somewhat hairy. 3. Create DisconnectPromise on CDPSession. One potential risk with that is all chained promises would hang around until the event is fired which might inadvertently cause memory leaks. On the other hand, adding such promise to Promise.race will remove dependency as soon as the race is finished. So this is the approach we're taking with one tweak: the promise is created locally inside Page. Ideally the disconnectPromise would throw when the session is closed but it may lead to uncaught promise errors if all chained promises are resolved, to avoid that the promise is resolved with an Error and Helper.waitForEvent throws it later. Fix #4733 --- experimental/puppeteer-firefox/lib/Page.js | 10 ++++++++-- .../puppeteer-firefox/lib/TimeoutSettings.js | 3 +++ experimental/puppeteer-firefox/lib/helper.js | 17 +++++++++++++---- lib/Page.js | 10 ++++++++-- lib/helper.js | 17 +++++++++++++---- test/launcher.spec.js | 17 +++++++++++++++++ test/page.spec.js | 13 +++++++++++++ 7 files changed, 75 insertions(+), 12 deletions(-) diff --git a/experimental/puppeteer-firefox/lib/Page.js b/experimental/puppeteer-firefox/lib/Page.js index 8c3898a811f64..c2ce1bdb3ed6f 100644 --- a/experimental/puppeteer-firefox/lib/Page.js +++ b/experimental/puppeteer-firefox/lib/Page.js @@ -241,6 +241,12 @@ class Page extends EventEmitter { } } + _sessionClosePromise() { + if (!this._disconnectPromise) + this._disconnectPromise = new Promise(fulfill => this._session.once(Events.JugglerSession.Disconnected, () => fulfill(new Error('Target closed')))); + return this._disconnectPromise; + } + /** * @param {(string|Function)} urlOrPredicate * @param {!{timeout?: number}=} options @@ -256,7 +262,7 @@ class Page extends EventEmitter { if (typeof urlOrPredicate === 'function') return !!(urlOrPredicate(request)); return false; - }, timeout); + }, timeout, this._sessionClosePromise()); } /** @@ -274,7 +280,7 @@ class Page extends EventEmitter { if (typeof urlOrPredicate === 'function') return !!(urlOrPredicate(response)); return false; - }, timeout); + }, timeout, this._sessionClosePromise()); } /** diff --git a/experimental/puppeteer-firefox/lib/TimeoutSettings.js b/experimental/puppeteer-firefox/lib/TimeoutSettings.js index c6b08d39671c9..de7cbbf47dae3 100644 --- a/experimental/puppeteer-firefox/lib/TimeoutSettings.js +++ b/experimental/puppeteer-firefox/lib/TimeoutSettings.js @@ -47,6 +47,9 @@ class TimeoutSettings { return DEFAULT_TIMEOUT; } + /** + * @return {number} + */ timeout() { if (this._defaultTimeout !== null) return this._defaultTimeout; diff --git a/experimental/puppeteer-firefox/lib/helper.js b/experimental/puppeteer-firefox/lib/helper.js index 78dd01d6889e0..1f417215c6a83 100644 --- a/experimental/puppeteer-firefox/lib/helper.js +++ b/experimental/puppeteer-firefox/lib/helper.js @@ -121,9 +121,11 @@ class Helper { * @param {!NodeJS.EventEmitter} emitter * @param {(string|symbol)} eventName * @param {function} predicate + * @param {number} timeout + * @param {!Promise} abortPromise * @return {!Promise} */ - static waitForEvent(emitter, eventName, predicate, timeout) { + static async waitForEvent(emitter, eventName, predicate, timeout, abortPromise) { let eventTimeout, resolveCallback, rejectCallback; const promise = new Promise((resolve, reject) => { resolveCallback = resolve; @@ -132,12 +134,10 @@ class Helper { const listener = Helper.addEventListener(emitter, eventName, event => { if (!predicate(event)) return; - cleanup(); resolveCallback(event); }); if (timeout) { eventTimeout = setTimeout(() => { - cleanup(); rejectCallback(new TimeoutError('Timeout exceeded while waiting for event')); }, timeout); } @@ -145,7 +145,16 @@ class Helper { Helper.removeEventListeners([listener]); clearTimeout(eventTimeout); } - return promise; + const result = await Promise.race([promise, abortPromise]).then(r => { + cleanup(); + return r; + }, e => { + cleanup(); + throw e; + }); + if (result instanceof Error) + throw result; + return result; } /** diff --git a/lib/Page.js b/lib/Page.js index dac0797f03f80..0ec70f53c56d7 100644 --- a/lib/Page.js +++ b/lib/Page.js @@ -694,6 +694,12 @@ class Page extends EventEmitter { return await this._frameManager.mainFrame().waitForNavigation(options); } + _sessionClosePromise() { + if (!this._disconnectPromise) + this._disconnectPromise = new Promise(fulfill => this._client.once(Events.CDPSession.Disconnected, () => fulfill(new Error('Target closed')))); + return this._disconnectPromise; + } + /** * @param {(string|Function)} urlOrPredicate * @param {!{timeout?: number}=} options @@ -709,7 +715,7 @@ class Page extends EventEmitter { if (typeof urlOrPredicate === 'function') return !!(urlOrPredicate(request)); return false; - }, timeout); + }, timeout, this._sessionClosePromise()); } /** @@ -727,7 +733,7 @@ class Page extends EventEmitter { if (typeof urlOrPredicate === 'function') return !!(urlOrPredicate(response)); return false; - }, timeout); + }, timeout, this._sessionClosePromise()); } /** diff --git a/lib/helper.js b/lib/helper.js index 217b04df5ccf4..d6bd31439e222 100644 --- a/lib/helper.js +++ b/lib/helper.js @@ -176,9 +176,11 @@ class Helper { * @param {!NodeJS.EventEmitter} emitter * @param {(string|symbol)} eventName * @param {function} predicate + * @param {number} timeout + * @param {!Promise} abortPromise * @return {!Promise} */ - static waitForEvent(emitter, eventName, predicate, timeout) { + static async waitForEvent(emitter, eventName, predicate, timeout, abortPromise) { let eventTimeout, resolveCallback, rejectCallback; const promise = new Promise((resolve, reject) => { resolveCallback = resolve; @@ -187,12 +189,10 @@ class Helper { const listener = Helper.addEventListener(emitter, eventName, event => { if (!predicate(event)) return; - cleanup(); resolveCallback(event); }); if (timeout) { eventTimeout = setTimeout(() => { - cleanup(); rejectCallback(new TimeoutError('Timeout exceeded while waiting for event')); }, timeout); } @@ -200,7 +200,16 @@ class Helper { Helper.removeEventListeners([listener]); clearTimeout(eventTimeout); } - return promise; + const result = await Promise.race([promise, abortPromise]).then(r => { + cleanup(); + return r; + }, e => { + cleanup(); + throw e; + }); + if (result instanceof Error) + throw result; + return result; } /** diff --git a/test/launcher.spec.js b/test/launcher.spec.js index 41a1e2b063802..b993eb2020d3c 100644 --- a/test/launcher.spec.js +++ b/test/launcher.spec.js @@ -84,6 +84,23 @@ module.exports.addTests = function({testRunner, expect, defaultBrowserOptions, p await browser.close(); }); }); + describe('Browser.close', function() { + it('should terminate network waiters', async({context, server}) => { + const browser = await puppeteer.launch(defaultBrowserOptions); + const remote = await puppeteer.connect({browserWSEndpoint: browser.wsEndpoint()}); + const newPage = await remote.newPage(); + const results = await Promise.all([ + newPage.waitForRequest(server.EMPTY_PAGE).catch(e => e), + newPage.waitForResponse(server.EMPTY_PAGE).catch(e => e), + browser.close() + ]); + for (let i = 0; i < 2; i++) { + const message = results[i].message; + expect(message).toContain('Target closed'); + expect(message).not.toContain('Timeout'); + } + }); + }); describe('Puppeteer.launch', function() { it('should reject all promises when browser is closed', async() => { const browser = await puppeteer.launch(defaultBrowserOptions); diff --git a/test/page.spec.js b/test/page.spec.js index b3fc082d6fc22..2bd3195eba9ee 100644 --- a/test/page.spec.js +++ b/test/page.spec.js @@ -77,6 +77,19 @@ module.exports.addTests = function({testRunner, expect, headless, puppeteer, CHR await newPage.close(); expect(newPage.isClosed()).toBe(true); }); + it('should terminate network waiters', async({context, server}) => { + const newPage = await context.newPage(); + const results = await Promise.all([ + newPage.waitForRequest(server.EMPTY_PAGE).catch(e => e), + newPage.waitForResponse(server.EMPTY_PAGE).catch(e => e), + newPage.close() + ]); + for (let i = 0; i < 2; i++) { + const message = results[i].message; + expect(message).toContain('Target closed'); + expect(message).not.toContain('Timeout'); + } + }); }); describe('Page.Events.Load', function() {