Bug 153817 – Object.getOwnPropertyDescriptor() returns incomplete descriptor for instance properties

• Component: Bindings
• Status: RESOLVED FIXED

[Chris Dumez]

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

[Chris Dumez]

Created attachment 270544 [details]
WIP Patch (does not work for Window)

[Chris Dumez]

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]

Created attachment 270553 [details]
Patch

[Chris Dumez]

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

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

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]

Created attachment 270605 [details]
Patch

[Build Bot]

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]

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

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

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]

Created attachment 270623 [details]
Patch

[Keith Miller]

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]

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

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

> 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 on attachment 270623 [details]
Patch

Clearing flags on attachment: 270623

Committed r196145: <http://trac.webkit.org/changeset/196145>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

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

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

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

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