Incremental changes in error reporting

chrome/browser/resources/google_now/utility.js

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 'use strict';
 
 /**
  * @fileoverview Utility objects and functions for Google Now extension.
  */
 
 // TODO(vadimt): Figure out the server name. Use it in the manifest and for
 // NOTIFICATION_CARDS_URL. Meanwhile, to use the feature, you need to manually
 // set the server name via local storage.
 
 /**
  * Notification server URL.
  */
 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;
 }
 
 /**
  * Checks for internal errors.
  * @param {boolean} condition Condition that must be true.
  * @param {string} message Diagnostic message for the case when the condition is
  *     false.
  */
 function verify(condition, message) {
   if (!condition)
-    throw new Error('\nASSERT: ' + message);
+    throw buildError('ASSERT: ' + message);
 }
 
 /**
  * Builds a request to the notification server.
  * @param {string} handlerName Server handler to send the request to.
  * @param {string} contentType Value for the Content-type header.
  * @return {XMLHttpRequest} Server request.
  */
 function buildServerRequest(handlerName, contentType) {
   var request = new XMLHttpRequest();
@@ skipping 44 matching lines @@
   /**
    * True if currently executed code runs in an instrumented callback.
    * @type {boolean}
    */
   var isInInstrumentedCallback = false;
 
   /**
    * 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'));
   }
 
   /**
    * Starts the first queued task.
    */
   function startFirst() {
     verify(queue.length >= 1, 'startFirst: queue is empty');
     verify(!isInTask, 'startFirst: already in task');
     isInTask = true;
 
@@ skipping 64 matching lines @@
 
     if (queue.length >= 1)
       startFirst();
   }
 
   // Limiting 1 error report per background page load.
   var errorReported = false;
 
   /**
    * 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) +
         '&line=' + encodeURIComponent(line) +
         '&trace=' + encodeURIComponent(filteredStack);
     var request = buildServerRequest('jserror',
                                      'application/x-www-form-urlencoded');
     request.onloadend = function(event) {
       console.log('sendErrorReport status: ' + request.status);
     };
     request.send(requestParameters);
   }
 
   /**
+   * 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}
    */
   var nextCallbackId = 0;
 
   /**
    * Adds error processing to an API callback.
    * @param {Function} callback Callback to instrument.
    * @param {boolean=} opt_isEventListener True if the callback is an event
    *      listener.
@@ skipping 29 matching lines @@
                'Instrumented callback is not instrumented upon exit');
         isInInstrumentedCallback = false;
 
         if (isTaskCallback) {
           verify(isInTask, 'wrapCallback: not in task at exit');
           isInTask = false;
           if (--taskPendingCallbackCount == 0)
             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);
       }
     };
   }
 
   /**
    * Instruments an API function to add error processing to its user
    * code-provided callback.
    * @param {Object} namespace Namespace of the API function.
    * @param {string} functionName Name of the API function.
    * @param {number} callbackParameter Index of the callback parameter to this
    *     API function.
    */
   function instrumentApiFunction(namespace, functionName, callbackParameter) {
     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');
       return originalFunction.apply(namespace, arguments);
     };
   }
 
   instrumentApiFunction(chrome.alarms, 'get', 1);
   instrumentApiFunction(chrome.alarms.onAlarm, 'addListener', 0);
   instrumentApiFunction(chrome.identity, 'getAuthToken', 1);
@@ skipping 201 matching lines @@
   // Poll for the sign in state every hour.
   // One hour is just an arbitrary amount of time chosen.
   chrome.alarms.create(alarmName, {periodInMinutes: 60});
 
   return {
     addListener: addListener,
     isSignedIn: isSignedIn,
     removeToken: removeToken
   };
 }