- 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