Cached Page and Frame don't need to be ref-counted.
authorakling@apple.com <akling@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 5 Sep 2013 21:25:32 +0000 (21:25 +0000)
committerakling@apple.com <akling@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 5 Sep 2013 21:25:32 +0000 (21:25 +0000)
<https://webkit.org/b/120710>

Reviewed by Anders Carlsson.

- CachedPage is owned by HistoryItem.
- CachedFrame is owned by CachedPage.

Remove the ref counting from these objects to make the code less confusing.

Added a new method:

- PassOwnPtr<CachedPage> PageCache::take(HistoryItem*)

..which is what it looks like - a combined get() and remove() that transfers
ownership of the CachedPage to the caller.

This is used by commitProvisionalLoad() and invalidateCurrentItemCachedPage()
to accomplish in one swoop what they used to do in awkwardly spaced steps.

* history/CachedFrame.h:
(WebCore::CachedFrame::create):
* history/CachedPage.cpp:
(WebCore::CachedPage::create):
* history/CachedPage.h:
* history/HistoryItem.h:
* history/PageCache.cpp:
(WebCore::PageCache::take):
* history/PageCache.h:
* loader/FrameLoader.cpp:
(WebCore::FrameLoader::commitProvisionalLoad):
(WebCore::FrameLoader::transitionToCommitted):
* loader/FrameLoader.h:
* loader/HistoryController.cpp:
(WebCore::HistoryController::invalidateCurrentItemCachedPage):

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

Source/WebCore/ChangeLog
Source/WebCore/history/CachedFrame.h
Source/WebCore/history/CachedPage.cpp
Source/WebCore/history/CachedPage.h
Source/WebCore/history/HistoryItem.h
Source/WebCore/history/PageCache.cpp
Source/WebCore/history/PageCache.h
Source/WebCore/loader/FrameLoader.cpp
Source/WebCore/loader/FrameLoader.h
Source/WebCore/loader/HistoryController.cpp

index c7398a3..419bb6c 100644 (file)
@@ -1,3 +1,41 @@
+2013-09-05  Andreas Kling  <akling@apple.com>
+
+        Cached Page and Frame don't need to be ref-counted.
+        <https://webkit.org/b/120710>
+
+        Reviewed by Anders Carlsson.
+
+        - CachedPage is owned by HistoryItem.
+        - CachedFrame is owned by CachedPage.
+
+        Remove the ref counting from these objects to make the code less confusing.
+
+        Added a new method:
+
+        - PassOwnPtr<CachedPage> PageCache::take(HistoryItem*)
+
+        ..which is what it looks like - a combined get() and remove() that transfers
+        ownership of the CachedPage to the caller.
+
+        This is used by commitProvisionalLoad() and invalidateCurrentItemCachedPage()
+        to accomplish in one swoop what they used to do in awkwardly spaced steps.
+
+        * history/CachedFrame.h:
+        (WebCore::CachedFrame::create):
+        * history/CachedPage.cpp:
+        (WebCore::CachedPage::create):
+        * history/CachedPage.h:
+        * history/HistoryItem.h:
+        * history/PageCache.cpp:
+        (WebCore::PageCache::take):
+        * history/PageCache.h:
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::commitProvisionalLoad):
+        (WebCore::FrameLoader::transitionToCommitted):
+        * loader/FrameLoader.h:
+        * loader/HistoryController.cpp:
+        (WebCore::HistoryController::invalidateCurrentItemCachedPage):
+
 2013-09-05  David Kilzer  <ddkilzer@apple.com>
 
         BUILD FIX (r155108): Add SynchronousLoaderClientCFNet.cpp to Xcode project
index c23266f..9838f73 100644 (file)
@@ -30,7 +30,6 @@
 #include "KURL.h"
 #include "ScriptCachedFrameData.h"
 #include <wtf/PassOwnPtr.h>
-#include <wtf/RefCounted.h>
 #include <wtf/RefPtr.h>
 
 namespace WebCore {
@@ -67,12 +66,12 @@ protected:
     bool m_isComposited;
 #endif
     
-    Vector<RefPtr<CachedFrame>> m_childFrames;
+    Vector<OwnPtr<CachedFrame>> m_childFrames;
 };
 
-class CachedFrame : public RefCounted<CachedFrame>, private CachedFrameBase {
+class CachedFrame : private CachedFrameBase {
 public:
-    static PassRefPtr<CachedFrame> create(Frame& frame) { return adoptRef(new CachedFrame(frame)); }
+    static PassOwnPtr<CachedFrame> create(Frame& frame) { return adoptPtr(new CachedFrame(frame)); }
 
     void open();
     void clear();
index 4192e60..8ddd331 100644 (file)
@@ -45,9 +45,9 @@ namespace WebCore {
 
 DEFINE_DEBUG_ONLY_GLOBAL(WTF::RefCountedLeakCounter, cachedPageCounter, ("CachedPage"));
 
-PassRefPtr<CachedPage> CachedPage::create(Page& page)
+PassOwnPtr<CachedPage> CachedPage::create(Page& page)
 {
-    return adoptRef(new CachedPage(page));
+    return adoptPtr(new CachedPage(page));
 }
 
 CachedPage::CachedPage(Page& page)
index 0d40382..d8fb3e0 100644 (file)
@@ -27,7 +27,6 @@
 #define CachedPage_h
 
 #include "CachedFrame.h"
-#include <wtf/RefCounted.h>
 
 namespace WebCore {
     
@@ -35,9 +34,9 @@ class Document;
 class DocumentLoader;
 class Page;
 
-class CachedPage : public RefCounted<CachedPage> {
+class CachedPage {
 public:
-    static PassRefPtr<CachedPage> create(Page&);
+    static PassOwnPtr<CachedPage> create(Page&);
     ~CachedPage();
 
     void restore(Page&);
@@ -67,7 +66,7 @@ private:
 
     double m_timeStamp;
     double m_expirationTime;
-    RefPtr<CachedFrame> m_cachedMainFrame;
+    OwnPtr<CachedFrame> m_cachedMainFrame;
     bool m_needStyleRecalcForVisitedLinks;
     bool m_needsFullStyleRecalc;
     bool m_needsCaptionPreferencesChanged;
index 7fe4511..573c1b1 100644 (file)
@@ -286,7 +286,7 @@ private:
     // PageCache controls these fields.
     HistoryItem* m_next;
     HistoryItem* m_prev;
-    RefPtr<CachedPage> m_cachedPage;
+    OwnPtr<CachedPage> m_cachedPage;
     
 #if PLATFORM(MAC)
     RetainPtr<id> m_viewState;
index d18f59e..5551fa4 100644 (file)
@@ -442,6 +442,29 @@ void PageCache::add(PassRefPtr<HistoryItem> prpItem, Page& page)
     prune();
 }
 
+PassOwnPtr<CachedPage> PageCache::take(HistoryItem* item)
+{
+    if (!item)
+        return nullptr;
+
+    OwnPtr<CachedPage> cachedPage = item->m_cachedPage.release();
+
+    removeFromLRUList(item);
+    --m_size;
+
+    item->deref(); // Balanced in add().
+
+    if (!cachedPage)
+        return nullptr;
+
+    if (cachedPage->hasExpired()) {
+        LOG(PageCache, "Not restoring page for %s from back/forward cache because cache entry has expired", item->url().string().ascii().data());
+        return nullptr;
+    }
+
+    return cachedPage.release();
+}
+
 CachedPage* PageCache::get(HistoryItem* item)
 {
     if (!item)
index 77baa7f..94dabeb 100644 (file)
@@ -51,6 +51,7 @@ namespace WebCore {
         void add(PassRefPtr<HistoryItem>, Page&); // Prunes if capacity() is exceeded.
         void remove(HistoryItem*);
         CachedPage* get(HistoryItem* item);
+        PassOwnPtr<CachedPage> take(HistoryItem*);
 
         int pageCount() const { return m_size; }
         int frameCount() const;
index 1d02225..2dbce6f 100644 (file)
@@ -1699,10 +1699,13 @@ void FrameLoader::clearProvisionalLoad()
 
 void FrameLoader::commitProvisionalLoad()
 {
-    RefPtr<CachedPage> cachedPage = m_loadingFromCachedPage ? pageCache()->get(history().provisionalItem()) : 0;
     RefPtr<DocumentLoader> pdl = m_provisionalDocumentLoader;
     Ref<Frame> protect(m_frame);
 
+    OwnPtr<CachedPage> cachedPage;
+    if (m_loadingFromCachedPage)
+        cachedPage = pageCache()->take(history().provisionalItem());
+
     LOG(PageCache, "WebCoreLoading %s: About to commit provisional load from previous URL '%s' to new URL '%s'", m_frame.tree().uniqueName().string().utf8().data(),
         m_frame.document() ? m_frame.document()->url().stringCenterEllipsizedToLength().utf8().data() : "",
         pdl ? pdl->url().stringCenterEllipsizedToLength().utf8().data() : "<no provisional DocumentLoader>");
@@ -1721,7 +1724,7 @@ void FrameLoader::commitProvisionalLoad()
     if (!cachedPage && !m_stateMachine.creatingInitialEmptyDocument())
         m_client.makeRepresentation(pdl.get());
 
-    transitionToCommitted(cachedPage);
+    transitionToCommitted(cachedPage.get());
 
     if (pdl && m_documentLoader) {
         // Check if the destination page is allowed to access the previous page's timing information.
@@ -1738,10 +1741,9 @@ void FrameLoader::commitProvisionalLoad()
     
     if (cachedPage && cachedPage->document()) {
         prepareForCachedPageRestore();
-        cachedPage->restore(*m_frame.page());
 
-        // The page should be removed from the cache immediately after a restoration in order for the PageCache to be consistent.
-        pageCache()->remove(history().currentItem());
+        // FIXME: This API should be turned around so that we ground CachedPage into the Page.
+        cachedPage->restore(*m_frame.page());
 
         dispatchDidCommitLoad();
 
@@ -1751,11 +1753,8 @@ void FrameLoader::commitProvisionalLoad()
             m_client.dispatchDidReceiveTitle(title);
 
         checkCompleted();
-    } else {
-        if (cachedPage)
-            pageCache()->remove(history().currentItem());
+    } else
         didOpenURL();
-    }
 
     LOG(Loading, "WebCoreLoading %s: Finished committing provisional load to URL %s", m_frame.tree().uniqueName().string().utf8().data(),
         m_frame.document() ? m_frame.document()->url().stringCenterEllipsizedToLength().utf8().data() : "");
@@ -1789,7 +1788,7 @@ void FrameLoader::commitProvisionalLoad()
     }
 }
 
-void FrameLoader::transitionToCommitted(PassRefPtr<CachedPage> cachedPage)
+void FrameLoader::transitionToCommitted(CachedPage* cachedPage)
 {
     ASSERT(m_client.hasWebView());
     ASSERT(m_state == FrameStateProvisional);
index 803eac1..fab57a3 100644 (file)
@@ -308,7 +308,7 @@ private:
     void addExtraFieldsToRequest(ResourceRequest&, FrameLoadType, bool isMainResource);
 
     void clearProvisionalLoad();
-    void transitionToCommitted(PassRefPtr<CachedPage>);
+    void transitionToCommitted(CachedPage*);
     void frameLoadCompleted();
 
     SubstituteData defaultSubstituteDataForURL(const KURL&);
index 90cdc37..41683ac 100644 (file)
@@ -224,21 +224,21 @@ void HistoryController::restoreDocumentState()
 
 void HistoryController::invalidateCurrentItemCachedPage()
 {
-    // When we are pre-commit, the currentItem is where the pageCache data resides    
-    CachedPage* cachedPage = pageCache()->get(currentItem());
+    // When we are pre-commit, the currentItem is where any page cache data resides.
+    if (!pageCache()->get(currentItem()))
+        return;
+
+    OwnPtr<CachedPage> cachedPage = pageCache()->take(currentItem());
 
     // FIXME: This is a grotesque hack to fix <rdar://problem/4059059> Crash in RenderFlow::detach
     // Somehow the PageState object is not properly updated, and is holding onto a stale document.
     // Both Xcode and FileMaker see this crash, Safari does not.
     
-    ASSERT(!cachedPage || cachedPage->document() == m_frame.document());
-    if (cachedPage && cachedPage->document() == m_frame.document()) {
+    ASSERT(cachedPage->document() == m_frame.document());
+    if (cachedPage->document() == m_frame.document()) {
         cachedPage->document()->setInPageCache(false);
         cachedPage->clear();
     }
-    
-    if (cachedPage)
-        pageCache()->remove(currentItem());
 }
 
 bool HistoryController::shouldStopLoadingForHistoryItem(HistoryItem* targetItem) const