Bug 24024 – REGRESSION (r39845): Assertion failure in -[WebHistoryItem dictionaryRepresentation] when archiving a submission to about:blank

• Status: RESOLVED
• Resolution: FIXED
• Component: History

[Alexey Proskuryakov]

Steps to reproduce:
1. Open the attached test case.
2. Close the window.
3. Wait a little to see the assertion fail.

[Alexey Proskuryakov]

Created attachment 27791 [details]
test case (assertion failure)

[Alexey Proskuryakov]

Created attachment 27792 [details]
proposed fix

[Adam Roben (:aroben)]

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?

[Alexey Proskuryakov]

(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!

[Alexey Proskuryakov]

I'm also wondering if HTTPS should be allowed here, even though the function is named setLastVisitWasHTTPNonGet().

[Brady Eidson]

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.

[Alexey Proskuryakov]

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));

[Mark Rowe (bdash)]

Comment on attachment 27828 [details]
allow https, too

Since the Windows code is working with a KURL it may as well use protocolInHTTPFamily.

[Alexey Proskuryakov]

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.

[Darin Adler]

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

[Alexey Proskuryakov]

Committed revision 41256.