Bug 108264

Summary: Objective-C API: Fix insertion of values greater than the max index allowed by the spec
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: JavaScriptCoreAssignee: Mark Hahnenberg <mhahnenberg>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, ggaren, joepeck, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Joseph Pecoraro 2013-01-29 17:08:53 PST
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.
Comment 1 Joseph Pecoraro 2013-01-29 17:19:30 PST
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.
Comment 2 Joseph Pecoraro 2013-01-29 18:22:35 PST
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.
Comment 3 Mark Hahnenberg 2013-01-29 18:33:49 PST
Created attachment 185370 [details]
Patch
Comment 4 Joseph Pecoraro 2013-01-29 18:41:34 PST
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.
Comment 5 Benjamin Poulain 2013-01-29 19:33:11 PST
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.
Comment 6 Joseph Pecoraro 2013-01-30 01:37:03 PST
(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.
Comment 7 Mark Hahnenberg 2013-01-30 08:30:57 PST
(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.
Comment 8 Darin Adler 2013-01-30 09:10:14 PST
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.
Comment 9 Mark Hahnenberg 2013-01-30 13:51:00 PST
Created attachment 185555 [details]
Patch
Comment 10 WebKit Review Bot 2013-01-31 10:56:31 PST
Comment on attachment 185555 [details]
Patch

Clearing flags on attachment: 185555

Committed r141443: <http://trac.webkit.org/changeset/141443>
Comment 11 WebKit Review Bot 2013-01-31 10:56:35 PST
All reviewed patches have been landed.  Closing bug.