Bug 148092

Created attachment 259180 [details] WIP patch

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

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 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

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

Created attachment 259199 [details] Patch

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.

Created attachment 259226 [details] Patch

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.

Created attachment 259231 [details] Patch

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 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.

Created attachment 259271 [details] Patch

Created attachment 259272 [details] Patch

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.

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 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.

(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.

Created attachment 259283 [details] Patch

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 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 on attachment 259283 [details] Patch r=me

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

Comment on attachment 259283 [details] Patch Clearing flags on attachment: 259283 Committed r188590: <http://trac.webkit.org/changeset/188590>

All reviewed patches have been landed. Closing bug.

(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