Bug 19422 - Distinct redirects from the same link do not create distinct history items
Summary: Distinct redirects from the same link do not create distinct history items
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: History (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nobody
URL: http://catandgirl.com/
Keywords: NeedsReduction
: 7814 (view as bug list)
Depends on:
Reported: 2008-06-06 17:48 PDT by Cameron Zwarich (cpst)
Modified: 2010-06-11 11:11 PDT (History)
4 users (show)

See Also:

Proposed patch (5.53 KB, patch)
2008-06-10 19:04 PDT, Cameron Zwarich (cpst)
beidson: review-
Details | Formatted Diff | Diff
Test case (4.24 KB, patch)
2008-06-12 00:41 PDT, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cameron Zwarich (cpst) 2008-06-06 17:48:02 PDT
If you click "Random Comic" on http://catandgirl.com/ and then click it again, it does not create a new history item. Thus, going back goes back the front page, no matter how many random comics were viewed. I suspect this is because the random comic link is the same on each page, but redirects the user to a different page each time.
Comment 1 Cameron Zwarich (cpst) 2008-06-10 01:39:36 PDT
The problem is that shouldTreatURLAsSameAsCurrent() is saying that the "random comic" link is the same as the current link. The check in shouldTreatURLAsSameAsCurrent() is

    return url == m_currentHistoryItem->url() || url == m_currentHistoryItem->originalURL();

If I remove the second part of this test, then the bug is fixed, with two caveats:

1) There is a back/forward history item, but no item in the history item.

2) If I click the link while it is redirecting, it seems I can make URLs appear in the history of the form "http://cantandgirl.com./..." with a period after ".com". The URL works, but I am pretty sure it isn't the URL being given by the server.

The check has been there at least since revision 2963:


If no one responds with a reason why this change is obviously a bad idea, I will just submit a patch with test cases for review.
Comment 2 Cameron Zwarich (cpst) 2008-06-10 19:04:02 PDT
Created attachment 21615 [details]
Proposed patch

Here it is, with a test case. The problem with the History menu isn't fixed, but I can work on that next if this change is acceptable.
Comment 3 Cameron Zwarich (cpst) 2008-06-10 23:07:24 PDT
*** Bug 7814 has been marked as a duplicate of this bug. ***
Comment 4 Brady Eidson 2008-06-11 10:10:16 PDT
Comment on attachment 21615 [details]
Proposed patch

So, I *think* this might break history with certain types of redirect chains.

Imagine visiting URL A, which redirects to URL B, which redirects back to A.  With this patch, I think the single navigation would result in 2 history items, which isn't correct.

I discussed this with Cameron on irc, and I he's trying to write a test to replicate my theoretical scenario
Comment 5 Cameron Zwarich (cpst) 2008-06-12 00:41:08 PDT
Created attachment 21654 [details]
Test case

Here is a test case for the existing behaviour. It passes with my patch applied. I put it up for review so I can land the test before working on this bug some more.

I am only dumping the back/forward list; is there any way to also dump global history? Since that is one of the issues with this bug, I would like to be able to test it.

Interestingly enough, this test case seems to expose a bug in WebKit. If I add a layoutTestController.queueBackNavigation(1) to the final page, it performs no navigation. Should I file this bug separately?
Comment 6 Darin Adler 2008-06-13 14:12:43 PDT
Comment on attachment 21654 [details]
Test case

Comment 7 Cameron Zwarich (cpst) 2008-06-13 15:34:05 PDT
Landed in r34525.
Comment 8 Cameron Zwarich (cpst) 2008-06-24 14:40:09 PDT
Comment on attachment 21654 [details]
Test case

Clearing review.
Comment 9 Graham Perrin 2008-11-01 19:51:04 PDT
Alongside this bug 19422 I am reading 

> Clicking on redirect link stores wrong entry in history
Comment 10 Alexey Proskuryakov 2010-06-11 11:11:30 PDT
This is still an issue in Safari 5. Brady, do you still think that the patch is incorrect, given the test case?