| Summary: | Assertion failure at WebCore/platform/network/CredentialStorage.cpp:85. | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Yongjun Zhang <yongjun_zhang> | ||||||||||
| Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | abarth, ap, commit-queue, dbates, ddkilzer, eric, webkit.review.bot, yongjun_zhang, yongjun.zhang | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | All | ||||||||||||
| OS: | All | ||||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=153250 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Yongjun Zhang
2010-08-23 16:05:59 PDT
Created attachment 65171 [details]
Attach the callstack when assertion failure happens.
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.
Created attachment 65189 [details]
remove redundant forward slashes. (add new line at end of test file).
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. 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.
(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. 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. Created attachment 65307 [details]
Remove the assertion because it is legal to have multiple trailing slashes after the path component.
Comment on attachment 65307 [details]
Remove the assertion because it is legal to have multiple trailing slashes after the path component.
OK
(In reply to comment #9) > (From update of attachment 65307 [details]) > OK Thanks! 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> http://trac.webkit.org/changeset/66005 might have broken SnowLeopard Intel Release (Tests) No way, all it did was remove an assertion. (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. :-/ This looks landed. |