Bug 155563 - [JSC] correctly handle indexed properties in Object.getOwnPropertyDescriptors
Summary: [JSC] correctly handle indexed properties in Object.getOwnPropertyDescriptors
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-03-16 15:01 PDT by Caitlin Potter (:caitp)
Modified: 2016-03-22 19:12 PDT (History)
8 users (show)

See Also:


Attachments
Patch (5.57 KB, patch)
2016-03-16 15:02 PDT, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff
Patch (6.85 KB, patch)
2016-03-17 06:01 PDT, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff
Patch (6.52 KB, patch)
2016-03-17 10:11 PDT, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Caitlin Potter (:caitp) 2016-03-16 15:01:07 PDT
[JSC] correctly handle indexed properties in Object.getOwnPropertyDescriptors
Comment 1 Caitlin Potter (:caitp) 2016-03-16 15:02:27 PDT
Created attachment 274224 [details]
Patch
Comment 2 Caitlin Potter (:caitp) 2016-03-16 15:02:55 PDT
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
Comment 3 Saam Barati 2016-03-16 19:54:07 PDT
Comment on attachment 274224 [details]
Patch

LGTM.
Have you run benchmarks? Do you know if this is used in any benchmarks?
Comment 4 Caitlin Potter (:caitp) 2016-03-16 20:04:29 PDT
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.
Comment 5 Caitlin Potter (:caitp) 2016-03-17 05:15:09 PDT
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
Comment 6 Caitlin Potter (:caitp) 2016-03-17 06:01:00 PDT
Created attachment 274287 [details]
Patch
Comment 7 Saam Barati 2016-03-17 08:52:37 PDT
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.
Comment 8 Caitlin Potter (:caitp) 2016-03-17 09:10:54 PDT
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
Comment 9 Caitlin Potter (:caitp) 2016-03-17 10:11:11 PDT
Created attachment 274299 [details]
Patch
Comment 10 Caitlin Potter (:caitp) 2016-03-17 10:20:42 PDT
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()`.
Comment 11 WebKit Commit Bot 2016-03-22 19:12:20 PDT
Comment on attachment 274299 [details]
Patch

Clearing flags on attachment: 274299

Committed r198572: <http://trac.webkit.org/changeset/198572>
Comment 12 WebKit Commit Bot 2016-03-22 19:12:25 PDT
All reviewed patches have been landed.  Closing bug.