From b04f95a586548afaab637d581577e737c3fc7b1f Mon Sep 17 00:00:00 2001
From: Joe Walker <jwalker@mozilla.com>
Date: Thu, 12 Mar 2015 13:51:34 +0000
Subject: [PATCH] runat-1128988: Pass Errors through protocol.js correctly

In Output.toJson we were assuming that `JSON.stringify(new Error('x'))`
would give us something useful. It actually just gives `{}`, so we
manually extract the properties of a standard Error.

We were using `error:true` to denote an error, but this was conflicting
with a special property in protocol.js, so we're using isError instead.
(Commands that fail are considered non-exceptional. Exceptions in GCLI
are for when GCLI itself fails)

The converters now need to cope with errors that are objects that look
like Errors (but not actually errors).

In the process of debugging this I refactored the code that kicks off
a remote execution in system.js.

Signed-off-by: Joe Walker <jwalker@mozilla.com>
---
 lib/gcli/cli.js                   | 16 ++++++++++++++--
 lib/gcli/converters/converters.js | 11 ++++++++++-
 lib/gcli/system.js                |  8 +-------
 3 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/lib/gcli/cli.js b/lib/gcli/cli.js
index 532ba001..161b18ce 100644
--- a/lib/gcli/cli.js
+++ b/lib/gcli/cli.js
@@ -2164,11 +2164,23 @@ Output.prototype.convert = function(type, conversionContext) {
 };
 
 Output.prototype.toJson = function() {
+  // Exceptions don't stringify, so we try a bit harder
+  var data = this.data;
+  if (this.error && JSON.stringify(this.data) === '{}') {
+    data = {
+      columnNumber: data.columnNumber,
+      fileName: data.fileName,
+      lineNumber: data.lineNumber,
+      message: data.message,
+      stack: data.stack
+    };
+  }
+
   return {
     typed: this.typed,
     type: this.type,
-    data: this.data,
-    error: this.error
+    data: data,
+    isError: this.error
   };
 };
 
diff --git a/lib/gcli/converters/converters.js b/lib/gcli/converters/converters.js
index 545755a4..84cab292 100644
--- a/lib/gcli/converters/converters.js
+++ b/lib/gcli/converters/converters.js
@@ -99,7 +99,7 @@ var errorDomConverter = {
   exec: function(ex, conversionContext) {
     var node = util.createElement(conversionContext.document, 'p');
     node.className = 'gcli-error';
-    node.textContent = ex;
+    node.textContent = errorStringConverter.exec(ex, conversionContext);
     return node;
   }
 };
@@ -112,6 +112,15 @@ var errorStringConverter = {
   from: 'error',
   to: 'string',
   exec: function(ex, conversionContext) {
+    if (typeof ex === 'string') {
+      return ex;
+    }
+    if (ex instanceof Error) {
+      return '' + ex;
+    }
+    if (typeof ex.message === 'string') {
+      return ex.message;
+    }
     return '' + ex;
   }
 };
diff --git a/lib/gcli/system.js b/lib/gcli/system.js
index 9d22ffbb..89c79e01 100644
--- a/lib/gcli/system.js
+++ b/lib/gcli/system.js
@@ -308,15 +308,9 @@ function addLocalFunctions(specs, front) {
     if (!commandSpec.isParent) {
       commandSpec.exec = function(args, context) {
         var typed = (context.prefix ? context.prefix + ' ' : '') + context.typed;
-
         return front.execute(typed).then(function(reply) {
           var typedData = context.typedData(reply.type, reply.data);
-          if (!reply.error) {
-            return typedData;
-          }
-          else {
-            throw typedData;
-          }
+          return reply.isError ? Promise.reject(typedData) : typedData;
         });
       };
     }