Bug 40660 – REGRESSION (r61207): Navigation via -[WebView goToBackForwardItem:] no longer works

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

[Mark Rowe (bdash)]

Created attachment 58861 [details]
Modified MiniBrowser to demonstrate problem

After r61207 calls to -[WebView goToBackForwardItem:] do not appear to initiate loads.  I’ve attached a version of Apple’s MiniBrowser sample code that uses -goToBackForwardItem: to perform the initial load.  When run against shipping WebKit it loads apple.com as expected.  When run against r61207 the WebView remains empty.

<rdar://problem/8097740>

[Mark Rowe (bdash)]

Created attachment 58862 [details]
Diff showing modification to MiniBrowser

[Brady Eidson]

The key change can be seen here:
http://trac.webkit.org/changeset/61207#file10 line 553

In HistoryController::recursiveGoToItem() 

    // If the item we're going to is a clone of the item we're at, then do
    // not load it again, and continue history traversal to its children.
    // The current frame tree and the frame tree snapshot in the item have
    // to match.
    if (item->itemSequenceNumber() == fromItem->itemSequenceNumber()
        ... // do some stuff with subframes but don't load in the current frame

In the minibrowser example attached,  the WebView is navigated to the same historyitem.  So, of course, the item and fromItem both have the same document sequence number because they are the same item.

Even if it wasn't explicit, the previous behavior was definitely to force the load when going to the same item.  We need to restore that.

[Darin Fisher (:fishd, Google)]

FrameLoader::loadItem has a check that item != currentItem.  It uses that to determine that navigateToDifferentDocument should be called instead of navigateWithinDocument.  We can certainly replicate that same check in HistoryController::recursiveGoToItem.  It seems logically that they should be similar.

[Darin Fisher (:fishd, Google)]

Notice that ScheduledHistoryNavigation (in RedirectScheduler.cpp) special cases go(0) to call FrameLoader::urlSelected instead of Page::goBackOrForward.  This explains why we don't see any problems with history.go(0).

Anyways, I'll put together the patch to HistoryController.cpp as mentioned above.

[Brady Eidson]

(In reply to comment #3)
> FrameLoader::loadItem has a check that item != currentItem.  It uses that to determine that navigateToDifferentDocument should be called instead of navigateWithinDocument.  We can certainly replicate that same check in HistoryController::recursiveGoToItem.  It seems logically that they should be similar.

I agree FrameLoader::loadItem has that check, but the code path we're talking about in this scenario is:


#0	0x10295eb7a in WebCore::HistoryController::recursiveGoToItem at HistoryController.cpp:537
#1	0x10295f024 in WebCore::HistoryController::goToItem at HistoryController.cpp:223
#2	0x102dc2410 in WebCore::Page::goToItem at Page.cpp:315
#3	0x1020d61a0 in -[WebView goToBackForwardItem:] at WebView.mm:3174

[Brady Eidson]

(In reply to comment #5)
> (In reply to comment #3)
> > FrameLoader::loadItem has a check that item != currentItem.  It uses that to determine that navigateToDifferentDocument should be called instead of navigateWithinDocument.  We can certainly replicate that same check in HistoryController::recursiveGoToItem.  It seems logically that they should be similar.
> 
> I agree FrameLoader::loadItem has that check, but the code path we're talking about in this scenario is:
> 
> 
> #0    0x10295eb7a in WebCore::HistoryController::recursiveGoToItem at HistoryController.cpp:537
> #1    0x10295f024 in WebCore::HistoryController::goToItem at HistoryController.cpp:223
> #2    0x102dc2410 in WebCore::Page::goToItem at Page.cpp:315
> #3    0x1020d61a0 in -[WebView goToBackForwardItem:] at WebView.mm:3174

Sorry, I misread your original comment.  You seemed to agree completely that this path should be similar. 

Thanks!  :)

[Darin Fisher (:fishd, Google)]

(In reply to comment #6)
> Sorry, I misread your original comment.  You seemed to agree completely that this path should be similar. 

Posting a patch shortly.  Sorry for not being able to get to it yesterday.

[Darin Fisher (:fishd, Google)]

Created attachment 59007 [details]
v1 patch

Unfortunately, this bug cannot be replicated with HTML+JS, so I have not included
any kind of regression test.

[Brady Eidson]

Comment on attachment 59007 [details]
v1 patch

Looks good.  No worries for not being yesterday, I appreciate the fix!

[Darin Fisher (:fishd, Google)]

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