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

@@ -15,6 +15,7 @@
  */
 
 const NetworkManager = require('./NetworkManager');
+const helper = require('./helper');
 
 class NavigatorWatcher {
   /**
@@ -37,19 +38,37 @@ class NavigatorWatcher {
    * @return {!Promise<!Map<string, !Response>>}
    */
   async waitForNavigation() {
-    this._init();
+    this._inflightRequests = 0;
+    this._requestIds = new Set();
+    /** @type {!Map<string, !Response>} */
+    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 loadEventFired = new Promise(fulfill => {
-    let watchdog = new Promise(fulfill => this._maximumTimer = setTimeout(fulfill, this._timeout)).then(() => new Error('Navigation Timeout Exceeded: ' + this._timeout + 'ms exceeded'));
+      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<string, !Response>} */
-    this._responses = new Map();
   }
 
   _cleanup() {
-    this._client.removeListener('Network.requestWillBeSent', this._loadingStartedHandler);
+    helper.removeEventListeners(this._eventListeners);
-    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);
 
     clearTimeout(this._idleTimer);
     clearTimeout(this._maximumTimer);

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;

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() {