Bug 46324

Summary: REGRESSION: page reload on back button after history.pushState with appearing/disappearing iframes
Product: WebKit Reporter: Mihai Parparita <mihaip@chromium.org>
Component: HistoryAssignee: Mihai Parparita <mihaip@chromium.org>
Status: RESOLVED FIXED    
Severity: Normal CC: ap@webkit.org, beidson@apple.com, commit-queue@webkit.org, fishd@chromium.org, msaboff@apple.com, rniwa@webkit.org, zr@facebook.com
Priority: P1 Keywords: NeedsRadar, Regression
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Mac OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch none

Description From 2010-09-22 17:48:27 PST
I don't have this quite reduced yet, but to reproduce:

1. Load http://www.facebook.com/mihai.parparita (must be logged in to Facebook, wait till the spinner stop loading)
2. Click on the "Info" tab
3. Observe the location changes to http://www.facebook.com/mihai.parparita?v=info (via pushState)
4. Press back

Instead of a popstate event firing, the whole page reloads. I narrowed this down to http://trac.webkit.org/changeset/66238/ (via a local revert). It looks like Facebook is changing window.name at some point.

Bug 45969 may be related.
------- Comment #1 From 2010-09-22 18:01:34 PST -------
http://crbug.com/56591/
------- Comment #2 From 2010-09-22 18:38:20 PST -------
I believe this is a reduction: http://persistent.info/webkit/test-cases/reload-on-back/container.html

ToT WebKit ends up reloading the page when going back (which restarts the test). The window.name changing appeared to be a red herring. r66238 did change the criteria for when we consider the framesets of two HistoryItem trees equivalent, so the replacing of the iframe makes sense as the trigger (the Facebook profile URL referenced has two iframes when the Wall tab is selected, and one when the Info one is chosen).
------- Comment #3 From 2010-09-22 18:56:19 PST -------
Actually, http://persistent.info/webkit/test-cases/reload-on-back/container2.html is probably a better reduction (with the page starting with two iframes and one being removed). That's because, looking at http://www.facebook.com/mihai.parparita, it has two iframes:

<iframe class=​"hidden_elem" id=​"u386716_5">​
<iframe src=​"http:​/​/​static.ak.facebook.com/​common/​redirectiframe.html" name=​"profileiframe" class=​"UIComposer_Upload_Iframe">

While http://www.facebook.com/mihai.parparita?v=info has just 1:

<iframe class=​"hidden_elem" id=​"u386716_5">​

The hidden_elem iframe persists between the tab change (I added a custom property to it and it was still there, so it wasn't being recreated).
------- Comment #4 From 2010-09-22 19:00:20 PST -------
I wanted to add that I'm experiencing the same issue when going to a profile, clicking a link to transition to another profile (this will happen via an AJAX transition/pushState), and then hitting back.
------- Comment #5 From 2010-09-25 12:36:49 PST -------
The specific change that triggered this regression is the change from comparing the document sequence number of just the top frame to doing it recursively for all frames:

http://trac.webkit.org/changeset/66238/trunk/WebCore/loader/FrameLoader.cpp

Since frames are being added/removed, this check fails and we're not in the "same document" codepath anymore.

I'm looking into which layout tests break if that change is reverted.
------- Comment #6 From 2010-09-25 14:36:59 PST -------
Created an attachment (id=68838) [details]
Patch
------- Comment #7 From 2010-09-25 14:42:51 PST -------
The attached patch fixes navigation between profile tabs. For profile-to-profile navigation, I can't seem to trigger Quickling in the first place. Zach, is that disabled on Facebook's end for Safari's user agent? It does seem to work correctly if I spoof Chrome's though.

Brady and/or Darin: do you have any better ideas for how to trigger same-document navigation when using pushState, but still do the right thing when navigating between framesets (where only some of the child frames may be different). The patch is my attempt to get both fast/history/history-subframe-with-name.html and fast/history/same-document-iframes-changing.html passing.
------- Comment #8 From 2010-09-26 21:04:25 PST -------
Mihai: Yeah, AJAX transitions for profile-to-profile/whole-site navigation are currently disabled for Safari's UA, but testing by spoofing Chrome's should work fine.
------- Comment #9 From 2010-09-26 21:09:43 PST -------
(From update of attachment 68838 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=68838&action=review

> LayoutTests/fast/history/same-document-iframes-changing.html:25
> +onload = function()

Generally, we prefer to use "window.onload =" so that we can run these tests in other browsers.

> WebCore/loader/FrameLoader.cpp:3240
> -    bool sameDocumentNavigation = currentItem && item != currentItem
> -        && item->hasSameDocuments(currentItem);
> +    bool sameDocumentNavigation = currentItem && item != currentItem;
> +    if (currentItem->stateObject() || item->stateObject())
> +        sameDocumentNavigation = sameDocumentNavigation && item->documentSequenceNumber() == currentItem->documentSequenceNumber();
> +    else
> +        sameDocumentNavigation = sameDocumentNavigation && item->hasSameDocuments(currentItem);

I'm not that familiar with this code.  Why is this work done by the FrameLoader and not by the HistoryItem?
------- Comment #10 From 2010-09-27 20:56:45 PST -------
Created an attachment (id=69016) [details]
Patch
------- Comment #11 From 2010-09-27 20:58:18 PST -------
(In reply to comment #9)
> > LayoutTests/fast/history/same-document-iframes-changing.html:25
> > +onload = function()
> 
> Generally, we prefer to use "window.onload =" so that we can run these tests in other browsers.

As far as I can tell (test case at http://persistent.info/webkit/test-cases/assign-to-onload.html) assigning to onload works in Firefox, Opera and IE.

> > WebCore/loader/FrameLoader.cpp:3240
> I'm not that familiar with this code.  Why is this work done by the FrameLoader and not by the HistoryItem?

It used to be much simpler. Agreed that moving it to HistoryItem makes sense. Got any name suggestions for shouldDoSameDocumentNavigationTo?

Latest patch has the above change. Also added a test for fragment changes (and tweaked the code to make it work for that too).
------- Comment #12 From 2010-09-29 12:25:08 PST -------
(From update of attachment 69016 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=69016&action=review

> WebCore/history/HistoryItem.cpp:494
> +// - The other item corresponds to the same document (for history entries created via pushState or fragment changes).

this comment doesn't seem to really match the code.  based on this comment, i expected to see hasSameDocumentTree called in cases where the documentSequenceNumbers differ.
------- Comment #13 From 2010-09-29 12:54:13 PST -------
(In reply to comment #12)
> > WebCore/history/HistoryItem.cpp:494
> > +// - The other item corresponds to the same document (for history entries created via pushState or fragment changes).
> 
> this comment doesn't seem to really match the code.  based on this comment, i expected to see hasSameDocumentTree called in cases where the documentSequenceNumbers differ.

Since hasSameDocumentTree starts by comparing the documentSequenceNumbers of the items, there's no point in calling it if the numbers differ (it will still return false).
------- Comment #14 From 2010-09-29 17:54:48 PST -------
(From update of attachment 69016 [details])
I'm going to let the cq land this, so that it can be merged into Chrome M7. Darin, let me know if you have suggestions for how the comment could be made clearer.
------- Comment #15 From 2010-09-29 20:07:16 PST -------
(From update of attachment 69016 [details])
Clearing flags on attachment: 69016

Committed r68742: <http://trac.webkit.org/changeset/68742>
------- Comment #16 From 2010-09-29 20:07:28 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #17 From 2010-11-23 21:19:36 PST -------
*** Bug 46310 has been marked as a duplicate of this bug. ***