Bug 13422 – REGRESSION: Page reload loses page position

• Status: VERIFIED
• Resolution: FIXED
• Component: Page Loading

[Justin]

In the last several nightly builds (a few weeks, I believe), page reload behavior has changed slightly (in my opinion, it has broken).  Reloading the page usually has normally returned the user to the position on the page the user was viewing prior to reloading, but now, the page is reloaded and the very top is displayed.

I did find an open bug (http://bugs.webkit.org/show_bug.cgi?id=10223) related to seemingly sporadic reload behavior scroll position on pages with frames, but that bug is much older than this one, and this one occurs on pages without frames.

[Brady Eidson]

Oh no, this is not good!  Can you regress this using the nightlies to find out when this broke?

[Justin]

Why certainly.  It worked up through revision 17354, and stopped working in 17388.

[Brady Eidson]

Thanks!  That'll probably be a big help.

Unfortunately, I know off the top of my hand thats when major loader refactoring was going on - many large patches completely upsetting the balance.  

I'll take a look at that range

[Brady Eidson]

Yup, my fears confirmed
http://trac.webkit.org/projects/webkit/changeset/17365 seems to be an extremely likely culrpit, though a few others in that range might be as well...

These "moving and renaming" patches often do more than just that (unintentionally of course)

[Darin Adler]

<rdar://problem/5153026>

[Cameron Zwarich (cpst)]

I can confirm that this regression is indeed caused by r17365. I will try to hunt down the exact change that causes it and hopefully come up with a patch.

[Cameron Zwarich (cpst)]

Created attachment 15846 [details]
Proposed patch

Here is a patch that fixes the bug. In revision 17365, many Objective C method bodies in WebFrame were moved to the C++ wrapper class WebFrameLoaderClient for performance reasons. However, in copying the updateHistoryForReload() method, a small error was introduced. In the original method, currItem is made the current history item of the frame:

WebHistoryItem *currItem = _private->currentItem;

In the copied version, this is changed to

WebHistoryItem *currItem = m_webFrame->_private->previousItem;

This patch corrects that change, which was carried over into the new loader code.

[Mark Rowe (bdash)]

Comment on attachment 15846 [details]
Proposed patch

This feels like it should be testable in a layout test.  It'd be handy to have a test around so we can be sure we don't reintroduce this regression in the future.

[Brady Eidson]

Comment on attachment 15846 [details]
Proposed patch

Looks like a good fix, and good catch on the mistake in the patch that caused the original regression.

r- because there is no layout test and this should be layout-testable

We'd be happy to help you write one (#webkit on irc.freenode.net) but you can also refer to the other layout tests for guidance

[Cameron Zwarich (cpst)]

Created attachment 15863 [details]
Proposed patch with test case

Here is the patch with a test case. It turns out that the success200-reload test was designed to scroll the document and record the change, but nobody ever checked if it scrolled in the test output when it was made. I always run the layout tests after adding a fix, so I must have accidentally been running them for the older version of the tree I was using to find the bug.

[Brady Eidson]

Comment on attachment 15863 [details]
Proposed patch with test case

HAH.  Very cool.

[Brady Eidson]

Landed in http://trac.webkit.org/projects/webkit/changeset/24934

[Justin]

Thanks all, I've confirmed that this is working now.  It's not clear to me whether I'm supposed to change the status, but I consider the bug fixed.

[Brady Eidson]

Mark it as verified, since you've VERIFIED it is fixed   =D

[Justin]

Verified :)

[Cameron Zwarich (cpst)]

I should note that this bug was actually noticed the day it was introduced by the person who introduced it:

http://trac.webkit.org/projects/webkit/changeset/17368

We should probably be more careful to check changed test results in the future.

[Darin Adler]

(In reply to comment #16)
> I should note that this bug was actually noticed the day it was introduced by
> the person who introduced it:
> 
> http://trac.webkit.org/projects/webkit/changeset/17368
> 
> We should probably be more careful to check changed test results in the future.

My bad!