Incremental changes in error reporting

chrome/browser/resources/google_now/utility.js

Index: chrome/browser/resources/google_now/utility.js
diff --git a/chrome/browser/resources/google_now/utility.js b/chrome/browser/resources/google_now/utility.js
index 7a8b2b182a3eef8afec6ccb9da5e5b5d728e723b..4aa2a2f37ee30f7030ffd4e9e184e5206a399875 100644
--- a/chrome/browser/resources/google_now/utility.js
+++ b/chrome/browser/resources/google_now/utility.js
@@ -20,12 +20,14 @@ var NOTIFICATION_CARDS_URL = localStorage['server_url'];
 var DEBUG_MODE = localStorage['debug_mode'];
 
 /**
- * Shows a message popup in debug mode.
- * @param {string} message Diagnostic message.
+ * Builds an error object.
+ * @param {string} message Error message.
+ * @return {Error} Error object.
  */
-function debugAlert(message) {
-  if (DEBUG_MODE)
-    alert(message);
+function buildError(message) {

[rgustafson, 2013/08/08 20:14:01]

buildNonRuntimeError? buildCustomError? This is more than just an error, so this
should be more than buildError. If you have something better than my naming,
good.

[vadimt, 2013/08/08 20:25:28]

You cannot build a runtime error. And 'custom' is overloaded. Manual? Built?
Probably, we can leave it as is.

[skare_, 2013/08/09 23:02:18]

agree it might be nice to differentiate somehow; 'buildError' doesn't seem to
indicate the side effect that the notFromRuntime expando will have.

[rgustafson, 2013/08/09 23:08:22]

Yeah, I still think this needs a modifier. I don't think build has the wide
meaning that you think it does because I don't think that differentiates enough.
Also, now that I'm looking at it again, the run time error terminology is a bit
confusing too. afaik technically everything that isn't a parse error is a run
time error in js..

[vadimt, 2013/08/10 00:45:27]

Done.

[vadimt, 2013/08/10 00:45:27]

Done.

+  var error = new Error(message);
+  error.notFromRuntime = true;
+  return error;
 }
 
 /**
@@ -36,7 +38,7 @@ function debugAlert(message) {
  */
 function verify(condition, message) {
   if (!condition)
-    throw new Error('\nASSERT: ' + message);
+    throw buildError('ASSERT: ' + message);
 }
 
 /**
@@ -101,14 +103,8 @@ function buildTaskManager(areConflicting) {
    * Checks that we run in an instrumented callback.
    */
   function checkInInstrumentedCallback() {
-    if (!isInInstrumentedCallback) {
-      // Cannot use verify() since no one will catch the exception.
-      // This check will detect bugs at the development stage, and is very
-      // unlikely to be seen by users.
-      var error = 'Not in instrumented callback: ' + new Error().stack;
-      console.error(error);
-      debugAlert(error);
-    }
+    if (!isInInstrumentedCallback)
+      reportError(buildError('Not in instrumented callback'));
   }
 
   /**
@@ -193,29 +189,45 @@ function buildTaskManager(areConflicting) {
 
   /**
    * Sends an error report to the server.
-   * @param {Error} error Error to report.
+   * @param {Error} error Error to send.
    */
   function sendErrorReport(error) {
-    var filteredStack = error.stack.replace(/.*\n/, '\n');
+    var filteredStack = error.notFromRuntime ?

[skare_, 2013/08/09 23:02:18]

comment here could help future readers.

[vadimt, 2013/08/10 00:45:27]

New name is self-explaining.

+        error.stack : error.stack.replace(/.*\n/, 'JS runtime error\n');
     var file;
     var line;
-    var topFrameMatches = filteredStack.match(/\(.*\)/);
-    // topFrameMatches's example:
-    // (chrome-extension://pmofbkohncoogjjhahejjfbppikbjigm/utility.js:308:19)
-    var crashLocation = topFrameMatches && topFrameMatches[0];
-    if (crashLocation) {
-      var topFrameElements =
-          crashLocation.substring(1, crashLocation.length - 1).split(':');
-      // topFrameElements for the above example will look like:
-      // [0] chrome-extension
-      // [1] //pmofbkohncoogjjhahejjfbppikbjigm/utility.js
-      // [2] 308
-      // [3] 19
+    var topFrameLineMatch = filteredStack.match(/\n    at .*\n/);
+    var topFrame = topFrameLineMatch && topFrameLineMatch[0];
+    if (topFrame) {
+      // Examples of a frame:
+      // 1. '\n    at someFunction (chrome-extension://
+      //     pmofbkohncoogjjhahejjfbppikbjigm/background.js:915:15)\n'
+      // 2. '\n    at chrome-extension://pmofbkohncoogjjhahejjfbppikbjigm/
+      //     utility.js:269:18\n'
+      // 3. '\n    at Function.target.(anonymous function) (extensions::
+      //     SafeBuiltins:19:14)\n'
+      // 4. '\n    at Event.dispatchToListener (event_bindings:382:22)\n'
+      var errorLocation;
+      // Find the the parentheses at the end of the line, if any.
+      var parenthesesMatch = topFrame.match(/\(.*\)\n/);
+      if (parenthesesMatch && parenthesesMatch[0]) {
+        errorLocation =
+            parenthesesMatch[0].substring(1, parenthesesMatch[0].length - 2);
+      } else {
+        errorLocation = topFrame;

[rgustafson, 2013/08/08 20:14:01]

Have you seen this happen? I don't know if attempting to set file and line are
worth it in this case.

[vadimt, 2013/08/08 20:25:28]

Sure, this example (2.) with anonymous top frame.

+      }
+
+      var topFrameElements = errorLocation.split(':');
+      // topFrameElements is an array that ends like:
+      // [N-3] //pmofbkohncoogjjhahejjfbppikbjigm/utility.js
+      // [N-2] 308
+      // [N-1] 19
       if (topFrameElements.length >= 3) {
-        file = topFrameElements[0] + ':' + topFrameElements[1];
-        line = topFrameElements[2];
+        file = topFrameElements[topFrameElements.length - 3];

[rgustafson, 2013/08/08 20:14:01]

Seems weird to just drop the chrome-extension.

[vadimt, 2013/08/08 20:25:28]

That's not a big deal, but this way, we support all 4 kinds of a frame.
Probably, not worth added complexity to add chrome-extension://

[rgustafson, 2013/08/09 23:08:22]

Okay, that's fine.

+        line = topFrameElements[topFrameElements.length - 2];
       }
     }
+
     var requestParameters =
         'error=' + encodeURIComponent(error.name) +
         '&script=' + encodeURIComponent(file) +
@@ -230,6 +242,24 @@ function buildTaskManager(areConflicting) {
   }
 
   /**
+   * Reports an error to the server and the user, as appropriate.
+   * @param {Error} error Error to report.
+   */
+  function reportError(error) {
+    var message = 'Critical error:\n' + error.stack;
+    console.error(message);
+    if (!errorReported) {
+      errorReported = true;
+      chrome.metricsPrivate.getIsCrashReportingEnabled(function(isEnabled) {
+        if (isEnabled)
+          sendErrorReport(error);
+        if (DEBUG_MODE)
+          alert(message);
+      });
+    }
+  }
+
+  /**
    * Unique ID of the next callback.
    * @type {number}
    */
@@ -279,16 +309,7 @@ function buildTaskManager(areConflicting) {
             finish();
         }
       } catch (error) {
-        var message = 'Uncaught exception:\n' + error.stack;
-        console.error(message);
-        if (!errorReported) {
-          errorReported = true;
-          chrome.metricsPrivate.getIsCrashReportingEnabled(function(isEnabled) {
-            if (isEnabled)
-              sendErrorReport(error);
-          });
-          debugAlert(message);
-        }
+        reportError(error);
       }
     };
   }
@@ -305,15 +326,15 @@ function buildTaskManager(areConflicting) {
     var originalFunction = namespace[functionName];
 
     if (!originalFunction)
-      debugAlert('Cannot instrument ' + functionName);
+      reportError(buildError('Cannot instrument ' + functionName));
 
     namespace[functionName] = function() {
       // This is the wrapper for the API function. Pass the wrapped callback to
       // the original function.
       var callback = arguments[callbackParameter];
       if (typeof callback != 'function') {
-        debugAlert('Argument ' + callbackParameter + ' of ' + functionName +
-              ' is not a function');
+        reportError(buildError('Argument ' + callbackParameter + ' of ' +
+            functionName + ' is not a function'));
       }
       arguments[callbackParameter] = wrapCallback(
           callback, functionName == 'addListener');