Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: abort page.waitForRequest/Response when page closes #4865

Merged
merged 8 commits into from Aug 21, 2019
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 8 additions & 2 deletions lib/Page.js
Expand Up @@ -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
Expand All @@ -709,7 +715,7 @@ class Page extends EventEmitter {
if (typeof urlOrPredicate === 'function')
return !!(urlOrPredicate(request));
return false;
}, timeout);
}, timeout, this._sessionClosePromise());
}

/**
Expand All @@ -727,7 +733,7 @@ class Page extends EventEmitter {
if (typeof urlOrPredicate === 'function')
return !!(urlOrPredicate(response));
return false;
}, timeout);
}, timeout, this._sessionClosePromise());
}

/**
Expand Down
16 changes: 12 additions & 4 deletions lib/helper.js
Expand Up @@ -176,9 +176,10 @@ class Helper {
* @param {!NodeJS.EventEmitter} emitter
* @param {(string|symbol)} eventName
* @param {function} predicate
* @param {!Promise<!Error>} abortPromise
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not you, but this method is missing a JSDoc for timeout. We should really lint for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Added a type annotation for this.

* @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;
Expand All @@ -187,20 +188,27 @@ 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);
}
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;
}

/**
Expand Down
17 changes: 17 additions & 0 deletions test/launcher.spec.js
Expand Up @@ -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);
Expand Down
13 changes: 13 additions & 0 deletions test/page.spec.js
Expand Up @@ -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() {
Expand Down