Bug 153817

Summary: Object.getOwnPropertyDescriptor() returns incomplete descriptor for instance properties
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: BindingsAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, buildbot, cgarcia, commit-queue, ggaren, keith_miller, mark.lam, msaboff, oliver, rniwa, sam, sbarati
Priority: P2 Keywords: WebExposed
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 140575    
Bug Blocks: 153895    
Attachments:
Description Flags
WIP Patch (does not work for Window)
none
Patch
none
Archive of layout-test-results from ews103 for mac-yosemite
none
Archive of layout-test-results from ews105 for mac-yosemite-wk2
none
Patch
none
Archive of layout-test-results from ews116 for mac-yosemite
none
Archive of layout-test-results from ews105 for mac-yosemite-wk2
none
Archive of layout-test-results from ews101 for mac-yosemite
none
Patch none

Description Chris Dumez 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).
Comment 1 Chris Dumez 2016-02-02 20:44:04 PST
Created attachment 270544 [details]
WIP Patch (does not work for Window)
Comment 2 Chris Dumez 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.
Comment 3 Chris Dumez 2016-02-02 22:34:04 PST
Created attachment 270553 [details]
Patch
Comment 4 Chris Dumez 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
Comment 5 Build Bot 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
Comment 6 Build Bot 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
Comment 7 Build Bot 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
Comment 8 Build Bot 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
Comment 9 Chris Dumez 2016-02-03 16:01:16 PST
Created attachment 270605 [details]
Patch
Comment 10 Build Bot 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
Comment 11 Build Bot 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
Comment 12 Build Bot 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
Comment 13 Build Bot 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
Comment 14 Build Bot 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
Comment 15 Build Bot 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
Comment 16 Chris Dumez 2016-02-03 20:17:43 PST
Created attachment 270623 [details]
Patch
Comment 17 Keith Miller 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;
Comment 18 Chris Dumez 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.
Comment 19 Chris Dumez 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)
Comment 20 Chris Dumez 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.
Comment 21 Chris Dumez 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.
Comment 22 Chris Dumez 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));
Comment 23 Geoffrey Garen 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.
Comment 24 Geoffrey Garen 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?
Comment 25 WebKit Commit Bot 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>
Comment 26 WebKit Commit Bot 2016-02-04 13:36:16 PST
All reviewed patches have been landed.  Closing bug.
Comment 27 Chris Dumez 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.
Comment 28 Chris Dumez 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."
Comment 29 Chris Dumez 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.
Comment 30 Chris Dumez 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.