Bug 46324 - REGRESSION: page reload on back button after history.pushState with appearing/disappearing iframes
: REGRESSION: page reload on back button after history.pushState with appearing...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: History
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P1 Normal
Assigned To: Mihai Parparita
: NeedsRadar, Regression
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-09-22 17:48 PDT by Mihai Parparita
Modified: 2010-11-23 21:19 PST (History)
7 users (show)

See Also:


Attachments
Patch (6.99 KB, patch)
2010-09-25 14:36 PDT, Mihai Parparita
no flags Details | Formatted Diff | Diff
Patch (14.64 KB, patch)
2010-09-27 20:56 PDT, Mihai Parparita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mihai Parparita 2010-09-22 17:48:27 PDT
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 Ryosuke Niwa 2010-09-22 18:01:34 PDT
http://crbug.com/56591/
Comment 2 Mihai Parparita 2010-09-22 18:38:20 PDT
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 Mihai Parparita 2010-09-22 18:56:19 PDT
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 Zach Rait 2010-09-22 19:00:20 PDT
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 Mihai Parparita 2010-09-25 12:36:49 PDT
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 Mihai Parparita 2010-09-25 14:36:59 PDT
Created attachment 68838 [details]
Patch
Comment 7 Mihai Parparita 2010-09-25 14:42:51 PDT
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 Zach Rait 2010-09-26 21:04:25 PDT
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 Adam Barth 2010-09-26 21:09:43 PDT
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?
Comment 10 Mihai Parparita 2010-09-27 20:56:45 PDT
Created attachment 69016 [details]
Patch
Comment 11 Mihai Parparita 2010-09-27 20:58:18 PDT
(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 Darin Fisher (:fishd, Google) 2010-09-29 12:25:08 PDT
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.
Comment 13 Mihai Parparita 2010-09-29 12:54:13 PDT
(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 Mihai Parparita 2010-09-29 17:54:48 PDT
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.
Comment 15 WebKit Commit Bot 2010-09-29 20:07:16 PDT
Comment on attachment 69016 [details]
Patch

Clearing flags on attachment: 69016

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