Make our bindings' GetOwnPropertySlot() behave according to specification

• Component: Bindings
• Status: RESOLVED FIXED

[Chris Dumez]

Make our bindings's GetOwnPropertySlot() behave according to specification:
https://heycam.github.io/webidl/#getownproperty-guts
https://heycam.github.io/webidl/#dfn-named-property-visibility

Summary from https://heycam.github.io/webidl/#dfn-named-property-visibility :
====
This should ensure that for objects with named properties, property resolution is done in the following order:

1. Indexed properties.
2. Unforgeable attributes and operations.
3. Then, if [OverrideBuiltins]:
    1. Named properties.
    2. Own properties.
    3. Properties from the prototype chain.
4. Otherwise, if not [OverrideBuiltins]:
    1. Own properties.
    2. Properties from the prototype chain.
    3. Named properties.
====

[Chris Dumez]

Created attachment 259180 [details]
WIP patch

[Build Bot]

Comment on attachment 259180 [details]
WIP patch

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

New failing tests:
http/tests/security/xssAuditor/full-block-javascript-link.html
http/tests/security/cross-frame-access-history-get-override.html
http/tests/security/xss-DENIED-defineProperty.html
http/tests/security/cross-frame-access-location-get-override.html
http/tests/security/xssAuditor/full-block-iframe-javascript-url.html
http/tests/security/cross-frame-access-history-get.html
http/tests/security/xssAuditor/full-block-script-tag.html
http/tests/security/cross-frame-access-callback-explicit-domain-DENY.html
http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag-parent-same-origin-deny.html
http/tests/security/xssAuditor/full-block-script-tag-cross-domain.html
fast/workers/worker-replace-global-constructor.html
http/tests/security/xssAuditor/full-block-base-href.html
http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag-in-body.html
http/tests/security/xssAuditor/xss-protection-parsing-03.html
http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag.html
http/tests/security/cross-frame-access-location-get.html
http/tests/security/sandboxed-iframe-blocks-access-from-parent.html
http/tests/security/XFrameOptions/x-frame-options-multiple-headers-sameorigin-deny.html
http/tests/plugins/cross-frame-object-access.html
http/tests/security/xssAuditor/full-block-link-onclick.html
http/tests/security/XFrameOptions/x-frame-options-deny.html
http/tests/security/xssAuditor/full-block-script-tag-with-source.html
http/tests/security/XFrameOptions/x-frame-options-parent-same-origin-deny.html
http/tests/security/cross-frame-access-getOwnPropertyDescriptor.html
http/tests/security/xssAuditor/xss-protection-parsing-04.html
http/tests/security/xssAuditor/block-does-not-leak-location.html
http/tests/security/xssAuditor/full-block-object-tag.html

[Build Bot]

Created attachment 259188 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 259180 [details]
WIP patch

Attachment 259180 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/69157

New failing tests:
http/tests/security/xssAuditor/full-block-javascript-link.html
http/tests/security/xss-DENIED-defineProperty.html
http/tests/security/xssAuditor/full-block-iframe-javascript-url.html
http/tests/security/xssAuditor/full-block-script-tag.html
http/tests/security/cross-frame-access-callback-explicit-domain-DENY.html
http/tests/security/xssAuditor/full-block-script-tag-cross-domain.html
fast/workers/worker-replace-global-constructor.html
http/tests/security/xssAuditor/full-block-base-href.html
http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag-in-body.html
http/tests/security/xssAuditor/xss-protection-parsing-03.html
http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag.html
http/tests/security/sandboxed-iframe-blocks-access-from-parent.html
http/tests/security/XFrameOptions/x-frame-options-multiple-headers-sameorigin-deny.html
http/tests/plugins/cross-frame-object-access.html
http/tests/security/xssAuditor/full-block-link-onclick.html
http/tests/security/XFrameOptions/x-frame-options-deny.html
http/tests/security/xssAuditor/full-block-script-tag-with-source.html
http/tests/security/XFrameOptions/x-frame-options-parent-same-origin-deny.html
http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag-parent-same-origin-deny.html
http/tests/security/xssAuditor/xss-protection-parsing-04.html
http/tests/security/xssAuditor/block-does-not-leak-location.html
http/tests/security/xssAuditor/full-block-object-tag.html

[Build Bot]

Created attachment 259189 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Chris Dumez]

Created attachment 259199 [details]
Patch

[WebKit Commit Bot]

Attachment 259199 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/scripts/test/ObjC/DOMTestOverrideBuiltins.mm:30:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestOverrideBuiltins.cpp:29:  wtf includes should be <wtf/file.h> instead of "wtf/file.h".  [build/include] [4]
ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestOverrideBuiltins.cpp:235:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 259226 [details]
Patch

[WebKit Commit Bot]

Attachment 259226 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/scripts/test/ObjC/DOMTestOverrideBuiltins.mm:30:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestOverrideBuiltins.cpp:29:  wtf includes should be <wtf/file.h> instead of "wtf/file.h".  [build/include] [4]
ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestOverrideBuiltins.cpp:235:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 259231 [details]
Patch

[WebKit Commit Bot]

Attachment 259231 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/scripts/test/ObjC/DOMTestOverrideBuiltins.mm:30:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestOverrideBuiltins.cpp:29:  wtf includes should be <wtf/file.h> instead of "wtf/file.h".  [build/include] [4]
ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestOverrideBuiltins.cpp:235:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gavin Barraclough]

Comment on attachment 259231 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Make our bindings's GetOwnPropertySlot() behave according to

bindings's –> bindings'

> Source/WebCore/ChangeLog:16
> +        According to the specification [1][2], we should do:

I think you have these two clauses (the numbered points) the wrong way around. If [OverrideBuiltins] you should check the named getter first (it overrides). If *NOT* [OverrideBuiltins] you check the static/own properties.

> Source/WebCore/ChangeLog:25
> +           2. Static / own properties

2. –> 3.

> Source/WebCore/ChangeLog:26
> +           3. Prototype check

I don't think there really is a "Prototype check" in this case. If !override there is the hasProperty check on the prototype chain to check whether we should suppress the named property. In the override case (where named getters are checked before static/own properties) there isn't really a prototype check because we're going to override regardless.

It is true that the new step will commonly be to seek the prototype chain for the property, but that's as a part of the behavior of getPropertySlot, and not of getOwnPropertySlot!

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:399
> +        push(@getOwnPropertySlotImpl, "    if (proto.isObject() && jsCast<${namespaceMaybe}JSObject*>(proto)->hasProperty(exec, propertyName))\n");

Per conversation, I don't think the prototype chain check here is strictly to spec – specifically the named property visibility algorithm calls out an exception for the named properties object. I think this is actually slightly different than we'd discussed – I don't think this refers to any object with named properties, I think it actually refers to something specific.

Per the spec (and something we don't yet implement), the Global (Window) object's named properties are actually meant to be on a separate object in the prototype chain – I think this is the NPO. So I think the spec is actually saying, if the prototype chain has been mutated such that in contains the window object, the hasProperty check should skip the Window object's named properties.

Not proposing any code change, but I think it would be good to add a FIXME comment noting that the hasProperty check is incorrect wrt the named properties object.

[Chris Dumez]

Created attachment 259271 [details]
Patch

[Chris Dumez]

Created attachment 259272 [details]
Patch

[Chris Dumez]

Comment on attachment 259231 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        Make our bindings's GetOwnPropertySlot() behave according to
> 
> bindings's –> bindings'

Oops, I knew this one.

>> Source/WebCore/ChangeLog:16
>> +        According to the specification [1][2], we should do:
> 
> I think you have these two clauses (the numbered points) the wrong way around. If [OverrideBuiltins] you should check the named getter first (it overrides). If *NOT* [OverrideBuiltins] you check the static/own properties.

Yes, I indeed reversed the two cases in the changelog. Fixed.

>> Source/WebCore/ChangeLog:25
>> +           2. Static / own properties
> 
> 2. –> 3.

Fixed.

>> Source/WebCore/ChangeLog:26
>> +           3. Prototype check
> 
> I don't think there really is a "Prototype check" in this case. If !override there is the hasProperty check on the prototype chain to check whether we should suppress the named property. In the override case (where named getters are checked before static/own properties) there isn't really a prototype check because we're going to override regardless.
> 
> It is true that the new step will commonly be to seek the prototype chain for the property, but that's as a part of the behavior of getPropertySlot, and not of getOwnPropertySlot!

Right, I dropped this 4th step.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:399
>> +        push(@getOwnPropertySlotImpl, "    if (proto.isObject() && jsCast<${namespaceMaybe}JSObject*>(proto)->hasProperty(exec, propertyName))\n");
> 
> Per conversation, I don't think the prototype chain check here is strictly to spec – specifically the named property visibility algorithm calls out an exception for the named properties object. I think this is actually slightly different than we'd discussed – I don't think this refers to any object with named properties, I think it actually refers to something specific.
> 
> Per the spec (and something we don't yet implement), the Global (Window) object's named properties are actually meant to be on a separate object in the prototype chain – I think this is the NPO. So I think the spec is actually saying, if the prototype chain has been mutated such that in contains the window object, the hasProperty check should skip the Window object's named properties.
> 
> Not proposing any code change, but I think it would be good to add a FIXME comment noting that the hasProperty check is incorrect wrt the named properties object.

You are right, I should have checked the definition of a "named properties object". I added a FIXME comment.

[WebKit Commit Bot]

Attachment 259272 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/scripts/test/ObjC/DOMTestOverrideBuiltins.mm:30:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestOverrideBuiltins.cpp:29:  wtf includes should be <wtf/file.h> instead of "wtf/file.h".  [build/include] [4]
ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestOverrideBuiltins.cpp:235:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 259272 [details]
Patch

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

r=me

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:437
> +        push(@getOwnPropertySlotImpl, "    if (thisObject->classInfo() == info() && canGetItemsForName(exec, &thisObject->impl(), propertyName)) {\n");
>          push(@getOwnPropertySlotImpl, "        slot.setCustom(thisObject, ReadOnly | DontDelete | DontEnum, thisObject->nameGetter);\n");

It's a shame that we'll do two hash lookups for the named item -- once in canGetItemsForName, and then again in nameGetter. An alternative would be just to invoke a single function that returned a a value, and then use slot.setValue.

[Chris Dumez]

(In reply to comment #17)
> Comment on attachment 259272 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=259272&action=review
> 
> r=me
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:437
> > +        push(@getOwnPropertySlotImpl, "    if (thisObject->classInfo() == info() && canGetItemsForName(exec, &thisObject->impl(), propertyName)) {\n");
> >          push(@getOwnPropertySlotImpl, "        slot.setCustom(thisObject, ReadOnly | DontDelete | DontEnum, thisObject->nameGetter);\n");
> 
> It's a shame that we'll do two hash lookups for the named item -- once in
> canGetItemsForName, and then again in nameGetter. An alternative would be
> just to invoke a single function that returned a a value, and then use
> slot.setValue.

Agreed, I can do this in a follow-up.

[Chris Dumez]

Created attachment 259283 [details]
Patch

[WebKit Commit Bot]

Attachment 259283 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/scripts/test/ObjC/DOMTestOverrideBuiltins.mm:30:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestOverrideBuiltins.cpp:29:  wtf includes should be <wtf/file.h> instead of "wtf/file.h".  [build/include] [4]
ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestOverrideBuiltins.cpp:235:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Comment on attachment 259283 [details]
Patch

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

Requesting review again due to JSC::HasImpureGetOwnPropertySlot change.

> Source/WebCore/ChangeLog:37
> +        - PerformanceTests/Bindings/childNodes-traversal.html: ~Same

New patch iteration that drops the JSC::HasImpureGetOwnPropertySlot flag for interfaces that have a named getter but NOT the [OverrideBuiltins] extended attribute.
Without this, this test would be >50% regressed.

> Source/WebCore/ChangeLog:38
> +        - PerformanceTests/Bindings/children-traversal.html: +104% :)

Nice progression thanks to the JSC::HasImpureGetOwnPropertySlot flag getting dropped for HTMLCollection.

> Source/WebCore/ChangeLog:70
> +        - Also drop the JSC::HasImpureGetOwnPropertySlot flag for interfaces

Explanation of the new change.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:915
> +    my $hasImpureNamedGetter = $interface->extendedAttributes->{"OverrideBuiltins"}

New change

[Geoffrey Garen]

Comment on attachment 259283 [details]
Patch

r=me

[Chris Dumez]

*** Bug 148116 has been marked as a duplicate of this bug. ***

[Chris Dumez]

Comment on attachment 259283 [details]
Patch

Clearing flags on attachment: 259283

Committed r188590: <http://trac.webkit.org/changeset/188590>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

(In reply to comment #18)
> (In reply to comment #17)
> > Comment on attachment 259272 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=259272&action=review
> > 
> > r=me
> > 
> > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:437
> > > +        push(@getOwnPropertySlotImpl, "    if (thisObject->classInfo() == info() && canGetItemsForName(exec, &thisObject->impl(), propertyName)) {\n");
> > >          push(@getOwnPropertySlotImpl, "        slot.setCustom(thisObject, ReadOnly | DontDelete | DontEnum, thisObject->nameGetter);\n");
> > 
> > It's a shame that we'll do two hash lookups for the named item -- once in
> > canGetItemsForName, and then again in nameGetter. An alternative would be
> > just to invoke a single function that returned a a value, and then use
> > slot.setValue.
> 
> Agreed, I can do this in a follow-up.

Follow-up: https://bugs.webkit.org/show_bug.cgi?id=148193