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 with a message that may be sent to the server.
+ * @param {string} message Error message. This message may be sent to the
+ *     server.
+ * @return {Error} Error object.
  */
-function debugAlert(message) {
-  if (DEBUG_MODE)
-    alert(message);
+function buildErrorWithMessageForServer(message) {
+  var error = new Error(message);
+  error.canSendMessageToServer = 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 buildErrorWithMessageForServer('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 48 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);
+      reportError(buildErrorWithMessageForServer(
+          '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 65 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.canSendMessageToServer ?
+        error.stack : error.stack.replace(/.*\n/, '(message removed)\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;
+      }
+
+      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];
+        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);

[arv (Not doing code reviews), 2013/08/12 21:31:30]

console.error(error)

also prints the stack. If you want to add extra info you can do:

console.error('Critical error', error)

[vadimt, 2013/08/12 23:32:22]

Couple of reasons why this would be not ideal:
1. console.error('Critical error', error) puts 'error' as an expandable object
in the output. In some cases, you cannot see the whole object unless you click
at it to expand. This is not good when, say, a tester copy-pastes console output
into the bug.
2. var message is used below for the alert, so we'd have code duplication.

+    if (!errorReported) {
+      errorReported = true;
+      chrome.metricsPrivate.getIsCrashReportingEnabled(function(isEnabled) {
+        if (isEnabled)

[skare_, 2013/08/12 20:47:14]

just checking, isEnabled == false and DEBUG_MODE == true is an expected-possible
case, correct?

[vadimt, 2013/08/12 20:57:39]

Correct.

+          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 31 matching lines @@
           verify(isInTask, 'wrapCallback: not in task at exit');
           isInTask = false;
           if (--taskPendingCallbackCount == 0)
             finish();
         }
 
         verify(isInInstrumentedCallback,
                'Instrumented callback is not instrumented upon exit');
         isInInstrumentedCallback = false;
       } 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);
       }
     };
   }
 
   /**
    * Returns an instrumented function.
    * @param {array} functionIdentifierParts Path to the chrome.* function.
    * @param {string} functionName Name of the chrome API function.
    * @param {number} callbackParameter Index of the callback parameter to this
    *     API function.
    * @return {function} An instrumented function.
    */
   function createInstrumentedFunction(
       functionIdentifierParts,
       functionName,
       callbackParameter) {
     return 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 ' +
-                   functionIdentifierParts.join('.') + '.' + functionName +
-                   ' is not a function');
+        reportError(buildErrorWithMessageForServer(
+            'Argument ' + callbackParameter + ' of ' +
+            functionIdentifierParts.join('.') + '.' + functionName +
+            ' is not a function'));
       }
       arguments[callbackParameter] = wrapCallback(
           callback, functionName == 'addListener');
 
       var chromeContainer = chrome;
       functionIdentifierParts.map(function(fragment) {
         chromeContainer = chromeContainer[fragment];
       });
       return chromeContainer[functionName].
           apply(chromeContainer, arguments);
     };
   }
 
   /**
    * Instruments an API function to add error processing to its user
    * code-provided callback.
    * @param {string} functionIdentifier Full identifier of the function without
    *     the 'chrome.' portion.
    * @param {number} callbackParameter Index of the callback parameter to this
    *     API function.
    */
   function instrumentChromeApiFunction(functionIdentifier, callbackParameter) {
     var functionIdentifierParts = functionIdentifier.split('.');
     var functionName = functionIdentifierParts.pop();
     var chromeContainer = chrome;
     var instrumentedContainer = instrumented;
     functionIdentifierParts.map(function(fragment) {
       chromeContainer = chromeContainer[fragment];
+      if (!chromeContainer) {
+        reportError(buildErrorWithMessageForServer(
+            'Cannot instrument ' + functionIdentifier));
+      }
+
       if (!(fragment in instrumentedContainer))
         instrumentedContainer[fragment] = {};
 
       instrumentedContainer = instrumentedContainer[fragment];
     });
 
     var targetFunction = chromeContainer[functionName];
-
-    if (!targetFunction)
-      debugAlert('Cannot instrument ' + functionName);
+    if (!targetFunction) {
+      reportError(buildErrorWithMessageForServer(
+          'Cannot instrument ' + functionIdentifier));
+    }
 
     instrumentedContainer[functionName] = createInstrumentedFunction(
         functionIdentifierParts,
         functionName,
         callbackParameter);
   }
 
   instrumentChromeApiFunction('alarms.get', 1);
   instrumentChromeApiFunction('alarms.onAlarm.addListener', 0);
   instrumentChromeApiFunction('identity.getAuthToken', 1);
@@ skipping 200 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
   };
 }