Closed Bug 1133746 Opened 5 years ago Closed 5 years ago

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

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(1 file)

No description provided.
Depends on: 1133760
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
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;
Attachment #8565509 - Flags: review?(efaustbmo) → review+
> 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 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?
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
Attachment #8565509 - Flags: review?(peterv) → review+
https://hg.mozilla.org/mozilla-central/rev/fa4a0af4fbaf
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.