Bug 148092 - Make our bindings' GetOwnPropertySlot() behave according to specification
Summary: Make our bindings' GetOwnPropertySlot() behave according to specification
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL: https://heycam.github.io/webidl/#geto...
Keywords:
: 148116 (view as bug list)
Depends on:
Blocks: 110611 147980
  Show dependency treegraph
 
Reported: 2015-08-17 14:02 PDT by Chris Dumez
Modified: 2016-01-29 15:53 PST (History)
7 users (show)

See Also:


Attachments
WIP patch (10.69 KB, patch)
2015-08-17 14:03 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-mavericks (787.81 KB, application/zip)
2015-08-17 14:39 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (775.23 KB, application/zip)
2015-08-17 14:45 PDT, Build Bot
no flags Details
Patch (63.91 KB, patch)
2015-08-17 16:26 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (65.59 KB, patch)
2015-08-17 20:04 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (65.73 KB, patch)
2015-08-17 20:36 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (65.95 KB, patch)
2015-08-18 09:28 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (65.95 KB, patch)
2015-08-18 09:29 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (70.79 KB, patch)
2015-08-18 11:35 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2015-08-17 14:02:24 PDT
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.
====
Comment 1 Chris Dumez 2015-08-17 14:03:11 PDT
Created attachment 259180 [details]
WIP patch
Comment 2 Build Bot 2015-08-17 14:39:56 PDT
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
Comment 3 Build Bot 2015-08-17 14:39:59 PDT
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
Comment 4 Build Bot 2015-08-17 14:45:00 PDT
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
Comment 5 Build Bot 2015-08-17 14:45:04 PDT
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
Comment 6 Chris Dumez 2015-08-17 16:26:46 PDT
Created attachment 259199 [details]
Patch
Comment 7 WebKit Commit Bot 2015-08-17 16:29:09 PDT
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.
Comment 8 Chris Dumez 2015-08-17 20:04:28 PDT
Created attachment 259226 [details]
Patch
Comment 9 WebKit Commit Bot 2015-08-17 20:06:33 PDT
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.
Comment 10 Chris Dumez 2015-08-17 20:36:40 PDT
Created attachment 259231 [details]
Patch
Comment 11 WebKit Commit Bot 2015-08-17 20:38:03 PDT
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.
Comment 12 Gavin Barraclough 2015-08-17 23:30:34 PDT
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.
Comment 13 Chris Dumez 2015-08-18 09:28:19 PDT
Created attachment 259271 [details]
Patch
Comment 14 Chris Dumez 2015-08-18 09:29:20 PDT
Created attachment 259272 [details]
Patch
Comment 15 Chris Dumez 2015-08-18 09:31:29 PDT
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.
Comment 16 WebKit Commit Bot 2015-08-18 09:32:29 PDT
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.
Comment 17 Geoffrey Garen 2015-08-18 10:07:59 PDT
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.
Comment 18 Chris Dumez 2015-08-18 10:09:18 PDT
(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.
Comment 19 Chris Dumez 2015-08-18 11:35:28 PDT
Created attachment 259283 [details]
Patch
Comment 20 WebKit Commit Bot 2015-08-18 11:38:26 PDT
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.
Comment 21 Chris Dumez 2015-08-18 11:39:44 PDT
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
Comment 22 Geoffrey Garen 2015-08-18 11:52:44 PDT
Comment on attachment 259283 [details]
Patch

r=me
Comment 23 Chris Dumez 2015-08-18 11:55:47 PDT
*** Bug 148116 has been marked as a duplicate of this bug. ***
Comment 24 Chris Dumez 2015-08-18 12:15:49 PDT
Comment on attachment 259283 [details]
Patch

Clearing flags on attachment: 259283

Committed r188590: <http://trac.webkit.org/changeset/188590>
Comment 25 Chris Dumez 2015-08-18 12:15:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 26 Chris Dumez 2015-08-19 15:20:54 PDT
(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