Skip to content

Commit

Permalink
fix(page): navigating 11 pages simultaneously should not throw warning (
Browse files Browse the repository at this point in the history
#3560)

NavigatorWatcher subscribes to Connection to get a `Disconnected` event,
causing us to hit the default max of 10 listeners constraint.

Technically we don't leak anything here and can safely bump
the maxListenersCount to Infinity.

However, we conveniently have `CDPSession`, and
can re-dispatch the event on it and keep the safety check in place.
  • Loading branch information
aslushnikov committed Nov 20, 2018
1 parent 86e0959 commit e2e43bc
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 2 deletions.
6 changes: 6 additions & 0 deletions lib/Connection.js
Expand Up @@ -228,6 +228,7 @@ class CDPSession extends EventEmitter {
callback.reject(rewriteError(callback.error, `Protocol error (${callback.method}): Target closed.`));
this._callbacks.clear();
this._connection = null;
this.emit(CDPSession.Events.Disconnected);
}

/**
Expand All @@ -240,6 +241,11 @@ class CDPSession extends EventEmitter {
return session;
}
}

CDPSession.Events = {
Disconnected: Symbol('CDPSession.Events.Disconnected'),
};

helper.tracePublicAPI(CDPSession);

/**
Expand Down
4 changes: 2 additions & 2 deletions lib/FrameManager.js
Expand Up @@ -20,7 +20,7 @@ const {helper, assert} = require('./helper');
const {ExecutionContext} = require('./ExecutionContext');
const {TimeoutError} = require('./Errors');
const {NetworkManager} = require('./NetworkManager');
const {Connection} = require('./Connection');
const {CDPSession} = require('./Connection');

const readFileAsync = helper.promisify(fs.readFile);

Expand Down Expand Up @@ -1162,7 +1162,7 @@ class NavigatorWatcher {
this._navigationRequest = null;
this._hasSameDocumentNavigation = false;
this._eventListeners = [
helper.addEventListener(Connection.fromSession(client), Connection.Events.Disconnected, () => this._terminate(new Error('Navigation failed because browser has disconnected!'))),
helper.addEventListener(client, CDPSession.Events.Disconnected, () => this._terminate(new Error('Navigation failed because browser has disconnected!'))),
helper.addEventListener(this._frameManager, FrameManager.Events.LifecycleEvent, this._checkLifecycleComplete.bind(this)),
helper.addEventListener(this._frameManager, FrameManager.Events.FrameNavigatedWithinDocument, this._navigatedWithinDocument.bind(this)),
helper.addEventListener(this._frameManager, FrameManager.Events.FrameDetached, this._onFrameDetached.bind(this)),
Expand Down
12 changes: 12 additions & 0 deletions test/page.spec.js
Expand Up @@ -752,6 +752,18 @@ module.exports.addTests = function({testRunner, expect, headless}) {
process.removeListener('warning', warningHandler);
expect(warning).toBe(null);
});
it('should not leak listeners during navigation of 11 pages', async({page, context, server}) => {
let warning = null;
const warningHandler = w => warning = w;
process.on('warning', warningHandler);
await Promise.all([...Array(20)].map(async() => {
const page = await context.newPage();
await page.goto(server.EMPTY_PAGE);
await page.close();
}));
process.removeListener('warning', warningHandler);
expect(warning).toBe(null);
});
it('should navigate to dataURL and fire dataURL requests', async({page, server}) => {
const requests = [];
page.on('request', request => requests.push(request));
Expand Down

0 comments on commit e2e43bc

Please sign in to comment.