Bug 108264 – Objective-C API: Fix insertion of values greater than the max index allowed by the spec

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

[Joseph Pecoraro]

Addressing some of my review comments on the ObjC JSC API:

    > Source/JavaScriptCore/API/JSValue.mm:306
    > +    if (index != (unsigned)index)
    > +        [self valueForProperty:[[JSValue valueWithDouble:index inContext:context] toString]];

    Missing return!

    > Source/JavaScriptCore/API/JSValue.mm:919
    > +        if ([object isKindOfClass:[NSArray class]])
    > +            return (ObjcContainerConvertor::Task){ object, JSObjectMakeArray(contextInternalContext(context), 0, NULL, 0), ContainerArray };

    NSArray is already handled above. These 2 lines can be removed.

    > Source/JavaScriptCore/API/JSValue.mm:922
    > +        if ([object isKindOfClass:[NSDictionary class]])
    > +            return (ObjcContainerConvertor::Task){ object, JSObjectMake(contextInternalContext(context), 0, 0), ContainerDictionary };

    NSDictionary is already handled above. These 2 lines can be removed.

[Joseph Pecoraro]

Hmm, I'd like to write a test for the valueAtIndex: change, but it seems I don't
have what I need to satisfy JS_OBJC_API_ENABLED. I can take a stab at it.

[Joseph Pecoraro]

Reassigning to Mark. I passed him a patch and test case, but I cannot test on 10.8 and he has a good understanding of this.

[Mark Hahnenberg]

Created attachment 185370 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 185370 [details]
Patch

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

> Source/JavaScriptCore/API/JSValue.mm:277
> +    // Properties that are higher than the max index allowed by the spec (2^32 - 1) are converted to normal double properties.

Would it be more accurate to say they are converted to string / keyed properties? The terminology "normal double property" sounds strange to me.

A comment like this would be good in the API header to clarify to users of the API what happens with indices outside the specced range. Since it looks like we've decided to handle it in a non-obvious way. It is non-obvious that this API might not change the Array's length.

[Benjamin Poulain]

Comment on attachment 185370 [details]
Patch

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

> Source/JavaScriptCore/API/tests/testapi.mm:272
> +        NSUInteger highIndex = UINT_MAX;
> +        [array setValue:value1 atIndex:lowIndex];
> +        checkResult(@"arrayLengthLow", [[array[@"length"] toNumber] unsignedIntegerValue] == (lowIndex + 1));
> +        [array setValue:value2 atIndex:highIndex];
> +        checkResult(@"arrayLengthHigh", [[array[@"length"] toNumber] unsignedIntegerValue] == (lowIndex + 1));
> +        checkResult(@"valueAtIndex:lowIndex", [[array valueAtIndex:lowIndex] toInt32] == 42);
> +        checkResult(@"valueAtIndex:highIndex", [[array valueAtIndex:highIndex] toInt32] == 24);

UINT_MAX is good but I would also have a test for the exact boundary. Such tests avoid off-by-1 bugs.

[Joseph Pecoraro]

(In reply to comment #5)
> (From update of attachment 185370 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185370&action=review
> 
> > Source/JavaScriptCore/API/tests/testapi.mm:272
> > +        NSUInteger highIndex = UINT_MAX;
> > +        [array setValue:value1 atIndex:lowIndex];
> > +        checkResult(@"arrayLengthLow", [[array[@"length"] toNumber] unsignedIntegerValue] == (lowIndex + 1));
> > +        [array setValue:value2 atIndex:highIndex];
> > +        checkResult(@"arrayLengthHigh", [[array[@"length"] toNumber] unsignedIntegerValue] == (lowIndex + 1));
> > +        checkResult(@"valueAtIndex:lowIndex", [[array valueAtIndex:lowIndex] toInt32] == 42);
> > +        checkResult(@"valueAtIndex:highIndex", [[array valueAtIndex:highIndex] toInt32] == 24);
> 
> UINT_MAX is good but I would also have a test for the exact boundary. Such tests avoid off-by-1 bugs.

Also, the path that this is meant to be testing should test an index above UINT_MAX anyways.

[Mark Hahnenberg]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 185370 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=185370&action=review
> > 
> > > Source/JavaScriptCore/API/tests/testapi.mm:272
> > > +        NSUInteger highIndex = UINT_MAX;
> > > +        [array setValue:value1 atIndex:lowIndex];
> > > +        checkResult(@"arrayLengthLow", [[array[@"length"] toNumber] unsignedIntegerValue] == (lowIndex + 1));
> > > +        [array setValue:value2 atIndex:highIndex];
> > > +        checkResult(@"arrayLengthHigh", [[array[@"length"] toNumber] unsignedIntegerValue] == (lowIndex + 1));
> > > +        checkResult(@"valueAtIndex:lowIndex", [[array valueAtIndex:lowIndex] toInt32] == 42);
> > > +        checkResult(@"valueAtIndex:highIndex", [[array valueAtIndex:highIndex] toInt32] == 24);
> > 
> > UINT_MAX is good but I would also have a test for the exact boundary. Such tests avoid off-by-1 bugs.
> 
> Also, the path that this is meant to be testing should test an index above UINT_MAX anyways.

Not quite. The max length of an Array in JS is UINT_MAX, so the max index is UINT_MAX - 1. But you're right that I should add something right below the max just to make sure things are working properly.

[Darin Adler]

Comment on attachment 185370 [details]
Patch

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

> Source/JavaScriptCore/API/JSValue.mm:278
> +    // Properties that are higher than the max index allowed by the spec (2^32 - 1) are converted to normal double properties.
>      if (index != (unsigned)index)

UINT_MAX will pass this test, even though it’s not a valid array index. That’s a bug. Not sure why the test case does not catch this.

> Source/JavaScriptCore/API/JSValue.mm:-871
> -        if ([object isKindOfClass:[NSArray class]])
> -            return (ObjcContainerConvertor::Task){ object, JSObjectMakeArray(contextRef, 0, NULL, 0), ContainerArray };
> -
> -        if ([object isKindOfClass:[NSDictionary class]])
> -            return (ObjcContainerConvertor::Task){ object, JSObjectMake(contextRef, 0, 0), ContainerDictionary };

No comment in change log explaining why this code was removed.

>>>> Source/JavaScriptCore/API/tests/testapi.mm:272
>>>> +        checkResult(@"valueAtIndex:highIndex", [[array valueAtIndex:highIndex] toInt32] == 24);
>>> 
>>> UINT_MAX is good but I would also have a test for the exact boundary. Such tests avoid off-by-1 bugs.
>> 
>> Also, the path that this is meant to be testing should test an index above UINT_MAX anyways.
> 
> Not quite. The max length of an Array in JS is UINT_MAX, so the max index is UINT_MAX - 1. But you're right that I should add something right below the max just to make sure things are working properly.

For clarity I think we should be testing at least UINT_MAX - 1, a valid array index, UINT_MAX, one invalid one, and UINT_MAX + 1, another invalid one that doesn’t happen to fit into a 32-bit integer.

[Mark Hahnenberg]

Created attachment 185555 [details]
Patch

[WebKit Review Bot]

Comment on attachment 185555 [details]
Patch

Clearing flags on attachment: 185555

Committed r141443: <http://trac.webkit.org/changeset/141443>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.