Bug 76009 – Unlimited history items saved in WebProcessProxy

• Status: UNCONFIRMED
• Component: History

m_backForwardListItemMap in WebProcessProxy can accumulate WebBackForwardListItem elements until WebKit runs out of available memory. There is a maximum capacity enforced on the WebBackForwardList class, but the same history items saved in the WebProcessProxy have no such cap.

Steps to Reproduce: 
 1) Go to any webpage
 2) Go to a different webpage
 3) Go back to 1) and repeat Steps

The size of WebProcessProxy::m_backForwardListItemMap grows with each new site visited, while the size of WebBackForwardList::m_entries stops at "DefaultCapacity" defined in WebBackForwardList.cpp. 

Produced bug on Windows and Mac OSX

SVN Revision: 104601

I plan to submit a patch shortly.

Created attachment 122139 [details]
Patch

Comment on attachment 122139 [details]
Patch

This is a simple fix:
- One new function that simply calls HashMap's remove function.
- The call of the new function where the same history element is currently being cleaned up for a separate copy of the data.

[Darin Adler]

Sam, Brady, I remember discussing this a while back. I thought that there was a reason that the history item could still matter even if it’s not still in the back/forward list. For example, a client might still be holding a reference to it.

[Brady Eidson]

(In reply to comment #3)
> Sam, Brady, I remember discussing this a while back. I thought that there was a reason that the history item could still matter even if it’s not still in the back/forward list. For example, a client might still be holding a reference to it.

I'm not sure.  A glance at the code implies that if the client is correctly reff'ing the WKBackForwardListItem, removing it from this map won't destroy it.

(In reply to comment #4)

Currently, the only function that exposes m_backForwardListItemMap items outside of WebProcessProxy is WebProcessProxy::webBackForwardItem(uint64_t itemID). 

This is only interfaced with by the WebPageProxy class via the following 4 functions: 
  WebPageProxy::shouldGoToBackForwardListItem(uint64_t itemID, ...
  WebPageProxy::willGoToBackForwardListItem(uint64_t itemID, ...
  WebPageProxy::backForwardAddItem(uint64_t itemID)
  WebPageProxy::backForwardGoToItem(uint64_t itemID, ...

All the code paths I see for these functions get their itemID from WebBackForwardListProxy::historyItemToIDMap()

WebBackForwardListProxy::historyItemToIDMap() enforces the capacity of history items via WebPageProxy::backForwardRemovedItem(uint64_t itemID), which is where I'm suggesting WebProcessProxy::m_backForwardListItemMap should also maintain the history capacity by removing the same items.



call path for WebPageProxy::backForwardRemovedItem to WebBackForwardListProxy::removeItem:
--------------------------------------------------------------------------
WebPageProxy::backForwardRemovedItem(uint64_t itemID)
  calls: process()->send(Messages::WebPage::DidRemoveBackForwardItem(itemID), m_pageID);
  WebPage::didRemoveBackForwardItem(uint64_t itemID)
    calls: WebBackForwardListProxy::removeItem(itemID);
    WebBackForwardListProxy::removeItem(itemID) removes itemID from historyItemToIDMap()

[WebKit Review Bot]

Comment on attachment 122139 [details]
Patch

Rejecting attachment 122139 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
xy.cpp
Hunk #1 FAILED at 3415.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebKit2/UIProcess/WebPageProxy.cpp.rej
patching file Source/WebKit2/UIProcess/WebProcessProxy.cpp
Hunk #1 succeeded at 198 (offset 3 lines).
patching file Source/WebKit2/UIProcess/WebProcessProxy.h
Hunk #1 succeeded at 89 with fuzz 1 (offset 1 line).

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force', '--reviewer', 'Darin Adler']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/15914026