Unify logic of stack trace generation for extension errors

extensions/renderer/resources/event.js

Index: extensions/renderer/resources/event.js
diff --git a/extensions/renderer/resources/event.js b/extensions/renderer/resources/event.js
index f62ab0ff21752e49a378396905b48c2d51bf2d98..8fe59deccf3d663960aa10ad5c4f06c30f04d2f1 100644
--- a/extensions/renderer/resources/event.js
+++ b/extensions/renderer/resources/event.js
@@ -102,7 +102,7 @@
     var id = eventNatives.AttachFilteredEvent(this.event_.eventName,
                                               listener.filters || {});
     if (id == -1)
-      throw new Error("Can't add listener");
+      throw new $Error.self("Can't add listener");

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

new Error()

     listener.id = id;
     this.listenerMap_[id] = listener;
     FilteredAttachmentStrategy.idToEventMap[id] = this.event_;
@@ -115,7 +115,7 @@
   FilteredAttachmentStrategy.prototype.detachListener =
       function(listener, manual) {
     if (listener.id == undefined)
-      throw new Error("listener.id undefined - '" + listener + "'");
+      throw new $Error.self("listener.id undefined - '" + listener + "'");
     var id = listener.id;
     delete this.listenerMap_[id];
     delete FilteredAttachmentStrategy.idToEventMap[id];
@@ -201,7 +201,8 @@
 
     if (!this.eventName) {
       if (this.eventOptions.supportsRules)

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

Oh man some of these are so subtle. Errors that we expect the extension to
handle should still be Error. Internal errors should be $Error. Ugh.

I'm wondering whether a correct solution to this would be to have an assert()
function exposed to the custom bindings (similar to how privates() is exposed)
which basically throws an $Error, but continue to throw an Error for those that
are intended to be passed back to the extension. I can't expect future
generations to know the distinction between $Error and Error.

In the meantime let's submit this, and I'll point out the places where we could
continue to throw a new Error. Basically anywhere where the validation is in
response to the extension vs our own internal state.

(in this file it's all those "this event doesn't support blah" and *really* we
should be solving that by having different types for each Event. ah well)

[robwu, 2014/08/19 17:35:57]

Now I'm a bit confused. Why should $Error.self be changed to Error?
$Error.self === window.Error if the extension hasn't modified the global, right?

[not at google - send to devlin, 2014/08/19 17:54:38]

There are 2 scenarios here:

(1) The error is the extension's fault, for example passing in an invalid schema
for a function. That throws an Error.
In this case, we would want the extension to be able to do something like:

window.Error.prototype.debugMe() = function() {
  console.log(whatever);
};
...
try {
  chrome.runtime.connect(42);
} catch(e) {
  e.debugMe();
}

some non-contrived version of that. Let them monkey patch however much they
want.


(2) The error is our fault, and we want to deal with that without worrying about
the extension mucking things up. For example, the following should *not* break
us:

window.Error = function(message) {
  console.log('constructed error', message);
};

but it probably would.


My suggestion for a solution here is rather than relying on people that write
these bindings to remember to use $Error for internal errors and Error for
external errors, just provide an assert() method which implicitly throws the
equivalent of $Error. assert() would be more convenient anyway.

Honestly we already provide logging.[D]CHECK. Maybe we should just forget about
this assert() thing and just make those easier to use (e.g. insert them in every
context rather than requiring we requireNative('logging') to access them).

[robwu, 2014/08/19 20:51:37]

I have reverted every use of "throw new $Error.self" (patch set 5). The only
thing that uses $Error is $Error.captureStackTrace in
uncaught_exception_handler.js.
It is safe to use this potentially unsafe constructor for throwing, because the
code at that point is already throwing an error. Extension code that catches
errors should use the new safeErrorToString method that I added for getting an
error message (see patch set 5).

-        throw new Error("Events that support rules require an event name.");
+        throw new $Error.self(

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

new Error()

+            "Events that support rules require an event name.");
       // Events without names cannot be managed by the browser by definition
       // (the browser has no way of identifying them).
       this.eventOptions.unmanaged = true;
@@ -227,7 +228,7 @@
   // its listeners.
   function registerArgumentMassager(name, callback) {
     if (eventArgumentMassagers[name])
-      throw new Error("Massager already registered for event: " + name);
+      throw new $Error.self("Massager already registered for event: " + name);
     eventArgumentMassagers[name] = callback;
   }
 
@@ -259,19 +260,19 @@
   // Registers a callback to be called when this event is dispatched.
   EventImpl.prototype.addListener = function(cb, filters) {
     if (!this.eventOptions.supportsListeners)
-      throw new Error("This event does not support listeners.");
+      throw new $Error.self("This event does not support listeners.");

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

new Error()

     if (this.eventOptions.maxListeners &&
         this.getListenerCount_() >= this.eventOptions.maxListeners) {
-      throw new Error("Too many listeners for " + this.eventName);
+      throw new $Error.self("Too many listeners for " + this.eventName);

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

new Error()

     }
     if (filters) {
       if (!this.eventOptions.supportsFilters)
-        throw new Error("This event does not support filters.");
+        throw new $Error.self("This event does not support filters.");
       if (filters.url && !(filters.url instanceof Array))
-        throw new Error("filters.url should be an array.");
+        throw new $Error.self("filters.url should be an array.");
       if (filters.serviceType &&
           !(typeof filters.serviceType === 'string')) {
-        throw new Error("filters.serviceType should be a string.")
+        throw new $Error.self("filters.serviceType should be a string.");

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

new Error() for all of these

       }
     }
     var listener = {callback: cb, filters: filters};
@@ -286,7 +287,7 @@
       allAttachedEvents[allAttachedEvents.length] = this;
       if (this.eventName) {
         if (attachedNamedEvents[this.eventName]) {
-          throw new Error("Event '" + this.eventName +
+          throw new $Error.self("Event '" + this.eventName +
                           "' is already attached.");
         }
         attachedNamedEvents[this.eventName] = this;
@@ -297,7 +298,7 @@
   // Unregisters a callback.
   EventImpl.prototype.removeListener = function(cb) {
     if (!this.eventOptions.supportsListeners)
-      throw new Error("This event does not support listeners.");
+      throw new $Error.self("This event does not support listeners.");

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

new Error()

 
     var idx = this.findListener_(cb);
     if (idx == -1)
@@ -312,7 +313,7 @@
         delete allAttachedEvents[i];
       if (this.eventName) {
         if (!attachedNamedEvents[this.eventName]) {
-          throw new Error(
+          throw new $Error.self(
               "Event '" + this.eventName + "' is not attached.");
         }
         delete attachedNamedEvents[this.eventName];
@@ -323,7 +324,7 @@
   // Test if the given callback is registered for this event.
   EventImpl.prototype.hasListener = function(cb) {
     if (!this.eventOptions.supportsListeners)
-      throw new Error("This event does not support listeners.");
+      throw new $Error.self("This event does not support listeners.");

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

new Error()

     return this.findListener_(cb) > -1;
   };
 
@@ -335,7 +336,7 @@
   // Returns the number of listeners on this event.
   EventImpl.prototype.getListenerCount_ = function() {
     if (!this.eventOptions.supportsListeners)
-      throw new Error("This event does not support listeners.");
+      throw new $Error.self("This event does not support listeners.");

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

new Error()

     return this.listeners.length;
   };
 
@@ -353,11 +354,11 @@
 
   EventImpl.prototype.dispatch_ = function(args, listenerIDs) {
     if (this.destroyed) {
-      throw new Error(this.eventName + ' was already destroyed at: ' +
+      throw new $Error.self(this.eventName + ' was already destroyed at: ' +
                       this.destroyed);
     }
     if (!this.eventOptions.supportsListeners)
-      throw new Error("This event does not support listeners.");
+      throw new $Error.self("This event does not support listeners.");

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

new Error()

 
     if (this.argSchemas && logging.DCHECK_IS_ON()) {
       try {
@@ -414,7 +415,7 @@
 
   EventImpl.prototype.addRules = function(rules, opt_cb) {
     if (!this.eventOptions.supportsRules)
-      throw new Error("This event does not support rules.");
+      throw new $Error.self("This event does not support rules.");

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

new Error()

 
     // Takes a list of JSON datatype identifiers and returns a schema fragment
     // that verifies that a JSON object corresponds to an array of only these
@@ -445,7 +446,7 @@
     };
 
     if (!this.eventOptions.conditions || !this.eventOptions.actions) {
-      throw new Error('Event ' + this.eventName + ' misses ' +
+      throw new $Error.self('Event ' + this.eventName + ' misses ' +
                       'conditions or actions in the API specification.');
     }
 
@@ -467,7 +468,7 @@
 
   EventImpl.prototype.removeRules = function(ruleIdentifiers, opt_cb) {
     if (!this.eventOptions.supportsRules)
-      throw new Error("This event does not support rules.");
+      throw new $Error.self("This event does not support rules.");

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

new Error()

     ensureRuleSchemasLoaded();
     // We remove the first parameter from the validation to give the user more
     // meaningful error messages.
@@ -484,7 +485,7 @@
 
   EventImpl.prototype.getRules = function(ruleIdentifiers, cb) {
     if (!this.eventOptions.supportsRules)
-      throw new Error("This event does not support rules.");
+      throw new $Error.self("This event does not support rules.");

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

new Error()

     ensureRuleSchemasLoaded();
     // We remove the first parameter from the validation to give the user more
     // meaningful error messages.