[JSC] correctly handle indexed properties in Object.getOwnPropertyDescriptors
Created attachment 274224 [details] Patch
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 on attachment 274224 [details] Patch LGTM. Have you run benchmarks? Do you know if this is used in any benchmarks?
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.
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
Created attachment 274287 [details] Patch
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 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
Created attachment 274299 [details] Patch
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 on attachment 274299 [details] Patch Clearing flags on attachment: 274299 Committed r198572: <http://trac.webkit.org/changeset/198572>
All reviewed patches have been landed. Closing bug.