[webkit-reviews] review denied: [Bug 130894] ASSERTION FAILED: url.containsOnlyASCII() in WebCore::checkEncodedString : [Attachment 228833] WIP path

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Peter Molnar
<pmolnar.u-szeged at partner.samsung.com>'s request for review:
Bug 130894: ASSERTION FAILED: url.containsOnlyASCII() in
WebCore::checkEncodedString
https://bugs.webkit.org/show_bug.cgi?id=130894

Attachment 228833: WIP path
https://bugs.webkit.org/attachment.cgi?id=228833&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=228833&action=review


> Source/WebCore/ChangeLog:8
> +	   Instead of the ASSERT, invalidate the URL if it contains non-ASCII
characters.

This significantly changes the semantics. The purpose of the ASSERT was to
ensure that URL parsing does what it's supposed to do. The new code makes it so
that URL parsing bever does anything useful for non-ASCII input. I think that
in this case, you need to demonstrate why this is desired behavior, not ask
reviewers to investigate on their own.

What do other browsers do on this test, do they parse the URL into something
valid?

This change definitely needs regression tests.

> Source/WebCore/platform/URL.cpp:981
> +    ASSERT_UNUSED(string, string.isEmpty() || isSchemeFirstChar(string[0]));


ASSERT_UNUSED is incorrect here, the string argument is used in the function.