Steps to reproduce: 1. Open the attached test case. 2. Close the window. 3. Wait a little to see the assertion fail.
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.