From 73f9c48081017c228ec1f1318c3c7946321e0538 Mon Sep 17 00:00:00 2001
From: Joel Einbinder <joel.einbinder@gmail.com>
Date: Thu, 14 Jun 2018 15:27:59 -0700
Subject: [PATCH] feat: nicer protocol error messages (#2742)

This patch:
- stops appending `undefined` to our protocol messages unnecessarily.
- rewrites `Cannot find execution context id` to `Execution context was destroyed, most likely because of a navigation.` when it occurs from a Puppeteer ExecutionContext. The error message is left alone if it occurs via a CDPSession.
---
 lib/Connection.js       | 18 ++++++++++++++++--
 lib/ExecutionContext.js | 16 +++++++++++++---
 test/cookies.spec.js    |  4 ++--
 test/page.spec.js       | 10 ++++++++++
 4 files changed, 41 insertions(+), 7 deletions(-)

diff --git a/lib/Connection.js b/lib/Connection.js
index 18fdedd4d83..1670a9ee70e 100644
--- a/lib/Connection.js
+++ b/lib/Connection.js
@@ -108,7 +108,7 @@ class Connection extends EventEmitter {
       if (callback) {
         this._callbacks.delete(object.id);
         if (object.error)
-          callback.reject(rewriteError(callback.error, `Protocol error (${callback.method}): ${object.error.message} ${object.error.data}`));
+          callback.reject(createProtocolError(callback.error, callback.method, object));
         else
           callback.resolve(object.result);
       }
@@ -213,7 +213,7 @@ class CDPSession extends EventEmitter {
       const callback = this._callbacks.get(object.id);
       this._callbacks.delete(object.id);
       if (object.error)
-        callback.reject(rewriteError(callback.error, `Protocol error (${callback.method}): ${object.error.message} ${object.error.data}`));
+        callback.reject(createProtocolError(callback.error, callback.method, object));
       else
         callback.resolve(object.result);
     } else {
@@ -256,6 +256,20 @@ class CDPSession extends EventEmitter {
 }
 helper.tracePublicAPI(CDPSession);
 
+/**
+ * @param {!Error} error
+ * @param {string} method
+ * @param {{error: {message: string, data: any}}} object
+ * @return {!Error}
+ */
+function createProtocolError(error, method, object) {
+  let message = `Protocol error (${method}): ${object.error.message}`;
+  if ('data' in object.error)
+    message += ` ${object.error.data}`;
+  if (object.error.message)
+    return rewriteError(error, message);
+}
+
 /**
  * @param {!Error} error
  * @param {string} message
diff --git a/lib/ExecutionContext.js b/lib/ExecutionContext.js
index 3f50809b90e..e1867bd1de6 100644
--- a/lib/ExecutionContext.js
+++ b/lib/ExecutionContext.js
@@ -64,13 +64,13 @@ class ExecutionContext {
     if (helper.isString(pageFunction)) {
       const contextId = this._contextId;
       const expression = /** @type {string} */ (pageFunction);
-      const { exceptionDetails, result: remoteObject } = await this._client.send('Runtime.evaluate', {
+      const {exceptionDetails, result: remoteObject} = await this._client.send('Runtime.evaluate', {
         expression,
         contextId,
         returnByValue: false,
         awaitPromise: true,
         userGesture: true
-      });
+      }).catch(rewriteError);
       if (exceptionDetails)
         throw new Error('Evaluation failed: ' + helper.getExceptionMessage(exceptionDetails));
       return this._objectHandleFactory(remoteObject);
@@ -83,7 +83,7 @@ class ExecutionContext {
       returnByValue: false,
       awaitPromise: true,
       userGesture: true
-    });
+    }).catch(rewriteError);
     if (exceptionDetails)
       throw new Error('Evaluation failed: ' + helper.getExceptionMessage(exceptionDetails));
     return this._objectHandleFactory(remoteObject);
@@ -116,6 +116,16 @@ class ExecutionContext {
       }
       return { value: arg };
     }
+
+    /**
+     * @param {!Error} error
+     * @return {!Protocol.Runtime.evaluateReturnValue}
+     */
+    function rewriteError(error) {
+      if (error.message.endsWith('Cannot find context with specified id'))
+        throw new Error('Execution context was destroyed, most likely because of a navigation.');
+      throw error;
+    }
   }
 
   /**
diff --git a/test/cookies.spec.js b/test/cookies.spec.js
index 4da87f33b0d..3db21ca9adc 100644
--- a/test/cookies.spec.js
+++ b/test/cookies.spec.js
@@ -124,7 +124,7 @@ module.exports.addTests = function({testRunner, expect}) {
         error = e;
       }
       expect(error).toBeTruthy();
-      expect(error.message).toEqual('Protocol error (Network.deleteCookies): At least one of the url and domain needs to be specified undefined');
+      expect(error.message).toEqual('Protocol error (Network.deleteCookies): At least one of the url and domain needs to be specified');
     });
 
     it('should not set a cookie with blank page URL', async function({page, server}) {
@@ -154,7 +154,7 @@ module.exports.addTests = function({testRunner, expect}) {
       }
       expect(error).toBeTruthy();
       expect(error.message).toEqual(
-          'Protocol error (Network.deleteCookies): At least one of the url and domain needs to be specified undefined'
+          'Protocol error (Network.deleteCookies): At least one of the url and domain needs to be specified'
       );
     });
 
diff --git a/test/page.spec.js b/test/page.spec.js
index 4d18336c43d..c29b98b17bc 100644
--- a/test/page.spec.js
+++ b/test/page.spec.js
@@ -226,6 +226,16 @@ module.exports.addTests = function({testRunner, expect, puppeteer, DeviceDescrip
         return audio.play();
       }
     });
+    it('should throw a nice error after a navigation', async({page, server}) => {
+      const executionContext = await page.mainFrame().executionContext();
+
+      await Promise.all([
+        page.waitForNavigation(),
+        executionContext.evaluate(() => window.location.reload())
+      ]);
+      const error = await executionContext.evaluate(() => null).catch(e => e);
+      expect(error.message).toContain('navigation');
+    });
   });
 
   describe('Page.setOfflineMode', function() {