Issue 4579 - Wrap rejection reason in Error object

polyfill.js

Index: polyfill.js
===================================================================
--- a/polyfill.js
+++ b/polyfill.js
@@ -55,16 +55,18 @@
 
       if (!object)
         return;
     }
 
     let func = object[name];
     object[name] = function(...args)
     {
+      let callStack = new Error().stack;
+
       if (typeof args[args.length - 1] == "function")
         return func.apply(object, args);
 
       // If the last argument is undefined, we drop it from the list assuming
       // it stands for the optional callback. We must do this, because we have
       // to replace it with our own callback. If we simply append our own
       // callback to the list, it won't match the signature of the function and
       // will cause an exception.
@@ -72,22 +74,42 @@
         args.pop();
 
       return new Promise((resolve, reject) =>
       {
         func.call(object, ...args, result =>
         {
           let error = browser.runtime.lastError;
           if (error)
+          {
+            // runtime.lastError on Chrome is a plain object with only a
+            // message property.
+            if (!(error instanceof Error))

[Sebastian Noack, 2017/10/19 04:30:24]

Why checking whether it is an error, if we know that it is anyway? If Chrome (or
other browsers) would change that, they would not only break this polyfill but
also every extension that checks for async errors.

[Manish Jethani, 2017/10/19 09:35:39]

It seems that only Chrome's runtime.lastError is not an Error instance. It may
be an Error instance on Edge, or maybe this is just omitted from the
documentation [1]

[1]:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/runtime/lastError
Since Chrome came first, I think extensions would assume by default that
runtime.lastError is not an Error instance but rather a plain object, so if
browsers wanted to change it to be an Error instance they could do so in a
backwards compatible way. I'm guessing that's what Edge is doing already.

[Manish Jethani, 2017/10/19 09:37:39]

Ollie just confirmed on IRC that it is in fact an Error instance on Edge.

+              error = new Error(error.message);
+
+            // Add a more helpful stack trace.
+            error.stack = callStack.replace(/^Error\n {4}at Object\./,

[Manish Jethani, 2017/10/19 00:32:13]

The stack trace from the caller's side is actually useful for debugging. If we
simply pass along the original stack trace it's useless.

Since this is only for debugging, we don't have to worry about compatibility
with Firefox.

Also the stack trace says "Object.runtime.sendMessage" so we replace it with
"browser.runtime.sendMessage"

[Sebastian Noack, 2017/10/19 04:30:24]

Not sure whether this is worth it:

* Firefox doesn't even provide these information. 
* Can we assume the assumptions your string replacement here is based on
  to be consistent across (different versions of) Chrome and Microsoft Edge?
* Creating an Error object and extracting it's stack in advance, regardless
  whether an error is going to occur, might add some unnecessary overhead.

[Wladimir Palant, 2017/10/19 07:51:35]

Making assumptions about the stack format and doing search&replace on it is
definitely the wrong thing to do. As to providing a more useful stack trace in
general, this is a good idea - way too often I see errors pop up in Chrome and
have trouble figuring out where these came from. The overhead retrieving the
stack trace should be negligible, particularly given that these are always async
calls that won't be used in a tight loop.

[Manish Jethani, 2017/10/19 09:35:39]

I think I see that this was a bit of overkill. I've removed it now. The stack
property is only useful for debugging and we get enough information as it is.
I agree with Wladimir here, it's useful and probably not expensive.

+                                            "Error\n    at browser.");
             reject(error);
+          }
           else
+          {
             resolve(result);
+          }
         });
       });
     };
+
+    // Set the name for debugging.
+    Object.defineProperty(object[name], "name", {

[Manish Jethani, 2017/10/19 00:32:13]

If we do this we get the name of the API in the stack trace.

[Sebastian Noack, 2017/10/19 04:30:24]

IMO this goes to far. Not even the original chrome.* API has useful names set
for their functions:

  > chrome.runtime.sendMessage
  ƒ () {
            var args = $Array.slice(arguments);
            $Object.setPrototypeOf(args, null);
            if (this.updateArgumentsPreValidate)
              args = $Function.apply(this.updateArguments…
  > chrome.runtime.sendMessage.name
  "bound "

[Manish Jethani, 2017/10/19 09:35:39]

Yes, but when the call throws an error synchronously, we do get the proper API
name in the trace. Since we're trying to present a synchronous-like error for an
asynchronous call, it would be ideal to add the proper API name as well.

Anyway, by default it presents as "Error at Object.object.(anonymous function)
[as sendMessage]" rather than "Error at browser.runtime.sendMessage". I think
this is fine on balance. I've removed this part of the change. It would a bit
confusing to tell the difference between storage.local.get and
storage.managed.get here, but that's OK, since we also get the stack trace with
line numbers and everything.

+      writable: false,
+      enumerable: false,
+      configurable: true,
+      value: api
+    });
   }
 
   function shouldWrapAPIs()
   {
     try
     {
       return !(browser.storage.local.get([]) instanceof Promise);
     }