Summary: | REGRESSION (r39845): Assertion failure in -[WebHistoryItem dictionaryRepresentation] when archiving a submission to about:blank | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexey Proskuryakov <ap> | ||||||||
Component: | History | Assignee: | Alexey Proskuryakov <ap> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | beidson | ||||||||
Priority: | P1 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Mac | ||||||||||
OS: | OS X 10.5 | ||||||||||
Attachments: |
|
Description
Alexey Proskuryakov
2009-02-19 07:08:11 PST
Created attachment 27791 [details]
test case (assertion failure)
Created attachment 27792 [details]
proposed fix
Comment on attachment 27792 [details] proposed fix > - entryPrivate->setLastVisitWasHTTPNonGet(!equalIgnoringCase(httpMethod, "GET")); > + entryPrivate->setLastVisitWasHTTPNonGet(!equalIgnoringCase(httpMethod, "GET") && equalIgnoringCase(url.protocol(), "http")); Should we be using protocolIs instead of equalIgnoringCase here? (In reply to comment #3) > Should we be using protocolIs instead of equalIgnoringCase here? Indeed, we should. I even checked KURL.h, but somehow only noticed the standalone version, which is not appropriate here! I'm also wondering if HTTPS should be allowed here, even though the function is named setLastVisitWasHTTPNonGet(). Comment on attachment 27792 [details]
proposed fix
I'm not near a debug build right now, so I can't try to reproduce this, but there's no stack trace in this bug so I can't even see what the assertion failure is.
"GET" vs non-"GET" (POST) is both an HTTP and HTTPS construct, and the history items *were* meant to store that flag for both http and https urls.
Created attachment 27828 [details]
allow https, too
The failing assertion is:
if (coreItem->lastVisitWasHTTPNonGet()) {
ASSERT(coreItem->urlString().startsWith("http:", false) || coreItem->urlString().startsWith("https:", false));
Comment on attachment 27828 [details]
allow https, too
Since the Windows code is working with a KURL it may as well use protocolInHTTPFamily.
I don't know what a protocol "family" is. This bug is all about being inaccurate with those families, so I chose to match the check in assertion exactly. Comment on attachment 27828 [details] allow https, too > if ([method length]) > - item->setLastVisitWasHTTPNonGet([method caseInsensitiveCompare:@"GET"]); > + item->setLastVisitWasHTTPNonGet([method caseInsensitiveCompare:@"GET"] && (![[url scheme] caseInsensitiveCompare:@"http"] || ![[url scheme] caseInsensitiveCompare:@"https"])); It doesn't make sense to me that some of this is in an if statement and some of this is in the argument to the set function. We should either do it all one way or all the other. I also don't like multiple call sites that all include the "http"/"https" pair and would really like to see a central function for this. Alexey, I know you don't like the term "HTTP family", but I'd prefer a function with a less concrete name than HTTPAndHTTPS. r=me as-is, though, despite those comments Committed revision 41256. |