Bug 155563 – [JSC] correctly handle indexed properties in Object.getOwnPropertyDescriptors

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Caitlin Potter (:caitp)]

[JSC] correctly handle indexed properties in Object.getOwnPropertyDescriptors

[Caitlin Potter (:caitp)]

Created attachment 274224 [details]
Patch

[Caitlin Potter (:caitp)]

The test case actually fails without https://bugs.webkit.org/show_bug.cgi?id=155560, but I've split these into two bugs since they are distinct

[Saam Barati]

Comment on attachment 274224 [details]
Patch

LGTM.
Have you run benchmarks? Do you know if this is used in any benchmarks?

[Caitlin Potter (:caitp)]

This method is fairly recent (stage 3 proposal, https://github.com/tc39/proposal-object-getownpropertydescriptors) --- I'm fairly sure it's not used in any of the big benchmarks yet.

[Caitlin Potter (:caitp)]

An example repro is `Object.getOwnPropertyDescriptors({ 0: 1 })[0]` (yields `undefined`) vs `Object.getOwnPropertyDescriptors({ 0: 1 })["0"]` (yields the right thing).

In debug builds, `Object.getOwnPropertyDescriptors({ 0: 1 })` fails a `ASSERT(!parseIndex())` in JSObject's directPutInline --- in release builds, it converts the index to a name, and subsequent accesses to the property via numbered keys fails. This is probably observable in other ways elsewhere in the library, which is unfortunate.

I'll write a test case that doesn't depend on the Proxy fix so this can be CQ'd

[Caitlin Potter (:caitp)]

Created attachment 274287 [details]
Patch

[Saam Barati]

Comment on attachment 274287 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=274287&action=review

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:253
> +        JSObject::put(descriptors, exec, propertyName, fromDescriptor, slot);

You need an exception check here. 
An exception check is also needed above
after constructEmptyObject(). I suggest
caching the VM in a local variable and checking
VM.exception()

> Source/JavaScriptCore/tests/es6/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js:17
>      }

Can we also add a test where the lack of an exception
check above is observable? I think it's probably doable
by having a setter/getter on Object.prototype that both
throw.

[Caitlin Potter (:caitp)]

Comment on attachment 274287 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=274287&action=review

>> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:253
>> +        JSObject::put(descriptors, exec, propertyName, fromDescriptor, slot);
> 
> You need an exception check here. 
> An exception check is also needed above
> after constructEmptyObject(). I suggest
> caching the VM in a local variable and checking
> VM.exception()

per spec, this should not cause an observable error (spec algorithm uses CreateDataProperty), which ignores accessors on the prototype. Is there a model for this behaviour in JSC that permits indexed properties?

>> Source/JavaScriptCore/tests/es6/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js:17
>>      }
> 
> Can we also add a test where the lack of an exception
> check above is observable? I think it's probably doable
> by having a setter/getter on Object.prototype that both
> throw.

as noted above, that shiuld be ignored, but ill add a test to verify that. Good catch

[Caitlin Potter (:caitp)]

Created attachment 274299 [details]
Patch

[Caitlin Potter (:caitp)]

Comment on attachment 274287 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=274287&action=review

>>> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:253
>>> +        JSObject::put(descriptors, exec, propertyName, fromDescriptor, slot);
>> 
>> You need an exception check here. 
>> An exception check is also needed above
>> after constructEmptyObject(). I suggest
>> caching the VM in a local variable and checking
>> VM.exception()
> 
> per spec, this should not cause an observable error (spec algorithm uses CreateDataProperty), which ignores accessors on the prototype. Is there a model for this behaviour in JSC that permits indexed properties?

I've added a model for `CreateDataProperty()` named `putOwnDataPropertyMayBeIndex` --- an unfortunate name, but does the right thing. Test has been added to verify this.

I've also added the cached reference to `vm()`.

[WebKit Commit Bot]

Comment on attachment 274299 [details]
Patch

Clearing flags on attachment: 274299

Committed r198572: <http://trac.webkit.org/changeset/198572>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.