Bug 32012 – JavaScript delete operator returns true for string properties

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

[Kent Hansen]

According to the ECMAScript 5 specification section 15.5.5.2, string properties whose names are valid string indices are not configurable, hence the delete operator should return false when applied to such a property.

[Kent Hansen]

Created attachment 44062 [details]
Proposed patch and test

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44062 [details] without any errors.

[Darin Adler]

Comment on attachment 44062 [details]
Proposed patch and test

Thanks for tackling this! This is almost right, but not quite.

> +    bool isStrictUInt32;
> +    unsigned i = propertyName.toStrictUInt32(&isStrictUInt32);
> +    if (isStrictUInt32 && internalValue()->canGetIndex(i))
> +        return false;

This code needs to use toArrayIndex rather than toStrictUInt32. These functions differ in how they handle the value 4294967295. And you should make sure there is a test case for that particular number and other numbers just higher and lower than that.

review-

[Kent Hansen]

Created attachment 44159 [details]
Revised patch with more test cases and using toArrayIndex() when converting identifier to string index

[Kent Hansen]

(In reply to comment #3)
> (From update of attachment 44062 [details])
> Thanks for tackling this! This is almost right, but not quite.
> 
> > +    bool isStrictUInt32;
> > +    unsigned i = propertyName.toStrictUInt32(&isStrictUInt32);
> > +    if (isStrictUInt32 && internalValue()->canGetIndex(i))
> > +        return false;
> 
> This code needs to use toArrayIndex rather than toStrictUInt32. These functions
> differ in how they handle the value 4294967295. And you should make sure there
> is a test case for that particular number and other numbers just higher and
> lower than that.
> 
> review-

Hi Darin, thanks!
I was using the same approach as used elsewhere in JSString to calculate the index. I've revised the patch and refactored the code to always use the same conversion method (in getOwnPropertyDescriptor() and getOwnPropertySlot()) too).
Let me know if you think it's worth it to do the refactoring in a separate commit and then add the deleteProperty()-specific stuff after that one.

[WebKit Review Bot]

Attachment 44159 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
JavaScriptCore/runtime/JSString.h:173:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1

[Darin Adler]

Comment on attachment 44159 [details]
Revised patch with more test cases and using toArrayIndex() when converting identifier to string index

> +        unsigned toIndex(const Identifier& propertyName, bool *ok);

WebKit coding style requires putting the "*" next to the "bool", not the "ok".

> +    inline unsigned JSString::toIndex(const Identifier& propertyName, bool *ok)
> +    {
> +        unsigned i = propertyName.toArrayIndex(ok);
> +        *ok = *ok && canGetIndex(i);
> +        return i;
> +    }

I think it's slightly strange to include the canGetIndex call inside a function named toIndex. That's inconsistent with other functions named toIndex, and so perhaps the function should have a different name. But since I don't have another to suggest, we can probably live with this. I would probably call the output variable something more like isIndex rather than ok given the slightly broader purpose of the function.

> +    (void)internalValue()->toIndex(propertyName, &isIndex);

WebKit code does not use this style, with explicit casts to (void) when ignoring function results.

> +description("This page tests deletion of properties on a string object.");
> +
> +var str = "abc";
> +shouldBe('str.length', '3');
> +shouldBe('delete str.length', 'false');
> +shouldBe('delete str[0]', 'false');
> +shouldBe('delete str[1]', 'false');
> +shouldBe('delete str[2]', 'false');
> +shouldBe('delete str[3]', 'true');
> +shouldBe('delete str[-1]', 'true');
> +shouldBe('delete str[4294967294]', 'true');
> +shouldBe('delete str[4294967295]', 'true');
> +shouldBe('delete str.foo', 'true');

Looking at the test and test results I now realize that there is no detectable behavior change from what I asked you to do in the last patch, since indices that are beyond of the length of the string are treated no differently from identifiers that are not valid indices. Thus the special value that is a UInt32 but not a valid index doesn't actually need special handling. That means that using toStrictUInt32 was already acceptable and even slightly more efficient. Using toIndex, while more logical, adds an unnecessary additional check for that special value.

Ideally I would like to see a test that includes 4294967296 as well, and also test cases for floating pointer numbers such as "0.0" and "0.1" to make sure that "0.0" correctly deletes the zero element and "0.1" correctly does not.

I think the patch is fine as is, but I'm going to say review- because of the two minor style issues that check-webkit-style will no doubt catch as well. You could also consider my comments. Sorry for giving you conflicting advice.

[Kent Hansen]

Created attachment 44220 [details]
Revised patch (back to basics) and more test cases

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44220 [details] without any errors.

[Kent Hansen]

(In reply to comment #7)
> Looking at the test and test results I now realize that there is no detectable
> behavior change from what I asked you to do in the last patch, since indices
> that are beyond of the length of the string are treated no differently from
> identifiers that are not valid indices. Thus the special value that is a UInt32
> but not a valid index doesn't actually need special handling. That means that
> using toStrictUInt32 was already acceptable and even slightly more efficient.
> Using toIndex, while more logical, adds an unnecessary additional check for
> that special value.

Right. Looking at the ECMA algorithm, using toStrictUInt32 should be sufficient; there isn't any side-effects even if we incorrectly fall through the initial "is array index" test when the input is 4294967295 since, as you noted, the subsequent bounds check will catch it.

I've reverted back to the initial patch. It irks me a bit that the "isStrictUInt32 and index is in range" check is done in three places now, but I'll leave any attempts at refactoring out of this patch. (By the way, the style checker only reported "code inside namespace should not be indented", which I disregarded since the rest of the functions in JSString.h are indented.)

> 
> Ideally I would like to see a test that includes 4294967296 as well, and also
> test cases for floating pointer numbers such as "0.0" and "0.1" to make sure
> that "0.0" correctly deletes the zero element and "0.1" correctly does not.

I've added these, and also a test for '0.0' (to make sure it's not treated as an array index).

[Kent Hansen]

(In reply to comment #10)
> I've reverted back to the initial patch. It irks me a bit that the
> "isStrictUInt32 and index is in range" check is done in three places now, but
> I'll leave any attempts at refactoring out of this patch. (By the way, the
> style checker only reported "code inside namespace should not be indented",
> which I disregarded since the rest of the functions in JSString.h are
> indented.)

Actually, I created a patch where deleteProperty() calls getOwnPropertyDescriptor(), and checks the configurable() attribute of that one. That's how the ECMA spec says the delete operator should behave, so e.g. if I implemented my own custom JSobject type I would expect that virtual method to be called. It also gets rid of code duplication. But then I realised there's a bit more overhead in calling that function (JSString::getStringPropertyDescriptor() creates a jsSingleCharacterSubstring), so I decided against it.

[Simon Hausmann]

Comment on attachment 44220 [details]
Revised patch (back to basics) and more test cases

Kent is not a committer yet. I'm pretty sure he meant to set cq :)

[WebKit Commit Bot]

Comment on attachment 44220 [details]
Revised patch (back to basics) and more test cases

Clearing flags on attachment: 44220

Committed r51724: <http://trac.webkit.org/changeset/51724>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.