From e7aacf6aada83a06e9ea7ab52282aad892a5647e Mon Sep 17 00:00:00 2001 From: Yury Semikhatsky Date: Mon, 19 Aug 2019 12:09:22 -0700 Subject: [PATCH 1/8] fix --- lib/Page.js | 17 ++++++++++++++--- lib/helper.js | 31 ++++++++++++++++++++++++++----- test/launcher.spec.js | 17 +++++++++++++++++ test/page.spec.js | 13 +++++++++++++ 4 files changed, 70 insertions(+), 8 deletions(-) diff --git a/lib/Page.js b/lib/Page.js index dac0797f03f80..923a73bcd36b4 100644 --- a/lib/Page.js +++ b/lib/Page.js @@ -25,7 +25,7 @@ const {EmulationManager} = require('./EmulationManager'); const {FrameManager} = require('./FrameManager'); const {Keyboard, Mouse, Touchscreen} = require('./Input'); const Tracing = require('./Tracing'); -const {helper, debugError, assert} = require('./helper'); +const {helper, debugError, assert, DisposablePromise} = require('./helper'); const {Coverage} = require('./Coverage'); const {Worker} = require('./Worker'); const {createJSHandle} = require('./JSHandle'); @@ -694,6 +694,17 @@ class Page extends EventEmitter { return await this._frameManager.mainFrame().waitForNavigation(options); } + _abortOnDisconnect() { + let reject; + const promise = new Promise((res, rej) => reject = rej); + const listener = helper.addEventListener(this._client, Events.CDPSession.Disconnected, (event) => + { + console.log("disconnected " + event); + reject(new Error('Target closed')); + }); + return new DisposablePromise(promise, () => helper.removeEventListeners([listener])); + } + /** * @param {(string|Function)} urlOrPredicate * @param {!{timeout?: number}=} options @@ -709,7 +720,7 @@ class Page extends EventEmitter { if (typeof urlOrPredicate === 'function') return !!(urlOrPredicate(request)); return false; - }, timeout); + }, timeout, this._abortOnDisconnect()); } /** @@ -727,7 +738,7 @@ class Page extends EventEmitter { if (typeof urlOrPredicate === 'function') return !!(urlOrPredicate(response)); return false; - }, timeout); + }, timeout, this._abortOnDisconnect()); } /** diff --git a/lib/helper.js b/lib/helper.js index 217b04df5ccf4..27b0b70ef4c5b 100644 --- a/lib/helper.js +++ b/lib/helper.js @@ -17,6 +17,21 @@ const {TimeoutError} = require('./Errors'); const debugError = require('debug')(`puppeteer:error`); const fs = require('fs'); +class DisposablePromise { + constructor(promise, disposeCallback) { + this._promise = promise; + this._onDispose = disposeCallback; + } + + promise() { + return this._promise; + } + + dispose() { + this._onDispose(); + } +} + class Helper { /** * @param {Function|string} fun @@ -178,7 +193,7 @@ class Helper { * @param {function} predicate * @return {!Promise} */ - static waitForEvent(emitter, eventName, predicate, timeout) { + static waitForEvent(emitter, eventName, predicate, timeout, abortSignal) { let eventTimeout, resolveCallback, rejectCallback; const promise = new Promise((resolve, reject) => { resolveCallback = resolve; @@ -187,20 +202,25 @@ 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); } function cleanup() { Helper.removeEventListeners([listener]); clearTimeout(eventTimeout); + abortSignal.dispose(); } - return promise; + return Promise.race([promise, abortSignal]).then(r => { + cleanup(); + return r; + }, e => { + cleanup(); + throw e; + }); } /** @@ -273,5 +293,6 @@ function assert(value, message) { module.exports = { helper: Helper, assert, - debugError + debugError, + DisposablePromise }; diff --git a/test/launcher.spec.js b/test/launcher.spec.js index 41a1e2b063802..cfc7153184028 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_fails_ffox('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..a111ceadc9eaf 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_fails_ffox('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() { From 2336d69310ca171ef304f96fe789037af69992a1 Mon Sep 17 00:00:00 2001 From: Yury Semikhatsky Date: Mon, 19 Aug 2019 16:31:50 -0700 Subject: [PATCH 2/8] fix --- lib/Page.js | 6 +----- lib/helper.js | 2 +- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/lib/Page.js b/lib/Page.js index 923a73bcd36b4..23e07598e438d 100644 --- a/lib/Page.js +++ b/lib/Page.js @@ -697,11 +697,7 @@ class Page extends EventEmitter { _abortOnDisconnect() { let reject; const promise = new Promise((res, rej) => reject = rej); - const listener = helper.addEventListener(this._client, Events.CDPSession.Disconnected, (event) => - { - console.log("disconnected " + event); - reject(new Error('Target closed')); - }); + const listener = helper.addEventListener(this._client, Events.CDPSession.Disconnected, event => reject(new Error('Target closed'))); return new DisposablePromise(promise, () => helper.removeEventListeners([listener])); } diff --git a/lib/helper.js b/lib/helper.js index 27b0b70ef4c5b..f0ef1434c31cf 100644 --- a/lib/helper.js +++ b/lib/helper.js @@ -214,7 +214,7 @@ class Helper { clearTimeout(eventTimeout); abortSignal.dispose(); } - return Promise.race([promise, abortSignal]).then(r => { + return Promise.race([promise, abortSignal.promise()]).then(r => { cleanup(); return r; }, e => { From b933d6397924da1986ea7b80e1eae2f53d4a264d Mon Sep 17 00:00:00 2001 From: Yury Semikhatsky Date: Mon, 19 Aug 2019 17:13:47 -0700 Subject: [PATCH 3/8] Use Session.disconnectPromise to listen for closure --- lib/Page.js | 13 ++++++------- lib/helper.js | 9 ++++++--- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/lib/Page.js b/lib/Page.js index 23e07598e438d..016def8aeea1b 100644 --- a/lib/Page.js +++ b/lib/Page.js @@ -694,11 +694,10 @@ class Page extends EventEmitter { return await this._frameManager.mainFrame().waitForNavigation(options); } - _abortOnDisconnect() { - let reject; - const promise = new Promise((res, rej) => reject = rej); - const listener = helper.addEventListener(this._client, Events.CDPSession.Disconnected, event => reject(new Error('Target closed'))); - return new DisposablePromise(promise, () => helper.removeEventListeners([listener])); + _sessionClosePromise() { + if (!this._disconnectPromise) + this._disconnectPromise = new Promise(fulfill => this._client.on(Events.CDPSession.Disconnected, () => fulfill(new Error('Target closed')))); + return this._disconnectPromise; } /** @@ -716,7 +715,7 @@ class Page extends EventEmitter { if (typeof urlOrPredicate === 'function') return !!(urlOrPredicate(request)); return false; - }, timeout, this._abortOnDisconnect()); + }, timeout, this._sessionClosePromise()); } /** @@ -734,7 +733,7 @@ class Page extends EventEmitter { if (typeof urlOrPredicate === 'function') return !!(urlOrPredicate(response)); return false; - }, timeout, this._abortOnDisconnect()); + }, timeout, this._sessionClosePromise()); } /** diff --git a/lib/helper.js b/lib/helper.js index f0ef1434c31cf..958808d2ac43d 100644 --- a/lib/helper.js +++ b/lib/helper.js @@ -191,9 +191,10 @@ class Helper { * @param {!NodeJS.EventEmitter} emitter * @param {(string|symbol)} eventName * @param {function} predicate + * @param {!Promise} abortPromise * @return {!Promise} */ - static waitForEvent(emitter, eventName, predicate, timeout, abortSignal) { + static async waitForEvent(emitter, eventName, predicate, timeout, abortPromise) { let eventTimeout, resolveCallback, rejectCallback; const promise = new Promise((resolve, reject) => { resolveCallback = resolve; @@ -212,15 +213,17 @@ class Helper { function cleanup() { Helper.removeEventListeners([listener]); clearTimeout(eventTimeout); - abortSignal.dispose(); } - return Promise.race([promise, abortSignal.promise()]).then(r => { + const result = await Promise.race([promise, abortPromise]).then(r => { cleanup(); return r; }, e => { cleanup(); throw e; }); + if (result instanceof Error) + throw result; + return result; } /** From 936825042b663dbadbd3b93e3e5318fd911e35cd Mon Sep 17 00:00:00 2001 From: Yury Semikhatsky Date: Mon, 19 Aug 2019 17:18:39 -0700 Subject: [PATCH 4/8] Removed unused code --- lib/Page.js | 2 +- lib/helper.js | 18 +----------------- 2 files changed, 2 insertions(+), 18 deletions(-) diff --git a/lib/Page.js b/lib/Page.js index 016def8aeea1b..bdc1740c9ad10 100644 --- a/lib/Page.js +++ b/lib/Page.js @@ -25,7 +25,7 @@ const {EmulationManager} = require('./EmulationManager'); const {FrameManager} = require('./FrameManager'); const {Keyboard, Mouse, Touchscreen} = require('./Input'); const Tracing = require('./Tracing'); -const {helper, debugError, assert, DisposablePromise} = require('./helper'); +const {helper, debugError, assert} = require('./helper'); const {Coverage} = require('./Coverage'); const {Worker} = require('./Worker'); const {createJSHandle} = require('./JSHandle'); diff --git a/lib/helper.js b/lib/helper.js index 958808d2ac43d..70eb1bb4b9fbe 100644 --- a/lib/helper.js +++ b/lib/helper.js @@ -17,21 +17,6 @@ const {TimeoutError} = require('./Errors'); const debugError = require('debug')(`puppeteer:error`); const fs = require('fs'); -class DisposablePromise { - constructor(promise, disposeCallback) { - this._promise = promise; - this._onDispose = disposeCallback; - } - - promise() { - return this._promise; - } - - dispose() { - this._onDispose(); - } -} - class Helper { /** * @param {Function|string} fun @@ -296,6 +281,5 @@ function assert(value, message) { module.exports = { helper: Helper, assert, - debugError, - DisposablePromise + debugError }; From 53253fabcf46a3249851f9f8dee498b5f17c7223 Mon Sep 17 00:00:00 2001 From: Yury Semikhatsky Date: Mon, 19 Aug 2019 18:05:31 -0700 Subject: [PATCH 5/8] fix linter --- lib/Page.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Page.js b/lib/Page.js index bdc1740c9ad10..0ec70f53c56d7 100644 --- a/lib/Page.js +++ b/lib/Page.js @@ -696,7 +696,7 @@ class Page extends EventEmitter { _sessionClosePromise() { if (!this._disconnectPromise) - this._disconnectPromise = new Promise(fulfill => this._client.on(Events.CDPSession.Disconnected, () => fulfill(new Error('Target closed')))); + this._disconnectPromise = new Promise(fulfill => this._client.once(Events.CDPSession.Disconnected, () => fulfill(new Error('Target closed')))); return this._disconnectPromise; } From f1fbfbb5d3305fb3c91148296a8068aeaa970990 Mon Sep 17 00:00:00 2001 From: Yury Semikhatsky Date: Tue, 20 Aug 2019 08:51:44 -0700 Subject: [PATCH 6/8] Added missing type annotation for timeout --- experimental/puppeteer-firefox/lib/TimeoutSettings.js | 3 +++ lib/helper.js | 1 + 2 files changed, 4 insertions(+) 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/lib/helper.js b/lib/helper.js index 70eb1bb4b9fbe..d6bd31439e222 100644 --- a/lib/helper.js +++ b/lib/helper.js @@ -176,6 +176,7 @@ class Helper { * @param {!NodeJS.EventEmitter} emitter * @param {(string|symbol)} eventName * @param {function} predicate + * @param {number} timeout * @param {!Promise} abortPromise * @return {!Promise} */ From a0591fd7eaa217c033aa63ca14234fd67789228f Mon Sep 17 00:00:00 2001 From: Yury Semikhatsky Date: Tue, 20 Aug 2019 09:02:21 -0700 Subject: [PATCH 7/8] Make Page.close test pass on FF --- experimental/puppeteer-firefox/lib/Page.js | 10 ++++++++-- experimental/puppeteer-firefox/lib/helper.js | 17 +++++++++++++---- test/page.spec.js | 2 +- 3 files changed, 22 insertions(+), 7 deletions(-) diff --git a/experimental/puppeteer-firefox/lib/Page.js b/experimental/puppeteer-firefox/lib/Page.js index 8c3898a811f64..6dbb589e642b6 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 = this._target._isClosedPromise.then(() => 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/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/test/page.spec.js b/test/page.spec.js index a111ceadc9eaf..2bd3195eba9ee 100644 --- a/test/page.spec.js +++ b/test/page.spec.js @@ -77,7 +77,7 @@ module.exports.addTests = function({testRunner, expect, headless, puppeteer, CHR await newPage.close(); expect(newPage.isClosed()).toBe(true); }); - it_fails_ffox('should terminate network waiters', async({context, server}) => { + 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), From 90a8f2daaa47542704830dbc1bb9207ae6b6768d Mon Sep 17 00:00:00 2001 From: Yury Semikhatsky Date: Wed, 21 Aug 2019 10:02:21 -0700 Subject: [PATCH 8/8] Make test pass in FF --- experimental/puppeteer-firefox/lib/Page.js | 2 +- test/launcher.spec.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/experimental/puppeteer-firefox/lib/Page.js b/experimental/puppeteer-firefox/lib/Page.js index 6dbb589e642b6..c2ce1bdb3ed6f 100644 --- a/experimental/puppeteer-firefox/lib/Page.js +++ b/experimental/puppeteer-firefox/lib/Page.js @@ -243,7 +243,7 @@ class Page extends EventEmitter { _sessionClosePromise() { if (!this._disconnectPromise) - this._disconnectPromise = this._target._isClosedPromise.then(() => new Error('Target closed')); + this._disconnectPromise = new Promise(fulfill => this._session.once(Events.JugglerSession.Disconnected, () => fulfill(new Error('Target closed')))); return this._disconnectPromise; } diff --git a/test/launcher.spec.js b/test/launcher.spec.js index cfc7153184028..b993eb2020d3c 100644 --- a/test/launcher.spec.js +++ b/test/launcher.spec.js @@ -85,7 +85,7 @@ module.exports.addTests = function({testRunner, expect, defaultBrowserOptions, p }); }); describe('Browser.close', function() { - it_fails_ffox('should terminate network waiters', async({context, server}) => { + 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();