WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(33.72 KB, patch)
2016-02-02 22:34 PST
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
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
Details
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
Details
Patch
(83.42 KB, patch)
2016-02-03 16:01 PST
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
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
Details
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
Details
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
Details
Patch
(93.92 KB, patch)
2016-02-03 20:17 PST
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Show Obsolete
(8)
View All
Add attachment
proposed patch, testcase, etc.
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
Created
attachment 270553
[details]
Patch
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
Created
attachment 270605
[details]
Patch
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
Created
attachment 270623
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug