Closed Bug 355430 Opened 18 years ago Closed 11 years ago

Stack information of uncaught Error object should be available in window.onerror

Categories

(Core :: DOM: Core & HTML, enhancement)

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla31

People

(Reporter: jesse, Assigned: evilpie)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: dev-doc-needed, relnote)

Attachments

(3 files, 5 obsolete files)

column numbers
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
v3
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
test
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.7) Gecko/20060909 Firefox/1.5.0.7 Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.7) Gecko/20060909 Firefox/1.5.0.7 Firefox supports window.onerror, from Internet Explorer, as a way of handling any JavaScript errors that go uncaught: window.onerror = function(msg, url, line) { ... }; When this function is invoked in Internet Explorer, the JavaScript stack from when the error was thrown is available using arguments.caller/callee. In IE it's as though the exact line where the error was caused calls the window.onerror function. In Firefox, the stack information is not available in this way. I think the best thing to do (and which I wish IE did), rather than calling onerror with the stack intact, is to expose the actual object that was thrown and not caught. In Fx, this object already has the stack property which will provide a good amount of information about the state of the JavaScript stack when the error was thrown. Additionally, any object that custom JavaScript code throws should also be available in the onerror function. So I can think of two quick and dirty ways of exposing the thrown object. One is to add a fourth parameter to the onerror function: window.onerror = function(msg, url, line, err) { ... }; Another would be to save the last uncaught error as a property of the window object: window.onerror = function(msg, url, line) { var err = window._mozLastError; }; I think that this change would be very helpful for developers working on complex JavaScript applications. We are used to Java, where we can wrap the main() function in a try/catch+print-stack-trace statement for diagnosis of any problem. Because JavaScript+Browser has so many possible entry points (event handlers, window.setTimeout, <script> loading, etc) there is not an easy idiom to use that replicate this catch-all functionality. window.onerror is the obvious mechanism to use but to be fully useful it needs to somehow expose the state of the stack when the error was raised. Reproducible: Always Steps to Reproduce: window.onerror = function(msg, url, line) { window.alert("Hey, no access to the stack!"); }; var willthrow = nothing.nope.nada; // an error
Blocks: tibco
Assignee: nobody → general
Component: General → DOM
Product: Firefox → Core
QA Contact: general → ian
Version: unspecified → 1.8 Branch
Isn't it just that .caller should work from within the error handler? It doesn't in Mozilla but used to with Netscape 4.7x like this:- window.onerror=myErrors function test() { nosuchfunction() } function myErrors(msg, url, line) { alert(myErrors.caller.toString()) } Running test() triggers an error but the alert shows null rather than the test() function listing as would be expected. This means you can't construct a caller history (.caller.caller.caller etc) which is exactly the sort of thing you want to do in an error handler. It is as if triggering onerror cause .caller to be wiped out, whereas .caller work fine in normal function usage.
See https://bugzilla.mozilla.org/show_bug.cgi?id=65683#c20 comment from Brendan Eich, .caller will never be available with window.onerror.
Assignee: general → nobody
Severity: normal → enhancement
QA Contact: ian → general
Version: 1.8 Branch → Trunk
This is a valid bug. It's fine if .caller isn't available, however, the only other way of getting stack information in onerror (new Error().stack) also does not work: it produces a stack which ends at onerror. Test code: <html> <body> <script> window.onerror = function onErrorHandler (message, url, linenumber) { alert("Error via new Error(): " + new Error().stack); var caughtError; try { throw new Error("whoops"); } catch (e) { caughtError = e; } alert("Error via try..catch: " + caughtError.stack); }; function func3 () { crash(); } function func2 () { func3(); } function func1 () { func2(); } func1(); </script> </body> </html>
https://bugs.webkit.org/show_bug.cgi?id=55092#c1 I wrote in that bug that I think you're expecting the wrong thing from your example, specifically from this line: alert(new Error().stack); -- as in your example, should not have anything other than itself on the stack. Just a "Error" error, with message == "". Furthermore, errors thrown from event handlers don't have access to the root cause of what triggered the event. I believe that's a security feature. However if what you want is for the originating error to propagate through to a global scrip error handler, then that should be your area of focus. And as stated in the WebKit bug, WHATWG seems like the right forum for such proposals. And I think this bug is INVALID.
new Error().stack in other circumstances provides the full stack, for example, calling element.focus() and checking new Error().stack in an onfocus handler shows the stack including code that called element.focus(). onerror is behaving specially here. About security: you seem to be discussing a quirk of the implementation rather than an actual security issue. Clearly there's not a security issue in being able to get stack information within onerror if it's also available via try..catch in the method that errors. Your suggestion of passing in or otherwise making available the original error object is interesting, and perhaps something you could raise with WhatWG, but is separate from the problem of new Error().stack providing incomplete information within onerror.
so, i'm confused. you're in window.onerror: data:text/html,<script>window.onerror=function c(){try {null.null}catch (e) {alert(e.stack); }}; function a(){try {b()} catch (e) {alert('lo');alert(e.stack);alert('op')}}; function b(){b()} window.setTimeout(a,0)</script> and you have as much access to the stack as anything else. the current frame is <onerror> which you can trivially skip. https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Error https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Error/Stack sure you can use: data:text/html,<script>window.onerror=function c(){try {null.null}catch (e) {alert(e.stack); }}; function a(){try {b()} catch (e) {alert('lo');e=new Error;alert(e.stack);alert('op')}}; function b(){b()} window.setTimeout(a,0)</script> and *lose* the stack, but i don't see that as particularly exciting - don't do that. since i seem to already have access to what you want, i'm resolving this as WORKSFORME: because `"Stack information of uncaught Error object should be available in window.onerror" *is* available. i'm pretty sure that if you're complaining about <new error().stack> not doing what you want then garrett would be right in that your request should be invalid. anyway, you really should just use try{}catch(e){} somewhere closer to where you're triggering problems. and if you want to do debugging, get a debugger. onerror is designed for generic error handling/reporting "Hi user, it seems something unexpected happened, my ui might be broken, it might be a good idea to reload".
Status: UNCONFIRMED → RESOLVED
Closed: 14 years ago
Resolution: --- → WORKSFORME
Re-opening: "WORKSFORME" is not the right resolution. An argument *could* be made for marking this "WONTFIX" -- however, I think that this would be a worthwhile feature. I think Garett and timeless got side-tracked by the suggested implementations -- one which should not work (there is no need to copy IE's design here), and one where the suggester does not understand how the Error constructor and/or error reporting in JSAPI works. Let's stop talking about proposed implementations and start talking about the enhancement proposed. The bug the OP reported is a real, honest-to-goodness RFE. When a window.onerror is invoked, we are currently passed the message, fileName and lineNumber from the uncaught exception. We should also be passed the stack. While it's true that we could avoid many uncaught exceptions by adding a try/catch block inside every closure we construct and hand to anything that will execute it after the current code runs to completion, this is a) unwieldy, b) error-prone, and c) incomplete. It is incomplete because it does not allow us to handle at least one real-world interesting case which continually plagues me: implementing module systems which load modules by dynamically injecting SCRIPT tags into the DOM. These are necessarily loaded without try/catch blocks. Uncaught exception handling is a good thing. Let's add stack alongside message, fileName and lineNumber.
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: WORKSFORME → ---
Another compelling real-world use case is capturing diagnostics, for example, when developers are reporting issues using a framework, or end users encounter errors and need to get information to support staff. A debugger does not help here and try..catch doesn't handle all cases (as Wesley also noted). However, Wesley, if you take the approach that the stack should be available as a forth argument (which I agree is a good implementation), from experience, someone is going to tell you to go bother the WhatWG instead. And again, I may not understand how JSAPI error reporting works, but new Error().stack in other circumstances provides the full stack, for example, calling element.focus() and checking new Error().stack in an onfocus handler shows the stack including code that called element.focus(). It seems a quirk of the implementation that this is not also true of onerror.
Not to give IE too much good press, but another useful thing about the call of 'window.onerror' occurring in the context of the error is that you can set a breakpoint there in the debugger and see the live callstack. Even so, given that manually walking the callstack is not always safe (recursion), an explicit stack parameter would be (far) simpler to consume.
I'm adding my support for a stack argument to be passed to window.onerror. The lack of a stack trace means that errors in minified JavaScript are nearly impossible to debug. Note that the 'column number' is now part of the spec, and will be the fourth argument to window.onerror, so the stack should be passed as the fifth argument. It would be far simpler if we could just pass an Error object to window.onerror, but I understand if there are issues with backwards compatibility and security.
We really need this for crash reporting for apps.
For what? (just trying to understand... if there is crash, there sure isn't any way to check the stack in the same process)
Terminology confusion. When laura says "crash" she really means "JS error". We'd like to implement a Socorro-like service for uncaught JS exceptions in web apps.
Yes, I also need this for error reporting. (FWIW, I maintain Errbit: https://github.com/errbit/errbit).
I'm working with unit tests in B2G Gaia and I think that we need this to handle the (uncaught) errors thrown by asynchronous tests and have the same meaningful information than if we'd try/catch it. Mocha does this well for synchronous tests (that's easy) and that would be nice to have this for async tests too.
There's a patch (although it probably rotted a bit after 2.5 years), that I think implements this, in bug 611161.
jimb says he can take this on, and ought to get to it in a couple of weeks post working on the content debugging in B2G. Thanks Jim!
Assignee: nobody → jimb
My calendar tells me I should ping jimb about this bug today - so - ping!
Flags: needinfo?(jimb)
FYI, this is being discussed on the ECMAScript side as well [1]. There can be security hazards if the stack contains sensitive infos (arguments). If you want more than a list of file/line/column/function name, I would recommand defining higher-privilege features. [1] http://wiki.ecmascript.org/doku.php?id=strawman:error_stack
(In reply to Laura Thomson :laura from comment #18) > My calendar tells me I should ping jimb about this bug today - so - ping! Hi, Laura! I've still got some B2G debugging work finished, but then I'm itching to get started on the captured stack work. Can I request a mid-July ping?
Flags: needinfo?(jimb)
(In reply to Jim Blandy :jimb from comment #20) > (In reply to Laura Thomson :laura from comment #18) > > My calendar tells me I should ping jimb about this bug today - so - ping! > > Hi, Laura! I've still got some B2G debugging work finished, but then I'm > itching to get started on the captured stack work. Can I request a mid-July > ping? Will do!
I will do the ping for you. Ping! Fixing this issue could save so many lives. And my hairs. :P
Thanks Bjorn...and needless to say, ping! dcamp, is jimb going to be the person to work on this?
Flags: needinfo?(dcamp)
Progress on this is happening on blink: https://code.google.com/p/chromium/issues/detail?id=147127 Webkit has said they would not add callstacks to onerror.
Worth noting that the spec now requires[1] 5 arguments passed to window.onerror: the original message, location, and lineno, and now the colno and the error object itself. Stack isn't an explicit argument, but is part of the error object, so should be available to those who want it anyway. [1] http://html5.org/tools/web-apps-tracker?from=8085&to=8086
Depends on: 630464
(In reply to Laura Thomson :laura from comment #11) > We really need this for crash reporting for apps. Nominating blocking-b2g for koi?.
blocking-b2g: --- → koi?
Does anyone have a good workaround for this to get the stack, other than wrapping all your code in try/catch?
There aren't any good workarounds, but the new spec with the Error object being passed to the `onerror` callback is now supported in Chrome Canary. It'd sure be nice to see some progress with FF.
I spoke with Dave about this. It doesn't seem like something we can get in for B2G 1.2 so I'm removing the nom. We'd also need bug 630464 fixed first.
blocking-b2g: koi? → ---
Blocks: 811299
Flags: needinfo?(dcamp)
Attached patch Hacky Patch (obsolete) (deleted) — — Splinter Review
I've had a go at getting *something* working. The attached patch works in simple cases, but needs more work: 1. It only seems to pass the exception through if it's an exception object (i.e. throwing a String doesn't appear in window.onerror — in Chrome it does). 2. I think if GC happened at the wrong time, the exception could be collected. This is only a concern if there are script blockers (if not everything happens on the stack) at which point it's stored in a queue but never marked during GC. I'm happy to improve it, but I'll need help understanding the proposed solution in #630464. (I tried that first, but the exception always came through as NULL. Probably I did something wrong :p). I'll also need someone to explain how to make sure the exception is not garbage collected. It feels like this might be a case for Persistent<>, but the docs for that say "don't use this" so I didn't.
Jim, here's a first patch by a contributor - do you think you might be able to take a look?
Flags: needinfo?(jimb)
Sure, I can take a look. I'll move this to my review queue --- which has gotten unacceptably deep, so I may not be able to get to this this week.
Flags: needinfo?(jimb)
Attachment #8355529 - Flags: review?(jimb)
Comment on attachment 8355529 [details] [diff] [review] Hacky Patch I am not ept to review this patch, but bz says he can review the DOM side, and jwalden has agreed to review the JS_GetPendingException call. It's great to see this getting worked on!
Attachment #8355529 - Flags: review?(jwalden+bmo)
Attachment #8355529 - Flags: review?(jimb)
Attachment #8355529 - Flags: review?(bzbarsky)
> 1. It only seems to pass the exception through if it's an exception object That's a jseng issue: it only sets a pending exception on the context in some cases when reporting it. Specifically, in js_ReportUncaughtException we have: 862 if (!reportp) { 863 JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, 864 JSMSG_UNCAUGHT_EXCEPTION, bytes); 865 } else { 866 /* Flag the error as an exception. */ 867 reportp->flags |= JSREPORT_EXCEPTION; 868 869 /* Pass the exception object. */ 870 JS_SetPendingException(cx, exn); 871 CallErrorReporter(cx, bytes, reportp); 872 } and the case when what you threw is not an object is the !reportp case. :( Eventually JS_ReportErrorNumber calls the error reporter, but there is no pending exception on cx by then. Jeff? > 2. I think if GC happened at the wrong time, the exception could be collected. Yes. We need to root the members in InternalScriptErrorEvent and ScriptErrorEvent. Olli, what's a sane way to do the former? That one is particularly bad because the clss is used both on-stack and on-heap... :( > but I'll need help understanding the proposed solution in #630464. That would help your problem #1, but not #2.
Flags: needinfo?(jwalden+bmo)
Flags: needinfo?(bugs)
And thank you for working on this! It's sorely needed.
Thanks for looking at this guys – and sorry the patch is more of a "place to start a discussion" than a fix. I did wonder while doing this whether it'd make sense to refactor things so that we hit window.onerror by doing a .dispatchEvent directly; and then leave the old mechanisms in place for the console and other things that want to know about it. That's again just a wild idea — while it seems that trying to jump out of the JS context and back in causes a lot of problems, not doing that is probably just as hard.
Sounds like we should implement ErrorEvent so that it can hold gc things, and at least C++ should be able to access those gc things (Looks like Bug 643325 added ErrorEvent, but it isn't the one we use for window.onerror :/) And then always just use such heap-only error events. I think ErrorEvent webidl could be just changed so that it has [ChromeOnly] readonly object exception, and the dictionary would need to have same thing too. ScriptErrorEvent is already heap-only, so we just need to root/unroot whatever it is keeping alive temporarily.
Flags: needinfo?(bugs)
> I did wonder while doing this whether it'd make sense to refactor things so that we hit > window.onerror by doing a .dispatchEvent directly That's what mostly ends up happening anyway, no? In particular, that's what ScriptErrorEvent::Run() is trying to do: create a DOM event and dispatch it. That reminds me: in the !sameOrigin case in that method, we do NOT want to pass through the exception object. We want to pass null instead. See http://www.whatwg.org/specs/web-apps/current-work/multipage/webappapis.html#report-the-error step 6.
> I think ErrorEvent webidl could be just changed so that it has [ChromeOnly] readonly > object exception http://www.whatwg.org/specs/web-apps/current-work/multipage/webappapis.html#errorevent says: readonly attribute any error; and the dictionary has "any error". Doing that should be fine as long as we pass null when the security check fails.
One other issue. If script A (not same-origin with the page) calls script B (same-origin) and then script B throws, then onerror would get a stack that shows information from script A. That seems moderately undesirable, actually. I wonder whether we can sanitize that stack...
(In reply to Boris Zbarsky [:bz] from comment #40) > One other issue. > > If script A (not same-origin with the page) calls script B (same-origin) and > then script B throws, then onerror would get a stack that shows information > from script A. That seems moderately undesirable, actually. I wonder > whether we can sanitize that stack... The only way that this can happen is with chrome calling content, right? Seems like we should just avoid calling the script's error reporter if the principals in the JSErrorReport don't subsume the scripted reporter. I kinda thought we already did that.
> The only way that this can happen is with chrome calling content, right? No, just content calling content. All within one page, even. Maybe we don't care about cases like this and only really care about sanitizing the parse error notifications onerror seems...
Attachment #8355529 - Flags: review?(jwalden+bmo) → review-
(In reply to Boris Zbarsky [:bz] from comment #42) > No, just content calling content. All within one page, even. Maybe we > don't care about cases like this and only really care about sanitizing the > parse error notifications onerror seems... Oh, right. Could we just check originPrincipals in that case?
See comment 40. Which originPrincipals are you proposing we check in that situation?
Uh, my review comments got eaten. :-( Will see if I can dig them up somehow. (In reply to Boris Zbarsky [:bz] from comment #34) > Specifically, in js_ReportUncaughtException we have: > > 862 if (!reportp) { > 863 JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, > 864 JSMSG_UNCAUGHT_EXCEPTION, bytes); > 865 } else { > 866 /* Flag the error as an exception. */ > 867 reportp->flags |= JSREPORT_EXCEPTION; > 868 > 869 /* Pass the exception object. */ > 870 JS_SetPendingException(cx, exn); > 871 CallErrorReporter(cx, bytes, reportp); > 872 } > > and the case when what you threw is not an object is the !reportp case. :( > Eventually JS_ReportErrorNumber calls the error reporter, but there is no > pending exception on cx by then. Jeff? So the question becomes: is it time to bite the bullet and get rid of error reporting, in some sort of incremental fashion? I'm not sure. It's been a thorn in our sides for a very long time, certainly. But I'm not sure how much work that is, or whether there are reasonable steps to take to break down the problem. More investigation needed (I've been meaning to do this for a bit, actually). (In reply to Boris Zbarsky [:bz] from comment #40) > If script A (not same-origin with the page) calls script B (same-origin) and > then script B throws, then onerror would get a stack that shows information > from script A. That seems moderately undesirable, actually. I wonder > whether we can sanitize that stack... Historically the loop to build up .stack had an early exit for some sort of principals mismatch. These days it's built into the frame iterator being used, and I don't know exactly what's being tested. In any case I think we only give you frames you should be able to see, already. Or, wait. That's going to be a truncated stack for the *throwing* script's point of view. But this bubbles up to the topmost caller's window.onerror, right? I think we can just add API to set .stack to the empty string in that case, or something. Really, this is why all this stuff needs to be exposed to debuggers, so that people will use those and won't want/depend on this level of information exposure when the debugger's not running. :-\ Another fun issue: this exposes the behavior of <script>syntax error</script>, and the required creation of a SyntaxError exception, as far as I could tell from the HTML spec. Now of course we can optimize out the onerror call when there's no hook around. But that might take some work to do, and might not be doable in the first pass.
Flags: needinfo?(jwalden+bmo)
Comment on attachment 8355529 [details] [diff] [review] Hacky Patch Review of attachment 8355529 [details] [diff] [review]: ----------------------------------------------------------------- Bleh, couldn't retrieve, spat them out from vague memory. :-( ::: dom/base/nsJSEnvironment.cpp @@ +463,4 @@ > InternalScriptErrorEvent errorevent(true, NS_LOAD_ERROR); > > errorevent.fileName = mFileName.get(); > + errorevent.exception = mException; This needs to occur/happen wrt the sameOrigin check below. And, um. Given that code's lack of dealing with column number as well as line number, are we censoring the column number correctly for onerror notifications right now? @@ +507,4 @@ > nsCOMPtr<nsIScriptGlobalObject> mScriptGlobal; > nsCOMPtr<nsIPrincipal> mOriginPrincipal; > bool mDispatchEvent; > + JS::Heap<JS::Value> mException; This needs to be traced, for the GC to handle this correctly. Just throwing it in here isn't rooting-safe: it handles barriers correctly, but something has to tell the GC about Heap instances for them to actually work to root stuff. @@ +553,5 @@ > // XXX this means we are not going to get error reports on non DOM contexts > nsIScriptContext *context = nsJSUtils::GetDynamicScriptContext(cx); > > + JS::Rooted<JS::Value> exn(cx, JSVAL_VOID); > + ::JS_GetPendingException(cx, &exn); JS::UndefinedValue() Honestly not sure I was being poked about this call in particular, it doesn't seem like there's especial complexity here... ::: dom/src/events/nsJSEventListener.cpp @@ +186,5 @@ > } > > + AutoSafeJSContext cx; > + exception.Construct(cx); > + exception.Value() = scriptEvent->exception.get(); This fills me with oh so much confidence that the "right" context is being used here. Are we sure this doesn't need wrapping into this compartment at some point or other? ::: dom/webidl/EventHandler.webidl @@ +21,4 @@ > typedef OnBeforeUnloadEventHandlerNonNull? OnBeforeUnloadEventHandler; > > [TreatNonCallableAsNull] > +callback OnErrorEventHandlerNonNull = boolean ((Event or DOMString) event, optional DOMString source, optional unsigned long lineno, optional unsigned long column, optional any exception); Unreadably long. Could we have [TreatNonCallableAsNull] callback OnErrorEventHandlerNonNull = boolean ((Event or DOMString) event, optional DOMString source, optional unsigned long lineno, optional unsigned long column, optional any exception); or something instead? ::: widget/ContentEvents.h @@ +33,4 @@ > > InternalScriptErrorEvent(bool aIsTrusted, uint32_t aMessage) : > WidgetEvent(aIsTrusted, aMessage, NS_SCRIPT_ERROR_EVENT), > + lineNr(0), errorMsg(nullptr), fileName(nullptr), exception(JSVAL_VOID) JS::UndefinedValue() @@ +39,5 @@ > > int32_t lineNr; > const PRUnichar* errorMsg; > const PRUnichar* fileName; > + JS::Heap<JS::Value> exception; Also needs tracing. But, this class appears to be used on the stack at least, so you might be able to use JS::Rooted<JS::Value> for this. @@ +54,4 @@ > // should duplicate the characters and free them at destructing. > errorMsg = nullptr; > fileName = nullptr; > + exception = JSVAL_VOID; JS::UndefinedValue() This method looks like it really wants to be using move semantics, or something. Separate bug, perhaps.
> Historically the loop to build up .stack had an early exit for some sort of principals > mismatch. It doesn't consider originPrincipal. > Really, this is why all this stuff needs to be exposed to debuggers That doesn't help. People use window.onerror for telemetry, which is not a problem debuggers can solve for you. > this exposes the behavior of <script>syntax error</script>, and the required creation of > a SyntaxError exception, as far as I could tell from the HTML spec. Do we not create a SyntaxError exception right now? > are we censoring the column number correctly for onerror notifications right now? We never send column numbers to onerror right now. Not least, because we used to not have them at all, right? > Honestly not sure I was being poked about this call in particular You were. This is the part that involves actual interaction with the JS engine and its invariants. Like "when is there an exn here?". > This fills me with oh so much confidence that the "right" context is being used here. The context is only being used for rooting. So it's the right context as long as it came from the right runtime. The fact that we need a context at all to root is dumb.... > Are we sure this doesn't need wrapping into this compartment at some point or other? What is "this compartment"? ;) The generated OnErrorEventHandlerNonNull::Call function will wrap all its arguments into the compartment of the function it will actually call, fwiw. > Unreadably long. Could we have Depends. We generally try to not reformat spec IDL when we copy/paste it into our tree... > Also needs tracing. But, this class appears to be used on the stack at least It's used on both the stack and heap, for extra fun.
(In reply to Boris Zbarsky [:bz] from comment #44) > See comment 40. Which originPrincipals are you proposing we check in that > situation? Ugh yeah. I guess to really do this right we need to store the stack in a per-frame non-stringified form, which is then filtered at the time of access. We're already going to need to turn .stack into a proper getter (with internal, reserved-slot state) in order to make Xrays work properly. So this could probably happen at the same time. I wonder what the performance impact would be?
We need to decide what the actual threat model is from onerror seeing exceptions. The main threat I'm aware of is that syntax errors from the initial parse should be hidden cross-site, because otherwise you can exfiltrate information from random non-script resources. I'm not sure that runtime errors have similar constraints... In particular, any runtime error that could happen with page-controlled script on the stack can be happily reported to window.onerror, since the page-controlled script could have just caught it, so onerror is not exposing any new attack surface.
Yeah. Mulling it over, I think the amount of care that would be required to rely on any such semantics is probably beyond the grasp of anyone who would be insane enough to try. We should probably just not worry about it.
Conrad, does that give you enough to go on, or did we end up rambling too much? Most of the security discussion is just noise; the important part is that when we sanitize out the filename/lineno we should also sanitize out the exception object...
Flags: needinfo?(me)
I think using JS::Rooted<JS::Value> answers my question about how to retain properly, and I can change it to redact the object when necessary. The remaining question was "why does this only work if it's an error object", but I can probably figure that out from the hints/reading the code better. My hacking time on this is limited, and I'm going to try and get all the tests passing with my other patch (column numbers in stack traces) before coming back to this one.
Flags: needinfo?(me)
> I think using JS::Rooted<JS::Value> answers my question about how to retain properly Actually, not quite. That's a stack-only class. For things that are on the heap you'll want a JS::Heap<JS::Value> and explicit tracing. :(
Comment on attachment 8355529 [details] [diff] [review] Hacky Patch r- for now because of the rooting issues, but this is a great start.
Attachment #8355529 - Flags: review?(bzbarsky) → review-
Hey Conrad! Do you think you are going to work on this again in the future?
Flags: needinfo?(me)
It's on my todo list, but I'd love for someone else to take it off. I probably won't get round to it for a few weeks at the earliest.
Flags: needinfo?(me)
I just looked at this. After bug 972312 I am not sure how to save the JS::Value into ErrorEventInit.
Yeah, I think you want to ignore ErrorEvent for v1. The patch I have basically works, but it has a few issues described above.
Tom, you presumably want to add "any error;" to the ErrorEventInit dictionary, as per http://www.whatwg.org/specs/web-apps/current-work/multipage/webappapis.html#the-errorevent-interface Then you need to make sure you use RootedDictionary<ErrorEventInit> on the stack, and you should just be able to set .error (it'll be a JS::Value in the struct).
Attached patch v1 (obsolete) (deleted) — — Splinter Review
I couldn't use "exception" in the idl file, for some reason that throws an error.
Assignee: jimb → evilpies
Attachment #8355529 - Attachment is obsolete: true
Status: REOPENED → ASSIGNED
Attachment #8391763 - Flags: review?(bzbarsky)
Attached patch column numbers (obsolete) (deleted) — — Splinter Review
I thought as a bonus I would throw in column numbers.
Attachment #8391767 - Flags: review?(bzbarsky)
Attached patch column numbers (deleted) — — Splinter Review
Just realized that this patch had almost no context.
Attachment #8391767 - Attachment is obsolete: true
Attachment #8391767 - Flags: review?(bzbarsky)
Keywords: dev-doc-needed
> I couldn't use "exception" in the idl file Where were you trying to use it?
In the ErrorEvent interface: 0:02.58 WebIDL.WebIDLError: error: invalid syntax, /home/tom/Projects/mozilla-inbound/dom/webidl/ErrorEvent.webidl line 14:25 0:02.58 readonly attribute any exception;
Attached patch v1 refreshed (obsolete) (deleted) — — Splinter Review
Attachment #8391763 - Attachment is obsolete: true
Attachment #8391763 - Flags: review?(bzbarsky)
Attachment #8391772 - Flags: review?(bzbarsky)
Attachment #8391915 - Flags: review?(bzbarsky)
My test try push isn't fully clean. https://tbpl.mozilla.org/?tree=Try&rev=bd70338c6d52 It seems like there is a crash with AutoSafeJSContext when handling an event from workers.
Attached patch v2 (obsolete) (deleted) — — Splinter Review
ThreadsafeAutoJSContext is exactly what I needed for the worker case. https://tbpl.mozilla.org/?tree=Try&rev=7ea5f64503d7
Attachment #8391915 - Attachment is obsolete: true
Attachment #8391915 - Flags: review?(bzbarsky)
Attachment #8391950 - Flags: review?(bzbarsky)
> 0:02.58 readonly attribute any exception; The attribute is called "error" in http://www.whatwg.org/specs/web-apps/current-work/multipage/webappapis.html#the-errorevent-interface as far as I can tell... But anyway, "exception" is a reserved word in WebIDL. You could use "_exception" to create a member whose JS reflection would be named "exception". But you don't need that here; you want "error".
Attached patch v3 (deleted) — — Splinter Review
Attachment #8391950 - Attachment is obsolete: true
Attachment #8391950 - Flags: review?(bzbarsky)
Attachment #8392196 - Flags: review?(bzbarsky)
Comment on attachment 8391772 [details] [diff] [review] column numbers r=me
Attachment #8391772 - Flags: review?(bzbarsky) → review+
Comment on attachment 8392196 [details] [diff] [review] v3 I'm so glad we killed InternalScriptErrorEvent and that the event codegen correctly traces the member... >+callback OnErrorEventHandlerNonNull = boolean ((Event or DOMString) event, Please leave the long line here. That's what the IDL is like in the spec, and we generally try to not change our IDL from the spec more than absolutely necesary. You should change all the other consumers of ErrorEventInit (WorkerPrivate, and the IDB code) to using RootedDictionary too. They don't set the Value member yet, but at least the worker one probably should (followup bug on that, please), and it would be nice to not have to worry about rooting hazards if they start setting it. Also, can you add a test? r=me with that. Thank you for pushing this over the line!
Attachment #8392196 - Flags: review?(bzbarsky) → review+
Attached patch test (deleted) — — Splinter Review
Attachment #8393728 - Flags: review?(bzbarsky)
Comment on attachment 8393728 [details] [diff] [review] test r=me
Attachment #8393728 - Flags: review?(bzbarsky) → review+
Blocks: 986459
Keywords: relnote
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: