Unify logic of stack trace generation for extension errors

extensions/renderer/resources/uncaught_exception_handler.js

Index: extensions/renderer/resources/uncaught_exception_handler.js
diff --git a/extensions/renderer/resources/uncaught_exception_handler.js b/extensions/renderer/resources/uncaught_exception_handler.js
index a3709b58f3c0f6c0e9de5d31e88e2381c5e167c5..58e81b114a9178ace41f3bf4cae100e09c559e67 100644
--- a/extensions/renderer/resources/uncaught_exception_handler.js
+++ b/extensions/renderer/resources/uncaught_exception_handler.js
@@ -4,18 +4,85 @@
 
 // Handles uncaught exceptions thrown by extensions. By default this is to
 // log an error message, but tests may override this behaviour.
-
 var handler = function(message, e) {
   console.error(message);
 };
 
-// |message| The message associated with the error.
-// |e|       The object that was thrown.
-exports.handle = function(message, e) {
+// message and asyncStack are safe strings,

[not at google - send to devlin, 2014/08/18 16:09:45]

Let's update to this to use JSDoc-style annotations. It's a failure of most of
the extension JS that we don't.

/**
 * <comment>.
 * 
 * @param {string} message
 *   <comment>
 * @param {Error=} error
 *   ...
 * @param {Array.<?>=} overrideStackTrace
 *   ...
 */

The best documentation I know of for JSDoc syntax is the closure compiler docs.

[robwu, 2014/08/18 20:59:47]

Done. http://usejsdoc.org is also a decent reference.

+// "e" could be a user-supplied object.

[Finnur, 2014/08/18 11:50:14]

It is a bit weird to see "e", given that the usual way of denoting params is to
use this notion: |param|.

[robwu, 2014/08/18 15:37:32]

Done (will send updated patch set after the review by Ben).

[not at google - send to devlin, 2014/08/18 16:09:45]

I also preferred the old message; "could be user-supplied object" - in other
words, it's optional? Could you say,

@param e
  The error that was thrown, if any. If no error was specified
  then <whatever happens>.

+function formatErrorMessage(message, e, asyncStack) {
+  // Append ": [error message]"
+  if (e) {
+    try {
+      message += ': ' + e;

[not at google - send to devlin, 2014/08/18 16:09:45]

You're relying on toString being defined for the error? I... guess that makes
sense. You might want to comment:

// Hopefully |e| defines a toString function or this may look
// like [object Object]. Error defines toString as 'Error: <message>'.

[robwu, 2014/08/18 20:59:46]

Done.

+    } catch (e) {
+      // This could be triggered by
+      // throw {toString: function() { throw 'Haha' } };
+      message += ': (cannot get error message)';

[not at google - send to devlin, 2014/08/18 16:09:45]

"Unknown error"?

[robwu, 2014/08/18 20:59:47]

I've chosen "Cannot get error message" because it is more specific than "unknown
error", thus presumably more easily Googleable. WDYT?

+    }
+  }
+
+  var stack;
+  try {
+    // If the stack was set, use it.
+    // e.stack could be void in the following common example:
+    // throw "Error message";
+    stack = $String.self(e && e.stack);
+  } catch (e) {}

[not at google - send to devlin, 2014/08/18 16:09:45]

When can this throw?

[robwu, 2014/08/18 20:59:47]

When an object with a throwing "get" getter is thrown:

throw {get stack() { return 'haahaha'; }};

+
+  // If a stack is not provided, capture a stack trace.
+  if (!asyncStack && !stack)

[not at google - send to devlin, 2014/08/18 16:09:45]

"asyncStack" assumes the intent of the caller was to pass in some asynchronous
stack trace. Though we know that this is *probably* the case, you could equally
get a stack trace with:

var stack;
try {
  trySomething();
} catch(e) {
  stack = e.stack;
}
aBunchOfOtherStuff();
if (stack)
  formatErrorMessage('oops', null, stack);

for whatever reason. That example is contrived, but the point is, that's not
asynchronous, it's just a different stack trace.

Which is a long way of saying: a better name would be something like
|overrideStackTrace|, and document it accordingly.

[robwu, 2014/08/18 20:59:47]

Done.

(for your specific example, note that if trySomething reached into user-supplied
code, then e could potentially be invalid (i.e. throwing, or not a string),
which violates the precondition that |asyncStack| is safe.)

I've called it |asyncStack| because the extra stack trace must not be a part of
the current stack trace. This can only be true if the call is asynchronous. If
the stack trace is allowed to be synchronous, then the code should implement
stack frame merge & deduplicate logic. This is complex and impossible to get
right, so I demand that the stack traces must be disjoint, by declaring the
extra stack trace as "asyncStack". 

+    stack = getStackTrace();
+
+  stack = filterExtensionStackTrace(stack);
+  if (stack)
+    message += '\n' + stack;
+
+  // If an asynchronouse stack trace was set, append it.
+  if (asyncStack)
+    message += '\n' + asyncStack;
+
+  return message;
+}
+
+function filterExtensionStackTrace(stack) {
+  if (!stack)
+    return '';
+  // Remove stack frames in the stack trace that and weren't associated with the

[Finnur, 2014/08/18 11:50:14]

Remove the word 'that'?

[robwu, 2014/08/18 15:37:32]

Removed "and", thanks for spotting.

+  // extension, to not confuse extension developers with internal details.
+  stack = $String.split(stack, '\n');
+  stack = $Array.filter(stack, function(line) {
+    return line.indexOf('chrome-extension://') >= 0;
+  });

[Finnur, 2014/08/18 11:50:14]

Is this filtering a necessary part of the solution or is it a nice-to-have? I'm
a bit worried we might in some cases filter out valuable stack traces for other
use cases than what you've tested...

[robwu, 2014/08/18 15:37:32]

The original stack handling logic also filtered the stack trace, so in that
regard the logic has not changed. One of the "valuable" stack traces that are is
by either method is the stack trace that originates from the JavaScript console.
This might or might not be a problem, but my last view is that it is not a
disaster because the snippets typed at the console are often small, and the
stack trace wouldn't be very useful (with stack frames like "...  at
<anonymous>:1:1").

+  return $Array.join(stack, '\n');
+}
+
+// Get a local reference to the captureStackTrace method to prevent extension
+// code from interfering with our method.
+var captureStackTrace = Error.captureStackTrace;
+function getStackTrace() {
+  var e = {};
+  captureStackTrace(e, getStackTrace);

[not at google - send to devlin, 2014/08/18 16:09:45]

Neat! I didn't know about this method.

+  return e.stack;
+}
+
+function getExtensionStackTrace() {
+  return filterExtensionStackTrace(getStackTrace());
+}
+
+// |message|     The message associated with the error.
+// |e|           The object that was thrown.
+// |asyncStack|  An optional stack trace to be appended to the message.
+//               It should not already be a part of e.stack.
+exports.handle = function(message, e, asyncStack) {

[not at google - send to devlin, 2014/08/18 16:09:45]

Also JSDoc this method (basically: any public method).

[robwu, 2014/08/18 20:59:47]

Done.

+  message = formatErrorMessage(message, e, asyncStack);
   handler(message, e);
 };
 
-// |newHandler| A function which matches |exports.handle|.
+// |newHandler| A function which matches |handler|.
 exports.setHandler = function(newHandler) {
   handler = newHandler;
 };
+
+exports.getStackTrace = getStackTrace;
+exports.getExtensionStackTrace = getExtensionStackTrace;