987669 - Implement Xrays to Error objects

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

This is a kind of object that often crosses privilege boundaries. For example, in bug 987222, exceptions that propagate into XBL end up looking like this:

System JS : ERROR (null):0 - uncaught exception: [Opaque]

We should implement useful Xrays here, which will at least make the [Opaque] say something more useful.

Comment 1

[Boris Zbarsky [:bzbarsky]]

I don't think we should be exposing info about an exception to a scope that couldn't normally read that info....

Comment 2

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #1)
> I don't think we should be exposing info about an exception to a scope that
> couldn't normally read that info....

Oh, for sure. The filtering policy will take care of that. The issue here is with the inverse wrapper, whereby we protect privileged code from less-privileged code by making the wrappers opaque (we do this for XBL scopes, where we don't have the compatibility burden we have in the general case).

The issue in bug 987222 is that the page throws an exception, but it gets reported in the XBL scope, where stringifying it gives "[Opaque]". Xrays will fix that for us.

Comment 3

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=f6305f82daf3

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 1 - Implement JS::ProtoKeyToId. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 2 - Implement IsErrorObjectKey and do more at compile-time. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 3 - Implement Xray support for the data properties on ErrorObject instances. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 4 - Add Xray support for FooError.prototype.name. v1

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 5 - Turn on Xrays for Error objects. v1

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 6 - Tests. v1

Comment 10

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8453287 [details] [diff] [review]
Part 3 - Implement Xray support for the data properties on ErrorObject instances. v1

Review of attachment 8453287 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +621,5 @@
> +            if (isErrorIntProperty || isErrorStringProperty) {
> +                RootedObject waiver(cx, wrapper);
> +                if (!WrapperFactory::WaiveXrayAndWrap(cx, &waiver))
> +                    return false;
> +                if (!JS_GetOwnPropertyDescriptorById(cx, waiver, id, desc))

This might be a stupid question, but is there a way that the underlying object has a proxy in its proto chain that can interrupt this? Normally that would not be an issue but after the waiving it... r=me with either adding an isJSProxy call here, or explaining me why this cannot be an issue and/or adding a test for it.

Comment 11

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #10)
> > +            if (isErrorIntProperty || isErrorStringProperty) {
> > +                RootedObject waiver(cx, wrapper);
> > +                if (!WrapperFactory::WaiveXrayAndWrap(cx, &waiver))
> > +                    return false;
> > +                if (!JS_GetOwnPropertyDescriptorById(cx, waiver, id, desc))
> 
> This might be a stupid question, but is there a way that the underlying
> object has a proxy in its proto chain that can interrupt this? Normally that
> would not be an issue but after the waiving it... r=me with either adding an
> isJSProxy call here, or explaining me why this cannot be an issue and/or
> adding a test for it.

It's a non-issue because we know the JSClass of target is ErrorObject::class_, and the lookup is |own|, which means we never hit the proto.

Comment 12

[Gabor Krizsanits (INACTIVE)]

Ah right, I forgot about the own part! Nevermind then.

Comment 13

[Bobby Holley (:bholley)]

Thanks for the reviews gabor!

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2f058f0f4c1c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8fed72d0f496
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4e45b8a55bfb
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e171c0317b82
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a2cbaa337120
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/48b4e65d344d

Comment 14

[Bobby Holley (:bholley)]

Followup bustage fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/d257e3805fc7

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2f058f0f4c1c
https://hg.mozilla.org/mozilla-central/rev/8fed72d0f496
https://hg.mozilla.org/mozilla-central/rev/4e45b8a55bfb
https://hg.mozilla.org/mozilla-central/rev/e171c0317b82
https://hg.mozilla.org/mozilla-central/rev/a2cbaa337120
https://hg.mozilla.org/mozilla-central/rev/48b4e65d344d
https://hg.mozilla.org/mozilla-central/rev/d257e3805fc7