Closed
Bug 872273
Opened 12 years ago
Closed 11 years ago
Better reporting of privileged exceptions in unprivileged scopes
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(6 files, 5 obsolete files)
(deleted),
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bent.mozilla
:
review+
|
Details | Diff | Splinter Review |
Right now, when an exception is thrown in an XBL scope and propagates to a content caller, we get very confusing and unhelpful reporting as JS tries to report the exception, and throws a new exception while doing so (due to failure to toString the opaque object).
In effect, you see a lot of:
Error: Permission denied to access property 'toString' at :0
I think we could do better here. We certainly don't want to invoke any potentially-custom toString methods, and certainly don't want to do it for things that aren't exceptions. If we just unconditionally unwrapped and toString-ed when reporting, content could take some privileged object and just throw it to toString it (think window.location).
However, for objects that are bonafide JS exceptions (with a full ExnPrivate and the like), I think it should be pretty safe to use the information in those data structures to populate a report.
This may require a bit of refactoring along the way.
Comment 1•12 years ago
|
||
> I think it should be pretty safe to use the information in those data structures to
> populate a report
As long as we don't make that report available to untrusted scripts.
Assignee | ||
Comment 2•12 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #1)
> > I think it should be pretty safe to use the information in those data structures to
> > populate a report
>
> As long as we don't make that report available to untrusted scripts.
Can you clarify what you mean here?
Comment 3•12 years ago
|
||
Per irc discussion, the point is that we need to make sure we don't expose things to windown.onerror that are not normally visible to the page script. But that doesn't affect what goes into our error-reporting UI in any way.
Assignee | ||
Comment 4•12 years ago
|
||
Assignee | ||
Comment 5•12 years ago
|
||
Ugh, looks like I'm running afoul of very particular string formatting. Let's just do this for security wrappers and be done with it.
https://tbpl.mozilla.org/?tree=Try&rev=b371a165fa3d
Assignee | ||
Comment 6•12 years ago
|
||
Oh, hm. That didn't solve the problem.
So the basic issue here is that the duck-typing code does lots of careful formatting (in particular, appending |$(ERROR_TYPE):| before the message. I'm sort of afraid of messing with all this stuff too much, especially because Waldo is apparently partway though patches to rewrite some of this stuff.
Assignee | ||
Comment 7•12 years ago
|
||
Assignee | ||
Comment 8•12 years ago
|
||
Oh wait, I forgot to |commit --amend|. Facepalm.
Assignee | ||
Comment 9•12 years ago
|
||
Assignee | ||
Comment 10•12 years ago
|
||
Still orange. :-(
So, I feel kind of stuck here. Waldo, you're about to refactor this stuff, right? Would it make sense for me to wait until after you're done? If so, can you mark the dep?
Flags: needinfo?(jwalden+bmo)
Comment 11•12 years ago
|
||
The refactoring, for some definition of "refactoring", would be bug 724768. I'm perfectly willing to finish it up at any point when I have time, but these days I keep getting pulled in enough other directions that I haven't gotten to it. Most immediately, those things are test262 integration into tinderbox, and enabling the Internationalization API in desktop Firefox. So there isn't especially "about to", from my point of view. :-\ Dunno where that leaves this, exactly.
Flags: needinfo?(jwalden+bmo)
Assignee | ||
Comment 12•11 years ago
|
||
Ok, it looks like bug 724768 does away with JSExnPrivate entirely, which significantly changes what this patch would look like. I'm going to tentatively block on that.
Depends on: 724768
Assignee | ||
Comment 13•11 years ago
|
||
I'm working on this again.
Assignee | ||
Comment 14•11 years ago
|
||
Assignee | ||
Comment 15•11 years ago
|
||
Assignee | ||
Comment 16•11 years ago
|
||
Assignee | ||
Comment 17•11 years ago
|
||
Assignee | ||
Comment 18•11 years ago
|
||
Green. Uploading patches and flagging for review.
Assignee | ||
Comment 19•11 years ago
|
||
We're going to need to start doing more work in js_ErrorFromException, which
will require a |cx| and may GC.
Assignee | ||
Comment 20•11 years ago
|
||
This lets js_ReportUncaughtException get the report directly from the underlying
Error object, rather than trying to duck-type it (which fails for security
wrappers).
Assignee | ||
Comment 21•11 years ago
|
||
Note that we have to update a test that was previously expecting to hit the
bail-out case at the bottom of js_ReportUncaughtException.
Assignee | ||
Updated•11 years ago
|
Attachment #750543 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8357212 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8357213 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8357214 -
Attachment is obsolete: true
Assignee | ||
Comment 22•11 years ago
|
||
We're going to need to start doing more work in js_ErrorFromException, which
will require a |cx| and may GC.
Attachment #8357216 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 23•11 years ago
|
||
This lets js_ReportUncaughtException get the report directly from the underlying
Error object, rather than trying to duck-type it (which fails for security
wrappers).
Attachment #8357217 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 24•11 years ago
|
||
Note that we have to update a test that was previously expecting to hit the
bail-out case at the bottom of js_ReportUncaughtException.
Attachment #8357218 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 25•11 years ago
|
||
This stuff is exactly rooted now, so this is all unnecessary.
Attachment #8357219 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 26•11 years ago
|
||
Historically, we've sometimes reported ErrorEvent.message as |Error.name|, and
sometimes as |Error.name: Error.message|, depending on which path we took
through js_ReportUncaughtException. We need to standardize on something, so
let's use the latter, since it contains strictly more information. This involves
updating a few tests.
Attachment #8357220 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 27•11 years ago
|
||
Attachment #8357221 -
Flags: review?(jwalden+bmo)
Comment on attachment 8357220 [details] [diff] [review]
Part 5 - Use js::ErrorReportToString for worker ErrorEvents. v1
Review of attachment 8357220 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/workers/WorkerPrivate.cpp
@@ +5136,2 @@
> if (aReport->ucmessage) {
> + JS::Rooted<JSString*> messageStr(aCx, js::ErrorReportToString(aCx, aReport));
Wait, can this fail? and what will JS_GetStringCharsZ() do if it does?
Nit: Please wrap to < 80 chars.
@@ +5136,3 @@
> if (aReport->ucmessage) {
> + JS::Rooted<JSString*> messageStr(aCx, js::ErrorReportToString(aCx, aReport));
> + message = JS_GetStringCharsZ(aCx, messageStr);
JS_GetStringCharsZAndLength would avoid a strlen here.
Attachment #8357220 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 29•11 years ago
|
||
Attachment #8357220 -
Attachment is obsolete: true
Attachment #8357283 -
Flags: review?(bent.mozilla)
Comment on attachment 8357283 [details] [diff] [review]
Part 5 - Use js::ErrorReportToString for worker ErrorEvents. v2
Review of attachment 8357283 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/workers/WorkerPrivate.cpp
@@ +5132,5 @@
> + // do. Traditionally (and mostly by accident), the |message| field of
> + // ErrorEvent has corresponded to |Name: Message| of the original Error
> + // object. Things have been cleaned up in the JS engine, so now we need to
> + // format this string explicitly.
> + JS::Rooted<JSString*> messageStr(aCx, js::ErrorReportToString(aCx, aReport));
Nit: this needs to be wrapped
Attachment #8357283 -
Flags: review?(bent.mozilla) → review+
Comment 31•11 years ago
|
||
Comment on attachment 8357216 [details] [diff] [review]
Part 1 - Remove non-cx variant of ErrorFromException, and make it take a HandleObject. v1
Review of attachment 8357216 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/promise/Promise.cpp
@@ +546,5 @@
> + JSContext* cx = nsContentUtils::GetDefaultJSContextForThread();
> + JSAutoRequest ar(cx);
> + JS::Rooted<JSObject*> obj(cx, &mResult.toObject());
> + JSAutoCompartment ac(cx, obj);
> + JSErrorReport* report = JS_ErrorFromException(cx, obj);
La la la I have no idea whether any of this makes any sense at all la la la...
Attachment #8357216 -
Flags: review?(jwalden+bmo) → review+
Comment 32•11 years ago
|
||
Comment on attachment 8357217 [details] [diff] [review]
Part 2 - Generate a JSErrorReport when we need one. v1
Review of attachment 8357217 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/vm/ErrorObject.cpp
@@ +111,5 @@
> +JSErrorReport *
> +js::ErrorObject::getOrCreateErrorReport(JSContext *cx)
> +{
> + if (getErrorReport())
> + return getErrorReport();
if (JSErrorReport *report = getErrorReport())
return report;
@@ +124,5 @@
> + report.exnType = type_;
> +
> + // Filename.
> + JSAutoByteString filenameStr;
> + filenameStr.encodeLatin1(cx, fileName());
Needs a null-check.
@@ +125,5 @@
> +
> + // Filename.
> + JSAutoByteString filenameStr;
> + filenameStr.encodeLatin1(cx, fileName());
> + report.filename = filenameStr.ptr();
Uh...this looks really dodgy. Presumably this doesn't transfer ownership. So isn't this filling in a pointer to dangling stack memory?
@@ +141,5 @@
> + return nullptr;
> + report.ucmessage = message->asStable().chars().get();
> +
> + // Cache and return.
> + JSErrorReport *copy = CopyErrorReport(cx, &report);
Null-check.
Attachment #8357217 -
Flags: review?(jwalden+bmo) → review-
Comment 33•11 years ago
|
||
Comment on attachment 8357218 [details] [diff] [review]
Part 3 - Don't ToString the exn if we already got a report out of it. v1
Review of attachment 8357218 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jsexn.cpp
@@ +727,5 @@
> + JSExnType type = static_cast<JSExnType>(reportp->exnType);
> + RootedString str(cx, cx->runtime()->emptyString);
> + if (type != JSEXN_NONE)
> + str = ClassName(GetExceptionProtoKey(type), cx);
> + RootedString toAppend(cx, JS_NewStringCopyZ(cx, ": "));
Using MOZ_UTF16(": ") and 2 might be better, with the corresponding method.
@@ +772,5 @@
> : nullptr;
>
> + // Be careful not to invoke ToString if we've already successfully extracted
> + // an error report, since the exception might be wrapped in a security
> + // wrapper, and ToString-ing it might throw.
Really we shouldn't invoke ToString ever in this case. I'm pretty sure this code is what causes self-hosting Error.prototype.toString to fail. But this is a marginal improvement.
@@ +854,5 @@
> + // |ErrorName: ErrorMessage|, and |ucmessage| is supposed to
> + // correspond to |ErrorMessage|. But this is what we've historically
> + // done for duck-typed error objects.
> + //
> + // If only this stuff could get specced one day...
Going purely by function-name, I'm not sure how or why reporting would ever be specified. :-)
Attachment #8357218 -
Flags: review?(jwalden+bmo) → review+
Comment 34•11 years ago
|
||
Comment on attachment 8357219 [details] [diff] [review]
Part 4 - Remove manual rooting from js_ReportUncaughtException. v1
Review of attachment 8357219 [details] [diff] [review]:
-----------------------------------------------------------------
Would r+ again!
Attachment #8357219 -
Flags: review?(jwalden+bmo) → review+
Comment 35•11 years ago
|
||
Comment on attachment 8357221 [details] [diff] [review]
Part 6 - Tests. v1
Review of attachment 8357221 [details] [diff] [review]:
-----------------------------------------------------------------
I'll look at this in-person, I guess, ran out of time to do it now (and don't have working laptop to do it on train, sadly).
Attachment #8357221 -
Flags: review?(jwalden+bmo)
Updated•11 years ago
|
Attachment #8357221 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 36•11 years ago
|
||
Comment on attachment 8357217 [details] [diff] [review]
Part 2 - Generate a JSErrorReport when we need one. v1
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #32)
> > +
> > + // Filename.
> > + JSAutoByteString filenameStr;
> > + filenameStr.encodeLatin1(cx, fileName());
> > + report.filename = filenameStr.ptr();
>
> Uh...this looks really dodgy. Presumably this doesn't transfer ownership.
> So isn't this filling in a pointer to dangling stack memory?
It should be fine. The function is set up to create a JSErrorReport on the stack, and then pass that to CopyErrorReport (which does the memcpy).
Attachment #8357217 -
Flags: review- → review+
Assignee | ||
Comment 37•11 years ago
|
||
Updated•11 years ago
|
Attachment #8357221 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 38•11 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/524b810fa489
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/a231c042c5d3
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/bea936ee8d8b
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/0302759868ba
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/509675067088
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/1a2cbf8c440e
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/46df3fd9b0dc
Comment 39•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a231c042c5d3
https://hg.mozilla.org/mozilla-central/rev/bea936ee8d8b
https://hg.mozilla.org/mozilla-central/rev/0302759868ba
https://hg.mozilla.org/mozilla-central/rev/509675067088
https://hg.mozilla.org/mozilla-central/rev/1a2cbf8c440e
https://hg.mozilla.org/mozilla-central/rev/46df3fd9b0dc
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Comment 41•11 years ago
|
||
I moved the test added here to the correct directory so it will actually be run:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e087156c289
(see commit message for details).
Assignee | ||
Comment 42•11 years ago
|
||
Whoa. Thanks for catching/fixing that dbaron!
You need to log in
before you can comment on or make changes to this bug.
Description
•