2011-01-05 Darin Adler <darin@apple.com>
authordarin@apple.com <darin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 6 Jan 2011 07:12:19 +0000 (07:12 +0000)
committerdarin@apple.com <darin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 6 Jan 2011 07:12:19 +0000 (07:12 +0000)
        Reviewed by Geoff Garen.

        Back/Forward entries in WebKit2 leak
        https://bugs.webkit.org/show_bug.cgi?id=51983

        Besides fixing the leak, this also fixes a problem where
        all history items were sent over to the UI process, but
        we wanted to send only back/forward items.

        * UIProcess/WebBackForwardList.cpp:
        (WebKit::WebBackForwardList::pageClosed): Added.
        Tells the web process about all the back/forward
        items being removed.
        (WebKit::WebBackForwardList::addItem): Ditto.
        Also removed a redundant call to didChangeBackForwardList.
        (WebKit::WebBackForwardList::clear): Ditto.

        * UIProcess/WebBackForwardList.h: Added pageClosed.

        * UIProcess/WebPageProxy.cpp:
        (WebKit::WebPageProxy::close): Added a call to pageClosed.
        (WebKit::WebPageProxy::backForwardRemovedItem): Added.
        Sends a message to the web page in the web process.

        * UIProcess/WebPageProxy.h: Added backForwardRemovedItem.

        * WebProcess/WebPage/WebBackForwardListProxy.cpp:
        (WebKit::updateBackForwardItem): Added an itemID argument,
        since callers will now be getting it and we don't want to
        get it twice. Removed the code to generate an ID. Also
        removed some local variables to make the code a little
        tighter and clearer.
        (WebKit::WK2NotifyHistoryItemChanged): Only call
        updateBackForwardItem for items that already have IDs.
        We don't want to send cross-process messages for every
        history item; just the ones that are top level back/forward
        items.
        (WebKit::WebBackForwardListProxy::removeItem):
        Added. For use when the UI process tells us to remove it.
        (WebKit::WebBackForwardListProxy::addItem): Added code to
        assign an ID and put this item into the maps. This is called
        exactly once on each back/forward item.

        * WebProcess/WebPage/WebBackForwardListProxy.h: Added
        removeItem.

        * WebProcess/WebPage/WebPage.cpp:
        (WebKit::WebPage::didRemoveBackForwardItem): Added.

        * WebProcess/WebPage/WebPage.h: Added didRemoveBackForwardItem.

        * WebProcess/WebPage/WebPage.messages.in: Added
        DidRemoveBackForwardItem message.

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@75144 268f45cc-cd09-0410-ab3c-d52691b4dbfc

WebKit2/ChangeLog
WebKit2/UIProcess/WebBackForwardList.cpp
WebKit2/UIProcess/WebBackForwardList.h
WebKit2/UIProcess/WebPageProxy.cpp
WebKit2/UIProcess/WebPageProxy.h
WebKit2/WebProcess/WebPage/WebBackForwardListProxy.cpp
WebKit2/WebProcess/WebPage/WebBackForwardListProxy.h
WebKit2/WebProcess/WebPage/WebPage.cpp
WebKit2/WebProcess/WebPage/WebPage.h
WebKit2/WebProcess/WebPage/WebPage.messages.in

index 0ed426253fae07a5b65092daa9ad5bf0c5c7efff..6c576ca0de531beaf73a80548f2af264eeef0162 100644 (file)
@@ -1,3 +1,59 @@
+2011-01-05  Darin Adler  <darin@apple.com>
+
+        Reviewed by Geoff Garen.
+
+        Back/Forward entries in WebKit2 leak
+        https://bugs.webkit.org/show_bug.cgi?id=51983
+
+        Besides fixing the leak, this also fixes a problem where
+        all history items were sent over to the UI process, but
+        we wanted to send only back/forward items.
+
+        * UIProcess/WebBackForwardList.cpp:
+        (WebKit::WebBackForwardList::pageClosed): Added.
+        Tells the web process about all the back/forward
+        items being removed.
+        (WebKit::WebBackForwardList::addItem): Ditto.
+        Also removed a redundant call to didChangeBackForwardList.
+        (WebKit::WebBackForwardList::clear): Ditto.
+
+        * UIProcess/WebBackForwardList.h: Added pageClosed.
+
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::close): Added a call to pageClosed.
+        (WebKit::WebPageProxy::backForwardRemovedItem): Added.
+        Sends a message to the web page in the web process.
+
+        * UIProcess/WebPageProxy.h: Added backForwardRemovedItem.
+
+        * WebProcess/WebPage/WebBackForwardListProxy.cpp:
+        (WebKit::updateBackForwardItem): Added an itemID argument,
+        since callers will now be getting it and we don't want to
+        get it twice. Removed the code to generate an ID. Also
+        removed some local variables to make the code a little
+        tighter and clearer.
+        (WebKit::WK2NotifyHistoryItemChanged): Only call
+        updateBackForwardItem for items that already have IDs.
+        We don't want to send cross-process messages for every
+        history item; just the ones that are top level back/forward
+        items.
+        (WebKit::WebBackForwardListProxy::removeItem):
+        Added. For use when the UI process tells us to remove it.
+        (WebKit::WebBackForwardListProxy::addItem): Added code to
+        assign an ID and put this item into the maps. This is called
+        exactly once on each back/forward item.
+
+        * WebProcess/WebPage/WebBackForwardListProxy.h: Added
+        removeItem.
+
+        * WebProcess/WebPage/WebPage.cpp:
+        (WebKit::WebPage::didRemoveBackForwardItem): Added.
+
+        * WebProcess/WebPage/WebPage.h: Added didRemoveBackForwardItem.
+
+        * WebProcess/WebPage/WebPage.messages.in: Added
+        DidRemoveBackForwardItem message.
+
 2011-01-05  Steve Falkenburg  <sfalken@apple.com>
 
         Reviewed by Darin Adler.
 2011-01-05  Steve Falkenburg  <sfalken@apple.com>
 
         Reviewed by Darin Adler.
index f90300c9b822174708a5d9de6d93c7e69deabab8..b351418aed77663bdaeff5ee771375331ce430da 100644 (file)
@@ -45,16 +45,28 @@ WebBackForwardList::~WebBackForwardList()
 {
 }
 
 {
 }
 
+void WebBackForwardList::pageClosed()
+{
+    if (m_page) {
+        size_t size = m_entries.size();
+        for (size_t i = 0; i < size; ++i)
+            m_page->backForwardRemovedItem(m_entries[i]->itemID());
+    }
+
+    m_page = 0;
+}
+
 void WebBackForwardList::addItem(WebBackForwardListItem* newItem)
 {
     if (m_capacity == 0 || !m_enabled)
         return;
 void WebBackForwardList::addItem(WebBackForwardListItem* newItem)
 {
     if (m_capacity == 0 || !m_enabled)
         return;
-    
+
     // Toss anything in the forward list    
     if (m_current != NoCurrentItemIndex) {
         unsigned targetSize = m_current + 1;
         while (m_entries.size() > targetSize) {
     // Toss anything in the forward list    
     if (m_current != NoCurrentItemIndex) {
         unsigned targetSize = m_current + 1;
         while (m_entries.size() > targetSize) {
-            RefPtr<WebBackForwardListItem> item = m_entries.last();
+            if (m_page)
+                m_page->backForwardRemovedItem(m_entries.last()->itemID());
             m_entries.removeLast();
         }
     }
             m_entries.removeLast();
         }
     }
@@ -62,12 +74,10 @@ void WebBackForwardList::addItem(WebBackForwardListItem* newItem)
     // Toss the first item if the list is getting too big, as long as we're not using it
     // (or even if we are, if we only want 1 entry).
     if (m_entries.size() == m_capacity && (m_current != 0 || m_capacity == 1)) {
     // Toss the first item if the list is getting too big, as long as we're not using it
     // (or even if we are, if we only want 1 entry).
     if (m_entries.size() == m_capacity && (m_current != 0 || m_capacity == 1)) {
-        RefPtr<WebBackForwardListItem> item = m_entries[0];
+        if (m_page)
+            m_page->backForwardRemovedItem(m_entries[0]->itemID());
         m_entries.remove(0);
         m_current--;
         m_entries.remove(0);
         m_current--;
-        
-        if (m_page)
-            m_page->didChangeBackForwardList();
     }
 
     m_entries.insert(m_current + 1, newItem);
     }
 
     m_entries.insert(m_current + 1, newItem);
@@ -173,12 +183,21 @@ PassRefPtr<ImmutableArray> WebBackForwardList::forwardListAsImmutableArrayWithLi
 
 void WebBackForwardList::clear()
 {
 
 void WebBackForwardList::clear()
 {
-    if (m_entries.size() <= 1)
+    size_t size = m_entries.size();
+    if (size <= 1)
         return;
 
         return;
 
-    RefPtr<WebBackForwardListItem> item = currentItem();
-    m_entries.resize(1);
-    m_entries[0] = item.release();
+    RefPtr<WebBackForwardListItem> currentItem = this->currentItem();
+
+    if (m_page) {
+        for (size_t i = 0; i < size; ++i) {
+            if (m_entries[i] != currentItem)
+                m_page->backForwardRemovedItem(m_entries[i]->itemID());
+        }
+    }
+
+    m_entries.shrink(1);
+    m_entries[0] = currentItem.release();
 
     m_current = 0;
 
 
     m_current = 0;
 
index 0e4a33cbae31334cdfa83a23832e253631233efa..f51ab264e75f67f4d8d9a129b23aca4ff99cc72f 100644 (file)
@@ -55,6 +55,7 @@ public:
     {
         return adoptRef(new WebBackForwardList(page));
     }
     {
         return adoptRef(new WebBackForwardList(page));
     }
+    void pageClosed();
 
     virtual ~WebBackForwardList();
 
 
     virtual ~WebBackForwardList();
 
index cd08af18454df83898b52adb9e7506b373fc0789..3be40d81ad8278b5cfb1a1dbba2f452a690e0582 100644 (file)
@@ -242,6 +242,8 @@ void WebPageProxy::close()
 
     m_isClosed = true;
 
 
     m_isClosed = true;
 
+    m_backForwardList->pageClosed();
+
     process()->disconnectFramesFromPage(this);
     m_mainFrame = 0;
 
     process()->disconnectFramesFromPage(this);
     m_mainFrame = 0;
 
@@ -2117,6 +2119,7 @@ WebPageCreationParameters WebPageProxy::creationParameters(const IntSize& size)
 }
 
 #if USE(ACCELERATED_COMPOSITING)
 }
 
 #if USE(ACCELERATED_COMPOSITING)
+
 void WebPageProxy::didEnterAcceleratedCompositing()
 {
     m_pageClient->pageDidEnterAcceleratedCompositing();
 void WebPageProxy::didEnterAcceleratedCompositing()
 {
     m_pageClient->pageDidEnterAcceleratedCompositing();
@@ -2126,6 +2129,7 @@ void WebPageProxy::didLeaveAcceleratedCompositing()
 {
     m_pageClient->pageDidLeaveAcceleratedCompositing();
 }
 {
     m_pageClient->pageDidLeaveAcceleratedCompositing();
 }
+
 #endif // USE(ACCELERATED_COMPOSITING)
 
 void WebPageProxy::backForwardClear()
 #endif // USE(ACCELERATED_COMPOSITING)
 
 void WebPageProxy::backForwardClear()
@@ -2175,4 +2179,9 @@ void WebPageProxy::setComplexTextInputEnabled(uint64_t pluginComplexTextInputIde
 }
 #endif
 
 }
 #endif
 
+void WebPageProxy::backForwardRemovedItem(uint64_t itemID)
+{
+    process()->send(Messages::WebPage::DidRemoveBackForwardItem(itemID), m_pageID);
+}
+
 } // namespace WebKit
 } // namespace WebKit
index 47ef4093f5d5040b2d248ffeb43ebf943931700d..3e0a406fa701e07ec49e0753cc20f4a19af890e9 100644 (file)
@@ -253,6 +253,8 @@ public:
 
     void receivedPolicyDecision(WebCore::PolicyAction, WebFrameProxy*, uint64_t listenerID);
 
 
     void receivedPolicyDecision(WebCore::PolicyAction, WebFrameProxy*, uint64_t listenerID);
 
+    void backForwardRemovedItem(uint64_t itemID);
+
     void didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::ArgumentDecoder*);
     void didReceiveSyncMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::ArgumentDecoder*, CoreIPC::ArgumentEncoder*);
 
     void didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::ArgumentDecoder*);
     void didReceiveSyncMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::ArgumentDecoder*, CoreIPC::ArgumentEncoder*);
 
index f56be00517d4bf4a7236f6706aea8cd1561864b1..ab357a9629de3667f24f710ead12ae527ad59b37 100644 (file)
@@ -70,40 +70,22 @@ static uint64_t generateHistoryItemID()
     return uniqueHistoryItemID;
 }
 
     return uniqueHistoryItemID;
 }
 
-static uint64_t getIDForHistoryItem(HistoryItem* item)
+static void updateBackForwardItem(uint64_t itemID, HistoryItem* item)
 {
 {
-    uint64_t itemID = 0;
-
-    std::pair<HistoryItemToIDMap::iterator, bool> result = historyItemToIDMap().add(item, 0);
-    if (result.second) {
-        itemID = generateHistoryItemID();
-        result.first->second = itemID;
-        idToHistoryItemMap().set(itemID, item);
-    } else
-        itemID = result.first->second;
-
-    ASSERT(itemID);
-    return itemID;
-}
-
-static void updateBackForwardItem(HistoryItem* item)
-{
-    uint64_t itemID = getIDForHistoryItem(item);
-    const String& originalURLString = item->originalURLString();
-    const String& urlString = item->urlString();
-    const String& title = item->title();
-
-    // FIXME: We only want to do this work for top level back/forward items.
-    // The best way to do that is probably to arrange for this entire function to only be called by top leve back/forward items.
     EncoderAdapter encoder;
     item->encodeBackForwardTree(encoder);
 
     EncoderAdapter encoder;
     item->encodeBackForwardTree(encoder);
 
-    WebProcess::shared().connection()->send(Messages::WebProcessProxy::AddBackForwardItem(itemID, originalURLString, urlString, title, encoder.data()), 0);
+    WebProcess::shared().connection()->send(Messages::WebProcessProxy::AddBackForwardItem(itemID,
+        item->originalURLString(), item->urlString(), item->title(), encoder.data()), 0);
 }
 
 static void WK2NotifyHistoryItemChanged(HistoryItem* item)
 {
 }
 
 static void WK2NotifyHistoryItemChanged(HistoryItem* item)
 {
-    updateBackForwardItem(item);
+    uint64_t itemID = historyItemToIDMap().get(item);
+    if (!itemID)
+        return;
+
+    updateBackForwardItem(itemID, item);
 }
 
 HistoryItem* WebBackForwardListProxy::itemForID(uint64_t itemID)
 }
 
 HistoryItem* WebBackForwardListProxy::itemForID(uint64_t itemID)
@@ -111,6 +93,15 @@ HistoryItem* WebBackForwardListProxy::itemForID(uint64_t itemID)
     return idToHistoryItemMap().get(itemID).get();
 }
 
     return idToHistoryItemMap().get(itemID).get();
 }
 
+void WebBackForwardListProxy::removeBackForwardItem(uint64_t itemID)
+{
+    IDToHistoryItemMap::iterator it = idToHistoryItemMap().find(itemID);
+    if (it == idToHistoryItemMap().end())
+        return;
+    historyItemToIDMap().remove(it->second);
+    idToHistoryItemMap().remove(it);
+}
+
 WebBackForwardListProxy::WebBackForwardListProxy(WebPage* page)
     : m_page(page)
 {
 WebBackForwardListProxy::WebBackForwardListProxy(WebPage* page)
     : m_page(page)
 {
@@ -119,11 +110,21 @@ WebBackForwardListProxy::WebBackForwardListProxy(WebPage* page)
 
 void WebBackForwardListProxy::addItem(PassRefPtr<HistoryItem> prpItem)
 {
 
 void WebBackForwardListProxy::addItem(PassRefPtr<HistoryItem> prpItem)
 {
+    RefPtr<HistoryItem> item = prpItem;
+
+    ASSERT(!historyItemToIDMap().contains(item));
+
     if (!m_page)
         return;
 
     if (!m_page)
         return;
 
-    RefPtr<HistoryItem> item = prpItem;
-    uint64_t itemID = historyItemToIDMap().get(item);
+    uint64_t itemID = generateHistoryItemID();
+
+    ASSERT(!idToHistoryItemMap().contains(itemID));
+
+    historyItemToIDMap().set(item, itemID);
+    idToHistoryItemMap().set(itemID, item);
+
+    updateBackForwardItem(itemID, item.get());
     m_page->send(Messages::WebPageProxy::BackForwardAddItem(itemID));
 }
 
     m_page->send(Messages::WebPageProxy::BackForwardAddItem(itemID));
 }
 
@@ -132,8 +133,7 @@ void WebBackForwardListProxy::goToItem(HistoryItem* item)
     if (!m_page)
         return;
 
     if (!m_page)
         return;
 
-    uint64_t itemID = historyItemToIDMap().get(item);
-    m_page->send(Messages::WebPageProxy::BackForwardGoToItem(itemID));
+    m_page->send(Messages::WebPageProxy::BackForwardGoToItem(historyItemToIDMap().get(item)));
 }
 
 HistoryItem* WebBackForwardListProxy::itemAtIndex(int itemIndex)
 }
 
 HistoryItem* WebBackForwardListProxy::itemAtIndex(int itemIndex)
@@ -148,8 +148,7 @@ HistoryItem* WebBackForwardListProxy::itemAtIndex(int itemIndex)
     if (!itemID)
         return 0;
 
     if (!itemID)
         return 0;
 
-    RefPtr<HistoryItem> item = idToHistoryItemMap().get(itemID);
-    return item.get();
+    return idToHistoryItemMap().get(itemID).get();
 }
 
 int WebBackForwardListProxy::backListCount()
 }
 
 int WebBackForwardListProxy::backListCount()
index 7cd90fee26aeba533dbfeda2d8a1db3f9a2ca02a..b96c761b7ea4cd67e6023dfdae9a919493080254 100644 (file)
@@ -38,6 +38,7 @@ public:
     static PassRefPtr<WebBackForwardListProxy> create(WebPage* page) { return adoptRef(new WebBackForwardListProxy(page)); }
 
     static WebCore::HistoryItem* itemForID(uint64_t);
     static PassRefPtr<WebBackForwardListProxy> create(WebPage* page) { return adoptRef(new WebBackForwardListProxy(page)); }
 
     static WebCore::HistoryItem* itemForID(uint64_t);
+    static void removeItem(uint64_t itemID);
 
     void clear();
 
 
     void clear();
 
index 938dddddb1d70eab51cc9eece5603ba69f20f1e8..c9cebcf305ebe8035436cf1d230d1321117bf6d2 100644 (file)
@@ -1518,6 +1518,11 @@ void WebPage::setCustomTextEncodingName(const String& encoding)
     m_page->mainFrame()->loader()->reloadWithOverrideEncoding(encoding);
 }
 
     m_page->mainFrame()->loader()->reloadWithOverrideEncoding(encoding);
 }
 
+void WebPage::didRemoveBackForwardItem(uint64_t itemID)
+{
+    WebBackForwardListProxy::removeItem(itemID);
+}
+
 #if PLATFORM(MAC)
 
 bool WebPage::isSpeaking()
 #if PLATFORM(MAC)
 
 bool WebPage::isSpeaking()
index 1c0584044c89ec88be2c9d7101fb751356730939..82d9ba31fcb77642bd2141dfe4e36d6a9bb7710f 100644 (file)
@@ -339,6 +339,8 @@ private:
 
     void restoreSession(const SessionState&);
 
 
     void restoreSession(const SessionState&);
 
+    void didRemoveBackForwardItem(uint64_t);
+
     void setDrawsBackground(bool);
     void setDrawsTransparentBackground(bool);
 
     void setDrawsBackground(bool);
     void setDrawsTransparentBackground(bool);
 
index cf65d69274605d0400cb5dad9917ed21d8c4e10b..d781ae18947f4d91d57e1570cf61eeaa4525819f 100644 (file)
@@ -49,6 +49,8 @@ messages -> WebPage {
     
     RestoreSession(WebKit::SessionState state)
 
     
     RestoreSession(WebKit::SessionState state)
 
+    DidRemoveBackForwardItem(uint64_t backForwardItemID)
+
     DidReceivePolicyDecision(uint64_t frameID, uint64_t listenerID, uint32_t policyAction, uint64_t downloadID)
 
     # Callbacks.
     DidReceivePolicyDecision(uint64_t frameID, uint64_t listenerID, uint32_t policyAction, uint64_t downloadID)
 
     # Callbacks.