Changeset 195605 in webkit

Timestamp:

Jan 26, 2016 11:57:49 AM (8 years ago)

Author:

Chris Dumez

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):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• history/CachedFrame.cpp (modified) (2 diffs)
• history/PageCache.cpp (modified) (3 diffs)
• history/PageCache.h (modified) (2 diffs)
• loader/FrameLoader.cpp (modified) (1 diff)
• page/Page.cpp (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2016-01-26  Chris Dumez  <cdumez@apple.com>
+
+        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  <bdakin@apple.com>
 

trunk/Source/WebCore/history/CachedFrame.cpp

@@ -40 +40 @@
 #include "HistoryController.h"
 #include "HistoryItem.h"
-#include "IgnoreOpensDuringUnloadCountIncrementer.h"
 #include "Logging.h"
 #include "MainFrame.h"
@@ -156 +155 @@
     m_view->detachCustomScrollbars();
 
-    m_document->setInPageCache(true);
-    frame.loader().stopLoading(UnloadEventPolicyUnloadAndPageHide);
-
-    {
-        // 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));
-    }
-
-    // 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.
-
+    ASSERT(m_document->inPageCache());
+
+    // 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.
     m_document->suspend();
 

trunk/Source/WebCore/history/PageCache.cpp

@@ -42 +42 @@
 #include "FrameView.h"
 #include "HistoryController.h"
+#include "IgnoreOpensDuringUnloadCountIncrementer.h"
 #include "Logging.h"
 #include "MainFrame.h"
@@ -273 +274 @@
 }
     
-bool PageCache::canCache(Page* page) const
-{
-    if (!page)
+bool PageCache::canCache(Page& page) const
+{
+    if (!m_maxSize) {
+        logPageCacheFailureDiagnosticMessage(&page, DiagnosticLoggingKeys::isDisabledKey());
         return false;
-
-    if (!m_maxSize) {
-        logPageCacheFailureDiagnosticMessage(page, DiagnosticLoggingKeys::isDisabledKey());
+    }
+
+    if (MemoryPressureHandler::singleton().isUnderMemoryPressure()) {
+        logPageCacheFailureDiagnosticMessage(&page, DiagnosticLoggingKeys::underMemoryPressureKey());
         return false;
     }
-
-    if (MemoryPressureHandler::singleton().isUnderMemoryPressure()) {
-        logPageCacheFailureDiagnosticMessage(page, DiagnosticLoggingKeys::underMemoryPressureKey());
-        return false;
-    }
-    
-    return canCachePage(*page);
+    
+    return canCachePage(page);
 }
 
@@ -375 +373 @@
 }
 
-void PageCache::add(HistoryItem& item, Page& page)
-{
-    ASSERT(canCache(&page));
-
-    // Remove stale cache entry if necessary.
-    remove(item);
-
-    item.m_cachedPage = std::make_unique<CachedPage>(page);
+static void setInPageCache(Page& page, bool isInPageCache)
+{
+    for (Frame* frame = &page.mainFrame(); frame; frame = frame->tree().traverseNext()) {
+        if (auto* document = frame->document())
+            document->setInPageCache(isInPageCache);
+    }
+}
+
+static void firePageHideEventRecursively(Frame& frame)
+{
+    auto* document = frame.document();
+    if (!document)
+        return;
+
+    // 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);

trunk/Source/WebCore/history/PageCache.h

@@ -47 +47 @@
     WEBCORE_EXPORT static PageCache& singleton();
 
-    bool canCache(Page*) const;
+    bool canCache(Page&) const;
 
     // Used when memory is low to prune some cached pages.
@@ -54 +54 @@
     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*);

trunk/Source/WebCore/loader/FrameLoader.cpp

@@ -1773 +1773 @@
     // 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)

trunk/Source/WebCore/page/Page.cpp

@@ -1864 +1864 @@
 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);
 }