[webkit-reviews] review denied: [Bug 131704] Simple ES6 feature:String prototype additions : [Attachment 237169] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Diego Pino <dpino at igalia.com>'s
request for review:
Bug 131704: Simple ES6 feature:String prototype additions
https://bugs.webkit.org/show_bug.cgi?id=131704

Attachment 237169: Patch
https://bugs.webkit.org/attachment.cgi?id=237169&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=237169&action=review


Thanks for working so hard on this. Sorry to keep coming up with new things,
but I realized a few issues i had overlooked earlier.

Also, this overloading problem is a bad thing to introduce into the String
class. We really don’t want to avoid it by simply having mysterious additional
"true" arguments at every call site; it’s simply an ugly pattern. I think the
direction we will want to go to clean this up is that instead of adding more
functions to String, we will want to add and use
StringView::contains/startsWith/endsWith and use StringView::substring to
restrict the ranges of the searches. When we do that we will then get rid of
these new overloads we just added. I suppose it’s OK to land them like this for
now, but since I do see some difficulty here in writing those functions
correctly without overflow problems we might want to just write the StringView
ones instead.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1571
> +    String stringToSearchIn = thisValue.toString(exec)->value(exec);
> +    String matchString = a0.toString(exec)->value(exec);

After each of these lines we need:

    if (exec->hadException())
	return JSValue::encode(jsUndefined());

That’s because toString can end up calling valueOf, which can raise an
exception. It’s tricky to construct test cases that cover this, but it can be
done by constructing multiple objects with valueOf functions, one that raises
an exception and others that have side effects. The trick is that if one
valueOf function raises an exception we can’t be seen calling the second one or
the third one, since toInt32 can also end up calling valueOf. The test needs to
check that the side effects of the subsequent valueOf functions do not occur
when an earlier one raises an exception. We have very little test coverage for
this particular fine point of getting these functions right. Making them work
properly is relatively straightforward as long as we are careful to evaluate
the arguments in the correct order and return if an exception occurs. Strictly
speaking we also want to decide if thisValue is converted to a string before or
after we check the type of argument 0.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1590
> +    String stringToSearchIn = thisValue.toString(exec)->value(exec);

Needs a hadException return statement as above.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1593
> +    String matchString = a0.toString(exec)->value(exec);

Needs a hadException return statement as above.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1615
> +    String stringToSearchIn = thisValue.toString(exec)->value(exec);

Needs a hadException return statement as above.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1616
> +    String matchString = a0.toString(exec)->value(exec);

Needs a hadException return statement as above.

> Source/WTF/wtf/text/StringImpl.cpp:1405
> +    ASSERT(matchString);

This assertion is incorrect and should be removed. References can’t be null.
I’m not sure why this compiles.

> Source/WTF/wtf/text/StringImpl.cpp:1407
> +    if (matchString.length() + startOffset > length())
> +	   return false;

This can overflow. It needs to be written so it is not subject to overflow. For
example, matchString can be 1 character long and startOffset can be 0xFFFFFFFF.
Then we’ll hit the assertion inside equalInner. Please write the code in a way
that is not susceptible to overflow.

> Source/WTF/wtf/text/StringImpl.cpp:1409
> +    return equalInner(this, startOffset, reinterpret_cast<const
char*>(matchString.characters8()), 
> +	   matchString.length(), caseSensitive);

This won’t work if the string is not an 8-bit string.

> Source/WTF/wtf/text/StringImpl.cpp:1438
> +    ASSERT(matchString);

This assertion is incorrect and should be removed. References can’t be null.
I’m not sure why this compiles.

> Source/WTF/wtf/text/StringImpl.cpp:1441
> +    int startOffset = endOffset - matchString.length();
> +    if (startOffset < 0)
> +	   return false;

This can overflow. It needs to be written so it is not subject to overflow. For
example, matchString can be 1 character long and endOffset can be 0x80000000.
Then startOffset will be 0x7FFFFFFF and we’ll hit the assertion inside
equalInner. Please write the code in a way that is not susceptible to overflow.


> Source/WTF/wtf/text/StringImpl.cpp:1443
> +    return equalInner(this, startOffset, reinterpret_cast<const
char*>(matchString.characters8()), 
> +	   matchString.length(), caseSensitive);

This won’t work if the string is not an 8-bit string.

> LayoutTests/js/script-tests/string-contains.js:1
> +description("This test checks the ES6 string functions startsWith(),
endsWith() and contains().");

We need more edge cases here. These tests would miss all the problems with the
range checking overflow in the functions.