[Webkit-unassigned] [Bug 131704] Simple ES6 feature:String prototype additions

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=131704





--- Comment #32 from Diego Pino <dpino at igalia.com>  2014-09-02 03:14:00 PST ---
(From update of attachment 237340)
View in context: https://bugs.webkit.org/attachment.cgi?id=237340&action=review

>> Source/JavaScriptCore/runtime/StringPrototype.cpp:1565
>> +        return JSValue::encode(jsUndefined());
> 
> Is the ordering right here? Does this need to be done before the checking of the argument?

Do you mean to check if there were an Exception right after calling toString(exec)? Reading other snippets of code in the same file what it seems to be the general pattern is to verify if an exception was raised after calling to "toString(exec)->value(exec)", for example StringPrototype::replaceUsingStringSearch(), and others.

> String searchString = searchValue.toString(exec)->value(exec);
> if (exec->hadException())
>    return JSValue::encode(jsUndefined());

>> Source/JavaScriptCore/runtime/StringPrototype.cpp:1572
>> +        return JSValue::encode(jsBoolean(false));
> 
> This is new. Is it correct? Is a null value supposed to turn into a false result or turn into the string "null"? What about undefined? What about a missing argument? What does the specification say?

No, it's not new. It has been there since the first proposed patch. In any case, good catch because this is not correct. What the specifications says is: 

Step 5. Let searchStr be ToString(searchString).

And the ToString() operation specifies that and undefined value is turned to an "undefined" string and null to string "null". So 'null'.startsWith(null) should be true and 'undefined'.startsWith() should be true too. I will add those cases to the test, plus some other more with numbers and booleans, as suggested below.

>> Source/JavaScriptCore/runtime/StringPrototype.cpp:1578
>> +        return JSValue::encode(jsUndefined());
> 
> It’s OK to include these two lines of code, but they are not needed. There is no way for the JavaScript code to detect whether we do the startsWith or not. The return value is undefined if we have an exception.

According to comment #26, I thought we were adding this check because toString(exec) may end up calling valueOf() and that might return an exception.

>> Source/WTF/wtf/text/StringImpl.cpp:1417
>> +        return false;
> 
> Here’s how we write this and avoid overflow:
> 
>     if (startOffset > length())
>         return false;
>     if (matchString.length() > length())
>         return false;
>     if (matchString.length() + startOffset > length())
>         return false;
> 
> No assertions. But also, I suggest we put these checks into equalInner rather than here. This function can just be a forwarder to equalInner.

Agree. But I think the last 'if' should check if the sum of matchString.length() + startOffset() is lower than matchString.length() or startOffset. Imagine length() is 0xFFFFFFFF, the result of matchString.length() + startOffset will be a 32-bit unsigned value too and could never be higher than 0xFFFFFFFF. When an overflow of unsigned values happens the result of the sum is lower than any of the addends. For instance, 0xFFFFFFFF + 0x1 = 0x0.

>> Source/WTF/wtf/text/StringImpl.cpp:1423
>> +        matchString.length(), caseSensitive);
> 
> This is wrong. We can’t just cast a run of 16-bit characters to a run of 8-bit characters. The code won’t compare all the characters! And it will convert individual bytes of the characters to lowercase if asked to not be case sensitive.

OK, I will look into this.

>> Source/WTF/wtf/text/StringImpl.cpp:1452
>> +    int startOffset = endOffset - matchString.length();
> 
> I am still not sure that I understand what an end offset is, so it’s really hard for me to understand what this function is trying to do.
> 
> But this is not the right way to write this to correctly range check and avoid overflow. We should instead just write this:
> 
>     if (endOffset > matchString.length())
>         return false;
>     unsigned startOffset = matchString.length() - endOffset;
> 
> Don’t involve signed arithmetic at all.
> 
> And we will need these checks:
> 
>     if (startOffset > length())
>         return false;
>     if (matchString.length() > length())
>         return false;
>     if (matchString.length() + startOffset > length())
>         return false;
> 
> But I suggest putting them inside equalInner.

endOffset is the end position of stringToSearchIn that matchString will match against. For instance:

'foo bar'.endsWith('ba', 6)  // true, if 6 is considered the last position of 'foo bar', 'ba' is matched.
'foo bar'.endsWith('ba')     // false, same as endsWith('ba', 7)

Agree with the refactoring and moving the overflow checks to equalInner().

>>> Source/WTF/wtf/text/StringImpl.cpp:1454
>>> +    ASSERT(!(endOffset <= matchString.length() ^ startOffset <= 0));
>> 
>> This reads as if 'endOffset' is greater or equals than 'matchLength', 'startOffset' should be positive or zero. On the other hand, if 'endOffset' is lower or equals than 'matchLength', 'startOffset' should be negative.
> 
> Fine, but it’s not appropriate to assert these things.

OK, I will remove them.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.