WebContent crash in WebCore::Page::sessionID() const + 0 (Page.cpp:1660)
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 29 May 2015 23:42:24 +0000 (23:42 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 29 May 2015 23:42:24 +0000 (23:42 +0000)
https://bugs.webkit.org/show_bug.cgi?id=145422
<rdar://problem/20613631>

Reviewed by Brady Eidson.

We sometimes crash when destroying a PageCache CachedFrame because its
DocumentLoader is still loading. This should never happen as we are not
supposed to let pages are still have pending loads into the PageCache.

However, we were using DocumentLoader::isLoadingInAPISense() as check
in PageCache::canCachePageContainingThisFrame() which is not exactly
what we want. isLoadingInAPISense() no longer considers subresource
loads once the frame as loaded. This means if the JS triggers a new
load in a subframe after it has been loaded, then isLoadingInAPISense()
will return false, despite the pending load.

This patch replaces the isLoadingInAPISense() check with isLoading()
as this will consider all pending loads, even after the frame is
loaded.

In most cases, using isLoadingInAPISense() was not an issue because
we call DocumentLoader::stopLoading() in all subframes before starting
a provisional load. However, nothing seems to prevent JS from
triggering a new load after that and before the new load gets committed
(which is when we save the page into PageCache).

No new test as we don't have a reliable reproduction case and the
issue is timing related.

* history/PageCache.cpp:
(WebCore::logCanCacheFrameDecision):
(WebCore::PageCache::canCachePageContainingThisFrame):
* page/DiagnosticLoggingKeys.cpp:
(WebCore::DiagnosticLoggingKeys::isLoading):
(WebCore::DiagnosticLoggingKeys::loadingAPISenseKey): Deleted.
* page/DiagnosticLoggingKeys.h:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@185017 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/history/PageCache.cpp
Source/WebCore/page/DiagnosticLoggingKeys.cpp
Source/WebCore/page/DiagnosticLoggingKeys.h

index 0077496..c7ee44f 100644 (file)
@@ -1,5 +1,45 @@
 2015-05-29  Chris Dumez  <cdumez@apple.com>
 
+        WebContent crash in WebCore::Page::sessionID() const + 0 (Page.cpp:1660)
+        https://bugs.webkit.org/show_bug.cgi?id=145422
+        <rdar://problem/20613631>
+
+        Reviewed by Brady Eidson.
+
+        We sometimes crash when destroying a PageCache CachedFrame because its
+        DocumentLoader is still loading. This should never happen as we are not
+        supposed to let pages are still have pending loads into the PageCache.
+
+        However, we were using DocumentLoader::isLoadingInAPISense() as check
+        in PageCache::canCachePageContainingThisFrame() which is not exactly
+        what we want. isLoadingInAPISense() no longer considers subresource
+        loads once the frame as loaded. This means if the JS triggers a new
+        load in a subframe after it has been loaded, then isLoadingInAPISense()
+        will return false, despite the pending load.
+
+        This patch replaces the isLoadingInAPISense() check with isLoading()
+        as this will consider all pending loads, even after the frame is
+        loaded.
+
+        In most cases, using isLoadingInAPISense() was not an issue because
+        we call DocumentLoader::stopLoading() in all subframes before starting
+        a provisional load. However, nothing seems to prevent JS from
+        triggering a new load after that and before the new load gets committed
+        (which is when we save the page into PageCache).
+
+        No new test as we don't have a reliable reproduction case and the
+        issue is timing related.
+
+        * history/PageCache.cpp:
+        (WebCore::logCanCacheFrameDecision):
+        (WebCore::PageCache::canCachePageContainingThisFrame):
+        * page/DiagnosticLoggingKeys.cpp:
+        (WebCore::DiagnosticLoggingKeys::isLoading):
+        (WebCore::DiagnosticLoggingKeys::loadingAPISenseKey): Deleted.
+        * page/DiagnosticLoggingKeys.h:
+
+2015-05-29  Chris Dumez  <cdumez@apple.com>
+
         Consider throttling DOM timers in iframes outside the viewport
         https://bugs.webkit.org/show_bug.cgi?id=145465
         <rdar://problem/20768957>
index 94eadaa..05a00e9 100644 (file)
@@ -86,7 +86,7 @@ enum ReasonFrameCannotBeInPageCache {
     HasSharedWorkers, // FIXME: Remove.
     NoHistoryItem,
     QuickRedirectComing,
-    IsLoadingInAPISense,
+    IsLoading,
     IsStopping,
     CannotSuspendActiveDOMObjects,
     DocumentLoaderUsesApplicationCache,
@@ -159,10 +159,10 @@ static unsigned logCanCacheFrameDecision(Frame& frame, DiagnosticLoggingClient&
         logPageCacheFailureDiagnosticMessage(diagnosticLoggingClient, DiagnosticLoggingKeys::quirkRedirectComingKey());
         rejectReasons |= 1 << QuickRedirectComing;
     }
-    if (frame.loader().documentLoader()->isLoadingInAPISense()) {
-        PCLOG("   -DocumentLoader is still loading in API sense");
-        logPageCacheFailureDiagnosticMessage(diagnosticLoggingClient, DiagnosticLoggingKeys::loadingAPISenseKey());
-        rejectReasons |= 1 << IsLoadingInAPISense;
+    if (frame.loader().documentLoader()->isLoading()) {
+        PCLOG("   -DocumentLoader is still loading");
+        logPageCacheFailureDiagnosticMessage(diagnosticLoggingClient, DiagnosticLoggingKeys::isLoadingKey());
+        rejectReasons |= 1 << IsLoading;
     }
     if (frame.loader().documentLoader()->isStopping()) {
         PCLOG("   -DocumentLoader is in the middle of stopping");
@@ -308,7 +308,7 @@ bool PageCache::canCachePageContainingThisFrame(Frame& frame)
         && !(frame.isMainFrame() && document->url().protocolIs("https") && documentLoader->response().cacheControlContainsNoStore())
         && frameLoader.history().currentItem()
         && !frameLoader.quickRedirectComing()
-        && !documentLoader->isLoadingInAPISense()
+        && !documentLoader->isLoading()
         && !documentLoader->isStopping()
         && document->canSuspendActiveDOMObjectsForPageCache()
         // FIXME: We should investigating caching frames that have an associated
index 1a0f3d7..e87da36 100644 (file)
@@ -248,9 +248,9 @@ String DiagnosticLoggingKeys::reasonKey()
     return ASCIILiteral("reason");
 }
 
-String DiagnosticLoggingKeys::loadingAPISenseKey()
+String DiagnosticLoggingKeys::isLoadingKey()
 {
-    return ASCIILiteral("loadingAPISense");
+    return ASCIILiteral("isLoading");
 }
 
 String DiagnosticLoggingKeys::documentLoaderStoppingKey()
index c4ddb2f..a98e7d2 100644 (file)
@@ -60,7 +60,7 @@ public:
     WEBCORE_EXPORT static String isReloadIgnoringCacheDataKey();
     static String loadedKey();
     static String loadingKey();
-    static String loadingAPISenseKey();
+    static String isLoadingKey();
     static String mainDocumentErrorKey();
     static String mainResourceKey();
     static String mediaKey();