Object.getOwnPropertyDescriptor() returns incomplete descriptor for unforgeable properties: http://heycam.github.io/webidl/#Unforgeable Such properties are on the instance (and non-configurable). Object.getOwnPropertyDescriptor(document, 'location') returns a descriptor whose get / set are undefined. These should be functions (as they are in Firefox and Chrome).
Created attachment 270544 [details] WIP Patch (does not work for Window)
Comment on attachment 270544 [details] WIP Patch (does not work for Window) View in context: https://bugs.webkit.org/attachment.cgi?id=270544&action=review > Source/JavaScriptCore/runtime/JSObject.cpp:2581 > + reifyAllStaticProperties(exec); Sadly, this does not work for Object.getOwnPropertyDescriptor(window, 'location'). For such calls, |this| is a JSDOMWindowShell / JSProxy, not a JSDOMWindow. Therefore, reifying does not do anything as the properties are on JSDOMWindow, not JSDOMWindowShell. I am trying to figure out why this happens and how to deal with it. Please let me know if you have information that can help.
Created attachment 270553 [details] Patch
Comment on attachment 270553 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270553&action=review > Source/JavaScriptCore/runtime/JSObject.cpp:2581 > + if (JSProxy* proxy = jsDynamicCast<JSProxy*>(this)) This seems to do the trick for the Window object. > LayoutTests/http/tests/security/cross-origin-window-property-access.html:-18 > - shouldThrow('Object.getOwnPropertyDescriptor(window, "location").get.call(crossOriginWindow)'); It is actually fine to access the location of a cross-origin frame as per [1]. I confirmed that Firefox allows accessing corssOriginWindow.location. [1] https://html.spec.whatwg.org/multipage/browsers.html#security-window
Comment on attachment 270553 [details] Patch Attachment 270553 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/775797 New failing tests: imported/w3c/web-platform-tests/html/dom/interfaces.html
Created attachment 270556 [details] Archive of layout-test-results from ews103 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 270553 [details] Patch Attachment 270553 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/775809 New failing tests: imported/w3c/web-platform-tests/html/dom/interfaces.html
Created attachment 270558 [details] Archive of layout-test-results from ews105 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Created attachment 270605 [details] Patch
Comment on attachment 270605 [details] Patch Attachment 270605 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/779151 New failing tests: js/dom/dom-as-prototype-assignment-exception.html
Created attachment 270616 [details] Archive of layout-test-results from ews116 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 270605 [details] Patch Attachment 270605 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/779209 New failing tests: js/dom/dom-as-prototype-assignment-exception.html
Created attachment 270617 [details] Archive of layout-test-results from ews105 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 270605 [details] Patch Attachment 270605 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/779236 New failing tests: js/dom/dom-as-prototype-assignment-exception.html
Created attachment 270618 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 270623 [details] Patch
Comment on attachment 270623 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270623&action=review > Source/JavaScriptCore/runtime/JSObject.cpp:2593 > + descriptor.setSetter(getBoundSlotBaseFunctionForGetterSetter(exec, propertyName, slot, getterSetter, JSBoundSlotBaseFunction::Type::Setter)); I don't think this is going to work work with CustomGetters that do not have a CustomGetterSetter object in the structure. Can you add the following test: Object.getOwnPropertyDescriptor(function foo() { }, "name").getter !== undefined;
(In reply to comment #17) > Comment on attachment 270623 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=270623&action=review > > > Source/JavaScriptCore/runtime/JSObject.cpp:2593 > > + descriptor.setSetter(getBoundSlotBaseFunctionForGetterSetter(exec, propertyName, slot, getterSetter, JSBoundSlotBaseFunction::Type::Setter)); > > I don't think this is going to work work with CustomGetters that do not have > a CustomGetterSetter object in the structure. Can you add the following test: > > Object.getOwnPropertyDescriptor(function foo() { }, "name").getter !== > undefined; Object.getOwnPropertyDescriptor(function foo() { }, "name").getter returns undefined in Safari, Firefox and Chrome.
(In reply to comment #18) > (In reply to comment #17) > > Comment on attachment 270623 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=270623&action=review > > > > > Source/JavaScriptCore/runtime/JSObject.cpp:2593 > > > + descriptor.setSetter(getBoundSlotBaseFunctionForGetterSetter(exec, propertyName, slot, getterSetter, JSBoundSlotBaseFunction::Type::Setter)); > > > > I don't think this is going to work work with CustomGetters that do not have > > a CustomGetterSetter object in the structure. Can you add the following test: > > > > Object.getOwnPropertyDescriptor(function foo() { }, "name").getter !== > > undefined; > > Object.getOwnPropertyDescriptor(function foo() { }, "name").getter returns > undefined in Safari, Firefox and Chrome. It is a value, not an accessor. The descriptor looks like this in all browsers: Object { value: "foo", writable: false, enumerable: false, configurable: true } (Except that Safari has it non-configurable for some reason, unlike other browsers)
Comment on attachment 270623 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270623&action=review > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2292 > + # FIXME: This does not seem right, we should likely use thisValue instead of slotBase here to match the specification: I am planning to drop this whole elsif part in a follow-up patch as it makes us behave like the Web IDL specification and Firefox. > LayoutTests/js/instance-property-getter-other-instance-expected.txt:10 > +FAIL locationGetter.call(window.document) === window.document.location should be true. Was false. Using |thisValue| instead of |slotBase| in the generated instance property getter bindings fixes this (and does not break any other layout test). I am planning to make this change in a follow-up patch unless others prefer I do this in this patch.
Comment on attachment 270623 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270623&action=review > LayoutTests/imported/w3c/web-platform-tests/html/dom/interfaces-expected.txt:3803 > +FAIL Window interface: attribute self The DOMWindow.self getter can only be used on instances of DOMWindow These failures are because the test tries to call the getter without an explicit |this| and expects it to work as this is a global Object. However, we currently throw an exception in this case. I am planning to address this in a follow-up by checking in the bindings that |thisValue| is undefined and then using the global object instead of throwing.
Comment on attachment 270623 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270623&action=review >> LayoutTests/imported/w3c/web-platform-tests/html/dom/interfaces-expected.txt:3803 >> +FAIL Window interface: attribute self The DOMWindow.self getter can only be used on instances of DOMWindow > > These failures are because the test tries to call the getter without an explicit |this| and expects it to work as this is a global Object. However, we currently throw an exception in this case. I am planning to address this in a follow-up by checking in the bindings that |thisValue| is undefined and then using the global object instead of throwing. To fix this, doing the following in the Global Object (JSDOMWindow) bindings seems to work: auto* castedThis = JSValue::decode(thisValue).isUndefined() ? toJSDOMWindow(state->callee()->globalObject()) : toJSDOMWindow(JSValue::decode(thisValue));
Comment on attachment 270623 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270623&action=review r=me >> LayoutTests/imported/w3c/web-platform-tests/html/dom/interfaces-expected.txt:3803 >> +FAIL Window interface: attribute self The DOMWindow.self getter can only be used on instances of DOMWindow > > These failures are because the test tries to call the getter without an explicit |this| and expects it to work as this is a global Object. However, we currently throw an exception in this case. I am planning to address this in a follow-up by checking in the bindings that |thisValue| is undefined and then using the global object instead of throwing. If you're talking about using toThis(), that's OK. But if you're thinking about doing some kind of custom substitution, you have to be careful because it might be a security hole.
> To fix this, doing the following in the Global Object (JSDOMWindow) bindings > seems to work: > auto* castedThis = JSValue::decode(thisValue).isUndefined() ? > toJSDOMWindow(state->callee()->globalObject()) : > toJSDOMWindow(JSValue::decode(thisValue)); Usually, we perform this substitution any time 'this' is any primitive value -- not just undefined. Also, what happens if thisValue is not a JSDOMWindow? Also, is there a spec that mandates use of the callee global object in this case?
Comment on attachment 270623 [details] Patch Clearing flags on attachment: 270623 Committed r196145: <http://trac.webkit.org/changeset/196145>
All reviewed patches have been landed. Closing bug.
(In reply to comment #24) > > To fix this, doing the following in the Global Object (JSDOMWindow) bindings > > seems to work: > > auto* castedThis = JSValue::decode(thisValue).isUndefined() ? > > toJSDOMWindow(state->callee()->globalObject()) : > > toJSDOMWindow(JSValue::decode(thisValue)); > > Usually, we perform this substitution any time 'this' is any primitive value > -- not just undefined. > > Also, what happens if thisValue is not a JSDOMWindow? > > Also, is there a spec that mandates use of the callee global object in this > case? I couldn't find where this is defined in the spec so I checked the behavior of Firefox.
(In reply to comment #27) > (In reply to comment #24) > > > To fix this, doing the following in the Global Object (JSDOMWindow) bindings > > > seems to work: > > > auto* castedThis = JSValue::decode(thisValue).isUndefined() ? > > > toJSDOMWindow(state->callee()->globalObject()) : > > > toJSDOMWindow(JSValue::decode(thisValue)); > > > > Usually, we perform this substitution any time 'this' is any primitive value > > -- not just undefined. > > > > Also, what happens if thisValue is not a JSDOMWindow? > > > > Also, is there a spec that mandates use of the callee global object in this > > case? > > I couldn't find where this is defined in the spec so I checked the behavior > of Firefox. Actually, this is the spec: http://heycam.github.io/webidl/#ImplicitThis "If the [ImplicitThis] extended attribute appears on an interface, it indicates that when a Function corresponding to one of the interface’s operations is invoked with the null or undefined value as the this value, that the ECMAScript global object will be used as the this value instead."
(In reply to comment #28) > (In reply to comment #27) > > (In reply to comment #24) > > > > To fix this, doing the following in the Global Object (JSDOMWindow) bindings > > > > seems to work: > > > > auto* castedThis = JSValue::decode(thisValue).isUndefined() ? > > > > toJSDOMWindow(state->callee()->globalObject()) : > > > > toJSDOMWindow(JSValue::decode(thisValue)); > > > > > > Usually, we perform this substitution any time 'this' is any primitive value > > > -- not just undefined. > > > > > > Also, what happens if thisValue is not a JSDOMWindow? > > > > > > Also, is there a spec that mandates use of the callee global object in this > > > case? > > > > I couldn't find where this is defined in the spec so I checked the behavior > > of Firefox. > > Actually, this is the spec: > http://heycam.github.io/webidl/#ImplicitThis > > "If the [ImplicitThis] extended attribute appears on an interface, it > indicates that when a Function corresponding to one of the interface’s > operations is invoked with the null or undefined value as the this value, > that the ECMAScript global object will be used as the this value instead." But this seems to be only for operations, not attribute getters: http://heycam.github.io/webidl/#es-operations (step 1.2) No such mention for attribute getters: http://heycam.github.io/webidl/#dfn-attribute-getter Maybe the W3C tests are Firefox don't match the Web IDL specification in this case.
(In reply to comment #29) > (In reply to comment #28) > > (In reply to comment #27) > > > (In reply to comment #24) > > > > > To fix this, doing the following in the Global Object (JSDOMWindow) bindings > > > > > seems to work: > > > > > auto* castedThis = JSValue::decode(thisValue).isUndefined() ? > > > > > toJSDOMWindow(state->callee()->globalObject()) : > > > > > toJSDOMWindow(JSValue::decode(thisValue)); > > > > > > > > Usually, we perform this substitution any time 'this' is any primitive value > > > > -- not just undefined. > > > > > > > > Also, what happens if thisValue is not a JSDOMWindow? > > > > > > > > Also, is there a spec that mandates use of the callee global object in this > > > > case? > > > > > > I couldn't find where this is defined in the spec so I checked the behavior > > > of Firefox. > > > > Actually, this is the spec: > > http://heycam.github.io/webidl/#ImplicitThis > > > > "If the [ImplicitThis] extended attribute appears on an interface, it > > indicates that when a Function corresponding to one of the interface’s > > operations is invoked with the null or undefined value as the this value, > > that the ECMAScript global object will be used as the this value instead." > > But this seems to be only for operations, not attribute getters: > http://heycam.github.io/webidl/#es-operations (step 1.2) > > No such mention for attribute getters: > http://heycam.github.io/webidl/#dfn-attribute-getter > > Maybe the W3C tests are Firefox don't match the Web IDL specification in > this case. I filed a bug again the IDL harness and made a pull request: https://github.com/w3c/testharness.js/issues/182 We'll see what happens.