1133746 - Add an option for "property is on expando" to DOMProxyShadowsResult

Status: RESOLVED FIXED

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

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Allow DOMProxyShadows to communicate to the JIT whether the shadowing is done by the expando object or not

Comment 2

[Eric Faust [:efaust]]

Comment on attachment 8565509 [details] [diff] [review]
Allow DOMProxyShadows to communicate to the JIT whether the shadowing is done by the expando object or not

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

::: dom/bindings/DOMJSProxyHandler.cpp
@@ +32,5 @@
>  
>  js::DOMProxyShadowsResult
>  DOMProxyShadows(JSContext* cx, JS::Handle<JSObject*> proxy, JS::Handle<jsid> id)
>  {
> +  JS::Rooted<JSObject*> expando(cx, DOMProxyHandler::GetExpandoObject(proxy));

This operation already looks up the expando slot. This is kinda clunky, but I guess better than trying to wrap the isOverrideBuiltins checking logic into GetExpandoObject, since it's presumably not useful to know that at the same time elsewhere?

::: js/src/jsfriendapi.h
@@ +1214,5 @@
>  uint32_t GetDOMProxyExpandoSlot();
>  DOMProxyShadowsCheck GetDOMProxyShadowsCheck();
> +inline bool DOMProxyIsShadowing(DOMProxyShadowsResult result) {
> +    return result == Shadows || result == ShadowsViaDirectExpando ||
> +        result == ShadowsViaIndirectExpando;

nit: Could stand to see nicer indenting here.

Perhaps

return result == Shadows ||
       result == ShadowsViaDirectExpando ||
       result == ShadowsViaIndirectExpando;

Comment 3

[Boris Zbarsky [:bzbarsky]]

> This operation already looks up the expando slot

Yeah.  I didn't really find a good way to deal with that...

> since it's presumably not useful to know that at the same time elsewhere?

Pretty much never, in fact.

> nit: Could stand to see nicer indenting here.

Done.

Comment 4

[Peter Van der Beken [:peterv]]

Comment on attachment 8565509 [details] [diff] [review]
Allow DOMProxyShadows to communicate to the JIT whether the shadowing is done by the expando object or not

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

::: dom/bindings/DOMJSProxyHandler.cpp
@@ +42,5 @@
>        return js::ShadowCheckFailed;
>  
> +    if (hasOwn) {
> +      return isOverrideBuiltins ?
> +        js::ShadowsViaIndirectExpando : js::ShadowsViaDirectExpando;

If an interface has OverrideBuiltins then expandos shouldn't shadow named properties. Doesn't this make them shadow?

Comment 5

[Boris Zbarsky [:bzbarsky]]

> then expandos shouldn't shadow named properties.

That's what the spec says, but it's not what we do right now.  Right now expandos shadow in our implementation, and I just preserved the behavior.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Posted https://lists.w3.org/Archives/Public/public-script-coord/2015JanMar/0120.html to get the spec sorted out.

Comment 7

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa4a0af4fbaf

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fa4a0af4fbaf