Closed Bug 1332713 Opened 3 years ago Closed 3 years ago

Various updates for Promise-typed attributes

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox53 --- affected
firefox54 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 3 obsolete files)

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.
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.
Attachment #8828969 - Flags: review?(bobbyholley)
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?
Attachment #8828969 - Flags: review?(bobbyholley) → review+
> 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.
(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.
Attachment #8828969 - Attachment is obsolete: true
Attachment #8828980 - Attachment is obsolete: true
Attachment #8828980 - Flags: review?(kyle)
Attachment #8828987 - Attachment is obsolete: true
Attachment #8828987 - Flags: review?(kyle)
Attachment #8828967 - Flags: review?(kyle) → review+
Attachment #8828968 - Flags: review?(kyle) → review+
Attachment #8829062 - Flags: review?(kyle) → review+
Sorry for the slow review on this, took me a bit to wrap my head around patches 3 and 4.
Not a problem at all.  Part 4 is a bit complicated.

Are there things that could use more documentation to make it more understandable?
Flags: needinfo?(kyle)
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.
Flags: needinfo?(kyle)
Pushed by bzbarsky@mozilla.com:
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
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.