Bug 44461 – Assertion failure at WebCore/platform/network/CredentialStorage.cpp:85.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Yongjun Zhang]

WebCore hits assertion failure at /WebCore/platform/network/CredentialStorage.cpp:85 when the url has double slash as folder separator.  For example, "http://foo.com/bar//test.htm".

[Yongjun Zhang]

Created attachment 65171 [details]
Attach the callstack when assertion failure happens.

[Yongjun Zhang]

Created attachment 65188 [details]
remove redundant forward slashes.

 Remove redundant forward slashes in directoryURL even though multiple slashes are valid in URL.  This fixes the assertion failure and possible protectionSpace mapping failure when two URLs differ only on redundant slashes and they are essentially the same.

[Yongjun Zhang]

Created attachment 65189 [details]
remove redundant forward slashes. (add new line at end of test file).

[Alexey Proskuryakov]

Comment on attachment 65189 [details]
remove redundant forward slashes. (add new line at end of test file).

> even though multiple slashes are valid in URL

I'm not really sure on the status of this validity. In file paths, multiple slashes always mean the same as a single one, but I think that in URLs, it's up to the server to make a decision. This thought is what has been preventing from fixing this myself.

I guess it's fine for us to do whatever Firefox is doing, and if it normalizes slashes before looking up a directory for stored credentials, so could we. But this patch doesn't quite implement that, as it only removes trailing slashes.

 +        while (index > 0 && directoryURL[index-1] == '/')

There should be spaces around "-".

> +            index--;

There is no practical difference for integer values, but for consistency with iterators, it's best to always use prefix increment and decrement. When the compiler cannot optimize it, postfix variants create temporary copies.

> +    xhr.open("GET", "resources/remember-bad-password//count-failures.php", false);

Ideally, the test would check the response to make sure that mapping doesn't fail (and it should pass in Firefox).

r=me.

[Alexey Proskuryakov]

Comment on attachment 65189 [details]
remove redundant forward slashes. (add new line at end of test file).

I expected at least the style issue to be fixed.

[Yongjun Zhang]

(In reply to comment #5)
> (From update of attachment 65189 [details])
> I expected at least the style issue to be fixed.

sorry, I misread your comments.  New patch coming.

[Yongjun Zhang]

I did some test with the assertion removed, and it turns out the assumption in my original patch is wrong.  For a URL with double-slash path, it will create another entry in PathToDefaultProtectionSpaceMap along with the URL with single-slash path.  So the query in HashMap will fail.  But it doesn't affect the authentication because http servers will handle it correctly.

Since it is perfectly legal to have multiple slash following the path, I believe we should remove the assertion or come up with a new one which accepts trailing slashes.

[Yongjun Zhang]

Created attachment 65307 [details]
Remove the assertion because it is legal to have multiple trailing slashes after the path component.

[Alexey Proskuryakov]

Comment on attachment 65307 [details]
Remove the assertion because it is legal to have multiple trailing slashes after the path component.

OK

[Yongjun Zhang]

(In reply to comment #9)
> (From update of attachment 65307 [details])
> OK

Thanks!

[WebKit Commit Bot]

Comment on attachment 65307 [details]
Remove the assertion because it is legal to have multiple trailing slashes after the path component.

Clearing flags on attachment: 65307

Committed r66005: <http://trac.webkit.org/changeset/66005>

[WebKit Review Bot]

http://trac.webkit.org/changeset/66005 might have broken SnowLeopard Intel Release (Tests)

[Alexey Proskuryakov]

No way, all it did was remove an assertion.

[Adam Barth]

(In reply to comment #13)
> No way, all it did was remove an assertion.

The SL bots are super flaky.  There are tests that fail on one of the slaves but not the other.  :-/

[Eric Seidel (no email)]

This looks landed.