Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
482603002:
Unify logic of stack trace generation for extension errors (Closed)
|
Created:
6 years, 4 months ago by robwu Modified:
6 years, 4 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, sadrul, kalyank, ben+ash_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionUnify logic of generating a stack trace for extension errors
Include the stack trace of the code that triggered the error.
Refactored the error generation method to prevent extension
code from breaking the state of the internals.
BUG=404406
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291392
Patch Set 1 #
Total comments: 31
Patch Set 2 : Processed reviewers' comments #
Total comments: 4
Patch Set 3 : Add $Error and $String.indexOf to safe builtins #
Total comments: 31
Patch Set 4 : Revert every use of "throw new $Error.self" #Patch Set 5 : Add method to safely serialize a thrown object #Patch Set 6 : Rietveld not working? #Patch Set 7 : Update expected error message in test #Patch Set 8 : update expected stack trace length #Messages
Total messages: 34 (0 generated)
On the face of it, I'm supportive of any effort to make exceptions more understandable to the developers. However, I don't feel like I know this code well enough to review it. The biggest contribution (by number of lines) is in uncaught_exception_handler.js, and git blame tells me kalman touched all the lines in that file, so maybe start with him? Also, in order to contribute code to Chromium you need to sign a CLA (Note: if you are submitting on behalf of a corporation then the corporate one needs to be signed and you should use your corporate email to submit patches). https://code.google.com/legal/individual-cla-v1.0.html (individual) or https://code.google.com/legal/corporate-cla-v1.0.html (corporate) I have a few comments on the patch, see below. https://codereview.chromium.org/482603002/diff/1/chrome/test/data/extensions/... File chrome/test/data/extensions/api_test/bindings/exception_in_handler_should_not_crash/page.js (right): https://codereview.chromium.org/482603002/diff/1/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/bindings/exception_in_handler_should_not_crash/page.js:7: function thrownewError(message) { nit: Capitalize n in new? https://codereview.chromium.org/482603002/diff/1/extensions/renderer/resource... File extensions/renderer/resources/uncaught_exception_handler.js (right): https://codereview.chromium.org/482603002/diff/1/extensions/renderer/resource... extensions/renderer/resources/uncaught_exception_handler.js:12: // "e" could be a user-supplied object. It is a bit weird to see "e", given that the usual way of denoting params is to use this notion: |param|. https://codereview.chromium.org/482603002/diff/1/extensions/renderer/resource... extensions/renderer/resources/uncaught_exception_handler.js:51: // Remove stack frames in the stack trace that and weren't associated with the Remove the word 'that'? https://codereview.chromium.org/482603002/diff/1/extensions/renderer/resource... extensions/renderer/resources/uncaught_exception_handler.js:56: }); Is this filtering a necessary part of the solution or is it a nice-to-have? I'm a bit worried we might in some cases filter out valuable stack traces for other use cases than what you've tested...
Ben, could you take a look? https://codereview.chromium.org/482603002/diff/1/chrome/test/data/extensions/... File chrome/test/data/extensions/api_test/bindings/exception_in_handler_should_not_crash/page.js (right): https://codereview.chromium.org/482603002/diff/1/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/bindings/exception_in_handler_should_not_crash/page.js:7: function thrownewError(message) { On 2014/08/18 11:50:14, Finnur wrote: > nit: Capitalize n in new? The casing is intentional, this function does nothing else than "throw new Error", hence it is called "thrownewError". https://codereview.chromium.org/482603002/diff/1/extensions/renderer/resource... File extensions/renderer/resources/uncaught_exception_handler.js (right): https://codereview.chromium.org/482603002/diff/1/extensions/renderer/resource... extensions/renderer/resources/uncaught_exception_handler.js:12: // "e" could be a user-supplied object. On 2014/08/18 11:50:14, Finnur wrote: > It is a bit weird to see "e", given that the usual way of denoting params is to > use this notion: |param|. Done (will send updated patch set after the review by Ben). https://codereview.chromium.org/482603002/diff/1/extensions/renderer/resource... extensions/renderer/resources/uncaught_exception_handler.js:51: // Remove stack frames in the stack trace that and weren't associated with the On 2014/08/18 11:50:14, Finnur wrote: > Remove the word 'that'? Removed "and", thanks for spotting. https://codereview.chromium.org/482603002/diff/1/extensions/renderer/resource... extensions/renderer/resources/uncaught_exception_handler.js:56: }); On 2014/08/18 11:50:14, Finnur wrote: > Is this filtering a necessary part of the solution or is it a nice-to-have? I'm > a bit worried we might in some cases filter out valuable stack traces for other > use cases than what you've tested... The original stack handling logic also filtered the stack trace, so in that regard the logic has not changed. One of the "valuable" stack traces that are is by either method is the stack trace that originates from the JavaScript console. This might or might not be a problem, but my last view is that it is not a disaster because the snippets typed at the console are often small, and the stack trace wouldn't be very useful (with stack frames like "... at <anonymous>:1:1").
https://codereview.chromium.org/482603002/diff/1/chrome/test/data/extensions/... File chrome/test/data/extensions/api_test/bindings/exception_in_handler_should_not_crash/page.js (right): https://codereview.chromium.org/482603002/diff/1/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/bindings/exception_in_handler_should_not_crash/page.js:7: function thrownewError(message) { I don't see why it matters in this case *what* the function does. The style-guide requests this kind of casing: throwNewError.
Thanks for doing this! I've wanted this for a long time. https://codereview.chromium.org/482603002/diff/1/chrome/test/data/extensions/... File chrome/test/data/extensions/api_test/bindings/exception_in_handler_should_not_crash/page.js (right): https://codereview.chromium.org/482603002/diff/1/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/bindings/exception_in_handler_should_not_crash/page.js:7: function thrownewError(message) { On 2014/08/18 15:37:32, robwu wrote: > On 2014/08/18 11:50:14, Finnur wrote: > > nit: Capitalize n in new? > > The casing is intentional, this function does nothing else than "throw new > Error", hence it is called "thrownewError". Nevertheless, I would prefer throwNewError. https://codereview.chromium.org/482603002/diff/1/extensions/renderer/module_s... File extensions/renderer/module_system.cc (right): https://codereview.chromium.org/482603002/diff/1/extensions/renderer/module_s... extensions/renderer/module_system.cc:556: "var Error = window.Error;"); This isn't any more desirable than using Error in the code. Is the purpose to have code safe against the extension tampering with the environment? Probably wise, but that would be better as $Error and using SafeBuiltins. https://codereview.chromium.org/482603002/diff/1/extensions/renderer/resource... File extensions/renderer/resources/uncaught_exception_handler.js (right): https://codereview.chromium.org/482603002/diff/1/extensions/renderer/resource... extensions/renderer/resources/uncaught_exception_handler.js:11: // message and asyncStack are safe strings, Let's update to this to use JSDoc-style annotations. It's a failure of most of the extension JS that we don't. /** * <comment>. * * @param {string} message * <comment> * @param {Error=} error * ... * @param {Array.<?>=} overrideStackTrace * ... */ The best documentation I know of for JSDoc syntax is the closure compiler docs. https://codereview.chromium.org/482603002/diff/1/extensions/renderer/resource... extensions/renderer/resources/uncaught_exception_handler.js:12: // "e" could be a user-supplied object. On 2014/08/18 15:37:32, robwu wrote: > On 2014/08/18 11:50:14, Finnur wrote: > > It is a bit weird to see "e", given that the usual way of denoting params is > to > > use this notion: |param|. > > Done (will send updated patch set after the review by Ben). I also preferred the old message; "could be user-supplied object" - in other words, it's optional? Could you say, @param e The error that was thrown, if any. If no error was specified then <whatever happens>. https://codereview.chromium.org/482603002/diff/1/extensions/renderer/resource... extensions/renderer/resources/uncaught_exception_handler.js:17: message += ': ' + e; You're relying on toString being defined for the error? I... guess that makes sense. You might want to comment: // Hopefully |e| defines a toString function or this may look // like [object Object]. Error defines toString as 'Error: <message>'. https://codereview.chromium.org/482603002/diff/1/extensions/renderer/resource... extensions/renderer/resources/uncaught_exception_handler.js:21: message += ': (cannot get error message)'; "Unknown error"? https://codereview.chromium.org/482603002/diff/1/extensions/renderer/resource... extensions/renderer/resources/uncaught_exception_handler.js:31: } catch (e) {} When can this throw? https://codereview.chromium.org/482603002/diff/1/extensions/renderer/resource... extensions/renderer/resources/uncaught_exception_handler.js:34: if (!asyncStack && !stack) "asyncStack" assumes the intent of the caller was to pass in some asynchronous stack trace. Though we know that this is *probably* the case, you could equally get a stack trace with: var stack; try { trySomething(); } catch(e) { stack = e.stack; } aBunchOfOtherStuff(); if (stack) formatErrorMessage('oops', null, stack); for whatever reason. That example is contrived, but the point is, that's not asynchronous, it's just a different stack trace. Which is a long way of saying: a better name would be something like |overrideStackTrace|, and document it accordingly. https://codereview.chromium.org/482603002/diff/1/extensions/renderer/resource... extensions/renderer/resources/uncaught_exception_handler.js:65: captureStackTrace(e, getStackTrace); Neat! I didn't know about this method. https://codereview.chromium.org/482603002/diff/1/extensions/renderer/resource... extensions/renderer/resources/uncaught_exception_handler.js:77: exports.handle = function(message, e, asyncStack) { Also JSDoc this method (basically: any public method).
https://codereview.chromium.org/482603002/diff/1/chrome/test/data/extensions/... File chrome/test/data/extensions/api_test/bindings/exception_in_handler_should_not_crash/page.js (right): https://codereview.chromium.org/482603002/diff/1/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/bindings/exception_in_handler_should_not_crash/page.js:7: function thrownewError(message) { On 2014/08/18 16:09:44, kalman wrote: > On 2014/08/18 15:37:32, robwu wrote: > > On 2014/08/18 11:50:14, Finnur wrote: > > > nit: Capitalize n in new? > > > > The casing is intentional, this function does nothing else than "throw new > > Error", hence it is called "thrownewError". > > Nevertheless, I would prefer throwNewError. Done. https://codereview.chromium.org/482603002/diff/1/extensions/renderer/module_s... File extensions/renderer/module_system.cc (right): https://codereview.chromium.org/482603002/diff/1/extensions/renderer/module_s... extensions/renderer/module_system.cc:556: "var Error = window.Error;"); On 2014/08/18 16:09:44, kalman wrote: > This isn't any more desirable than using Error in the code. Is the purpose to > have code safe against the extension tampering with the environment? Probably > wise, but that would be better as $Error and using SafeBuiltins. This protects against redefining the global Error constructor, because extensions cannot modify the Error variable in the local scope. It is sufficient because most of the uses of Error are of the form "throw new Error". The only other use of the error constructor is Error.captureStackTrace, but that method is also stored in a local variable. I'm using Error instead of $Error to allow extension code to use "instanceof Error" and "Error.stackTraceLimit". I've added a comment. https://codereview.chromium.org/482603002/diff/1/extensions/renderer/resource... File extensions/renderer/resources/uncaught_exception_handler.js (right): https://codereview.chromium.org/482603002/diff/1/extensions/renderer/resource... extensions/renderer/resources/uncaught_exception_handler.js:11: // message and asyncStack are safe strings, On 2014/08/18 16:09:45, kalman wrote: > Let's update to this to use JSDoc-style annotations. It's a failure of most of > the extension JS that we don't. > > /** > * <comment>. > * > * @param {string} message > * <comment> > * @param {Error=} error > * ... > * @param {Array.<?>=} overrideStackTrace > * ... > */ > > The best documentation I know of for JSDoc syntax is the closure compiler docs. Done. http://usejsdoc.org is also a decent reference. https://codereview.chromium.org/482603002/diff/1/extensions/renderer/resource... extensions/renderer/resources/uncaught_exception_handler.js:17: message += ': ' + e; On 2014/08/18 16:09:45, kalman wrote: > You're relying on toString being defined for the error? I... guess that makes > sense. You might want to comment: > > // Hopefully |e| defines a toString function or this may look > // like [object Object]. Error defines toString as 'Error: <message>'. Done. https://codereview.chromium.org/482603002/diff/1/extensions/renderer/resource... extensions/renderer/resources/uncaught_exception_handler.js:21: message += ': (cannot get error message)'; On 2014/08/18 16:09:45, kalman wrote: > "Unknown error"? I've chosen "Cannot get error message" because it is more specific than "unknown error", thus presumably more easily Googleable. WDYT? https://codereview.chromium.org/482603002/diff/1/extensions/renderer/resource... extensions/renderer/resources/uncaught_exception_handler.js:31: } catch (e) {} On 2014/08/18 16:09:45, kalman wrote: > When can this throw? When an object with a throwing "get" getter is thrown: throw {get stack() { return 'haahaha'; }}; https://codereview.chromium.org/482603002/diff/1/extensions/renderer/resource... extensions/renderer/resources/uncaught_exception_handler.js:34: if (!asyncStack && !stack) On 2014/08/18 16:09:45, kalman wrote: > "asyncStack" assumes the intent of the caller was to pass in some asynchronous > stack trace. Though we know that this is *probably* the case, you could equally > get a stack trace with: > > var stack; > try { > trySomething(); > } catch(e) { > stack = e.stack; > } > aBunchOfOtherStuff(); > if (stack) > formatErrorMessage('oops', null, stack); > > for whatever reason. That example is contrived, but the point is, that's not > asynchronous, it's just a different stack trace. > > Which is a long way of saying: a better name would be something like > |overrideStackTrace|, and document it accordingly. Done. (for your specific example, note that if trySomething reached into user-supplied code, then e could potentially be invalid (i.e. throwing, or not a string), which violates the precondition that |asyncStack| is safe.) I've called it |asyncStack| because the extra stack trace must not be a part of the current stack trace. This can only be true if the call is asynchronous. If the stack trace is allowed to be synchronous, then the code should implement stack frame merge & deduplicate logic. This is complex and impossible to get right, so I demand that the stack traces must be disjoint, by declaring the extra stack trace as "asyncStack". https://codereview.chromium.org/482603002/diff/1/extensions/renderer/resource... extensions/renderer/resources/uncaught_exception_handler.js:77: exports.handle = function(message, e, asyncStack) { On 2014/08/18 16:09:45, kalman wrote: > Also JSDoc this method (basically: any public method). Done.
https://codereview.chromium.org/482603002/diff/1/extensions/renderer/module_s... File extensions/renderer/module_system.cc (right): https://codereview.chromium.org/482603002/diff/1/extensions/renderer/module_s... extensions/renderer/module_system.cc:556: "var Error = window.Error;"); On 2014/08/18 20:59:46, robwu wrote: > On 2014/08/18 16:09:44, kalman wrote: > > This isn't any more desirable than using Error in the code. Is the purpose to > > have code safe against the extension tampering with the environment? Probably > > wise, but that would be better as $Error and using SafeBuiltins. > > This protects against redefining the global Error constructor, because > extensions cannot modify the Error variable in the local scope. It is sufficient > because most of the uses of Error are of the form "throw new Error". The only > other use of the error constructor is Error.captureStackTrace, but that method > is also stored in a local variable. > > I'm using Error instead of $Error to allow extension code to use "instanceof > Error" and "Error.stackTraceLimit". I've added a comment. Maybe I'm misunderstanding - that doesn't prevent extensions from overriding the error constructor. They can still do window.Error = function MySpecialError(message) { console.log('oops'); }; and what is stopping extension code from doing instanceof $Error? https://codereview.chromium.org/482603002/diff/20001/extensions/renderer/reso... File extensions/renderer/resources/uncaught_exception_handler.js (right): https://codereview.chromium.org/482603002/diff/20001/extensions/renderer/reso... extensions/renderer/resources/uncaught_exception_handler.js:18: * message. This stack trace must not include stack frames of |e.stack|. Why can't it include stack frames of |e.stack|?
https://codereview.chromium.org/482603002/diff/1/extensions/renderer/module_s... File extensions/renderer/module_system.cc (right): https://codereview.chromium.org/482603002/diff/1/extensions/renderer/module_s... extensions/renderer/module_system.cc:556: "var Error = window.Error;"); On 2014/08/18 23:13:10, kalman wrote: > On 2014/08/18 20:59:46, robwu wrote: > > On 2014/08/18 16:09:44, kalman wrote: > > > This isn't any more desirable than using Error in the code. Is the purpose > to > > > have code safe against the extension tampering with the environment? > Probably > > > wise, but that would be better as $Error and using SafeBuiltins. > > > > This protects against redefining the global Error constructor, because > > extensions cannot modify the Error variable in the local scope. It is > sufficient > > because most of the uses of Error are of the form "throw new Error". The only > > other use of the error constructor is Error.captureStackTrace, but that method > > is also stored in a local variable. > > > > I'm using Error instead of $Error to allow extension code to use "instanceof > > Error" and "Error.stackTraceLimit". I've added a comment. > > Maybe I'm misunderstanding - that doesn't prevent extensions from overriding the > error constructor. They can still do > > window.Error = function MySpecialError(message) { > console.log('oops'); > }; I assumed that the module system is run before the extension is initialized. When window.Error is stored in a local variable, the extension cannot mess with it any more. > > and what is stopping extension code from doing instanceof $Error? An extension cannot easily get a reference to $Error, since it a variable that is only exposed via the module system. Besides, $Error is non-standard. https://codereview.chromium.org/482603002/diff/20001/extensions/renderer/reso... File extensions/renderer/resources/uncaught_exception_handler.js (right): https://codereview.chromium.org/482603002/diff/20001/extensions/renderer/reso... extensions/renderer/resources/uncaught_exception_handler.js:18: * message. This stack trace must not include stack frames of |e.stack|. On 2014/08/18 23:13:11, kalman wrote: > Why can't it include stack frames of |e.stack|? The stack traces are concatenated. If the stack traces overlap, then it looks confusing to the developer. Imagine var message = 'Message'; var e = new Error('Some error'); var stackTrace = new Error().stack; Then the result of formatErrorMessage would be: Message: Some error <identical stack trace> <identical stack trace>
https://codereview.chromium.org/482603002/diff/1/extensions/renderer/module_s... File extensions/renderer/module_system.cc (right): https://codereview.chromium.org/482603002/diff/1/extensions/renderer/module_s... extensions/renderer/module_system.cc:556: "var Error = window.Error;"); On 2014/08/18 23:27:18, robwu wrote: > On 2014/08/18 23:13:10, kalman wrote: > > On 2014/08/18 20:59:46, robwu wrote: > > > On 2014/08/18 16:09:44, kalman wrote: > > > > This isn't any more desirable than using Error in the code. Is the purpose > > to > > > > have code safe against the extension tampering with the environment? > > Probably > > > > wise, but that would be better as $Error and using SafeBuiltins. > > > > > > This protects against redefining the global Error constructor, because > > > extensions cannot modify the Error variable in the local scope. It is > > sufficient > > > because most of the uses of Error are of the form "throw new Error". The > only > > > other use of the error constructor is Error.captureStackTrace, but that > method > > > is also stored in a local variable. > > > > > > I'm using Error instead of $Error to allow extension code to use "instanceof > > > Error" and "Error.stackTraceLimit". I've added a comment. > > > > Maybe I'm misunderstanding - that doesn't prevent extensions from overriding > the > > error constructor. They can still do > > > > window.Error = function MySpecialError(message) { > > console.log('oops'); > > }; > > I assumed that the module system is run before the extension is initialized. > When window.Error is stored in a local variable, the extension cannot mess with > it any more. Right. Unfortunately that's not true, these modules are loaded only when they're used not ahead of time, so the extension can mess with whatever it wants before we get a chance. That's why $Function, $Array etc exist - they *are* saved before the extension has a chance to run. > > > > > > and what is stopping extension code from doing instanceof $Error? > > An extension cannot easily get a reference to $Error, since it a variable that > is only exposed via the module system. Besides, $Error is non-standard. Yes the idea of $Function and $Array - and $Error - is that it's intentionally hidden from the extension. The only functionality you really need is that captureStackTrace (and stackTraceLimit) right, not the Error constructor? Those are the sorts of things you'd expose. See safe_builtins. https://codereview.chromium.org/482603002/diff/20001/extensions/renderer/reso... File extensions/renderer/resources/uncaught_exception_handler.js (right): https://codereview.chromium.org/482603002/diff/20001/extensions/renderer/reso... extensions/renderer/resources/uncaught_exception_handler.js:18: * message. This stack trace must not include stack frames of |e.stack|. On 2014/08/18 23:27:18, robwu wrote: > On 2014/08/18 23:13:11, kalman wrote: > > Why can't it include stack frames of |e.stack|? > > The stack traces are concatenated. If the stack traces overlap, then it looks > confusing to the developer. > > Imagine > > var message = 'Message'; > var e = new Error('Some error'); > var stackTrace = new Error().stack; > > Then the result of formatErrorMessage would be: > > Message: Some error > <identical stack trace> > <identical stack trace> Ah I understand. Perhaps "priorStackTrace" or similar, then?
https://codereview.chromium.org/482603002/diff/1/extensions/renderer/module_s... File extensions/renderer/module_system.cc (right): https://codereview.chromium.org/482603002/diff/1/extensions/renderer/module_s... extensions/renderer/module_system.cc:556: "var Error = window.Error;"); On 2014/08/18 23:39:16, kalman wrote: > On 2014/08/18 23:27:18, robwu wrote: > > On 2014/08/18 23:13:10, kalman wrote: > > > On 2014/08/18 20:59:46, robwu wrote: > > > > On 2014/08/18 16:09:44, kalman wrote: > > > > > This isn't any more desirable than using Error in the code. Is the > purpose > > > to > > > > > have code safe against the extension tampering with the environment? > > > Probably > > > > > wise, but that would be better as $Error and using SafeBuiltins. > > > > > > > > This protects against redefining the global Error constructor, because > > > > extensions cannot modify the Error variable in the local scope. It is > > > sufficient > > > > because most of the uses of Error are of the form "throw new Error". The > > only > > > > other use of the error constructor is Error.captureStackTrace, but that > > method > > > > is also stored in a local variable. > > > > > > > > I'm using Error instead of $Error to allow extension code to use > "instanceof > > > > Error" and "Error.stackTraceLimit". I've added a comment. > > > > > > Maybe I'm misunderstanding - that doesn't prevent extensions from overriding > > the > > > error constructor. They can still do > > > > > > window.Error = function MySpecialError(message) { > > > console.log('oops'); > > > }; > > > > I assumed that the module system is run before the extension is initialized. > > When window.Error is stored in a local variable, the extension cannot mess > with > > it any more. > > Right. Unfortunately that's not true, these modules are loaded only when they're > used not ahead of time, so the extension can mess with whatever it wants before > we get a chance. That's why $Function, $Array etc exist - they *are* saved > before the extension has a chance to run. > > > > > > > > > > > and what is stopping extension code from doing instanceof $Error? > > > > An extension cannot easily get a reference to $Error, since it a variable that > > is only exposed via the module system. Besides, $Error is non-standard. > > Yes the idea of $Function and $Array - and $Error - is that it's intentionally > hidden from the extension. > > The only functionality you really need is that captureStackTrace (and > stackTraceLimit) right, not the Error constructor? Those are the sorts of things > you'd expose. See safe_builtins. Done. https://codereview.chromium.org/482603002/diff/20001/extensions/renderer/reso... File extensions/renderer/resources/uncaught_exception_handler.js (right): https://codereview.chromium.org/482603002/diff/20001/extensions/renderer/reso... extensions/renderer/resources/uncaught_exception_handler.js:18: * message. This stack trace must not include stack frames of |e.stack|. On 2014/08/18 23:39:16, kalman wrote: > On 2014/08/18 23:27:18, robwu wrote: > > On 2014/08/18 23:13:11, kalman wrote: > > > Why can't it include stack frames of |e.stack|? > > > > The stack traces are concatenated. If the stack traces overlap, then it looks > > confusing to the developer. > > > > Imagine > > > > var message = 'Message'; > > var e = new Error('Some error'); > > var stackTrace = new Error().stack; > > > > Then the result of formatErrorMessage would be: > > > > Message: Some error > > <identical stack trace> > > <identical stack trace> > > Ah I understand. Perhaps "priorStackTrace" or similar, then? Done.
Looks fine apart from those million comments I made :) and I wanted to look at the uncaught_exception_handler.js diff again, but something odd has happened. https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... File extensions/renderer/resources/binding.js (right): https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... extensions/renderer/resources/binding.js:364: ' already defined in ' + schema.namespace); Fix indentation. https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... File extensions/renderer/resources/event.js (right): https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... extensions/renderer/resources/event.js:105: throw new $Error.self("Can't add listener"); new Error() https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... extensions/renderer/resources/event.js:203: if (this.eventOptions.supportsRules) 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) https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... extensions/renderer/resources/event.js:204: throw new $Error.self( new Error() https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... extensions/renderer/resources/event.js:263: throw new $Error.self("This event does not support listeners."); new Error() https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... extensions/renderer/resources/event.js:266: throw new $Error.self("Too many listeners for " + this.eventName); new Error() https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... extensions/renderer/resources/event.js:275: throw new $Error.self("filters.serviceType should be a string."); new Error() for all of these https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... extensions/renderer/resources/event.js:301: throw new $Error.self("This event does not support listeners."); new Error() https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... extensions/renderer/resources/event.js:327: throw new $Error.self("This event does not support listeners."); new Error() https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... extensions/renderer/resources/event.js:339: throw new $Error.self("This event does not support listeners."); new Error() https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... extensions/renderer/resources/event.js:361: throw new $Error.self("This event does not support listeners."); new Error() https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... extensions/renderer/resources/event.js:418: throw new $Error.self("This event does not support rules."); new Error() https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... extensions/renderer/resources/event.js:471: throw new $Error.self("This event does not support rules."); new Error() https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... extensions/renderer/resources/event.js:488: throw new $Error.self("This event does not support rules."); new Error() https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... File extensions/renderer/resources/extension_custom_bindings.js (right): https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... extensions/renderer/resources/extension_custom_bindings.js:107: throw new $Error.self(sendRequestIsDisabled); All of these: new Error() https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... File extensions/renderer/resources/messaging.js (right): https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... extensions/renderer/resources/messaging.js:360: throw new $Error.self('Invalid arguments to ' + functionName + '.'); new Error() https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... File extensions/renderer/resources/permissions_custom_bindings.js (right): https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... extensions/renderer/resources/permissions_custom_bindings.js:45: throw new $Error.self("Too many keys in object-style permission."); new Error() ... I think? This stuff is confusing. https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... File extensions/renderer/resources/platform_app.js (right): https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... extensions/renderer/resources/platform_app.js:31: throw new $Error.self(message); new Error() https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... File extensions/renderer/resources/runtime_custom_bindings.js (right): https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... extensions/renderer/resources/runtime_custom_bindings.js:142: throw new $Error.self('Invalid arguments to connect.'); new Error() https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... extensions/renderer/resources/runtime_custom_bindings.js:149: throw new $Error.self('Invalid arguments to connectNative.'); new Error() https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... extensions/renderer/resources/runtime_custom_bindings.js:158: throw new $Error.self('Error connecting to extension ' + targetId); new Error() https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... extensions/renderer/resources/runtime_custom_bindings.js:190: throw new $Error.self('Error connecting to native app: ' + nativeAppName); new Error() ... I think. https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... File extensions/renderer/resources/schema_utils.js (right): https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... extensions/renderer/resources/schema_utils.js:113: throw new $Error.self("Invocation of form " + new Error() for this whole file. https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... File extensions/renderer/resources/set_icon.js (right): https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... extensions/renderer/resources/set_icon.js:126: throw new $Error.self( new Error() for this whole file (which is horribly written by the way, but thankfully there is a pending code review to fix it) https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... File extensions/renderer/resources/uncaught_exception_handler.js (right): https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... extensions/renderer/resources/uncaught_exception_handler.js:23: function formatErrorMessage(message, e, priorStackTrace) { The diff on this file is super confusing, seems like it was a diff against the previous patch? Reitveld being crazy? Me being crazy? https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... File extensions/renderer/resources/utils.js (right): https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... extensions/renderer/resources/utils.js:41: throw new $Error.self( I have no idea about this one.
https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... File extensions/renderer/resources/event.js (right): https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... extensions/renderer/resources/event.js:203: if (this.eventOptions.supportsRules) On 2014/08/19 16:45:55, kalman wrote: > 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) 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? https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... File extensions/renderer/resources/uncaught_exception_handler.js (right): https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... extensions/renderer/resources/uncaught_exception_handler.js:23: function formatErrorMessage(message, e, priorStackTrace) { On 2014/08/19 16:45:56, kalman wrote: > The diff on this file is super confusing, seems like it was a diff against the > previous patch? Reitveld being crazy? Me being crazy? This diff looks fine to me. I renamed the printStack variable as requested and updated the comments accordingly; Besides that, I made use of the new $Error and $String.indexOf objects. Perhaps you clicked on "View" instead of "Delta from patch set 2" (or vice versa, depending on your definition of super confusing ;) )
https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... File extensions/renderer/resources/event.js (right): https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... extensions/renderer/resources/event.js:203: if (this.eventOptions.supportsRules) On 2014/08/19 17:35:57, robwu wrote: > On 2014/08/19 16:45:55, kalman wrote: > > 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) > > 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? 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). https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... File extensions/renderer/resources/uncaught_exception_handler.js (right): https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... extensions/renderer/resources/uncaught_exception_handler.js:23: function formatErrorMessage(message, e, priorStackTrace) { On 2014/08/19 17:35:57, robwu wrote: > On 2014/08/19 16:45:56, kalman wrote: > > The diff on this file is super confusing, seems like it was a diff against the > > previous patch? Reitveld being crazy? Me being crazy? > > This diff looks fine to me. > I renamed the printStack variable as requested and updated the comments > accordingly; > Besides that, I made use of the new $Error and $String.indexOf objects. > > Perhaps you clicked on "View" instead of "Delta from patch set 2" > (or vice versa, depending on your definition of super confusing ;) ) I clicked on View which is supposed to show the diff against the original, not the last patchset. But it's showing a diff against the last patchset.
https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... File extensions/renderer/resources/event.js (right): https://codereview.chromium.org/482603002/diff/40001/extensions/renderer/reso... extensions/renderer/resources/event.js:203: if (this.eventOptions.supportsRules) On 2014/08/19 17:54:38, kalman wrote: > On 2014/08/19 17:35:57, robwu wrote: > > On 2014/08/19 16:45:55, kalman wrote: > > > 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) > > > > 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? > > 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). 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).
lgtm
The CQ bit was checked by rob@robwu.nl
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rob@robwu.nl/482603002/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/43820) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/48988) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/43831) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
The CQ bit was checked by rob@robwu.nl
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rob@robwu.nl/482603002/160001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rob@robwu.nl
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rob@robwu.nl/482603002/180001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rob@robwu.nl
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rob@robwu.nl/482603002/200001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Message was sent while issue was closed.
Committed patchset #8 (200001) as 291392 |
