Title: [219501] trunk/Source/WebCore
- Revision
- 219501
- Author
- cdu...@apple.com
- Date
- 2017-07-14 01:17:10 -0700 (Fri, 14 Jul 2017)
Log Message
PageCache::removeAllItemsForPage(Page&) may reenter itself and cause crashes
https://bugs.webkit.org/show_bug.cgi?id=174473
<rdar://problem/32177485>
Reviewed by Antti Koivisto.
This could happen when a Page containing an SVGImage is removed from PageCache and
this resulted in the destruction of the SVGImage. Because the SVGImage has an internal
utility Page, it will also call PageCache::removeAllItemsForPage(WebCore::Page&) upon
destruction, causing us to reenter.
Address the issue by not calling PageCache::removeAllItemsForPage() for utility pages
since those cannot be in PageCache in the first place.
Also add assertions to make sure:
1. We never insert a utility page into PageCache
2. PageCache::removeAllItemsForPage() does not reenter
No new tests, because I was unable to write a test which reproduced the crash. This
is in theory testable using an API test which enables PageCache, loads a page
containing an SVGImage, navigates away from this page so that it goes into PageCache,
and then calls [WebView _close]. However, when I tried writing such test, I could
not get the SVGImage to get destroyed while PageCache::removeAllItemsForPage() is
called for the top-level page for some reason. Something seems to be keeping the
SVGImage alive longer. I tried disabling the MemoryCache but it did not help.
* history/PageCache.cpp:
(WebCore::PageCache::addIfCacheable):
(WebCore::PageCache::removeAllItemsForPage):
* history/PageCache.h:
* page/Page.cpp:
(WebCore::Page::~Page):
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (219500 => 219501)
--- trunk/Source/WebCore/ChangeLog 2017-07-14 07:07:49 UTC (rev 219500)
+++ trunk/Source/WebCore/ChangeLog 2017-07-14 08:17:10 UTC (rev 219501)
@@ -1,3 +1,38 @@
+2017-07-14 Chris Dumez <cdu...@apple.com>
+
+ PageCache::removeAllItemsForPage(Page&) may reenter itself and cause crashes
+ https://bugs.webkit.org/show_bug.cgi?id=174473
+ <rdar://problem/32177485>
+
+ Reviewed by Antti Koivisto.
+
+ This could happen when a Page containing an SVGImage is removed from PageCache and
+ this resulted in the destruction of the SVGImage. Because the SVGImage has an internal
+ utility Page, it will also call PageCache::removeAllItemsForPage(WebCore::Page&) upon
+ destruction, causing us to reenter.
+
+ Address the issue by not calling PageCache::removeAllItemsForPage() for utility pages
+ since those cannot be in PageCache in the first place.
+
+ Also add assertions to make sure:
+ 1. We never insert a utility page into PageCache
+ 2. PageCache::removeAllItemsForPage() does not reenter
+
+ No new tests, because I was unable to write a test which reproduced the crash. This
+ is in theory testable using an API test which enables PageCache, loads a page
+ containing an SVGImage, navigates away from this page so that it goes into PageCache,
+ and then calls [WebView _close]. However, when I tried writing such test, I could
+ not get the SVGImage to get destroyed while PageCache::removeAllItemsForPage() is
+ called for the top-level page for some reason. Something seems to be keeping the
+ SVGImage alive longer. I tried disabling the MemoryCache but it did not help.
+
+ * history/PageCache.cpp:
+ (WebCore::PageCache::addIfCacheable):
+ (WebCore::PageCache::removeAllItemsForPage):
+ * history/PageCache.h:
+ * page/Page.cpp:
+ (WebCore::Page::~Page):
+
2017-07-14 Aaron Chu <aaron_...@apple.com>
AX: VoiceOver silent or skipping over time values on media player.
Modified: trunk/Source/WebCore/history/PageCache.cpp (219500 => 219501)
--- trunk/Source/WebCore/history/PageCache.cpp 2017-07-14 07:07:49 UTC (rev 219500)
+++ trunk/Source/WebCore/history/PageCache.cpp 2017-07-14 08:17:10 UTC (rev 219501)
@@ -409,6 +409,8 @@
if (!page || !canCache(*page))
return;
+ ASSERT_WITH_MESSAGE(!page->isUtilityPage(), "Utility pages such as SVGImage pages should never go into PageCache");
+
setPageCacheState(*page, Document::AboutToEnterPageCache);
// Focus the main frame, defocusing a focused subframe (if we have one). We do this here,
@@ -462,6 +464,11 @@
void PageCache::removeAllItemsForPage(Page& page)
{
+#if !ASSERT_DISABLED
+ ASSERT_WITH_MESSAGE(!m_isInRemoveAllItemsForPage, "We should not reenter this method");
+ SetForScope<bool> inRemoveAllItemsForPageScope { m_isInRemoveAllItemsForPage, true };
+#endif
+
for (auto it = m_items.begin(); it != m_items.end();) {
// Increment iterator first so it stays valid after the removal.
auto current = it;
Modified: trunk/Source/WebCore/history/PageCache.h (219500 => 219501)
--- trunk/Source/WebCore/history/PageCache.h 2017-07-14 07:07:49 UTC (rev 219500)
+++ trunk/Source/WebCore/history/PageCache.h 2017-07-14 08:17:10 UTC (rev 219501)
@@ -78,6 +78,10 @@
ListHashSet<RefPtr<HistoryItem>> m_items;
unsigned m_maxSize {0};
+#if !ASSERT_DISABLED
+ bool m_isInRemoveAllItemsForPage { false };
+#endif
+
friend class WTF::NeverDestroyed<PageCache>;
};
Modified: trunk/Source/WebCore/page/Page.cpp (219500 => 219501)
--- trunk/Source/WebCore/page/Page.cpp 2017-07-14 07:07:49 UTC (rev 219500)
+++ trunk/Source/WebCore/page/Page.cpp 2017-07-14 08:17:10 UTC (rev 219501)
@@ -331,7 +331,8 @@
m_scrollingCoordinator->pageDestroyed();
backForward().close();
- PageCache::singleton().removeAllItemsForPage(*this);
+ if (!isUtilityPage())
+ PageCache::singleton().removeAllItemsForPage(*this);
#ifndef NDEBUG
pageCounter.decrement();
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
