From c4904b4e109653c4e69ed8add7dbaf5be35965f2 Mon Sep 17 00:00:00 2001 From: Andrey Lushnikov Date: Sat, 22 Jul 2017 17:03:58 -0700 Subject: [PATCH] Do not leak event listeners on navigation This patch: - introduces helper.addEventListener/helper.removeEventListeners to simplify event management - moves NavigatorWatchdog over to the helper.addEventListener to stop leaking event listeners --- lib/NavigatorWatcher.js | 52 ++++++++++++++++++++--------------------- lib/helper.js | 20 ++++++++++++++++ test/test.js | 11 ++++++++- 3 files changed, 56 insertions(+), 27 deletions(-) diff --git a/lib/NavigatorWatcher.js b/lib/NavigatorWatcher.js index e6ba97a2168..906ce840438 100644 --- a/lib/NavigatorWatcher.js +++ b/lib/NavigatorWatcher.js @@ -15,6 +15,7 @@ */ const NetworkManager = require('./NetworkManager'); +const helper = require('./helper'); class NavigatorWatcher { /** @@ -37,19 +38,37 @@ class NavigatorWatcher { * @return {!Promise>} */ async waitForNavigation() { - this._init(); + this._inflightRequests = 0; + this._requestIds = new Set(); + /** @type {!Map} */ + this._responses = new Map(); + + this._eventListeners = [ + helper.addEventListener(this._client, 'Network.requestWillBeSent', this._onLoadingStarted.bind(this)), + helper.addEventListener(this._client, 'Network.loadingFinished', this._onLoadingCompleted.bind(this)), + helper.addEventListener(this._client, 'Network.loadingFailed', this._onLoadingCompleted.bind(this)), + helper.addEventListener(this._client, 'Network.webSocketCreated', this._onLoadingStarted.bind(this)), + helper.addEventListener(this._client, 'Network.webSocketClosed', this._onLoadingCompleted.bind(this)), + helper.addEventListener(this._networkManager, NetworkManager.Events.Response, this._onResponse.bind(this)), + ]; + + let certificateError = new Promise(fulfill => { + this._eventListeners.push(helper.addEventListener(this._client, 'Security.certificateError', fulfill)); + }).then(error => 'SSL Certificate error: ' + error.errorType); - let certificateError = new Promise(fulfill => this._client.once('Security.certificateError', fulfill)) - .then(error => new Error('SSL Certiciate error: ' + error.errorType)); let networkIdle = new Promise(fulfill => this._networkIdleCallback = fulfill).then(() => null); - let loadEventFired = new Promise(fulfill => this._client.once('Page.loadEventFired', fulfill)).then(() => null); - let watchdog = new Promise(fulfill => this._maximumTimer = setTimeout(fulfill, this._timeout)).then(() => new Error('Navigation Timeout Exceeded: ' + this._timeout + 'ms exceeded')); + let loadEventFired = new Promise(fulfill => { + this._eventListeners.push(helper.addEventListener(this._client, 'Page.loadEventFired', fulfill)); + }).then(() => null); + + let watchdog = new Promise(fulfill => this._maximumTimer = setTimeout(fulfill, this._timeout)) + .then(() => 'Navigation Timeout Exceeded: ' + this._timeout + 'ms exceeded'); try { // Await for the command to throw exception in case of illegal arguments. const error = await Promise.race([certificateError, watchdog, this._waitUntil === 'load' ? loadEventFired : networkIdle]); if (error) - throw error; + throw new Error(error); return this._responses; } finally { this._cleanup(); @@ -86,29 +105,10 @@ class NavigatorWatcher { } _init() { - this._loadingStartedHandler = this._onLoadingStarted.bind(this); - this._loadingCompletedHandler = this._onLoadingCompleted.bind(this); - this._onResponseHandler = this._onResponse.bind(this); - this._client.on('Network.requestWillBeSent', this._loadingStartedHandler); - this._client.on('Network.loadingFinished', this._loadingCompletedHandler); - this._client.on('Network.loadingFailed', this._loadingCompletedHandler); - this._client.on('Network.webSocketCreated', this._loadingStartedHandler); - this._client.on('Network.webSocketClosed', this._loadingCompletedHandler); - this._networkManager.on(NetworkManager.Events.Response, this._onResponseHandler); - - this._inflightRequests = 0; - this._requestIds = new Set(); - /** @type {!Map} */ - this._responses = new Map(); } _cleanup() { - this._client.removeListener('Network.requestWillBeSent', this._loadingStartedHandler); - this._client.removeListener('Network.loadingFinished', this._loadingCompletedHandler); - this._client.removeListener('Network.loadingFailed', this._loadingCompletedHandler); - this._client.removeListener('Network.webSocketCreated', this._loadingStartedHandler); - this._client.removeListener('Network.webSocketClosed', this._loadingCompletedHandler); - this._networkManager.removeListener(NetworkManager.Events.Response, this._onResponseHandler); + helper.removeEventListeners(this._eventListeners); clearTimeout(this._idleTimer); clearTimeout(this._maximumTimer); diff --git a/lib/helper.js b/lib/helper.js index 94c53e1655f..364f0a5c4e1 100644 --- a/lib/helper.js +++ b/lib/helper.js @@ -115,6 +115,26 @@ class Helper { return `"${text}"`; } } + + /** + * @param {!EventEmitter} emitter + * @param {string} eventName + * @param {function(?)} handler + * @return {{emitter: !EventEmitter, eventName: string, handler: function(?)}} + */ + static addEventListener(emitter, eventName, handler) { + emitter.on(eventName, handler); + return { emitter, eventName, handler }; + } + + /** + * @param {!Array<{emitter: !EventEmitter, eventName: string, handler: function(?)}>} + */ + static removeEventListeners(listeners) { + for (let listener of listeners) + listener.emitter.removeListener(listener.eventName, listener.handler); + listeners.splice(0, listeners.length); + } } module.exports = Helper; diff --git a/test/test.js b/test/test.js index 6b854ca22e5..cac8fe46905 100644 --- a/test/test.js +++ b/test/test.js @@ -364,7 +364,7 @@ describe('Puppeteer', function() { } catch (e) { error = e; } - expect(error.message).toContain('SSL Certiciate error'); + expect(error.message).toContain('SSL Certificate error'); })); it('should fail when exceeding maximum navigation timeout', SX(async function() { let error = null; @@ -492,6 +492,15 @@ describe('Puppeteer', function() { // Expect navigation to succeed. expect(response.ok).toBe(true); })); + it('should not leak listeners durint navigation', SX(async function() { + let warning = null; + const warningHandler = w => warning = w; + process.on('warning', warningHandler); + for (let i = 0; i < 20; ++i) + await page.navigate(EMPTY_PAGE); + process.removeListener('warning', warningHandler); + expect(warning).toBe(null); + })); }); describe('Page.waitForNavigation', function() {