Bug 54426 – WebFrameLoaderClient::shouldGoToHistoryItem needs implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Alice Liu]

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>

[Brady Eidson]

Created attachment 82478 [details]
Patch v1

[Darin Adler]

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.

[Brady Eidson]

(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.

[Brady Eidson]

Created attachment 82483 [details]
Patch v2

[Darin Adler]

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.

[Early Warning System Bot]

Attachment 82483 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7907995

[Brady Eidson]

(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.