Bug 40660 - REGRESSION (r61207): Navigation via -[WebView goToBackForwardItem:] no longer works
Summary: REGRESSION (r61207): Navigation via -[WebView goToBackForwardItem:] no longer...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.6
: P1 Normal
Assignee: Darin Fisher (:fishd, Google)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-06-16 01:29 PDT by Mark Rowe (bdash)
Modified: 2010-06-17 10:23 PDT (History)
3 users (show)

See Also:


Attachments
Modified MiniBrowser to demonstrate problem (26.83 KB, application/zip)
2010-06-16 01:29 PDT, Mark Rowe (bdash)
no flags Details
Diff showing modification to MiniBrowser (1.54 KB, patch)
2010-06-16 01:29 PDT, Mark Rowe (bdash)
no flags Details | Formatted Diff | Diff
v1 patch (1.57 KB, patch)
2010-06-17 10:05 PDT, Darin Fisher (:fishd, Google)
beidson: review+
beidson: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Rowe (bdash) 2010-06-16 01:29:15 PDT
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>
Comment 1 Mark Rowe (bdash) 2010-06-16 01:29:50 PDT
Created attachment 58862 [details]
Diff showing modification to MiniBrowser
Comment 2 Brady Eidson 2010-06-16 12:52:57 PDT
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.
Comment 3 Darin Fisher (:fishd, Google) 2010-06-16 13:05:27 PDT
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.
Comment 4 Darin Fisher (:fishd, Google) 2010-06-16 13:09:22 PDT
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.
Comment 5 Brady Eidson 2010-06-16 13:14:56 PDT
(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
Comment 6 Brady Eidson 2010-06-16 13:16:22 PDT
(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!  :)
Comment 7 Darin Fisher (:fishd, Google) 2010-06-17 10:01:05 PDT
(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.
Comment 8 Darin Fisher (:fishd, Google) 2010-06-17 10:05:53 PDT
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.
Comment 9 Brady Eidson 2010-06-17 10:15:08 PDT
Comment on attachment 59007 [details]
v1 patch

Looks good.  No worries for not being yesterday, I appreciate the fix!
Comment 10 Darin Fisher (:fishd, Google) 2010-06-17 10:23:59 PDT
Landed as http://trac.webkit.org/changeset/61335