Bug 140426 - put_by_val_direct need to check the property is index or not for using putDirect / putDirectIndex
Summary: put_by_val_direct need to check the property is index or not for using putDir...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on: 140694 140775 143422
Blocks:
  Show dependency treegraph
 
Reported: 2015-01-13 20:36 PST by Yusuke Suzuki
Modified: 2015-04-07 00:27 PDT (History)
6 users (show)

See Also:


Attachments
Patch (18.21 KB, patch)
2015-01-13 20:44 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (18.62 KB, patch)
2015-01-13 20:54 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (18.75 KB, patch)
2015-01-13 21:13 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (65.21 KB, patch)
2015-01-16 05:29 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-mavericks (581.49 KB, application/zip)
2015-01-16 06:40 PST, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (945.09 KB, application/zip)
2015-01-16 06:46 PST, Build Bot
no flags Details
Patch (63.95 KB, patch)
2015-01-19 19:36 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (64.20 KB, patch)
2015-01-19 19:50 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-mavericks-wk2 (970.90 KB, application/zip)
2015-01-19 20:43 PST, Build Bot
no flags Details
Patch (64.19 KB, patch)
2015-01-20 00:10 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (63.70 KB, patch)
2015-01-20 21:45 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (13.94 KB, patch)
2015-04-06 12:07 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (13.92 KB, patch)
2015-04-06 12:14 PDT, Yusuke Suzuki
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2015-01-13 20:36:51 PST
put_by_val_direct need to check the property is index or not for using putDirect / putDirectIndex
Comment 1 Yusuke Suzuki 2015-01-13 20:44:05 PST
Created attachment 244575 [details]
Patch
Comment 2 Yusuke Suzuki 2015-01-13 20:51:40 PST
Comment on attachment 244575 [details]
Patch

WIP
Comment 3 Yusuke Suzuki 2015-01-13 20:54:24 PST
Created attachment 244577 [details]
Patch
Comment 4 Yusuke Suzuki 2015-01-13 20:58:37 PST
Comment on attachment 244577 [details]
Patch

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

Added comments

> Source/JavaScriptCore/dfg/DFGOperations.cpp:101
> +        ASSERT_WITH_MESSAGE(index != PropertyName::NotAnIndex, "Since JSValue::isUInt32 returns true only when the boxed value is int32_t and positive, it doesn't return true for uint32_t max value that is PropertyName::NotAnIndex.");

isUInt32 return true when the boxed value is int32_t and positive.
So `index` is not PropertyName::NotAnIndex, that is UINT32_MAX.

So, numbers larger than 0x7fffffff (INT32_MAX) are dropped into the isDouble path.

> Source/JavaScriptCore/dfg/DFGOperations.cpp:109
> +        if (propertyAsDouble == propertyAsUInt32 && propertyAsUInt32 != PropertyName::NotAnIndex) {

When the double number is 0xffffffff that is UINT32_MAX, it is non-index, so we cannot use putDirectIndex.
We fallback it to the toString and putDirect path.

> Source/JavaScriptCore/dfg/DFGOperations.cpp:132
> +            unsigned index = propertyName.asIndex();

If the string "0" (like "index" string) comes, we need to use putDirectIndex for such a indentifier.
Therefore, we check it here.
Comment 5 Yusuke Suzuki 2015-01-13 21:08:19 PST
Oops, I'll update the test.
Comment 6 Yusuke Suzuki 2015-01-13 21:13:40 PST
Created attachment 244579 [details]
Patch
Comment 7 Geoffrey Garen 2015-01-15 11:16:08 PST
Comment on attachment 244579 [details]
Patch

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

r- because I see at least one unfixed case in the code, and I think I see one missing test case.

Please see my comment below for an alternative proposal for how to fix this bug.

> Source/JavaScriptCore/dfg/DFGOperations.cpp:110
> -        if (propertyAsDouble == propertyAsUInt32) {
> +        if (propertyAsDouble == propertyAsUInt32 && propertyAsUInt32 != PropertyName::NotAnIndex) {
>              putByVal<strict, direct>(exec, baseValue, propertyAsUInt32, value);

The function signatures for putByVal, putByIndex, and so on accept unsigned as their input, and do not seem to forbid PropertyName::NotAnIndex (UINT_MAX) in any explicit way.

However, I do see that the PropertyName class implicitly forbids UINT_MAX by using UINT_MAX as an in-band signal that a string could not convert to a number (because it was null or empty, or contained non-digit characters).

There are lots of places in the code that call putByIndex, or that do numeric conversion by hand, and I'm afraid that there are too many to audit to make sure that none of them allow a UINT_MAX to slip through. For example, searching for 10 seconds found me NetscapePluginInstanceProxy::setProperty, which seems to allow UINT_MAX to slip through.

So, I think the approach of using UINT_MAX as an in-band signal is a fragile design, and it's insufficient to patch it at these call sites in the DFG.

I propose an alternative solution to these bugs:

(1) Change PropertyName::asIndex() to return std::pair<uint32_t, bool>, with the bool indicating whether the integer is valid or not.

(2) Change callers of PropertyName::asIndex() to check the bool instead of comparing against PropertyName::NotAnIndex.

(3) Allow callers to pass UINT_MAX to putByIndex.

What do you think?

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:846
> +    // Don't put to an object if toString throws an exception.

Does your test cover the case where toString throws an exception? I looked for this case and I didn't see it. Maybe I missed it?
Comment 8 Yusuke Suzuki 2015-01-15 13:06:51 PST
Comment on attachment 244579 [details]
Patch

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

Thank you for your review!

>> Source/JavaScriptCore/dfg/DFGOperations.cpp:110
>>              putByVal<strict, direct>(exec, baseValue, propertyAsUInt32, value);
> 
> The function signatures for putByVal, putByIndex, and so on accept unsigned as their input, and do not seem to forbid PropertyName::NotAnIndex (UINT_MAX) in any explicit way.
> 
> However, I do see that the PropertyName class implicitly forbids UINT_MAX by using UINT_MAX as an in-band signal that a string could not convert to a number (because it was null or empty, or contained non-digit characters).
> 
> There are lots of places in the code that call putByIndex, or that do numeric conversion by hand, and I'm afraid that there are too many to audit to make sure that none of them allow a UINT_MAX to slip through. For example, searching for 10 seconds found me NetscapePluginInstanceProxy::setProperty, which seems to allow UINT_MAX to slip through.
> 
> So, I think the approach of using UINT_MAX as an in-band signal is a fragile design, and it's insufficient to patch it at these call sites in the DFG.
> 
> I propose an alternative solution to these bugs:
> 
> (1) Change PropertyName::asIndex() to return std::pair<uint32_t, bool>, with the bool indicating whether the integer is valid or not.
> 
> (2) Change callers of PropertyName::asIndex() to check the bool instead of comparing against PropertyName::NotAnIndex.
> 
> (3) Allow callers to pass UINT_MAX to putByIndex.
> 
> What do you think?

Your proposal sounds very nice. Specially handing UINT_MAX value as an exception is not good design. We should force it by type and compiler errors.

However I have one issue about it.
Currently, PropertyName classifies itself into 2, an index property and not-an-index property.
The reason why we do may be that ECMAScript spec describes that the array index is 0 - 2^23 -1 range. (in sec 15.4)
And some behavior of abstract operations in the spec becomes different.
For example, Array.[[DefineOwnProperty]] recognizes the property is array index or not.
And according to this information, the process of Array.[[DefineOwnProperty]] becomes different. (Expanding "length" property or not etc.)

Based on this spec, JSC provides 2 functions for [[DefineOwnProperty]]. JSObject::defineOwnNonIndexProperty and JSObject::defineOwnIndexedProperty.
And these implmenetations are quite different.
Accepting UINT_MAX in all index operations requires all subsequent changes of Indexed/NonIndex operations, and it makes the implementation complicated since the term "index" becomes different from the spec.

To solve this problem efficiently, I slightly modified your nice proposal,

1. Change PropertyName::asIndex() returns WTF::Optional<uint32_t> value. In this time, when the value is UINT_MAX, we makes it invalid optional value.
2. Change callers of PropertyName::asIndex() to check the bool instead of comparing against PropertyName::NotAnIndex. At this time, since we makes the UINT_MAX as invalid, it will be filtered here.
3. Allow callers to pass UINT_MAX to putByIndex (and other JSC_EXPORT_PRIVATE annotated APIs). It allows the user outside JSC to pass UINT_MAX as index.

In the case of (3), the current implementation of putByIndex already accepts UINT_MAX as index while putDirectIndex / putDirect don't accept this.
http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/runtime/JSObject.cpp#L415
I think this is good design since put*Direct* should not be called from the outside of the JSC.
If the API is annotated with JS_EXPORT_PRIVATE, we should accept UINT_MAX as index like the current putByIndex do.
What do you think of?

>> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:846
>> +    // Don't put to an object if toString throws an exception.
> 
> Does your test cover the case where toString throws an exception? I looked for this case and I didn't see it. Maybe I missed it?

Thank you! I'll add it to the test cases.
Comment 9 Yusuke Suzuki 2015-01-15 13:17:09 PST
Comment on attachment 244579 [details]
Patch

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

>>> Source/JavaScriptCore/dfg/DFGOperations.cpp:110
>>>              putByVal<strict, direct>(exec, baseValue, propertyAsUInt32, value);
>> 
>> The function signatures for putByVal, putByIndex, and so on accept unsigned as their input, and do not seem to forbid PropertyName::NotAnIndex (UINT_MAX) in any explicit way.
>> 
>> However, I do see that the PropertyName class implicitly forbids UINT_MAX by using UINT_MAX as an in-band signal that a string could not convert to a number (because it was null or empty, or contained non-digit characters).
>> 
>> There are lots of places in the code that call putByIndex, or that do numeric conversion by hand, and I'm afraid that there are too many to audit to make sure that none of them allow a UINT_MAX to slip through. For example, searching for 10 seconds found me NetscapePluginInstanceProxy::setProperty, which seems to allow UINT_MAX to slip through.
>> 
>> So, I think the approach of using UINT_MAX as an in-band signal is a fragile design, and it's insufficient to patch it at these call sites in the DFG.
>> 
>> I propose an alternative solution to these bugs:
>> 
>> (1) Change PropertyName::asIndex() to return std::pair<uint32_t, bool>, with the bool indicating whether the integer is valid or not.
>> 
>> (2) Change callers of PropertyName::asIndex() to check the bool instead of comparing against PropertyName::NotAnIndex.
>> 
>> (3) Allow callers to pass UINT_MAX to putByIndex.
>> 
>> What do you think?
> 
> Your proposal sounds very nice. Specially handing UINT_MAX value as an exception is not good design. We should force it by type and compiler errors.
> 
> However I have one issue about it.
> Currently, PropertyName classifies itself into 2, an index property and not-an-index property.
> The reason why we do may be that ECMAScript spec describes that the array index is 0 - 2^23 -1 range. (in sec 15.4)
> And some behavior of abstract operations in the spec becomes different.
> For example, Array.[[DefineOwnProperty]] recognizes the property is array index or not.
> And according to this information, the process of Array.[[DefineOwnProperty]] becomes different. (Expanding "length" property or not etc.)
> 
> Based on this spec, JSC provides 2 functions for [[DefineOwnProperty]]. JSObject::defineOwnNonIndexProperty and JSObject::defineOwnIndexedProperty.
> And these implmenetations are quite different.
> Accepting UINT_MAX in all index operations requires all subsequent changes of Indexed/NonIndex operations, and it makes the implementation complicated since the term "index" becomes different from the spec.
> 
> To solve this problem efficiently, I slightly modified your nice proposal,
> 
> 1. Change PropertyName::asIndex() returns WTF::Optional<uint32_t> value. In this time, when the value is UINT_MAX, we makes it invalid optional value.
> 2. Change callers of PropertyName::asIndex() to check the bool instead of comparing against PropertyName::NotAnIndex. At this time, since we makes the UINT_MAX as invalid, it will be filtered here.
> 3. Allow callers to pass UINT_MAX to putByIndex (and other JSC_EXPORT_PRIVATE annotated APIs). It allows the user outside JSC to pass UINT_MAX as index.
> 
> In the case of (3), the current implementation of putByIndex already accepts UINT_MAX as index while putDirectIndex / putDirect don't accept this.
> http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/runtime/JSObject.cpp#L415
> I think this is good design since put*Direct* should not be called from the outside of the JSC.
> If the API is annotated with JS_EXPORT_PRIVATE, we should accept UINT_MAX as index like the current putByIndex do.
> What do you think of?

And another proposal (and maybe it's orthogonal to the previous our proposal) is  introducing ArrayIndex class instead of using unsigned as array index.
And provides a factory function like,

Optional<ArrayIndex> ArrayIndex::from(unsigned maybeIndex)
{
    if (maybeIndex == UINT32_MAX)
        return Nullopt;
    return ArrayIndex(maybeIndex);  // private constructor
}

It forces the provided value is ArrayIndex by type checker. What do you think of?
Comment 10 Yusuke Suzuki 2015-01-16 05:29:19 PST
Created attachment 244762 [details]
Patch
Comment 11 Yusuke Suzuki 2015-01-16 05:37:17 PST
Comment on attachment 244762 [details]
Patch

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

Updated the patch. I changed Identifier::asIndex to return Optional<uint32_t>.

However, I don't apply my proposal that changing unsigned index parameter to ArrayIndex type. 
So this patch doesn't handle the case that the caller calls very low-level API (putDirect, defineOwnIndexedProperty etc.) with unsigned arguments.
If it's prefereable, I'll change it so :)

> Source/JavaScriptCore/runtime/PropertyName.h:70
> +    if (value == UINT_MAX)

When the value is UINT_MAX, we make it Nullopt since it's not an index.

> LayoutTests/js/dfg-put-by-val-direct-with-edge-numbers-expected.txt:56
> +PASS lookupWithKey2(toStringThrowsError) threw exception 'Error: ng' on all iterations including after DFG tier-up.

Introduce the test that a key throws an error.

> LayoutTests/resources/js-test-pre.js:278
> +function dfgShouldThrow(theFunction, _a, _e)

Introduce new test helper dfgShouldThrow.
Comment 12 Build Bot 2015-01-16 06:40:52 PST
Comment on attachment 244762 [details]
Patch

Attachment 244762 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5356741017468928

New failing tests:
fast/xmlhttprequest/xmlhttprequest-responsetype-sync-request.html
js/dom/dom-as-prototype-assignment-exception.html
js/dom/dom-attributes-on-mismatch-type.html
js/dom/shadow-navigator-geolocation-in-strict-mode-does-not-throw.html
Comment 13 Build Bot 2015-01-16 06:40:56 PST
Created attachment 244763 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 14 Build Bot 2015-01-16 06:46:37 PST
Comment on attachment 244762 [details]
Patch

Attachment 244762 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6635351707746304

New failing tests:
fast/xmlhttprequest/xmlhttprequest-responsetype-sync-request.html
js/dom/dom-as-prototype-assignment-exception.html
js/dom/dom-attributes-on-mismatch-type.html
js/dom/shadow-navigator-geolocation-in-strict-mode-does-not-throw.html
Comment 15 Build Bot 2015-01-16 06:46:40 PST
Created attachment 244764 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 16 Geoffrey Garen 2015-01-19 15:34:07 PST
Comment on attachment 244762 [details]
Patch

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

This approach looks good to me. Any insight into the failing tests on the EWS bots? I have to say r- because of the test failures.

> Source/JavaScriptCore/dfg/DFGOperations.cpp:133
> +            Optional<uint32_t> optionalIndex = propertyName.asIndex();
> +            if (!optionalIndex)

I would just call this "index".

Also, in simple cases like this, I like to combine lines:

if (Optional<uint32_t> index = propertyName.asIndex())
    // putDirectIndex
else
    // putDirect

> Source/JavaScriptCore/jit/JITOperations.cpp:509
> +    // Don't put to an object if toString throws an exception.
> +    Identifier property = subscript.toString(callFrame)->toIdentifier(callFrame);
> +    if (!callFrame->vm().exception()) {

This is a good place to use early return in the case of exception -- that way, the resulting code uses less indentation.

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:848
> +    // Don't put to an object if toString throws an exception.
> +    Identifier property = subscript.toString(exec)->toIdentifier(exec);
> +    if (!exec->vm().exception()) {

Early return.

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:851
> +        Optional<uint32_t> optionalIndex = propertyName.asIndex();
> +        if (!optionalIndex) {

Combine.

> Source/JavaScriptCore/runtime/Arguments.cpp:165
> +    Optional<uint32_t> optionalIndex = propertyName.asIndex();
> +    if (optionalIndex) {

Combine.

> Source/JavaScriptCore/runtime/JSArray.cpp:162
> +    Optional<uint32_t> optionalIndex = propertyName.asIndex();
> +    if (optionalIndex) {

Combine.

> Source/JavaScriptCore/runtime/JSCJSValue.cpp:123
> +    Optional<uint32_t> optionalIndex = propertyName.asIndex();
> +    if (optionalIndex) {

Combine.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:328
> +    Optional<uint32_t> optionalIndex = propertyName.asIndex();
> +    if (optionalIndex) {

Combine.

> Source/JavaScriptCore/runtime/JSObject.cpp:343
> +    Optional<uint32_t> optionalIndex = propertyName.asIndex();
> +    if (optionalIndex) {

Combine.

> Source/JavaScriptCore/runtime/JSObject.cpp:1261
> +    Optional<uint32_t> optionalIndex = propertyName.asIndex();
> +    if (optionalIndex)

Combine.

> Source/JavaScriptCore/runtime/JSObject.cpp:2663
> +    Optional<uint32_t> optionalIndex = propertyName.asIndex();
> +    if (optionalIndex) {

Combine.

> Source/JavaScriptCore/runtime/JSObject.h:1248
> +    Optional<uint32_t> optionalIndex = propertyName.asIndex();
> +    if (optionalIndex)

Combine.

> Source/JavaScriptCore/runtime/JSObject.h:1278
> +    Optional<uint32_t> optionalIndex = propertyName.asIndex();
> +    if (optionalIndex)

Combine.

> Source/JavaScriptCore/runtime/LiteralParser.cpp:653
> +                Optional<uint32_t> optionalIndex = ident.asIndex();
> +                if (optionalIndex)

Combine.

> Source/JavaScriptCore/runtime/Structure.cpp:1014
> +    Optional<uint32_t> optionalIndex = propertyName.asIndex();
> +    if (optionalIndex)

Combine.

> Source/WebCore/bindings/js/JSHTMLAllCollectionCustom.cpp:69
> +        Optional<uint32_t> optionalIndex = toUInt32FromStringImpl(string.impl());
> +        if (optionalIndex)

Combine.

> Source/WebCore/bindings/js/JSHTMLAllCollectionCustom.cpp:79
> +    Optional<uint32_t> optionalIndex = toUInt32FromStringImpl(exec->argument(1).toWTFString(exec).impl());
> +    if (optionalIndex) {

Combine.

> Source/WebCore/bindings/scripts/test/JS/JSFloat64Array.cpp:219
> +    Optional<uint32_t> optionalIndex = propertyName.asIndex();
> +    if (optionalIndex) {

Combine.

> Source/WebCore/bridge/runtime_array.cpp:123
> +    Optional<uint32_t> optionalIndex = propertyName.asIndex();
> +    if (optionalIndex) {

Combine.
Comment 17 Yusuke Suzuki 2015-01-19 19:34:17 PST
Comment on attachment 244762 [details]
Patch

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

Thank you for your review :) I'll upload the revised patch.
In my main development environment (Linux WebKitGTK port), there's no failures. So later, I'll check it on my OSX machine.

>> Source/JavaScriptCore/dfg/DFGOperations.cpp:133
>> +            if (!optionalIndex)
> 
> I would just call this "index".
> 
> Also, in simple cases like this, I like to combine lines:
> 
> if (Optional<uint32_t> index = propertyName.asIndex())
>     // putDirectIndex
> else
>     // putDirect

That looks nice! I'll change to that.

>> Source/JavaScriptCore/jit/JITOperations.cpp:509
>> +    if (!callFrame->vm().exception()) {
> 
> This is a good place to use early return in the case of exception -- that way, the resulting code uses less indentation.

Nice. I've fixed.

>> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:848
>> +    if (!exec->vm().exception()) {
> 
> Early return.

Done

>> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:851
>> +        if (!optionalIndex) {
> 
> Combine.

Done

>> Source/JavaScriptCore/runtime/Arguments.cpp:165
>> +    if (optionalIndex) {
> 
> Combine.

Done

>> Source/JavaScriptCore/runtime/JSArray.cpp:162
>> +    if (optionalIndex) {
> 
> Combine.

Done

>> Source/JavaScriptCore/runtime/JSCJSValue.cpp:123
>> +    if (optionalIndex) {
> 
> Combine.

Done

>> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:328
>> +    if (optionalIndex) {
> 
> Combine.

Done

>> Source/JavaScriptCore/runtime/JSObject.cpp:343
>> +    if (optionalIndex) {
> 
> Combine.

Done

>> Source/JavaScriptCore/runtime/JSObject.cpp:1261
>> +    if (optionalIndex)
> 
> Combine.

Done

>> Source/JavaScriptCore/runtime/JSObject.cpp:2663
>> +    if (optionalIndex) {
> 
> Combine.

Done

>> Source/JavaScriptCore/runtime/JSObject.h:1248
>> +    if (optionalIndex)
> 
> Combine.

Done

>> Source/JavaScriptCore/runtime/JSObject.h:1278
>> +    if (optionalIndex)
> 
> Combine.

Done

>> Source/JavaScriptCore/runtime/LiteralParser.cpp:653
>> +                if (optionalIndex)
> 
> Combine.

Done

>> Source/JavaScriptCore/runtime/Structure.cpp:1014
>> +    if (optionalIndex)
> 
> Combine.

Done

>> Source/WebCore/bindings/js/JSHTMLAllCollectionCustom.cpp:69
>> +        if (optionalIndex)
> 
> Combine.

Done

>> Source/WebCore/bindings/js/JSHTMLAllCollectionCustom.cpp:79
>> +    if (optionalIndex) {
> 
> Combine.

Done

>> Source/WebCore/bindings/scripts/test/JS/JSFloat64Array.cpp:219
>> +    if (optionalIndex) {
> 
> Combine.

Done

>> Source/WebCore/bridge/runtime_array.cpp:123
>> +    if (optionalIndex) {
> 
> Combine.

Done
Comment 18 Yusuke Suzuki 2015-01-19 19:36:16 PST
Created attachment 244954 [details]
Patch
Comment 19 Yusuke Suzuki 2015-01-19 19:50:06 PST
Created attachment 244957 [details]
Patch
Comment 20 Yusuke Suzuki 2015-01-19 20:20:32 PST
Ah, seeing the test failures, I've found that the expectation file includes line number of CONSOLE MESSAGE. Maybe, adding dfgShouldThrow into js-test-pre.js causes this.
Comment 21 Build Bot 2015-01-19 20:43:51 PST
Comment on attachment 244957 [details]
Patch

Attachment 244957 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6196256799981568

New failing tests:
fast/xmlhttprequest/xmlhttprequest-responsetype-sync-request.html
js/dom/dom-as-prototype-assignment-exception.html
js/dom/dom-attributes-on-mismatch-type.html
js/dom/shadow-navigator-geolocation-in-strict-mode-does-not-throw.html
Comment 22 Build Bot 2015-01-19 20:43:54 PST
Created attachment 244962 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 23 Yusuke Suzuki 2015-01-20 00:10:36 PST
Created attachment 244971 [details]
Patch
Comment 24 Geoffrey Garen 2015-01-20 12:36:21 PST
Comment on attachment 244971 [details]
Patch

r=me
Comment 25 WebKit Commit Bot 2015-01-20 13:14:47 PST
Comment on attachment 244971 [details]
Patch

Clearing flags on attachment: 244971

Committed r178751: <http://trac.webkit.org/changeset/178751>
Comment 26 WebKit Commit Bot 2015-01-20 13:14:54 PST
All reviewed patches have been landed.  Closing bug.
Comment 27 Joseph Pecoraro 2015-01-20 13:51:59 PST
Looks like this may have caused test failures on the Apple Mavericks 32-bit JSC test bot:
https://build.webkit.org/builders/Apple%20Mavericks%2032-bit%20JSC%20%28BuildAndTest%29/builds/6876
https://build.webkit.org/builders/Apple%20Mavericks%2032-bit%20JSC%20%28BuildAndTest%29/builds/6876/steps/webkit-32bit-jsc-test/logs/stdio

> ** The following JSC stress test failures have been introduced:
> 	jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout
> 	jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-dfg-eager-no-cjit
> 	jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-ftl
> 	jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-ftl-eager-no-cjit
> 	jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-ftl-no-cjit
> 	jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-no-cjit
> 	jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-no-llint

Errors looking like:

> jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout: PASS lookupWithKey("-4.2") is 42 on all iterations including after DFG tier-up.
> jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout: Exception: ReferenceError: Can't find variable: dfgShouldThrow
> jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout: global code@dfg-put-by-val-direct-with-edge-numbers.js:86:15
> jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout: PASS successfullyParsed is true

Should we roll out this change, or somehow skip the test?
Comment 28 Yusuke Suzuki 2015-01-20 14:16:16 PST
(In reply to comment #27)
> Looks like this may have caused test failures on the Apple Mavericks 32-bit
> JSC test bot:
> https://build.webkit.org/builders/Apple%20Mavericks%2032-
> bit%20JSC%20%28BuildAndTest%29/builds/6876
> https://build.webkit.org/builders/Apple%20Mavericks%2032-
> bit%20JSC%20%28BuildAndTest%29/builds/6876/steps/webkit-32bit-jsc-test/logs/
> stdio
> 
> > ** The following JSC stress test failures have been introduced:
> > 	jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout
> > 	jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-dfg-eager-no-cjit
> > 	jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-ftl
> > 	jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-ftl-eager-no-cjit
> > 	jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-ftl-no-cjit
> > 	jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-no-cjit
> > 	jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-no-llint
> 
> Errors looking like:
> 
> > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout: PASS lookupWithKey("-4.2") is 42 on all iterations including after DFG tier-up.
> > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout: Exception: ReferenceError: Can't find variable: dfgShouldThrow
> > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout: global code@dfg-put-by-val-direct-with-edge-numbers.js:86:15
> > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout: PASS successfullyParsed is true
> 
> Should we roll out this change, or somehow skip the test?

Thank you for informing us!!

Hmm, "Can't find variable: dfgShouldThrow" is very curious behavior because,

(1): dfgShouldThrow is actually defined in js-test-pre.js.
(2): dfgShouldBe works before raising this error.
(3): dfgShouldBe uses dfgCompiled, this is defined after dfgShouldThrow in js-test-pre.js.

Geoffrey, what do you think of?
Comment 29 Joseph Pecoraro 2015-01-20 14:27:54 PST
Comment on attachment 244971 [details]
Patch

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

> LayoutTests/resources/js-test-pre.js:623
> +    debug("WARN: dfgShouldBe() expects a function and two strings");

Nit: This looks like a copy/paste error, this should be "dfgShouldThrow()" in the error message.
Comment 30 Geoffrey Garen 2015-01-20 14:29:11 PST
> Geoffrey, what do you think of?

Strange. I don't know what's going on here.

Can you reproduce this failure by running in 32-bit mode?
Comment 31 Joseph Pecoraro 2015-01-20 14:31:52 PST
(In reply to comment #30)
> > Geoffrey, what do you think of?
> 
> Strange. I don't know what's going on here.
> 
> Can you reproduce this failure by running in 32-bit mode?

I agree, strange. However, I will start a rollout, since the bots are continuing to fail on these tests.
Comment 32 WebKit Commit Bot 2015-01-20 14:34:04 PST
Re-opened since this is blocked by bug 140694
Comment 33 Yusuke Suzuki 2015-01-20 14:34:46 PST
(In reply to comment #29)
> Comment on attachment 244971 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=244971&action=review
> 
> > LayoutTests/resources/js-test-pre.js:623
> > +    debug("WARN: dfgShouldBe() expects a function and two strings");
> 
> Nit: This looks like a copy/paste error, this should be "dfgShouldThrow()"
> in the error message.

Thanks! I'll fix it at the same time.
Comment 34 Yusuke Suzuki 2015-01-20 14:35:18 PST
(In reply to comment #30)
> > Geoffrey, what do you think of?
> 
> Strange. I don't know what's going on here.
> 
> Can you reproduce this failure by running in 32-bit mode?

OK. I'll build it with OSX in 32-bit mode and attempt to reproduce it :)
Comment 35 Joseph Pecoraro 2015-01-20 14:37:10 PST
(In reply to comment #28)
> (In reply to comment #27)
> > Looks like this may have caused test failures on the Apple Mavericks 32-bit
> > JSC test bot:
> > https://build.webkit.org/builders/Apple%20Mavericks%2032-
> > bit%20JSC%20%28BuildAndTest%29/builds/6876
> > https://build.webkit.org/builders/Apple%20Mavericks%2032-
> > bit%20JSC%20%28BuildAndTest%29/builds/6876/steps/webkit-32bit-jsc-test/logs/
> > stdio
> > 
> > > ** The following JSC stress test failures have been introduced:
> > > 	jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout
> > > 	jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-dfg-eager-no-cjit
> > > 	jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-ftl
> > > 	jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-ftl-eager-no-cjit
> > > 	jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-ftl-no-cjit
> > > 	jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-no-cjit
> > > 	jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-no-llint
> > 
> > Errors looking like:
> > 
> > > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout: PASS lookupWithKey("-4.2") is 42 on all iterations including after DFG tier-up.
> > > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout: Exception: ReferenceError: Can't find variable: dfgShouldThrow
> > > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout: global code@dfg-put-by-val-direct-with-edge-numbers.js:86:15
> > > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout: PASS successfullyParsed is true
> > 
> > Should we roll out this change, or somehow skip the test?
> 
> Thank you for informing us!!
> 
> Hmm, "Can't find variable: dfgShouldThrow" is very curious behavior because,
> 
> (1): dfgShouldThrow is actually defined in js-test-pre.js.
> (2): dfgShouldBe works before raising this error.
> (3): dfgShouldBe uses dfgCompiled, this is defined after dfgShouldThrow in
> js-test-pre.js.
> 
> Geoffrey, what do you think of?

Wait!!

There are multiple js-test-pre.js. I suspect you might need to update at least one other to get your change. Maybe the http one?

./LayoutTests/http/tests/resources/js-test-pre.js
./LayoutTests/http/tests/webgl/1.0.2/resources/webgl_test_files/resources/js-test-pre.js
./LayoutTests/js/mozilla/resources/js-test-pre.js
./LayoutTests/resources/js-test-pre.js
Comment 36 Joseph Pecoraro 2015-01-20 14:42:06 PST
> There are multiple js-test-pre.js. I suspect you might need to update at
> least one other to get your change. Maybe the http one?

Hmm, there are a bunch of differences between them, and the other dfg functions are not even available in the http one. So this is probably not it =(.
Comment 37 Yusuke Suzuki 2015-01-20 16:48:23 PST
I've built WebKit with

$ Tools/Scripts/build-webkit --debug --32-bit

on OSX Yosemite x86_64 environment.
And run LayoutTests with

$ Tools/Scripts/run-webkit-tests --debug --32-bit -f 'js/dfg-put-by-val-direct-with-edge-numbers.html'

But the test has passed in my environment. Is it a correct way to build & test 32-bit mode WebKit?
Comment 38 Geoffrey Garen 2015-01-20 17:14:14 PST
I think to reproduce what the bot did, you need to use run-esc-stress-tests or run-javascriptcore-tests.
Comment 39 Yusuke Suzuki 2015-01-20 17:51:52 PST
(In reply to comment #38)
> I think to reproduce what the bot did, you need to use run-esc-stress-tests
> or run-javascriptcore-tests.

Thank you. I've reproduced that.
Comment 40 Yusuke Suzuki 2015-01-20 18:30:16 PST
(In reply to comment #38)
> I think to reproduce what the bot did, you need to use run-esc-stress-tests
> or run-javascriptcore-tests.

When moving dfgShouldThrow function into dfg-put-by-val-direct-with-edge-numbers.js itself, I've found that stress tests pass.
Geoffrey, what do you think about this solution?

Yes, possibly, it's not fundamental solution for this problem.
Now I guess that there may be some issues in lookupping variables from GlobalObject's segmeted vector and it may cause this problem.
Comment 41 Yusuke Suzuki 2015-01-20 21:45:01 PST
Created attachment 245047 [details]
Patch
Comment 42 Geoffrey Garen 2015-01-21 13:39:56 PST
Comment on attachment 245047 [details]
Patch

r=me
Comment 43 Geoffrey Garen 2015-01-21 13:40:32 PST
> When moving dfgShouldThrow function into
> dfg-put-by-val-direct-with-edge-numbers.js itself, I've found that stress
> tests pass.
> Geoffrey, what do you think about this solution?

I think this is an OK work-around in the short term. r=me

> Yes, possibly, it's not fundamental solution for this problem.
> Now I guess that there may be some issues in lookupping variables from
> GlobalObject's segmeted vector and it may cause this problem.

I think this is worth investigating in a follow-up bug. This behavior is very strange!
Comment 44 WebKit Commit Bot 2015-01-22 00:54:49 PST
Comment on attachment 245047 [details]
Patch

Clearing flags on attachment: 245047

Committed r178894: <http://trac.webkit.org/changeset/178894>
Comment 45 WebKit Commit Bot 2015-01-22 00:54:54 PST
All reviewed patches have been landed.  Closing bug.
Comment 46 Alexey Proskuryakov 2015-01-22 11:26:16 PST
This broke JSC tests, I'm going to roll out.

https://build.webkit.org/builders/Apple%20Yosemite%20Release%20WK2%20%28Tests%29/builds/2207/steps/jscore-test/logs/stdio

jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-ftl-eager-no-cjit: Timed out after 339.000000 seconds!
jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-ftl-eager-no-cjit: 1   0x1046ce7d3 WTF::threadEntryPoint(void*)
jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-ftl-eager-no-cjit: 2   0x1046cecbf WTF::wtfThreadEntryPoint(void*)
jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-ftl-eager-no-cjit: 3   0x7fff8fb492fc _pthread_body
jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-ftl-eager-no-cjit: 4   0x7fff8fb49279 _pthread_body
jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-ftl-eager-no-cjit: 5   0x7fff8fb474b1 thread_start
jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-ftl-eager-no-cjit: test_script_14898: line 2: 24711 Segmentation fault: 11  "$@" ../../../../.vm/JavaScriptCore.framework/Resources/jsc --useFTLJIT\=false --enableFunctionDotArguments\=true --testTheFTL\=true --useFTLJIT\=true --enableConcurrentJIT\=false --thresholdForJITAfterWarmUp\=100 --thresholdForJITAfterWarmUp\=10 --thresholdForJITSoon\=10 --thresholdForOptimizeAfterWarmUp\=20 --thresholdForOptimizeAfterLongWarmUp\=20 --thresholdForOptimizeSoon\=20 --thresholdForFTLOptimizeAfterWarmUp\=20 --thresholdForFTLOptimizeSoon\=20 resources/standalone-pre.js dfg-put-by-val-direct-with-edge-numbers.js resources/standalone-post.js
jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-ftl-eager-no-cjit: ERROR: Unexpected exit code: 139
Comment 47 Alexey Proskuryakov 2015-01-22 11:27:54 PST
Bindings tests are also broken, although those probably just need an update.

https://build.webkit.org/builders/Apple%20Yosemite%20Release%20WK2%20(Tests)/builds/2207/steps/bindings-generation-tests/logs/stdio
Comment 48 WebKit Commit Bot 2015-01-22 11:29:47 PST
Re-opened since this is blocked by bug 140775
Comment 49 Yusuke Suzuki 2015-01-25 00:58:55 PST
(In reply to comment #46)
> This broke JSC tests, I'm going to roll out.
> 
> https://build.webkit.org/builders/
> Apple%20Yosemite%20Release%20WK2%20%28Tests%29/builds/2207/steps/jscore-test/
> logs/stdio
> 
> jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-
> numbers.js.layout-ftl-eager-no-cjit: Timed out after 339.000000 seconds!
> jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-
> numbers.js.layout-ftl-eager-no-cjit: 1   0x1046ce7d3
> WTF::threadEntryPoint(void*)
> jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-
> numbers.js.layout-ftl-eager-no-cjit: 2   0x1046cecbf
> WTF::wtfThreadEntryPoint(void*)
> jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-
> numbers.js.layout-ftl-eager-no-cjit: 3   0x7fff8fb492fc _pthread_body
> jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-
> numbers.js.layout-ftl-eager-no-cjit: 4   0x7fff8fb49279 _pthread_body
> jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-
> numbers.js.layout-ftl-eager-no-cjit: 5   0x7fff8fb474b1 thread_start
> jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-
> numbers.js.layout-ftl-eager-no-cjit: test_script_14898: line 2: 24711
> Segmentation fault: 11  "$@"
> ../../../../.vm/JavaScriptCore.framework/Resources/jsc --useFTLJIT\=false
> --enableFunctionDotArguments\=true --testTheFTL\=true --useFTLJIT\=true
> --enableConcurrentJIT\=false --thresholdForJITAfterWarmUp\=100
> --thresholdForJITAfterWarmUp\=10 --thresholdForJITSoon\=10
> --thresholdForOptimizeAfterWarmUp\=20
> --thresholdForOptimizeAfterLongWarmUp\=20 --thresholdForOptimizeSoon\=20
> --thresholdForFTLOptimizeAfterWarmUp\=20 --thresholdForFTLOptimizeSoon\=20
> resources/standalone-pre.js dfg-put-by-val-direct-with-edge-numbers.js
> resources/standalone-post.js
> jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-
> numbers.js.layout-ftl-eager-no-cjit: ERROR: Unexpected exit code: 139

Thank you!
I'll investigate it...
And I'll update the patch to fix for binding tests.
Comment 50 Yusuke Suzuki 2015-04-06 06:10:02 PDT
In http://trac.webkit.org/changeset/182406, refactoring part (changing uint32_t => Option<uint32_t>) was landed.

So I'll clean up the patch to make it smaller. And investigate the issue!
Comment 51 Yusuke Suzuki 2015-04-06 12:07:32 PDT
Created attachment 250218 [details]
Patch
Comment 52 Yusuke Suzuki 2015-04-06 12:14:32 PDT
Created attachment 250220 [details]
Patch
Comment 53 Yusuke Suzuki 2015-04-06 12:16:14 PDT
Comment on attachment 250220 [details]
Patch

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

Attempt to re-land this patch!

> Source/JavaScriptCore/ChangeLog:10
> +        This patch checks toString-ed Identifier is index or not to choose putDirect / putDirectIndex.

Now, Optional<uint32_t> patch is splitted and landed. So the patch for this issue becomes cleaner.

> Source/JavaScriptCore/ChangeLog:23
> +        (toStringThrowsError.toString):

Move LayoutTests provided in the previous patch to stress tests.

> Source/JavaScriptCore/tests/stress/dfg-put-by-val-direct-with-edge-numbers.js:9
> +noInline(lookupWithKey);

I think this is missing in the previous patch (and causes timeout).

> Source/JavaScriptCore/tests/stress/dfg-put-by-val-direct-with-edge-numbers.js:83
> +noInline(lookupWithKey2);

Ditto.
Comment 54 Darin Adler 2015-04-06 14:00:09 PDT
Comment on attachment 250220 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGOperations.cpp:105
> -        putByVal<strict, direct>(exec, baseValue, property.asUInt32(), value);
> +        uint32_t index = property.asUInt32();
> +        ASSERT_WITH_MESSAGE(isIndex(index), "Since JSValue::isUInt32 returns true only when the boxed value is int32_t and positive, it doesn't return true for uint32_t max value (0xFFFFFFFF) that is not array index.");
> +        putByVal<strict, direct>(exec, baseValue, index, value);

The message here might be a good comment to have explaining this assert, but it doesn’t seem helpful as an assertion message. I would write this assertion:

    // Despite its name, JSValue::isUInt32 will return true only for positive boxed int32_t; all those values are valid array indices.
    ASSERT(isIndex(property.asUInt32()));

And I would leave the actual putByVal line alone; I don’t think we should put the thing into a local variable just to share code between the real code and the assertion.

> Source/JavaScriptCore/jit/JITOperations.cpp:505
> +        uint32_t index = subscript.asUInt32();
> +        ASSERT_WITH_MESSAGE(isIndex(index), "Since JSValue::isUInt32 returns true only when the boxed value is int32_t and positive, it doesn't return true for uint32_t max value (0xFFFFFFFF) that is not array index.");
> +        baseObject->putDirectIndex(callFrame, index, value, 0, isStrictMode ? PutDirectIndexShouldThrow : PutDirectIndexShouldNotThrow);

Same comment here as above. Code should just be:

    baseObject->putDirectIndex(callFrame, subscript.asUInt32(), value, 0, isStrictMode ? PutDirectIndexShouldThrow : PutDirectIndexShouldNotThrow);

> Source/JavaScriptCore/jit/JITOperations.cpp:511
> +        double subscriptAsDouble = subscript.asDouble();
> +        uint32_t subscriptAsUInt32 = static_cast<uint32_t>(subscriptAsDouble);

I’m surprised this is really the code we want. Does this handle values such as 2^32 properly? Do we have test coverage for this?

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:802
> +        ASSERT_WITH_MESSAGE(isIndex(index), "Since JSValue::isUInt32 returns true only when the boxed value is int32_t and positive, it doesn't return true for uint32_t max value (0xFFFFFFFF) that is not array index.");

Same thought about this assertion as above.
Comment 55 Darin Adler 2015-04-06 16:38:29 PDT
No, 0xFFFFFFFF is not an index! That is a JavaScript language feature, not something specific about our ending!
Comment 56 Darin Adler 2015-04-06 16:39:02 PDT
(In reply to comment #55)
> No, 0xFFFFFFFF is not an index! That is a JavaScript language feature, not
> something specific about our ending!

Oops, please ignore that comment.
Comment 57 Yusuke Suzuki 2015-04-07 00:22:47 PDT
Comment on attachment 250220 [details]
Patch

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

Thank you for your review.

>> Source/JavaScriptCore/dfg/DFGOperations.cpp:105
>> +        putByVal<strict, direct>(exec, baseValue, index, value);
> 
> The message here might be a good comment to have explaining this assert, but it doesn’t seem helpful as an assertion message. I would write this assertion:
> 
>     // Despite its name, JSValue::isUInt32 will return true only for positive boxed int32_t; all those values are valid array indices.
>     ASSERT(isIndex(property.asUInt32()));
> 
> And I would leave the actual putByVal line alone; I don’t think we should put the thing into a local variable just to share code between the real code and the assertion.

Looks very nice! I'll change it.

>> Source/JavaScriptCore/jit/JITOperations.cpp:505
>> +        baseObject->putDirectIndex(callFrame, index, value, 0, isStrictMode ? PutDirectIndexShouldThrow : PutDirectIndexShouldNotThrow);
> 
> Same comment here as above. Code should just be:
> 
>     baseObject->putDirectIndex(callFrame, subscript.asUInt32(), value, 0, isStrictMode ? PutDirectIndexShouldThrow : PutDirectIndexShouldNotThrow);

Ditto

>> Source/JavaScriptCore/jit/JITOperations.cpp:511
>> +        uint32_t subscriptAsUInt32 = static_cast<uint32_t>(subscriptAsDouble);
> 
> I’m surprised this is really the code we want. Does this handle values such as 2^32 properly? Do we have test coverage for this?

Yes, it correctly handles values (int32_t max, uint32_t max) range.
0x7fffffff, 0x80000000 and others are covered by the tests.

>> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:802
>> +        ASSERT_WITH_MESSAGE(isIndex(index), "Since JSValue::isUInt32 returns true only when the boxed value is int32_t and positive, it doesn't return true for uint32_t max value (0xFFFFFFFF) that is not array index.");
> 
> Same thought about this assertion as above.

Ditto.

> Source/JavaScriptCore/tests/stress/dfg-put-by-val-direct-with-edge-numbers.js:68
> +    '"-4.2"',

Oops. During migration from layout-tests to stress tests, their stringified representation ('"' + X + '"', '+0.0', etc)  should be striped.
So I'll land it after stripping it.
Comment 58 Yusuke Suzuki 2015-04-07 00:27:05 PDT
Committed r182452: <http://trac.webkit.org/changeset/182452>