Bug 24024 - REGRESSION (r39845): Assertion failure in -[WebHistoryItem dictionaryRepresentation] when archiving a submission to about:blank
Summary: REGRESSION (r39845): Assertion failure in -[WebHistoryItem dictionaryRepresen...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: History (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P1 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-02-19 07:08 PST by Alexey Proskuryakov
Modified: 2009-02-26 10:31 PST (History)
1 user (show)

See Also:


Attachments
test case (assertion failure) (369 bytes, text/html)
2009-02-19 07:08 PST, Alexey Proskuryakov
no flags Details
proposed fix (2.86 KB, patch)
2009-02-19 07:17 PST, Alexey Proskuryakov
beidson: review-
Details | Formatted Diff | Diff
allow https, too (2.92 KB, patch)
2009-02-20 03:37 PST, Alexey Proskuryakov
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2009-02-19 07:08:11 PST
Steps to reproduce:
1. Open the attached test case.
2. Close the window.
3. Wait a little to see the assertion fail.
Comment 1 Alexey Proskuryakov 2009-02-19 07:08:43 PST
Created attachment 27791 [details]
test case (assertion failure)
Comment 2 Alexey Proskuryakov 2009-02-19 07:17:12 PST
Created attachment 27792 [details]
proposed fix
Comment 3 Adam Roben (:aroben) 2009-02-19 08:08:48 PST
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?
Comment 4 Alexey Proskuryakov 2009-02-19 09:52:35 PST
(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!
Comment 5 Alexey Proskuryakov 2009-02-19 23:52:52 PST
I'm also wondering if HTTPS should be allowed here, even though the function is named setLastVisitWasHTTPNonGet().
Comment 6 Brady Eidson 2009-02-20 00:05:56 PST
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.
Comment 7 Alexey Proskuryakov 2009-02-20 03:37:41 PST
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 8 Mark Rowe (bdash) 2009-02-23 01:21:28 PST
Comment on attachment 27828 [details]
allow https, too

Since the Windows code is working with a KURL it may as well use protocolInHTTPFamily.
Comment 9 Alexey Proskuryakov 2009-02-23 01:40:17 PST
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 10 Darin Adler 2009-02-26 09:45:08 PST
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
Comment 11 Alexey Proskuryakov 2009-02-26 10:31:25 PST
Committed revision 41256.