Do not leave dangling promises when sending messages over protocol (#742)

It's very bad to have 'unhandled promise rejection' that can't be
handled in user code. These errors will exit node process in a near
future.

This patch avoids 'unhandled promise rejection' while sending protocol
messages.

This patch:
- introduces `puppeteer:error` debug scope and starts using it for all
  swalloed errors.
- makes sure that every `client.send` method is either awaited or its
  errors are handled.
- starts return promises from Request.continue() and Request.abort().
- starts swallow errors from Request.contine() and Request.abort().

The last is the most important part of the patch. Since
`Request.continue()` might try to continue canceled request, we should
disregard the error.

Fixes #627.

docs/api.md

@@ -1314,6 +1314,7 @@ If request fails at some point, then instead of 'requestfinished' event (and pos
 If request gets a 'redirect' response, the request is successfully finished with the 'requestfinished' event, and a new request is  issued to a redirected url.
 
 #### request.abort()
+- returns: <[Promise]>
 
 Aborts request. To use this, request interception should be enabled with `page.setRequestInterceptionEnabled`.
 Exception is immediately thrown if the request interception is not enabled.
@@ -1324,6 +1325,7 @@ Exception is immediately thrown if the request interception is not enabled.
   - `method` <[string]> If set changes the request method (e.g. `GET` or `POST`)
   - `postData` <[string]> If set changes the post data of request
   - `headers` <[Object]> If set changes the request HTTP headers
+- returns: <[Promise]>
 
 Continues request with optional request overrides. To use this, request interception should be enabled with `page.setRequestInterceptionEnabled`.
 Exception is immediately thrown if the request interception is not enabled.

lib/Browser.js

@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-const helper = require('./helper');
+const {helper} = require('./helper');
 const Page = require('./Page');
 
 class Browser {

lib/Dialog.js

@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-const helper = require('./helper');
+const {helper} = require('./helper');
 
 class Dialog {
   /**

lib/ElementHandle.js

@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 const path = require('path');
-const helper = require('./helper');
+const {helper} = require('./helper');
 
 class ElementHandle {
   /**

lib/FrameManager.js

@@ -16,7 +16,7 @@
 
 const fs = require('fs');
 const EventEmitter = require('events');
-const helper = require('./helper');
+const {helper} = require('./helper');
 const ElementHandle = require('./ElementHandle');
 
 class FrameManager extends EventEmitter {

lib/Input.js

@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-const helper = require('./helper');
+const {helper} = require('./helper');
 
 class Keyboard {
   /**

lib/Launcher.js

@@ -22,7 +22,7 @@ const Connection = require('./Connection');
 const Browser = require('./Browser');
 const readline = require('readline');
 const fs = require('fs');
-const helper = require('./helper');
+const {helper} = require('./helper');
 const ChromiumRevision = require('../package.json').puppeteer.chromium_revision;
 
 const CHROME_PROFILE_PATH = path.join(os.tmpdir(), 'puppeteer_dev_profile-');

lib/NavigatorWatcher.js

@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-const helper = require('./helper');
+const {helper} = require('./helper');
 
 class NavigatorWatcher {
   /**

lib/NetworkManager.js

@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 const EventEmitter = require('events');
-const helper = require('./helper');
+const {helper, debugError} = require('./helper');
 const Multimap = require('./Multimap');
 const url = require('url');
 
@@ -66,7 +66,7 @@ class NetworkManager extends EventEmitter {
    * @param {string} userAgent
    */
   async setUserAgent(userAgent) {
-    return this._client.send('Network.setUserAgentOverride', { userAgent });
+    await this._client.send('Network.setUserAgentOverride', { userAgent });
   }
 
   /**
@@ -240,32 +240,40 @@ class Request {
   /**
    * @param {!Object=} overrides
    */
-  continue(overrides = {}) {
+  async continue(overrides = {}) {
     // DataURL's are not interceptable. In this case, do nothing.
     if (this.url.startsWith('data:'))
       return;
     console.assert(this._interceptionId, 'Request Interception is not enabled!');
     console.assert(!this._interceptionHandled, 'Request is already handled!');
     this._interceptionHandled = true;
-    this._client.send('Network.continueInterceptedRequest', {
+    await this._client.send('Network.continueInterceptedRequest', {
       interceptionId: this._interceptionId,
       url: overrides.url,
       method: overrides.method,
       postData: overrides.postData,
       headers: overrides.headers,
+    }).catch(error => {
+      // In certain cases, protocol will return error if the request was already canceled
+      // or the page was closed. We should tolerate these errors.
+      debugError(error);
     });
   }
 
-  abort() {
+  async abort() {
     // DataURL's are not interceptable. In this case, do nothing.
     if (this.url.startsWith('data:'))
       return;
     console.assert(this._interceptionId, 'Request Interception is not enabled!');
     console.assert(!this._interceptionHandled, 'Request is already handled!');
     this._interceptionHandled = true;
-    this._client.send('Network.continueInterceptedRequest', {
+    await this._client.send('Network.continueInterceptedRequest', {
       interceptionId: this._interceptionId,
       errorReason: 'Failed'
+    }).catch(error => {
+      // In certain cases, protocol will return error if the request was already canceled
+      // or the page was closed. We should tolerate these errors.
+      debugError(error);
     });
   }
 }

lib/Page.js

@@ -24,7 +24,7 @@ const EmulationManager = require('./EmulationManager');
 const FrameManager = require('./FrameManager');
 const {Keyboard, Mouse, Touchscreen} = require('./Input');
 const Tracing = require('./Tracing');
-const helper = require('./helper');
+const {helper, debugError} = require('./helper');
 
 class Page extends EventEmitter {
   /**
@@ -153,7 +153,7 @@ class Page extends EventEmitter {
     this._client.send('Security.handleCertificateError', {
       eventId: event.eventId,
       action: 'continue'
-    });
+    }).catch(debugError);
   }
 
   /**
@@ -293,7 +293,7 @@ class Page extends EventEmitter {
       const {name, seq, args} = JSON.parse(event.args[1].value);
       const result = await this._pageBindings[name](...args);
       const expression = helper.evaluationString(deliverResult, name, seq, result);
-      this._client.send('Runtime.evaluate', { expression });
+      this._client.send('Runtime.evaluate', { expression }).catch(debugError);
 
       function deliverResult(name, seq, result) {
         window[name]['callbacks'].get(seq)(result);
@@ -387,8 +387,11 @@ class Page extends EventEmitter {
    * @return {!Promise<?Response>}
    */
   async reload(options) {
-    this._client.send('Page.reload');
-    return this.waitForNavigation(options);
+    const [response] = await Promise.all([
+      this.waitForNavigation(options),
+      this._client.send('Page.reload')
+    ]);
+    return response;
   }
 
   /**
@@ -431,9 +434,11 @@ class Page extends EventEmitter {
     const entry = history.entries[history.currentIndex + delta];
     if (!entry)
       return null;
-    const result = this.waitForNavigation(options);
-    this._client.send('Page.navigateToHistoryEntry', {entryId: entry.id});
-    return result;
+    const [response] = await Promise.all([
+      this.waitForNavigation(options),
+      this._client.send('Page.navigateToHistoryEntry', {entryId: entry.id}),
+    ]);
+    return response;
   }
 
   /**

lib/Puppeteer.js

@@ -13,7 +13,7 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-const helper = require('./helper');
+const {helper} = require('./helper');
 const Launcher = require('./Launcher');
 
 class Puppeteer {

lib/Tracing.js

@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 const fs = require('fs');
-const helper = require('./helper');
+const {helper} = require('./helper');
 
 class Tracing {
   /**

lib/helper.js

@@ -14,6 +14,7 @@
  * limitations under the License.
  */
 
+const debugError = require('debug')(`puppeteer:error`);
 /** @type {?Map<string, boolean>} */
 let apiCoverage = null;
 class Helper {
@@ -94,12 +95,11 @@ class Helper {
   static async releaseObject(client, remoteObject) {
     if (!remoteObject.objectId)
       return;
-    try {
-      await client.send('Runtime.releaseObject', {objectId: remoteObject.objectId});
-    } catch (e) {
+    await client.send('Runtime.releaseObject', {objectId: remoteObject.objectId}).catch(error => {
       // Exceptions might happen in case of a page been navigated or closed.
       // Swallow these since they are harmless and we don't leak anything in this case.
-    }
+      debugError(error);
+    });
   }
 
   /**
@@ -217,4 +217,7 @@ class Helper {
   }
 }
 
-module.exports = Helper;
+module.exports = {
+  helper: Helper,
+  debugError
+};

test/test.js

@@ -17,7 +17,7 @@
 const fs = require('fs');
 const rm = require('rimraf').sync;
 const path = require('path');
-const helper = require('../lib/helper');
+const {helper} = require('../lib/helper');
 if (process.env.COVERAGE)
   helper.recordPublicAPICoverage();
 console.log('Testing on Node', process.version);
@@ -986,6 +986,20 @@ describe('Page', function() {
       expect(requests.length).toBe(2);
       expect(requests[1].response().status).toBe(404);
     }));
+    it('should not throw "Invalid Interception Id" if the request was cancelled', SX(async function() {
+      await page.setContent('<iframe></iframe>');
+      await page.setRequestInterceptionEnabled(true);
+      let request = null;
+      page.on('request', async r => request = r);
+      page.$eval('iframe', (frame, url) => frame.src = url, EMPTY_PAGE),
+      // Wait for request interception.
+      await waitForEvents(page, 'request');
+      // Delete frame to cause request to be canceled.
+      await page.$eval('iframe', frame => frame.remove());
+      let error = null;
+      await request.continue().catch(e => error = e);
+      expect(error).toBe(null);
+    }));
   });
 
   describe('Page.Events.Dialog', function() {