REGRESSION: page reload on back button after history.pushState with appearing/disappearing iframes

[Mihai Parparita]

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.

[Ryosuke Niwa]

http://crbug.com/56591/

[Mihai Parparita]

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

[Mihai Parparita]

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

[Zach Rait]

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.

[Mihai Parparita]

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.

[Mihai Parparita]

Created attachment 68838 [details]
Patch

[Mihai Parparita]

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.

[Zach Rait]

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.

[Adam Barth]

Comment on attachment 68838 [details]
Patch

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?

[Mihai Parparita]

Created attachment 69016 [details]
Patch

[Mihai Parparita]

(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).

[Darin Fisher (:fishd, Google)]

Comment on attachment 69016 [details]
Patch

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.

[Mihai Parparita]

(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).

[Mihai Parparita]

Comment on attachment 69016 [details]
Patch

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.

[WebKit Commit Bot]

Comment on attachment 69016 [details]
Patch

Clearing flags on attachment: 69016

Committed r68742: <http://trac.webkit.org/changeset/68742>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Mihai Parparita]

*** Bug 46310 has been marked as a duplicate of this bug. ***