1332713 - Various updates for Promise-typed attributes

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

https://github.com/heycam/webidl/pull/217 made the following relevant changes:

1) Disallow writable Promise-typed attributes.
2) Disallow Promise-typed attributes that have [LenientSetter], [PutForwards], [Replaceable], since those will induce a setter.
3) Disallow [SameObject] on Promise attributes, because that will no longer be guaranteed because....
4) An exception thrown from a Promise-typed attribute getter is turned into a rejected Promise that is returned.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Disallow Promise-typed attributes with setters

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Disallow [SameObject] Promise-typed attributes

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Implement a version of XrayAwareCalleeGlobal that works for specialized getters

The problem is that JSJitGetterInfo doesn't contain a callee.  So we can't use
xpc::XrayAwareCalleeGlobal in a specialized getter, because we don't have our
callee function available.

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 8828969 [details] [diff] [review]
part 3.  Implement a version of XrayAwareCalleeGlobal that works for specialized getters

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

r=me with those things addressed, though I'm still a bit fuzzy on this.

::: dom/bindings/BindingUtils.cpp
@@ +3601,5 @@
>    return GetCurrentThreadWorkerGlobal();
>  }
> +
> +bool
> +XrayAwareCalleeGlobalForSpecializedGetters(JSContext* cx,

I think we should move this next to the other XrayAwareCalleeGlobal implementation.

@@ +3615,5 @@
> +    // Our current compartment would generaly get xrays to thisObj.  That means
> +    // we're presumably doing a call over Xrays, an the compartment of the
> +    // callee is presumably that of thisObj.  This isn't _necessarily_ true
> +    // (e.g. chrome code could be using a chrome-side getter and doing .call()
> +    // with a content-side this value), but people shouldn't do that!

This also applies to web-extensions and to XOWs, right? I can't think of anything bad that would happen, but it's probably worth describing what the impact is if somebody did something tricky.

::: dom/bindings/BindingUtils.h
@@ +3209,5 @@
>  // understanding of all the code that will run while we're using the return
>  // value, including the SpiderMonkey parts.
>  JSObject* UnprivilegedJunkScopeOrWorkerGlobal();
> +
> +// We need this function because in a specialized getter we don't have a callee,

Nit: odd comma?

Comment 5

[Boris Zbarsky [:bzbarsky]]

> I think we should move this next to the other XrayAwareCalleeGlobal implementation.

OK.  Do you want this in the xpc namespace then?

> This also applies to web-extensions and to XOWs, right?

In practice, this only applies to promise-returning attributes, and we don't support those on XOWs.

It does apply to web-extensions.

If someone does something tricky, we will create the rejected promise that we reject with our exception in the "wrong" compartment.  In particular, we might create it in the content compartment even though we should really have created it in the chrome compartment (for the case when a chrome getter is invoked with a content object instead of just invoking the xrayed getter).

> Nit: odd comma?

Fixed.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Make Promise-returning getters return a rejected Promise on exception instead of throwing

Comment 7

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #5)
> > I think we should move this next to the other XrayAwareCalleeGlobal implementation.
> 
> OK.  Do you want this in the xpc namespace then?

Sure, whatever makes them the most analogous and makes us most likely to find one while touching the other.

> 
> > This also applies to web-extensions and to XOWs, right?
> 
> In practice, this only applies to promise-returning attributes, and we don't
> support those on XOWs.
> 
> It does apply to web-extensions.
> 
> If someone does something tricky, we will create the rejected promise that
> we reject with our exception in the "wrong" compartment.  In particular, we
> might create it in the content compartment even though we should really have
> created it in the chrome compartment (for the case when a chrome getter is
> invoked with a content object instead of just invoking the xrayed getter).

OK - add it as a comment?
> 
> > Nit: odd comma?
> 
> Fixed.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 3 updated to comments

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Make Promise-returning getters return a rejected Promise on exception instead of throwing

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Make Promise-returning getters return a rejected Promise on exception instead of throwing

Comment 11

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Sorry for the slow review on this, took me a bit to wrap my head around patches 3 and 4.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Not a problem at all.  Part 4 is a bit complicated.

Are there things that could use more documentation to make it more understandable?

Comment 13

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Nah, I think it's fine as is. Just hadn't been in that part of the code in a while and am moving a little slow this week due to moving.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/480e0aecb146
part 1.  Disallow Promise-typed attributes with setters.  r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/263cbf5af4f1
part 2.  Disallow [SameObject] Promise-typed attributes.  r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/44460e3f0d1a
part 3.  Implement a version of XrayAwareCalleeGlobal that works for specialized getters.  r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/432ed6f1eef1
part 4.  Make Promise-returning getters return a rejected Promise on exception instead of throwing.  r=qdot

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/480e0aecb146
https://hg.mozilla.org/mozilla-central/rev/263cbf5af4f1
https://hg.mozilla.org/mozilla-central/rev/44460e3f0d1a
https://hg.mozilla.org/mozilla-central/rev/432ed6f1eef1