create single path for handling errors from the protocol (#977)

* create single path for handling errors from the protocol * make handleRawError synchronous
GoogleChrome · Nov 23, 2016 · 74a7665 · 74a7665
1 parent fcf697c
commit 74a7665
Show file tree

Hide file tree

Showing 3 changed files with 47 additions and 31 deletions.
diff --git a/lighthouse-core/gather/connections/connection.js b/lighthouse-core/gather/connections/connection.js
@@ -95,33 +95,41 @@ class Connection {
       const callback = this._callbacks.get(object.id);
       this._callbacks.delete(object.id);
 
-      if (object.error) {
-        return this._handleRawError(object, callback);
-      }
-      log.formatProtocol('method <= browser OK',
+      // handleRawError returns or throws synchronously; wrap to put into promise chain.
+      return callback.resolve(Promise.resolve().then(_ => {
+        if (object.error) {
+          return this.handleRawError(object.error, callback.method);
+        }
+
+        log.formatProtocol('method <= browser OK',
           {method: callback.method, params: object.result}, 'verbose');
-      callback.resolve(object.result);
-      return;
+        return object.result;
+      }));
     }
     log.formatProtocol('<= event',
         {method: object.method, params: object.params}, 'verbose');
     this.emitNotification(object.method, object.params);
   }
 
   /**
-   * @param {{error: {message: string}}} object
-   * @param {{reject: function(*), method: string}} callback
-   * @private
+   * Handles error responses from the protocol, absorbing errors we don't care
+   * about and throwing on the rest.
+   *
+   * Currently the only error ignored is from defensive calls of `DOM.disable`
+   * when already disabled.
+   * @param {{message: string}} error
+   * @param {string} method Protocol method that received the error response.
+   * @throws {Error}
+   * @protected
    */
-  _handleRawError(object, callback) {
-    // We proactively disable a few domains. Ignore any errors
-    if (object.error.message && object.error.message.includes('DOM agent hasn\'t been enabled')) {
-      callback.resolve();
+  handleRawError(error, method) {
+    // We proactively disable the DOM domain. Ignore any errors.
+    if (error.message && error.message.includes('DOM agent hasn\'t been enabled')) {
       return;
     }
-    log.formatProtocol('method <= browser ERR',
-        {method: callback.method}, 'error');
-    callback.reject(new Error(`Raw Protocol (${callback.method}) ${object.error.message}`));
+
+    log.formatProtocol('method <= browser ERR', {method}, 'error');
+    throw new Error(`Protocol error (${method}): ${error.message}`);
   }
 
   /**

diff --git a/lighthouse-core/gather/connections/extension.js b/lighthouse-core/gather/connections/extension.js
@@ -99,25 +99,34 @@ class ExtensionConnection extends Connection {
 
   /**
    * @override
-   * @param {!string} method
+   * @param {!string} command
    * @param {!Object} params
    * @return {!Promise}
    */
   sendCommand(command, params) {
     return new Promise((resolve, reject) => {
       log.formatProtocol('method => browser', {method: command, params: params}, 'verbose');
       if (!this._tabId) {
-        log.error('No tabId set for sendCommand');
+        log.error('ExtensionConnection', 'No tabId set for sendCommand');
       }
+
       chrome.debugger.sendCommand({tabId: this._tabId}, command, params, result => {
         if (chrome.runtime.lastError) {
-          log.formatProtocol('method <= browser ERR', {method: command, params: result}, 'error');
-          return reject(chrome.runtime.lastError);
-        }
-
-        if (result.wasThrown) {
-          log.formatProtocol('method <= browser ERR', {method: command, params: result}, 'error');
-          return reject(result.exceptionDetails);
+          // The error from the extension has a `message` property that is the
+          // stringified version of the actual protocol error object.
+          const message = chrome.runtime.lastError.message;
+          let error;
+          try {
+            error = JSON.parse(message);
+          } catch (e) {}
+          error = error || {message: 'Unknown debugger protocol error.'};
+
+          // handleRawError returns or throws synchronously, so try/catch awkwardly.
+          try {
+            return resolve(this.handleRawError(error, command));
+          } catch (err) {
+            return reject(err);
+          }
         }
 
         log.formatProtocol('method <= browser OK', {method: command, params: result}, 'verbose');

diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js
@@ -155,7 +155,10 @@ class Driver {
         } else {
           resolve(result.result.value);
         }
-      }).catch(reject);
+      }).catch(err => {
+        clearTimeout(asyncTimeout);
+        reject(err);
+      });
     });
   }
 
@@ -475,11 +478,7 @@ class Driver {
     // Disable any domains that could interfere or add overhead to the trace
     return this.sendCommand('Debugger.disable')
       .then(_ => this.sendCommand('CSS.disable'))
-      .then(_ => {
-        return this.sendCommand('DOM.disable')
-          // If it wasn't already enabled, it will throw; ignore these. See #861
-          .catch(_ => {});
-      })
+      .then(_ => this.sendCommand('DOM.disable'))
       // Enable Page domain to wait for Page.loadEventFired
       .then(_ => this.sendCommand('Page.enable'))
       .then(_ => this.sendCommand('Tracing.start', tracingOpts));