Bug 140426 – put_by_val_direct need to check the property is index or not for using putDirect / putDirectIndex

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

put_by_val_direct need to check the property is index or not for using putDirect / putDirectIndex

[Yusuke Suzuki]

Created attachment 244575 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 244575 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 244577 [details]
Patch

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Oops, I'll update the test.

[Yusuke Suzuki]

Created attachment 244579 [details]
Patch

[Geoffrey Garen]

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?

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

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?

[Yusuke Suzuki]

Created attachment 244762 [details]
Patch

[Yusuke Suzuki]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Geoffrey Garen]

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.

[Yusuke Suzuki]

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

[Yusuke Suzuki]

Created attachment 244954 [details]
Patch

[Yusuke Suzuki]

Created attachment 244957 [details]
Patch

[Yusuke Suzuki]

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.

[Build Bot]

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

[Build Bot]

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

[Yusuke Suzuki]

Created attachment 244971 [details]
Patch

[Geoffrey Garen]

Comment on attachment 244971 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 244971 [details]
Patch

Clearing flags on attachment: 244971

Committed r178751: <http://trac.webkit.org/changeset/178751>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

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 [e-mail]: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?

[Yusuke Suzuki]

(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 [e-mail]: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?

[Joseph Pecoraro]

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.

[Geoffrey Garen]

> 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?

[Joseph Pecoraro]

(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.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 140694

[Yusuke Suzuki]

(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.

[Yusuke Suzuki]

(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 :)

[Joseph Pecoraro]

(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 [e-mail]: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

[Joseph Pecoraro]

> 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 =(.

[Yusuke Suzuki]

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?

[Geoffrey Garen]

I think to reproduce what the bot did, you need to use run-esc-stress-tests or run-javascriptcore-tests.

[Yusuke Suzuki]

(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.

[Yusuke Suzuki]

(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.

[Yusuke Suzuki]

Created attachment 245047 [details]
Patch

[Geoffrey Garen]

Comment on attachment 245047 [details]
Patch

r=me

[Geoffrey Garen]

> 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!

[WebKit Commit Bot]

Comment on attachment 245047 [details]
Patch

Clearing flags on attachment: 245047

Committed r178894: <http://trac.webkit.org/changeset/178894>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

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

[Alexey Proskuryakov]

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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 140775

[Yusuke Suzuki]

(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.

[Yusuke Suzuki]

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!

[Yusuke Suzuki]

Created attachment 250218 [details]
Patch

[Yusuke Suzuki]

Created attachment 250220 [details]
Patch

[Yusuke Suzuki]

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.

[Darin Adler]

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.

[Darin Adler]

No, 0xFFFFFFFF is not an index! That is a JavaScript language feature, not something specific about our ending!

[Darin Adler]

(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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r182452: <http://trac.webkit.org/changeset/182452>