[Crash] com.apple.WebKit.WebContent at WebKit: WebKit::WebPage::fromC…

…orePage() https://bugs.webkit.org/show_bug.cgi?id=167738 <rdar://problem/30229990> Reviewed by Andreas Kling. Source/WebCore: Upon destruction of a Page, we destroy the BackForwardClient, which is supposed to keep track of HistoryItems associated to this particular page and remove them from the PageCache. Given the crash trace, the issue seems to be that some HistoryItems associated with the Page sometimes linger in the PageCache *after* the Page has been destroyed, which leads to crashes later on when pruning the PageCache. In order to make the process more robust, this patch refactors the code so that the Page is now in charge of removing all its associated HistoryItems from the PageCache instead of relying on the BackForwardClient. Also, instead of having the Page keep track of which HistoryItems are associated with it (which is error prone), we now scan all PageCache entries instead to find which ones are associated with the Page. While this is in theory slower, this is much safer and in practice not an issue because the PageCache usually has 3-5 entries. No new tests, could not reproduce. * history/CachedPage.cpp: (WebCore::CachedPage::CachedPage): * history/CachedPage.h: (WebCore::CachedPage::page): * history/PageCache.cpp: (WebCore::PageCache::removeAllItemsForPage): * history/PageCache.h: * page/Page.cpp: (WebCore::Page::~Page): Source/WebKit/mac: The BackForwardClient no longer needs to worry about removing HistoryItems from the PageCache now that WebCore takes care of it. * History/BackForwardList.mm: (BackForwardList::close): Source/WebKit/win: The BackForwardClient no longer needs to worry about removing HistoryItems from the PageCache now that WebCore takes care of it. * BackForwardList.cpp: (BackForwardList::close): Source/WebKit2: The BackForwardClient no longer needs to worry about removing HistoryItems from the PageCache now that WebCore takes care of it. * WebProcess/WebPage/WebBackForwardListProxy.cpp: (WebKit::WebBackForwardListProxy::addItemFromUIProcess): (WebKit::WebBackForwardListProxy::addItem): (WebKit::WebBackForwardListProxy::close): * WebProcess/WebPage/WebBackForwardListProxy.h: Canonical link: https://commits.webkit.org/184783@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@211569 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · Feb 2, 2017 · 80728d0 · 80728d0
1 parent c8606c7
commit 80728d0
Show file tree

Hide file tree

Showing 13 changed files with 100 additions and 16 deletions.
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,38 @@
+2017-02-02  Chris Dumez  <cdumez@apple.com>
+
+        [Crash] com.apple.WebKit.WebContent at WebKit: WebKit::WebPage::fromCorePage()
+        https://bugs.webkit.org/show_bug.cgi?id=167738
+        <rdar://problem/30229990>
+
+        Reviewed by Andreas Kling.
+
+        Upon destruction of a Page, we destroy the BackForwardClient, which is supposed
+        to keep track of HistoryItems associated to this particular page and remove them
+        from the PageCache. Given the crash trace, the issue seems to be that some
+        HistoryItems associated with the Page sometimes linger in the PageCache *after*
+        the Page has been destroyed, which leads to crashes later on when pruning the
+        PageCache.
+
+        In order to make the process more robust, this patch refactors the code so that
+        the Page is now in charge of removing all its associated HistoryItems from the
+        PageCache instead of relying on the BackForwardClient. Also, instead of having
+        the Page keep track of which HistoryItems are associated with it (which is
+        error prone), we now scan all PageCache entries instead to find which ones are
+        associated with the Page. While this is in theory slower, this is much safer
+        and in practice not an issue because the PageCache usually has 3-5 entries.
+
+        No new tests, could not reproduce.
+
+        * history/CachedPage.cpp:
+        (WebCore::CachedPage::CachedPage):
+        * history/CachedPage.h:
+        (WebCore::CachedPage::page):
+        * history/PageCache.cpp:
+        (WebCore::PageCache::removeAllItemsForPage):
+        * history/PageCache.h:
+        * page/Page.cpp:
+        (WebCore::Page::~Page):
+
 2017-02-02  Antti Koivisto  <antti@apple.com>
 
         Column progression wrong after enabling pagination on RTL document

diff --git a/Source/WebCore/history/CachedPage.cpp b/Source/WebCore/history/CachedPage.cpp
@@ -50,7 +50,8 @@ namespace WebCore {
 DEFINE_DEBUG_ONLY_GLOBAL(WTF::RefCountedLeakCounter, cachedPageCounter, ("CachedPage"));
 
 CachedPage::CachedPage(Page& page)
-    : m_expirationTime(monotonicallyIncreasingTime() + page.settings().backForwardCacheExpirationInterval())
+    : m_page(page)
+    , m_expirationTime(monotonicallyIncreasingTime() + page.settings().backForwardCacheExpirationInterval())
     , m_cachedMainFrame(std::make_unique<CachedFrame>(page.mainFrame()))
 {
 #ifndef NDEBUG

diff --git a/Source/WebCore/history/CachedPage.h b/Source/WebCore/history/CachedPage.h
@@ -42,6 +42,7 @@ class CachedPage {
     void restore(Page&);
     void clear();
 
+    Page& page() const { return m_page; }
     Document* document() const { return m_cachedMainFrame->document(); }
     DocumentLoader* documentLoader() const { return m_cachedMainFrame->documentLoader(); }
 
@@ -58,6 +59,7 @@ class CachedPage {
     void markForContentsSizeChanged() { m_needsUpdateContentsSize = true; }
 
 private:
+    Page& m_page;
     double m_expirationTime;
     std::unique_ptr<CachedFrame> m_cachedMainFrame;
 #if ENABLE(VIDEO_TRACK)

diff --git a/Source/WebCore/history/PageCache.cpp b/Source/WebCore/history/PageCache.cpp
@@ -466,6 +466,19 @@ std::unique_ptr<CachedPage> PageCache::take(HistoryItem& item, Page* page)
     return cachedPage;
 }
 
+void PageCache::removeAllItemsForPage(Page& page)
+{
+    for (auto it = m_items.begin(); it != m_items.end();) {
+        // Increment iterator first so it stays invalid after the removal.
+        auto current = it;
+        ++it;
+        if (&(*current)->m_cachedPage->page() == &page) {
+            (*current)->m_cachedPage = nullptr;
+            m_items.remove(current);
+        }
+    }
+}
+
 CachedPage* PageCache::get(HistoryItem& item, Page* page)
 {
     CachedPage* cachedPage = item.m_cachedPage.get();

diff --git a/Source/WebCore/history/PageCache.h b/Source/WebCore/history/PageCache.h
@@ -57,6 +57,8 @@ class PageCache {
     CachedPage* get(HistoryItem&, Page*);
     std::unique_ptr<CachedPage> take(HistoryItem&, Page*);
 
+    void removeAllItemsForPage(Page&);
+
     unsigned pageCount() const { return m_items.size(); }
     WEBCORE_EXPORT unsigned frameCount() const;
 

diff --git a/Source/WebCore/page/Page.cpp b/Source/WebCore/page/Page.cpp
@@ -325,6 +325,7 @@ Page::~Page()
         m_scrollingCoordinator->pageDestroyed();
 
     backForward().close();
+    PageCache::singleton().removeAllItemsForPage(*this);
 
 #ifndef NDEBUG
     pageCounter.decrement();

diff --git a/Source/WebKit/mac/ChangeLog b/Source/WebKit/mac/ChangeLog
@@ -1,3 +1,17 @@
+2017-02-02  Chris Dumez  <cdumez@apple.com>
+
+        [Crash] com.apple.WebKit.WebContent at WebKit: WebKit::WebPage::fromCorePage()
+        https://bugs.webkit.org/show_bug.cgi?id=167738
+        <rdar://problem/30229990>
+
+        Reviewed by Andreas Kling.
+
+        The BackForwardClient no longer needs to worry about removing HistoryItems
+        from the PageCache now that WebCore takes care of it.
+
+        * History/BackForwardList.mm:
+        (BackForwardList::close):
+
 2017-02-02  Yongjun Zhang  <yongjun_zhang@apple.com>
 
         In iOS, we should take background assertion when accessing localstorage databases.

diff --git a/Source/WebKit/mac/History/BackForwardList.mm b/Source/WebKit/mac/History/BackForwardList.mm
@@ -225,8 +225,6 @@
 
 void BackForwardList::close()
 {
-    for (auto& item : m_entries)
-        PageCache::singleton().remove(item);
     m_entries.clear();
     m_entryHash.clear();
     m_webView = nullptr;

diff --git a/Source/WebKit/win/BackForwardList.cpp b/Source/WebKit/win/BackForwardList.cpp
@@ -225,8 +225,6 @@ Vector<Ref<HistoryItem>>& BackForwardList::entries()
 
 void BackForwardList::close()
 {
-    for (auto& item : m_entries)
-        PageCache::singleton().remove(item);
     m_entries.clear();
     m_entryHash.clear();
     m_closed = true;

diff --git a/Source/WebKit/win/ChangeLog b/Source/WebKit/win/ChangeLog
@@ -1,3 +1,17 @@
+2017-02-02  Chris Dumez  <cdumez@apple.com>
+
+        [Crash] com.apple.WebKit.WebContent at WebKit: WebKit::WebPage::fromCorePage()
+        https://bugs.webkit.org/show_bug.cgi?id=167738
+        <rdar://problem/30229990>
+
+        Reviewed by Andreas Kling.
+
+        The BackForwardClient no longer needs to worry about removing HistoryItems
+        from the PageCache now that WebCore takes care of it.
+
+        * BackForwardList.cpp:
+        (BackForwardList::close):
+
 2017-01-28  Yoav Weiss  <yoav@yoav.ws>
 
         Add Link Preload as an off-by-default experimental feature menu item.

diff --git a/Source/WebKit2/ChangeLog b/Source/WebKit2/ChangeLog
@@ -1,3 +1,20 @@
+2017-02-02  Chris Dumez  <cdumez@apple.com>
+
+        [Crash] com.apple.WebKit.WebContent at WebKit: WebKit::WebPage::fromCorePage()
+        https://bugs.webkit.org/show_bug.cgi?id=167738
+        <rdar://problem/30229990>
+
+        Reviewed by Andreas Kling.
+
+        The BackForwardClient no longer needs to worry about removing HistoryItems
+        from the PageCache now that WebCore takes care of it.
+
+        * WebProcess/WebPage/WebBackForwardListProxy.cpp:
+        (WebKit::WebBackForwardListProxy::addItemFromUIProcess):
+        (WebKit::WebBackForwardListProxy::addItem):
+        (WebKit::WebBackForwardListProxy::close):
+        * WebProcess/WebPage/WebBackForwardListProxy.h:
+
 2017-02-02  Anders Carlsson  <andersca@apple.com>
 
         <rdar://problem/30323148> Webkit Nightly on 10.10 broken

diff --git a/Source/WebKit2/WebProcess/WebPage/WebBackForwardListProxy.cpp b/Source/WebKit2/WebProcess/WebPage/WebBackForwardListProxy.cpp
@@ -100,8 +100,6 @@ void WebBackForwardListProxy::addItemFromUIProcess(uint64_t itemID, Ref<HistoryI
     ASSERT(!historyItemToIDMap().contains(item.ptr()));
     ASSERT(!idToHistoryItemMap().contains(itemID));
 
-    m_associatedItemIDs.add(itemID);
-
     historyItemToIDMap().set<ItemAndPageID>(item.ptr(), { .itemID = itemID, .pageID = pageID });
     idToHistoryItemMap().set(itemID, item.ptr());
 }
@@ -154,8 +152,6 @@ void WebBackForwardListProxy::addItem(Ref<HistoryItem>&& item)
 
     ASSERT(!idToHistoryItemMap().contains(itemID));
 
-    m_associatedItemIDs.add(itemID);
-
     historyItemToIDMap().set<ItemAndPageID>(item.ptr(), { .itemID = itemID, .pageID = m_page->pageID() });
     idToHistoryItemMap().set(itemID, item.ptr());
 
@@ -214,12 +210,6 @@ int WebBackForwardListProxy::forwardListCount()
 
 void WebBackForwardListProxy::close()
 {
-    for (auto& itemID : m_associatedItemIDs) {
-        if (HistoryItem* item = itemForID(itemID))
-            WebCore::PageCache::singleton().remove(*item);
-    }
-
-    m_associatedItemIDs.clear();
     m_page = nullptr;
 }
 

diff --git a/Source/WebKit2/WebProcess/WebPage/WebBackForwardListProxy.h b/Source/WebKit2/WebProcess/WebPage/WebBackForwardListProxy.h
@@ -60,7 +60,6 @@ class WebBackForwardListProxy : public WebCore::BackForwardClient {
     void close() override;
 
     WebPage* m_page;
-    HashSet<uint64_t> m_associatedItemIDs;
 };
 
 } // namespace WebKit