Bug 54426 - WebFrameLoaderClient::shouldGoToHistoryItem needs implementation
Summary: WebFrameLoaderClient::shouldGoToHistoryItem needs implementation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-02-14 17:55 PST by Alice Liu
Modified: 2011-02-15 12:32 PST (History)
1 user (show)

See Also:


Attachments
Patch v1 (10.83 KB, patch)
2011-02-15 10:01 PST, Brady Eidson
darin: review+
beidson: commit-queue-
Details | Formatted Diff | Diff
Patch v2 (10.91 KB, patch)
2011-02-15 10:35 PST, Brady Eidson
darin: 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 Alice Liu 2011-02-14 17:55:22 PST
WebKit2 WebFrameLoaderClient::shouldGoToHistoryItem needs implementation.

I think we need this function so that there's a central point where all history navigation goes through (regardless of whether it's in the b/f cache or not) and asks the client for its policy on the action.  See -[WebFrameLoaderClient shouldGoToHistoryItem:]

<rdar://problem/9002047>
Comment 1 Brady Eidson 2011-02-15 10:01:38 PST
Created attachment 82478 [details]
Patch v1
Comment 2 Darin Adler 2011-02-15 10:13:14 PST
Comment on attachment 82478 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=82478&action=review

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:858
> +        // We should never be considering navigating to an item that is not actually in the back/forward list.
> +        ASSERT_NOT_REACHED();

This can’t happen due to a race condition?

> Source/WebKit2/WebProcess/WebPage/WebBackForwardListProxy.cpp:123
> +    return historyItemToIDMap().get(item);

If this can be passed 0 then it needs a special case for 0 to return 0 since get would crash. If not, then I suggest an extra assert even though HashMap will fail an assertion too.
Comment 3 Brady Eidson 2011-02-15 10:34:41 PST
(In reply to comment #2)
> (From update of attachment 82478 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82478&action=review
> 
> > Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:858
> > +        // We should never be considering navigating to an item that is not actually in the back/forward list.
> > +        ASSERT_NOT_REACHED();
> 
> This can’t happen due to a race condition?

I'm not sure I follow your question.

The only way we even consider a back/forward navigation that would call into this "should go to history item" is if that item was already in the back/forward list, so the item had better be in the WebProcess's map.  I don't think we can ever hit this case, hence the ASSERT.

However you did make me think of a race condition in the opposite direction, where the UIProcess has removed a back/forward item, messaged the web process, and the web process hadn't gotten the message yet.

In this case, the UIProcess would've have an item for the itemID and we'd crash trying to make the API call.

I'll upload a new patch.

> 
> > Source/WebKit2/WebProcess/WebPage/WebBackForwardListProxy.cpp:123
> > +    return historyItemToIDMap().get(item);
> 
> If this can be passed 0 then it needs a special case for 0 to return 0 since get would crash. If not, then I suggest an extra assert even though HashMap will fail an assertion too.

And it'll have this fix as well.
Comment 4 Brady Eidson 2011-02-15 10:35:46 PST
Created attachment 82483 [details]
Patch v2
Comment 5 Darin Adler 2011-02-15 10:36:35 PST
Comment on attachment 82483 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=82483&action=review

> Source/WebKit2/UIProcess/WebPageProxy.cpp:485
> +    shouldGoToBackForwardItem = item ? m_loaderClient.shouldGoToBackForwardListItem(this, item) : false;

I think this would look better with && rather than ? : for the null handling.
Comment 6 Early Warning System Bot 2011-02-15 12:02:50 PST
Attachment 82483 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7907995
Comment 7 Brady Eidson 2011-02-15 12:31:29 PST
(In reply to comment #6)
> Attachment 82483 [details] did not build on qt:
> Build output: http://queues.webkit.org/results/7907995

Was going to fix, but someone else beat me to it.  Sorry 'bout this.