Title: [211569] trunk/Source
Revision
211569
Author
cdu...@apple.com
Date
2017-02-02 10:31:54 -0800 (Thu, 02 Feb 2017)

Log Message

[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.

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:

Modified Paths

Diff

Modified: trunk/Source/WebCore/ChangeLog (211568 => 211569)


--- trunk/Source/WebCore/ChangeLog	2017-02-02 17:52:44 UTC (rev 211568)
+++ trunk/Source/WebCore/ChangeLog	2017-02-02 18:31:54 UTC (rev 211569)
@@ -1,3 +1,38 @@
+2017-02-02  Chris Dumez  <cdu...@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  <an...@apple.com>
 
         Column progression wrong after enabling pagination on RTL document

Modified: trunk/Source/WebCore/history/CachedPage.cpp (211568 => 211569)


--- trunk/Source/WebCore/history/CachedPage.cpp	2017-02-02 17:52:44 UTC (rev 211568)
+++ trunk/Source/WebCore/history/CachedPage.cpp	2017-02-02 18:31:54 UTC (rev 211569)
@@ -50,7 +50,8 @@
 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

Modified: trunk/Source/WebCore/history/CachedPage.h (211568 => 211569)


--- trunk/Source/WebCore/history/CachedPage.h	2017-02-02 17:52:44 UTC (rev 211568)
+++ trunk/Source/WebCore/history/CachedPage.h	2017-02-02 18:31:54 UTC (rev 211569)
@@ -42,6 +42,7 @@
     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 @@
     void markForContentsSizeChanged() { m_needsUpdateContentsSize = true; }
 
 private:
+    Page& m_page;
     double m_expirationTime;
     std::unique_ptr<CachedFrame> m_cachedMainFrame;
 #if ENABLE(VIDEO_TRACK)

Modified: trunk/Source/WebCore/history/PageCache.cpp (211568 => 211569)


--- trunk/Source/WebCore/history/PageCache.cpp	2017-02-02 17:52:44 UTC (rev 211568)
+++ trunk/Source/WebCore/history/PageCache.cpp	2017-02-02 18:31:54 UTC (rev 211569)
@@ -466,6 +466,19 @@
     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();

Modified: trunk/Source/WebCore/history/PageCache.h (211568 => 211569)


--- trunk/Source/WebCore/history/PageCache.h	2017-02-02 17:52:44 UTC (rev 211568)
+++ trunk/Source/WebCore/history/PageCache.h	2017-02-02 18:31:54 UTC (rev 211569)
@@ -57,6 +57,8 @@
     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;
 

Modified: trunk/Source/WebCore/page/Page.cpp (211568 => 211569)


--- trunk/Source/WebCore/page/Page.cpp	2017-02-02 17:52:44 UTC (rev 211568)
+++ trunk/Source/WebCore/page/Page.cpp	2017-02-02 18:31:54 UTC (rev 211569)
@@ -325,6 +325,7 @@
         m_scrollingCoordinator->pageDestroyed();
 
     backForward().close();
+    PageCache::singleton().removeAllItemsForPage(*this);
 
 #ifndef NDEBUG
     pageCounter.decrement();

Modified: trunk/Source/WebKit/mac/ChangeLog (211568 => 211569)


--- trunk/Source/WebKit/mac/ChangeLog	2017-02-02 17:52:44 UTC (rev 211568)
+++ trunk/Source/WebKit/mac/ChangeLog	2017-02-02 18:31:54 UTC (rev 211569)
@@ -1,3 +1,17 @@
+2017-02-02  Chris Dumez  <cdu...@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_zh...@apple.com>
 
         In iOS, we should take background assertion when accessing localstorage databases.

Modified: trunk/Source/WebKit/mac/History/BackForwardList.mm (211568 => 211569)


--- trunk/Source/WebKit/mac/History/BackForwardList.mm	2017-02-02 17:52:44 UTC (rev 211568)
+++ trunk/Source/WebKit/mac/History/BackForwardList.mm	2017-02-02 18:31:54 UTC (rev 211569)
@@ -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;

Modified: trunk/Source/WebKit/win/BackForwardList.cpp (211568 => 211569)


--- trunk/Source/WebKit/win/BackForwardList.cpp	2017-02-02 17:52:44 UTC (rev 211568)
+++ trunk/Source/WebKit/win/BackForwardList.cpp	2017-02-02 18:31:54 UTC (rev 211569)
@@ -225,8 +225,6 @@
 
 void BackForwardList::close()
 {
-    for (auto& item : m_entries)
-        PageCache::singleton().remove(item);
     m_entries.clear();
     m_entryHash.clear();
     m_closed = true;

Modified: trunk/Source/WebKit/win/ChangeLog (211568 => 211569)


--- trunk/Source/WebKit/win/ChangeLog	2017-02-02 17:52:44 UTC (rev 211568)
+++ trunk/Source/WebKit/win/ChangeLog	2017-02-02 18:31:54 UTC (rev 211569)
@@ -1,3 +1,17 @@
+2017-02-02  Chris Dumez  <cdu...@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  <y...@yoav.ws>
 
         Add Link Preload as an off-by-default experimental feature menu item.

Modified: trunk/Source/WebKit2/ChangeLog (211568 => 211569)


--- trunk/Source/WebKit2/ChangeLog	2017-02-02 17:52:44 UTC (rev 211568)
+++ trunk/Source/WebKit2/ChangeLog	2017-02-02 18:31:54 UTC (rev 211569)
@@ -1,3 +1,20 @@
+2017-02-02  Chris Dumez  <cdu...@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  <ander...@apple.com>
 
         <rdar://problem/30323148> Webkit Nightly on 10.10 broken

Modified: trunk/Source/WebKit2/WebProcess/WebPage/WebBackForwardListProxy.cpp (211568 => 211569)


--- trunk/Source/WebKit2/WebProcess/WebPage/WebBackForwardListProxy.cpp	2017-02-02 17:52:44 UTC (rev 211568)
+++ trunk/Source/WebKit2/WebProcess/WebPage/WebBackForwardListProxy.cpp	2017-02-02 18:31:54 UTC (rev 211569)
@@ -100,8 +100,6 @@
     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 @@
 
     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 @@
 
 void WebBackForwardListProxy::close()
 {
-    for (auto& itemID : m_associatedItemIDs) {
-        if (HistoryItem* item = itemForID(itemID))
-            WebCore::PageCache::singleton().remove(*item);
-    }
-
-    m_associatedItemIDs.clear();
     m_page = nullptr;
 }
 

Modified: trunk/Source/WebKit2/WebProcess/WebPage/WebBackForwardListProxy.h (211568 => 211569)


--- trunk/Source/WebKit2/WebProcess/WebPage/WebBackForwardListProxy.h	2017-02-02 17:52:44 UTC (rev 211568)
+++ trunk/Source/WebKit2/WebProcess/WebPage/WebBackForwardListProxy.h	2017-02-02 18:31:54 UTC (rev 211569)
@@ -60,7 +60,6 @@
     void close() override;
 
     WebPage* m_page;
-    HashSet<uint64_t> m_associatedItemIDs;
 };
 
 } // namespace WebKit
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to