Issue 29582713: Issue 4579 - Wrap rejection reason in Error object (Closed)
|
Created:
Oct. 19, 2017, 12:29 a.m. by Manish Jethani Modified:
Oct. 20, 2017, 10:51 a.m. Base URL:
https://hg.adblockplus.org/adblockpluschrome/ Visibility:
Public. |
DescriptionIssue 4579 - Wrap rejection reason in Error object
Patch Set 1 #
Total comments: 10
Patch Set 2 : Remove unnecessary code #
Total comments: 4
Patch Set 3 : Add stack trace only on Chrome #MessagesTotal messages: 12
Patch Set 1 https://codereview.adblockplus.org/29582713/diff/29582714/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29582713/diff/29582714/polyfill.js#newcode89 polyfill.js:89: error.stack = callStack.replace(/^Error\n {4}at Object\./, 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" https://codereview.adblockplus.org/29582713/diff/29582714/polyfill.js#newcode102 polyfill.js:102: Object.defineProperty(object[name], "name", { If we do this we get the name of the API in the stack trace.
https://codereview.adblockplus.org/29582713/diff/29582714/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29582713/diff/29582714/polyfill.js#newcode85 polyfill.js:85: if (!(error instanceof Error)) 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. https://codereview.adblockplus.org/29582713/diff/29582714/polyfill.js#newcode89 polyfill.js:89: error.stack = callStack.replace(/^Error\n {4}at Object\./, On 2017/10/19 00:32:13, Manish Jethani wrote: > 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" 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. https://codereview.adblockplus.org/29582713/diff/29582714/polyfill.js#newcode102 polyfill.js:102: Object.defineProperty(object[name], "name", { On 2017/10/19 00:32:13, Manish Jethani wrote: > If we do this we get the name of the API in the stack trace. 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 "
I mostly agree with Sebastian, with one exception. https://codereview.adblockplus.org/29582713/diff/29582714/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29582713/diff/29582714/polyfill.js#newcode89 polyfill.js:89: error.stack = callStack.replace(/^Error\n {4}at Object\./, 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.
Patch Set 2: Remove unnecessary code https://codereview.adblockplus.org/29582713/diff/29582714/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29582713/diff/29582714/polyfill.js#newcode85 polyfill.js:85: if (!(error instanceof Error)) On 2017/10/19 04:30:24, Sebastian Noack wrote: > Why checking whether it is an error, if we know that it is anyway? If Chrome (or 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 > other browsers) would change that, they would not only break this polyfill but > also every extension that checks for async errors. 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. https://codereview.adblockplus.org/29582713/diff/29582714/polyfill.js#newcode89 polyfill.js:89: error.stack = callStack.replace(/^Error\n {4}at Object\./, On 2017/10/19 04:30:24, Sebastian Noack wrote: > On 2017/10/19 00:32:13, Manish Jethani wrote: > > 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" > > 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? 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. > * Creating an Error object and extracting it's stack in advance, regardless > whether an error is going to occur, might add some unnecessary overhead. I agree with Wladimir here, it's useful and probably not expensive. https://codereview.adblockplus.org/29582713/diff/29582714/polyfill.js#newcode102 polyfill.js:102: Object.defineProperty(object[name], "name", { On 2017/10/19 04:30:24, Sebastian Noack wrote: > On 2017/10/19 00:32:13, Manish Jethani wrote: > > If we do this we get the name of the API in the stack trace. > > IMO this goes to far. Not even the original chrome.* API has useful names set > for their functions: > > [...] 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.
https://codereview.adblockplus.org/29582713/diff/29582714/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29582713/diff/29582714/polyfill.js#newcode85 polyfill.js:85: if (!(error instanceof Error)) On 2017/10/19 09:35:39, Manish Jethani wrote: > On 2017/10/19 04:30:24, Sebastian Noack wrote: > > Why checking whether it is an error, if we know that it is anyway? If Chrome > (or > > 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 Ollie just confirmed on IRC that it is in fact an Error instance on Edge.
LGTM
(By the way, have you been testing this on top of the webpack changes? The error messages might have gotten a bit better now we have proper source maps.)
https://codereview.adblockplus.org/29582713/diff/29583555/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29582713/diff/29583555/polyfill.js#newcode84 polyfill.js:84: // message property. Perhaps, this comment could be a little bit more explicit, mentioning that lastError is already an Error instance on Microsoft Edge. https://codereview.adblockplus.org/29582713/diff/29583555/polyfill.js#newcode89 polyfill.js:89: error.stack = callStack; How does the stack on Microsoft Edge (where browser.runtime.lastError is an Error instance) look like? Does it make sense to replace it, or should this line rather be moved inside the conditional block above?
Patch Set 3: Add stack trace only on Chrome https://codereview.adblockplus.org/29582713/diff/29583555/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29582713/diff/29583555/polyfill.js#newcode84 polyfill.js:84: // message property. On 2017/10/19 18:09:56, Sebastian Noack wrote: > Perhaps, this comment could be a little bit more explicit, mentioning that > lastError is already an Error instance on Microsoft Edge. Done. https://codereview.adblockplus.org/29582713/diff/29583555/polyfill.js#newcode89 polyfill.js:89: error.stack = callStack; On 2017/10/19 18:09:56, Sebastian Noack wrote: > How does the stack on Microsoft Edge (where browser.runtime.lastError is an > Error instance) look like? Does it make sense to replace it, or should this line > rather be moved inside the conditional block above? I'm not sure, but I think we should just move it inside the condition and let Ollie move it out if he needs the debugging info. This is really just for us developers, I don't suppose anyone else is interested in the stack trace.
LGTM
Message was sent while issue was closed.
LGTM 
|
