Bug 173229 – [WebIDL] Re-implement GetOwnPropertySlot[ByIndex] generation to more closely follow WebIDL

• Component: New Bugs
• Status: RESOLVED FIXED

[Sam Weinig]

[WebIDL] Re-implement GetOwnPropertySlot[ByIndex] generation to more closely follow WebIDL

[Sam Weinig]

Created attachment 312576 [details]
Patch

[Sam Weinig]

Created attachment 312578 [details]
Patch

[Build Bot]

Comment on attachment 312578 [details]
Patch

Attachment 312578 [details] did not pass bindings-ews (mac):
Output: http://webkit-queues.webkit.org/results/3908781

New failing tests:
(JS) JSTestGenerateIsReachable.cpp
(JS) JSTestNode.cpp
(JS) JSTestObj.cpp

[Sam Weinig]

Created attachment 312580 [details]
Patch

[Build Bot]

Comment on attachment 312580 [details]
Patch

Attachment 312580 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3909241

New failing tests:
js/dom/prototype-chain-caching-with-impure-get-own-property-slot-traps-5.html

[Build Bot]

Created attachment 312583 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Sam Weinig]

Created attachment 312586 [details]
Patch

[Chris Dumez]

Comment on attachment 312586 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312586&action=review

> Source/WebCore/ChangeLog:14
> +        Add new IDL type, IDLDOMWindowShell, to represent the mapping from Frame -> JSDOMWindowShell,

I don't get that. Why map a Frame to a JSDOMWindowShell. A DOMWindow to map to a JSDOMWindowShell IMO. A JSDOMWindowShell if the wrapper exposed to JS for DOMWindow, and it proxies everything to JSDOMWindow.
You can have a DOMWindow / JSDOMWindow / JSDOMWindowShell without frame.

[Sam Weinig]

(In reply to Chris Dumez from comment #8)
> Comment on attachment 312586 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=312586&action=review
> 
> > Source/WebCore/ChangeLog:14
> > +        Add new IDL type, IDLDOMWindowShell, to represent the mapping from Frame -> JSDOMWindowShell,
> 
> I don't get that. Why map a Frame to a JSDOMWindowShell. A DOMWindow to map
> to a JSDOMWindowShell IMO. A JSDOMWindowShell if the wrapper exposed to JS
> for DOMWindow, and it proxies everything to JSDOMWindow.
> You can have a DOMWindow / JSDOMWindow / JSDOMWindowShell without frame.

That isn't really the model that it was designed for. The JSDOMWindowShell is the wrapper for the Frame, in the sense that even as the Documents and DOMWindows change, as the Frame navigates, the JSDOMWindowShell always stays the same.

In fact, the way you get to a JSDOMWindowShell is always via the Frame (by way of it's ScriptController).

[Chris Dumez]

Comment on attachment 312586 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312586&action=review

> Source/WebCore/ChangeLog:35
> +        (WebCore::JSHTMLDocument::getOwnPropertySlot):.

I would add to the Changelog that you changed behavior. Named properties on HTML document no longer override own properties, they only override properties from the prototype chain (as per the WebIDL specification for OverrideBuiltins).

> Source/WebCore/bindings/js/JSDOMWindowShell.h:39
> +// FIXME: Rename to JSWindowProxy to match HTML spec naming.

or JSDOMWindowProxy since we use JSDOMWindow instead of JSWindow

> Source/WebCore/bindings/js/JSDOMWindowShell.h:75
> +JSC::JSValue toJS(JSC::ExecState*, Frame&);

Maybe a comment to explain that this returns a JSDOMWindowShell? This is not necessarily obvious given that the parameter is a Frame.

> Source/WebCore/html/HTMLFrameSetElement.cpp:231
> +Frame* HTMLFrameSetElement::namedItem(const AtomicString& name)

I really feel that it is weird to return a Frame here. I think we should return a DOMWindow.

Let consider window.self. It is defined as a returning a WindowProxy in the spec. What's next? Are we going to update DOMWindow::self() to return a Frame* ?

> Source/WebCore/html/HTMLFrameSetElement.idl:41
> +    getter DOMWindowShell (DOMString name);

So now we start using DOMWindowShell in the IDL? If so, we should do it consistently (And rename to WindowProxy to match the spec). So far, we've been using DOMWindow everywhere.

> LayoutTests/ChangeLog:9
> +        Update results. These now match Firefox.

I would explain here that this is a progression because you fixed a bug in HTMLdocument's getOwnPropertySlot:
Named properties on HTML document no longer override own properties, they only override properties from the prototype chain (as per the WebIDL specification for OverrideBuiltins).

> LayoutTests/ChangeLog:11
> +        * js/dom/dfg-prototype-chain-caching-with-impure-get-own-property-slot-traps-4-expected.txt:

I looked at this one and this seems again to be caused by the bug fix I mentioned earlier. Image with name foo no longer overrides own property with name foo (which is of type "function").

Instead of rebaselining with FAIL lines, I think we should fix those tests. Since they want to test impure get own property. I would suggest we update them to move the foo property from document to the document prototype. Then I believe the tests would PASS and keep testing what they want to test (A named property can override another property because of OverrideBuiltins).

[Sam Weinig]

(In reply to Chris Dumez from comment #10)
> Comment on attachment 312586 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=312586&action=review
> 
> > Source/WebCore/ChangeLog:35
> > +        (WebCore::JSHTMLDocument::getOwnPropertySlot):.
> 
> I would add to the Changelog that you changed behavior. Named properties on
> HTML document no longer override own properties, they only override
> properties from the prototype chain (as per the WebIDL specification for
> OverrideBuiltins).

Ok.

> 
> > Source/WebCore/bindings/js/JSDOMWindowShell.h:39
> > +// FIXME: Rename to JSWindowProxy to match HTML spec naming.
> 
> or JSDOMWindowProxy since we use JSDOMWindow instead of JSWindow
> 
> > Source/WebCore/bindings/js/JSDOMWindowShell.h:75
> > +JSC::JSValue toJS(JSC::ExecState*, Frame&);
> 
> Maybe a comment to explain that this returns a JSDOMWindowShell? This is not
> necessarily obvious given that the parameter is a Frame.

Sure.

> 
> > Source/WebCore/html/HTMLFrameSetElement.cpp:231
> > +Frame* HTMLFrameSetElement::namedItem(const AtomicString& name)
> 
> I really feel that it is weird to return a Frame here. I think we should
> return a DOMWindow.
> 
> Let consider window.self. It is defined as a returning a WindowProxy in the
> spec. What's next? Are we going to update DOMWindow::self() to return a
> Frame* ?

I actually think that we should update all places that return a DOMWindow for bindings purposes to return a Frame, as that is ultimately more accurate, and the toJS() for DOMWindow, just goes and gets the Frame.

Perhaps this could be made less confusing if we added a real DOMWindowProxy class that lives off the Frame. A more realistic version might be to just renamed the already poorly named ScriptController to DOMWindowProxy.

But, I think that should be left to another change, so I will update this patch to use DOMWindow, and remove the IDLDOMWindowShell stuff.

> 
> > LayoutTests/ChangeLog:9
> > +        Update results. These now match Firefox.
> 
> I would explain here that this is a progression because you fixed a bug in
> HTMLdocument's getOwnPropertySlot:
> Named properties on HTML document no longer override own properties, they
> only override properties from the prototype chain (as per the WebIDL
> specification for OverrideBuiltins).

Ok.

> 
> > LayoutTests/ChangeLog:11
> > +        * js/dom/dfg-prototype-chain-caching-with-impure-get-own-property-slot-traps-4-expected.txt:
> 
> I looked at this one and this seems again to be caused by the bug fix I
> mentioned earlier. Image with name foo no longer overrides own property with
> name foo (which is of type "function").
> 
> Instead of rebaselining with FAIL lines, I think we should fix those tests.
> Since they want to test impure get own property. I would suggest we update
> them to move the foo property from document to the document prototype. Then
> I believe the tests would PASS and keep testing what they want to test (A
> named property can override another property because of OverrideBuiltins).

Ok.

[Sam Weinig]

Created attachment 312609 [details]
Patch

[Chris Dumez]

Comment on attachment 312609 [details]
Patch

r=me!

[WebKit Commit Bot]

Comment on attachment 312609 [details]
Patch

Clearing flags on attachment: 312609

Committed r218126: <http://trac.webkit.org/changeset/218126>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Saam Barati]

Comment on attachment 312609 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312609&action=review

> Source/WebCore/ChangeLog:16
> +        in isVisibleNamedProperty, switch to VMInquiry as we use elsewhere and add a missing FIXME.

Why VMInquiry? This seems observable if we're calling into various JS hooks like [[Get]] and [[HasOwnProperty]], etc.

[Sam Weinig]

(In reply to Saam Barati from comment #16)
> Comment on attachment 312609 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=312609&action=review
> 
> > Source/WebCore/ChangeLog:16
> > +        in isVisibleNamedProperty, switch to VMInquiry as we use elsewhere and add a missing FIXME.
> 
> Why VMInquiry? This seems observable if we're calling into various JS hooks
> like [[Get]] and [[HasOwnProperty]], etc.

This was a while ago, so my memory is not great, but I believe if you don't, you might end up recursing indefinitely, since you could be in a [[Get]], etc. when calling this helper. 

The spec just says things like "If O has an own property named P, then return false." here (https://heycam.github.io/webidl/#dfn-named-property-visibility) and it usually specifies and links to a the ECMAScript spec when it wants you to ensure a hook is called.

[Saam Barati]

(In reply to Sam Weinig from comment #17)
> (In reply to Saam Barati from comment #16)
> > Comment on attachment 312609 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=312609&action=review
> > 
> > > Source/WebCore/ChangeLog:16
> > > +        in isVisibleNamedProperty, switch to VMInquiry as we use elsewhere and add a missing FIXME.
> > 
> > Why VMInquiry? This seems observable if we're calling into various JS hooks
> > like [[Get]] and [[HasOwnProperty]], etc.
> 
> This was a while ago, so my memory is not great, but I believe if you don't,
> you might end up recursing indefinitely, since you could be in a [[Get]],
> etc. when calling this helper. 
> 
> The spec just says things like "If O has an own property named P, then
> return false." here
> (https://heycam.github.io/webidl/#dfn-named-property-visibility) and it
> usually specifies and links to a the ECMAScript spec when it wants you to
> ensure a hook is called.

Do you know if this can be called on arbitrary JS object types?

[Sam Weinig]

(In reply to Saam Barati from comment #18)
> (In reply to Sam Weinig from comment #17)
> > (In reply to Saam Barati from comment #16)
> > > Comment on attachment 312609 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=312609&action=review
> > > 
> > > > Source/WebCore/ChangeLog:16
> > > > +        in isVisibleNamedProperty, switch to VMInquiry as we use elsewhere and add a missing FIXME.
> > > 
> > > Why VMInquiry? This seems observable if we're calling into various JS hooks
> > > like [[Get]] and [[HasOwnProperty]], etc.
> > 
> > This was a while ago, so my memory is not great, but I believe if you don't,
> > you might end up recursing indefinitely, since you could be in a [[Get]],
> > etc. when calling this helper. 
> > 
> > The spec just says things like "If O has an own property named P, then
> > return false." here
> > (https://heycam.github.io/webidl/#dfn-named-property-visibility) and it
> > usually specifies and links to a the ECMAScript spec when it wants you to
> > ensure a hook is called.
> 
> Do you know if this can be called on arbitrary JS object types?

Since it is part of property lookup, I don't think so.