- Revision
- 195605
- Author
- [email protected]
- Date
- 2016-01-26 11:57:49 -0800 (Tue, 26 Jan 2016)
Log Message
Make sure a page is still PageCache-able after firing the 'pagehide' events
https://bugs.webkit.org/show_bug.cgi?id=153449
Reviewed by Andreas Kling.
Make sure a page is still PageCache-able after firing the 'pagehide'
events and abort if it isn't. This should improve robustness and it is
easy for pagehide event handlers to do things that would make a Page no
longer PageCache-able and this leads to bugs that are difficult to
investigate.
To achieve this, the 'pagehide' event firing logic was moved out of the
CachedFrame constructor. It now happens earlier in
PageCache::addIfCacheable() after checking if the page is cacheable and
before constructing the CachedPage / CachedFrames. After firing the
'pagehide' event in PageCache::addIfCacheable(), we check again that
the page is still cacheable and we abort early if it is not.
* history/CachedFrame.cpp:
(WebCore::CachedFrame::CachedFrame):
* history/PageCache.cpp:
(WebCore::setInPageCache):
(WebCore::firePageHideEventRecursively):
(WebCore::PageCache::addIfCacheable):
* history/PageCache.h:
* loader/FrameLoader.cpp:
(WebCore::FrameLoader::commitProvisionalLoad):
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (195604 => 195605)
--- trunk/Source/WebCore/ChangeLog 2016-01-26 19:33:38 UTC (rev 195604)
+++ trunk/Source/WebCore/ChangeLog 2016-01-26 19:57:49 UTC (rev 195605)
@@ -1,3 +1,33 @@
+2016-01-26 Chris Dumez <[email protected]>
+
+ Make sure a page is still PageCache-able after firing the 'pagehide' events
+ https://bugs.webkit.org/show_bug.cgi?id=153449
+
+ Reviewed by Andreas Kling.
+
+ Make sure a page is still PageCache-able after firing the 'pagehide'
+ events and abort if it isn't. This should improve robustness and it is
+ easy for pagehide event handlers to do things that would make a Page no
+ longer PageCache-able and this leads to bugs that are difficult to
+ investigate.
+
+ To achieve this, the 'pagehide' event firing logic was moved out of the
+ CachedFrame constructor. It now happens earlier in
+ PageCache::addIfCacheable() after checking if the page is cacheable and
+ before constructing the CachedPage / CachedFrames. After firing the
+ 'pagehide' event in PageCache::addIfCacheable(), we check again that
+ the page is still cacheable and we abort early if it is not.
+
+ * history/CachedFrame.cpp:
+ (WebCore::CachedFrame::CachedFrame):
+ * history/PageCache.cpp:
+ (WebCore::setInPageCache):
+ (WebCore::firePageHideEventRecursively):
+ (WebCore::PageCache::addIfCacheable):
+ * history/PageCache.h:
+ * loader/FrameLoader.cpp:
+ (WebCore::FrameLoader::commitProvisionalLoad):
+
2016-01-26 Beth Dakin <[email protected]>
Rubber-stamped by Tim Horton.
Modified: trunk/Source/WebCore/history/CachedFrame.cpp (195604 => 195605)
--- trunk/Source/WebCore/history/CachedFrame.cpp 2016-01-26 19:33:38 UTC (rev 195604)
+++ trunk/Source/WebCore/history/CachedFrame.cpp 2016-01-26 19:57:49 UTC (rev 195605)
@@ -39,7 +39,6 @@
#include "FrameView.h"
#include "HistoryController.h"
#include "HistoryItem.h"
-#include "IgnoreOpensDuringUnloadCountIncrementer.h"
#include "Logging.h"
#include "MainFrame.h"
#include "Page.h"
@@ -155,26 +154,13 @@
// Custom scrollbar renderers will get reattached when the document comes out of the page cache
m_view->detachCustomScrollbars();
- m_document->setInPageCache(true);
- frame.loader().stopLoading(UnloadEventPolicyUnloadAndPageHide);
+ ASSERT(m_document->inPageCache());
- {
- // The following will fire the pagehide event in each subframe and the HTML specification states
- // that the parent document's ignore-opens-during-unload counter should be incremented while the
- // pagehide event is being fired in its subframes:
- // https://html.spec.whatwg.org/multipage/browsers.html#unload-a-document
- IgnoreOpensDuringUnloadCountIncrementer ignoreOpensDuringUnloadCountIncrementer(m_document.get());
+ // Create the CachedFrames for all Frames in the FrameTree.
+ for (Frame* child = frame.tree().firstChild(); child; child = child->tree().nextSibling())
+ m_childFrames.append(std::make_unique<CachedFrame>(*child));
- // Create the CachedFrames for all Frames in the FrameTree.
- for (Frame* child = frame.tree().firstChild(); child; child = child->tree().nextSibling())
- m_childFrames.append(std::make_unique<CachedFrame>(*child));
- }
-
- // Active DOM objects must be suspended before we cache the frame script data,
- // but after we've fired the pagehide event, in case that creates more objects.
- // Suspending must also happen after we've recursed over child frames, in case
- // those create more objects.
-
+ // Active DOM objects must be suspended before we cache the frame script data.
m_document->suspend();
m_cachedFrameScriptData = std::make_unique<ScriptCachedFrameData>(frame);
Modified: trunk/Source/WebCore/history/PageCache.cpp (195604 => 195605)
--- trunk/Source/WebCore/history/PageCache.cpp 2016-01-26 19:33:38 UTC (rev 195604)
+++ trunk/Source/WebCore/history/PageCache.cpp 2016-01-26 19:57:49 UTC (rev 195605)
@@ -41,6 +41,7 @@
#include "FrameLoaderClient.h"
#include "FrameView.h"
#include "HistoryController.h"
+#include "IgnoreOpensDuringUnloadCountIncrementer.h"
#include "Logging.h"
#include "MainFrame.h"
#include "MemoryPressureHandler.h"
@@ -272,22 +273,19 @@
return globalPageCache;
}
-bool PageCache::canCache(Page* page) const
+bool PageCache::canCache(Page& page) const
{
- if (!page)
- return false;
-
if (!m_maxSize) {
- logPageCacheFailureDiagnosticMessage(page, DiagnosticLoggingKeys::isDisabledKey());
+ logPageCacheFailureDiagnosticMessage(&page, DiagnosticLoggingKeys::isDisabledKey());
return false;
}
if (MemoryPressureHandler::singleton().isUnderMemoryPressure()) {
- logPageCacheFailureDiagnosticMessage(page, DiagnosticLoggingKeys::underMemoryPressureKey());
+ logPageCacheFailureDiagnosticMessage(&page, DiagnosticLoggingKeys::underMemoryPressureKey());
return false;
}
- return canCachePage(*page);
+ return canCachePage(page);
}
void PageCache::pruneToSizeNow(unsigned size, PruningReason pruningReason)
@@ -374,14 +372,57 @@
return emptyString();
}
-void PageCache::add(HistoryItem& item, Page& page)
+static void setInPageCache(Page& page, bool isInPageCache)
{
- ASSERT(canCache(&page));
+ for (Frame* frame = &page.mainFrame(); frame; frame = frame->tree().traverseNext()) {
+ if (auto* document = frame->document())
+ document->setInPageCache(isInPageCache);
+ }
+}
- // Remove stale cache entry if necessary.
- remove(item);
+static void firePageHideEventRecursively(Frame& frame)
+{
+ auto* document = frame.document();
+ if (!document)
+ return;
- item.m_cachedPage = std::make_unique<CachedPage>(page);
+ // stopLoading() will fire the pagehide event in each subframe and the HTML specification states
+ // that the parent document's ignore-opens-during-unload counter should be incremented while the
+ // pagehide event is being fired in its subframes:
+ // https://html.spec.whatwg.org/multipage/browsers.html#unload-a-document
+ IgnoreOpensDuringUnloadCountIncrementer ignoreOpensDuringUnloadCountIncrementer(document);
+
+ frame.loader().stopLoading(UnloadEventPolicyUnloadAndPageHide);
+
+ for (RefPtr<Frame> child = frame.tree().firstChild(); child; child = child->tree().nextSibling())
+ firePageHideEventRecursively(*child);
+}
+
+void PageCache::addIfCacheable(HistoryItem& item, Page* page)
+{
+ if (item.isInPageCache())
+ return;
+
+ if (!page || !canCache(*page))
+ return;
+
+ // Make sure all the documents know they are being added to the PageCache.
+ setInPageCache(*page, true);
+
+ // Fire the pagehide event in all frames.
+ firePageHideEventRecursively(page->mainFrame());
+
+ // Check that the page is still page-cacheable after firing the pagehide event. The JS event handlers
+ // could have altered the page in a way that could prevent caching.
+ if (!canCache(*page)) {
+ setInPageCache(*page, false);
+ return;
+ }
+
+ // Make sure we no longer fire any JS events past this point.
+ NoEventDispatchAssertion assertNoEventDispatch;
+
+ item.m_cachedPage = std::make_unique<CachedPage>(*page);
item.m_pruningReason = PruningReason::None;
m_items.add(&item);
Modified: trunk/Source/WebCore/history/PageCache.h (195604 => 195605)
--- trunk/Source/WebCore/history/PageCache.h 2016-01-26 19:33:38 UTC (rev 195604)
+++ trunk/Source/WebCore/history/PageCache.h 2016-01-26 19:57:49 UTC (rev 195605)
@@ -46,14 +46,14 @@
// Function to obtain the global page cache.
WEBCORE_EXPORT static PageCache& singleton();
- bool canCache(Page*) const;
+ bool canCache(Page&) const;
// Used when memory is low to prune some cached pages.
WEBCORE_EXPORT void pruneToSizeNow(unsigned maxSize, PruningReason);
WEBCORE_EXPORT void setMaxSize(unsigned); // number of pages to cache.
unsigned maxSize() const { return m_maxSize; }
- void add(HistoryItem&, Page&); // Prunes if maxSize() is exceeded.
+ void addIfCacheable(HistoryItem&, Page*); // Prunes if maxSize() is exceeded.
WEBCORE_EXPORT void remove(HistoryItem&);
CachedPage* get(HistoryItem&, Page*);
std::unique_ptr<CachedPage> take(HistoryItem&, Page*);
Modified: trunk/Source/WebCore/loader/FrameLoader.cpp (195604 => 195605)
--- trunk/Source/WebCore/loader/FrameLoader.cpp 2016-01-26 19:33:38 UTC (rev 195604)
+++ trunk/Source/WebCore/loader/FrameLoader.cpp 2016-01-26 19:57:49 UTC (rev 195605)
@@ -1772,9 +1772,8 @@
// Check to see if we need to cache the page we are navigating away from into the back/forward cache.
// We are doing this here because we know for sure that a new page is about to be loaded.
- HistoryItem& item = *history().currentItem();
- if (!m_frame.tree().parent() && PageCache::singleton().canCache(m_frame.page()) && !item.isInPageCache())
- PageCache::singleton().add(item, *m_frame.page());
+ if (!m_frame.tree().parent() && history().currentItem())
+ PageCache::singleton().addIfCacheable(*history().currentItem(), m_frame.page());
if (m_loadType != FrameLoadType::Replace)
closeOldDataSources();
Modified: trunk/Source/WebCore/page/Page.cpp (195604 => 195605)
--- trunk/Source/WebCore/page/Page.cpp 2016-01-26 19:33:38 UTC (rev 195604)
+++ trunk/Source/WebCore/page/Page.cpp 2016-01-26 19:57:49 UTC (rev 195605)
@@ -1863,7 +1863,7 @@
bool Page::canTabSuspend()
{
- return s_tabSuspensionIsEnabled && !m_isPrerender && (m_pageThrottler.activityState() == PageActivityState::NoFlags) && PageCache::singleton().canCache(this);
+ return s_tabSuspensionIsEnabled && !m_isPrerender && (m_pageThrottler.activityState() == PageActivityState::NoFlags) && PageCache::singleton().canCache(*this);
}
void Page::setIsTabSuspended(bool shouldSuspend)
