RESOLVED FIXED 153817
Object.getOwnPropertyDescriptor() returns incomplete descriptor for instance properties
https://bugs.webkit.org/show_bug.cgi?id=153817
Summary Object.getOwnPropertyDescriptor() returns incomplete descriptor for instance ...
Chris Dumez
Reported 2016-02-02 20:37:28 PST
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).
Attachments
WIP Patch (does not work for Window) (30.92 KB, patch)
2016-02-02 20:44 PST, Chris Dumez
no flags
Patch (33.72 KB, patch)
2016-02-02 22:34 PST, Chris Dumez
no flags
Archive of layout-test-results from ews103 for mac-yosemite (912.03 KB, application/zip)
2016-02-02 23:33 PST, Build Bot
no flags
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (960.58 KB, application/zip)
2016-02-02 23:38 PST, Build Bot
no flags
Patch (83.42 KB, patch)
2016-02-03 16:01 PST, Chris Dumez
no flags
Archive of layout-test-results from ews116 for mac-yosemite (806.71 KB, application/zip)
2016-02-03 17:29 PST, Build Bot
no flags
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (981.49 KB, application/zip)
2016-02-03 17:43 PST, Build Bot
no flags
Archive of layout-test-results from ews101 for mac-yosemite (995.33 KB, application/zip)
2016-02-03 17:46 PST, Build Bot
no flags
Patch (93.92 KB, patch)
2016-02-03 20:17 PST, Chris Dumez
no flags
Chris Dumez
Comment 1 2016-02-02 20:44:04 PST
Created attachment 270544 [details] WIP Patch (does not work for Window)
Chris Dumez
Comment 2 2016-02-02 20:48:46 PST
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.
Chris Dumez
Comment 3 2016-02-02 22:34:04 PST
Chris Dumez
Comment 4 2016-02-02 22:37:38 PST
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
Build Bot
Comment 5 2016-02-02 23:33:45 PST
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
Build Bot
Comment 6 2016-02-02 23:33:49 PST
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
Build Bot
Comment 7 2016-02-02 23:38:31 PST
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
Build Bot
Comment 8 2016-02-02 23:38:35 PST
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
Chris Dumez
Comment 9 2016-02-03 16:01:16 PST
Build Bot
Comment 10 2016-02-03 17:29:42 PST
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
Build Bot
Comment 11 2016-02-03 17:29:46 PST
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
Build Bot
Comment 12 2016-02-03 17:43:19 PST
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
Build Bot
Comment 13 2016-02-03 17:43:23 PST
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
Build Bot
Comment 14 2016-02-03 17:45:56 PST
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
Build Bot
Comment 15 2016-02-03 17:46:00 PST
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
Chris Dumez
Comment 16 2016-02-03 20:17:43 PST
Keith Miller
Comment 17 2016-02-03 21:19:31 PST
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;
Chris Dumez
Comment 18 2016-02-03 21:54:17 PST
(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.
Chris Dumez
Comment 19 2016-02-03 21:56:28 PST
(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)
Chris Dumez
Comment 20 2016-02-03 22:01:35 PST
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.
Chris Dumez
Comment 21 2016-02-03 22:08:13 PST
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.
Chris Dumez
Comment 22 2016-02-04 12:54:03 PST
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));
Geoffrey Garen
Comment 23 2016-02-04 12:56:53 PST
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.
Geoffrey Garen
Comment 24 2016-02-04 13:08:45 PST
> 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?
WebKit Commit Bot
Comment 25 2016-02-04 13:36:09 PST
Comment on attachment 270623 [details] Patch Clearing flags on attachment: 270623 Committed r196145: <http://trac.webkit.org/changeset/196145>
WebKit Commit Bot
Comment 26 2016-02-04 13:36:16 PST
All reviewed patches have been landed. Closing bug.
Chris Dumez
Comment 27 2016-02-04 14:09:51 PST
(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.
Chris Dumez
Comment 28 2016-02-04 14:17:19 PST
(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."
Chris Dumez
Comment 29 2016-02-04 14:21:57 PST
(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.
Chris Dumez
Comment 30 2016-02-04 15:11:34 PST
(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.
Note You need to log in before you can comment on or make changes to this bug.