Summary: | WebFrameLoaderClient::shouldGoToHistoryItem needs implementation | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alice Liu <alice.barraclough> | ||||||
Component: | WebKit API | Assignee: | Brady Eidson <beidson> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | webkit-ews | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | PC | ||||||||
OS: | OS X 10.5 | ||||||||
Attachments: |
|
Description
Alice Liu
2011-02-14 17:55:22 PST
Created attachment 82478 [details]
Patch v1
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. (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. Created attachment 82483 [details]
Patch v2
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. Attachment 82483 [details] did not build on qt: Build output: http://queues.webkit.org/results/7907995 (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. |