Closed Bug 872273 Opened 11 years ago Closed 10 years ago

Better reporting of privileged exceptions in unprivileged scopes

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(6 files, 5 obsolete files)

10.97 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
6.70 KB, patch
bholley
: review+
Details | Diff | Splinter Review
5.97 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
4.53 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
2.31 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.96 KB, 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.
> 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.
(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?
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.
No longer blocks: 871887
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
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.
Attached patch WIP. v1 (obsolete) — Splinter Review
Oh wait, I forgot to |commit --amend|. Facepalm.
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)
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)
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
Blocks: 933574
Depends on: 956385
I'm working on this again.
Green. Uploading patches and flagging for review.
We're going to need to start doing more work in js_ErrorFromException, which
will require a |cx| and may GC.
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).
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 #750543 - Attachment is obsolete: true
Attachment #8357212 - Attachment is obsolete: true
Attachment #8357213 - Attachment is obsolete: true
Attachment #8357214 - Attachment is obsolete: true
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)
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)
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)
This stuff is exactly rooted now, so this is all unnecessary.
Attachment #8357219 - Flags: review?(jwalden+bmo)
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)
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)
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 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 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 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 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 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)
Attachment #8357221 - Flags: review?(jwalden+bmo)
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+
Attachment #8357221 - Flags: review?(jwalden+bmo) → review+
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).
Whoa. Thanks for catching/fixing that dbaron!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: