Bug 40451 – history.back() example that works in a main frame breaks in an iframe

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

[Brady Eidson]

I don't have the example handy, but the developer I'm working with right now will attach a fantastic reduction later.

If an application navigates using a combination of anchors to move forward, and history.back() to go backwards, we run into a problem when it's hosted in an iframe.

When the user has history.back()'ed all the way to the site right after the original # anchor, the first call to history.back() doesn't return to the original page (visibly, anyways).

A second click on history.back() goes to the page before the web app was ever visited.

I can post a proof of concept soon, but the original developer will also attach his reduction in a few days.

[Jean-Marc Trinon]

Created attachment 58425 [details]
reduction example


test.html contains a web page that navigates through different elements in an "iPhonish" style. 

Going to previous level is done by clicking button at the left of the navigation bar at the top.
 
 iframe.html loads test.html in an iframe.

When in the iframe, it's impossible to navigate back to the top level element.

[Brady Eidson]

Note that this example is broken in the way described in both ToT WebKit, Safari 5, and Chrome 5, but works fine in Firefox.

[Brady Eidson]

I haven't had a chance to scour the HTML5 spec yet, but I wonder if changes to support things like the hashchange event and push/pop state actually specify that our behavior is correct.

[Darin Fisher (:fishd, Google)]

Note: history.back() is not really relevant.  The same issue occurs if you click the back button in the browser.

[Brady Eidson]

Of course - history.back() is defined to be the same as clicking the back button.

Programmatically or manually, "going back" in general has the problem  :)

[Darin Fisher (:fishd, Google)]

When embedded in an IFRAME, it seems like WebKit is failing to change the location of the IFRAME from e.g. #_settings to #_home upon navigating back.

While debugging the back navigation, one thing that is striking is that the isTargetItem property is set to the true for the topframe and not the inner frame.  That seems quite surprising.  It causes HistoryController::recursiveGoToItem to just navigate the top frame from iframe.html to iframe.html, which ends up being a no-op.  As a result, the navigation completes without doing anything.

The source of the mixed up isTargetItem flag probably will explain why we see this bug.  I wonder if the initial redirection from test.html to test.html#_home could be related.

[Darin Fisher (:fishd, Google)]

Interestingly, if I load iframe.html, click on settings, then hit reload, and then click on settings again, back/forward buttons now start working properly!

[Darin Fisher (:fishd, Google)]

So, I think this is an issue with isTargetItem not being expressive enough.  We use that to determine which frame was the target of a navigation, but that is only really applicable when navigating forward.  If you are navigating back, then what we really want to know is which frames are different from the current frame configuration.  The checks in recursiveGoToItem are not adequate.

I think we really need a HistoryItem identifier.  If a HistoryItem is really just a clone of another HistoryItem, then they should have the same identifier.  That way, during a history traversal, we'd know in recursiveGoToItem that we should not modify a frame.

[Darin Fisher (:fishd, Google)]

Created attachment 58496 [details]
reduced test case

[Darin Fisher (:fishd, Google)]

Created attachment 58498 [details]
reduced test case (v2)

[Darin Fisher (:fishd, Google)]

I have a patch based on comment #8 that fixes this bug and seems to pass existing layout tests.

[Darin Fisher (:fishd, Google)]

Created attachment 58518 [details]
v1 patch

[WebKit Review Bot]

Attachment 58518 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/loader/HistoryController.cpp:562:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Fisher (:fishd, Google)]

Created attachment 58520 [details]
v2 patch [fixed style error]

[Brady Eidson]

Comment on attachment 58520 [details]
v2 patch [fixed style error]

I like the approach here, and think it is fundamentally right.  But...

> Index: WebCore/loader/FrameLoader.cpp
> ===================================================================
> --- WebCore/loader/FrameLoader.cpp	(revision 60978)
> +++ WebCore/loader/FrameLoader.cpp	(working copy)
> @@ -3542,14 +3542,11 @@ void FrameLoader::navigateToDifferentDoc
>  void FrameLoader::loadItem(HistoryItem* item, FrameLoadType loadType)
>  {
>      // We do same-document navigation in the following cases:
> -    // - The HistoryItem has a history state object
> -    // - Navigating to an anchor within the page, with no form data stored on the target item or the current history entry,
> -    //   and the URLs in the frame tree match the history item for fragment scrolling.
> -    // - The HistoryItem is not the same as the current item, because such cases are treated as a new load.
> +    // - The HistoryItem corresponds to the same document.
> +    // - The HistoryItem is not the same as the current item.
>      HistoryItem* currentItem = history()->currentItem();
> -    bool sameDocumentNavigation = ((!item->formData() && !(currentItem && currentItem->formData()) && history()->urlsMatchItem(item))
> -                                  || (currentItem && item->documentSequenceNumber() == currentItem->documentSequenceNumber()))
> -                                  && item != currentItem;
> +    bool sameDocumentNavigation = currentItem && item != currentItem
> +        && item->documentSequenceNumber() == currentItem->documentSequenceNumber();

I'm concerned about the removal of the formData check here.  It's possible that it's not an issue because of the expressiveness of the item sequence number concept, but I haven't had enough coffee this afternoon to really think it through just by reading this patch.


> @@ -659,10 +656,6 @@ void HistoryController::pushState(PassRe
>      item->setStateObject(stateObject);
>      item->setURLString(urlString);
>  
> -    // Since the document isn't changed as a result of a pushState call, we
> -    // should preserve the DocumentSequenceNumber of the previous item.
> -    item->setDocumentSequenceNumber(m_previousItem->documentSequenceNumber());
> -    
>      page->backForwardList()->pushStateItem(item.release());
>  }

I'm not entirely clear why this change is needed.

I'm provisionally r-'ing this patch, waiting for an explanation on both of these points.  It should be pretty easy to convince me, at which point I'll r+  :)

[Darin Fisher (:fishd, Google)]

(In reply to comment #15)
> (From update of attachment 58520 [details])
> I like the approach here, and think it is fundamentally right.  But...
> 
> > Index: WebCore/loader/FrameLoader.cpp
> > ===================================================================
> > --- WebCore/loader/FrameLoader.cpp	(revision 60978)
> > +++ WebCore/loader/FrameLoader.cpp	(working copy)
> > @@ -3542,14 +3542,11 @@ void FrameLoader::navigateToDifferentDoc
> >  void FrameLoader::loadItem(HistoryItem* item, FrameLoadType loadType)
> >  {
> >      // We do same-document navigation in the following cases:
> > -    // - The HistoryItem has a history state object
> > -    // - Navigating to an anchor within the page, with no form data stored on the target item or the current history entry,
> > -    //   and the URLs in the frame tree match the history item for fragment scrolling.
> > -    // - The HistoryItem is not the same as the current item, because such cases are treated as a new load.
> > +    // - The HistoryItem corresponds to the same document.
> > +    // - The HistoryItem is not the same as the current item.
> >      HistoryItem* currentItem = history()->currentItem();
> > -    bool sameDocumentNavigation = ((!item->formData() && !(currentItem && currentItem->formData()) && history()->urlsMatchItem(item))
> > -                                  || (currentItem && item->documentSequenceNumber() == currentItem->documentSequenceNumber()))
> > -                                  && item != currentItem;
> > +    bool sameDocumentNavigation = currentItem && item != currentItem
> > +        && item->documentSequenceNumber() == currentItem->documentSequenceNumber();
> 
> I'm concerned about the removal of the formData check here.  It's possible that it's not an issue because of the expressiveness of the item sequence number concept, but I haven't had enough coffee this afternoon to really think it through just by reading this patch.

Right, I believe the documentSequenceNumber check subsumes the formdata plus
urlsMatchItem check.  Perhaps I should create a test to verify this.


> 
> 
> > @@ -659,10 +656,6 @@ void HistoryController::pushState(PassRe
> >      item->setStateObject(stateObject);
> >      item->setURLString(urlString);
> >  
> > -    // Since the document isn't changed as a result of a pushState call, we
> > -    // should preserve the DocumentSequenceNumber of the previous item.
> > -    item->setDocumentSequenceNumber(m_previousItem->documentSequenceNumber());
> > -    
> >      page->backForwardList()->pushStateItem(item.release());
> >  }
> 
> I'm not entirely clear why this change is needed.

This change removes redundancy.  The createItemTree call now sets the documentSequenceNumber.


> I'm provisionally r-'ing this patch, waiting for an explanation on both of these points.  It should be pretty easy to convince me, at which point I'll r+  :)

[Darin Fisher (:fishd, Google)]

Also, I should have at least removed the now unused code for urlsMatchItem.

[Brady Eidson]

(In reply to comment #16)
> (In reply to comment #15)
> > I'm concerned about the removal of the formData check here.  It's possible that it's not an issue because of the expressiveness of the item sequence number concept, but I haven't had enough coffee this afternoon to really think it through just by reading this patch.
> 
> Right, I believe the documentSequenceNumber check subsumes the formdata plus
> urlsMatchItem check.  Perhaps I should create a test to verify this.

(In reply to comment #17)
> Also, I should have at least removed the now unused code for urlsMatchItem.

While I was reviewing, I was searching urlsMatchItem to see if it was now unused  ;)

I'd feel a lot better about the removal of the formData check if you ran a test.  :)

[Darin Fisher (:fishd, Google)]

Created attachment 58792 [details]
v3 patch

Now with a test for submitting a form to a reference fragment.  We expect the
form submission to create a new document, and so navigating back should also
create a new document.

[Darin Fisher (:fishd, Google)]

Created attachment 58795 [details]
v4 patch

Now with the unused HistoryController::urlsMatchItem removed.

[Brady Eidson]

Comment on attachment 58795 [details]
v4 patch

Nice.

[Darin Fisher (:fishd, Google)]

Landed as http://trac.webkit.org/changeset/61207

[Mihai Parparita]

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

[Adam Barth]

Re-opening at the request of bencc on #webkit.

[Adam Barth]

See https://bugs.webkit.org/show_bug.cgi?id=53856 for an explanation.

[Darin Fisher (:fishd, Google)]

(In reply to comment #25)
> See https://bugs.webkit.org/show_bug.cgi?id=53856 for an explanation.

I read https://bugs.webkit.org/show_bug.cgi?id=53856#c4 several times, and I find it fairly confusing.  At any rate, I don't think we should be re-opening this "old" bug for a new patch.  Let's use a new bug for whatever new changes are needed.  Maybe we should use bug 53856 for that?