Adding logging to diagnose crashes resulting from provisional document loader unexpec...
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 5 Nov 2019 21:47:20 +0000 (21:47 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 5 Nov 2019 21:47:20 +0000 (21:47 +0000)
https://bugs.webkit.org/show_bug.cgi?id=203837

Reviewed by Geoffrey Garen.

Added various logging for DocumentLoader and FrameLoader to figure out why
FrameLoader::m_provisionalDocumentLoader can be nullptr in some cases.

* loader/DocumentLoader.cpp:
(WebCore::DocumentLoader::setRequest):
(WebCore::DocumentLoader::willSendRequest):
* loader/FrameLoader.cpp:
(WebCore::FrameLoader::setupForReplace):
(WebCore::FrameLoader::clearProvisionalLoadForPolicyCheck):
(WebCore::FrameLoader::stopAllLoaders):
(WebCore::FrameLoader::stopForBackForwardCache):
(WebCore::FrameLoader::clearProvisionalLoad):
(WebCore::FrameLoader::transitionToCommitted):
(WebCore::FrameLoader::continueFragmentScrollAfterNavigationPolicy):
(WebCore::FrameLoader::continueLoadAfterNavigationPolicy):

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

Source/WebCore/ChangeLog
Source/WebCore/loader/DocumentLoader.cpp
Source/WebCore/loader/FrameLoader.cpp

index c990ebc..7d79022 100644 (file)
@@ -1,3 +1,26 @@
+2019-11-05  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Adding logging to diagnose crashes resulting from provisional document loader unexpectedly being nullptr
+        https://bugs.webkit.org/show_bug.cgi?id=203837
+
+        Reviewed by Geoffrey Garen.
+
+        Added various logging for DocumentLoader and FrameLoader to figure out why
+        FrameLoader::m_provisionalDocumentLoader can be nullptr in some cases.
+
+        * loader/DocumentLoader.cpp:
+        (WebCore::DocumentLoader::setRequest):
+        (WebCore::DocumentLoader::willSendRequest):
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::setupForReplace):
+        (WebCore::FrameLoader::clearProvisionalLoadForPolicyCheck):
+        (WebCore::FrameLoader::stopAllLoaders):
+        (WebCore::FrameLoader::stopForBackForwardCache):
+        (WebCore::FrameLoader::clearProvisionalLoad):
+        (WebCore::FrameLoader::transitionToCommitted):
+        (WebCore::FrameLoader::continueFragmentScrollAfterNavigationPolicy):
+        (WebCore::FrameLoader::continueLoadAfterNavigationPolicy):
+
 2019-11-05  Oriol Brufau  <obrufau@igalia.com>
 
         [css-lists] Implement list-style-type: <string>
index fc33301..d4ab3ec 100644 (file)
@@ -223,8 +223,12 @@ void DocumentLoader::setRequest(const ResourceRequest& req)
     ASSERT(!m_committed);
 
     m_request = req;
-    if (shouldNotifyAboutProvisionalURLChange)
+    if (shouldNotifyAboutProvisionalURLChange) {
+        // Logging for <rdar://problem/54830233>.
+        if (!frameLoader()->provisionalDocumentLoader())
+            RELEASE_LOG_IF_ALLOWED("DocumentLoader::setRequest: With no provisional document loader (frame = %p, main = %d)", m_frame, m_frame ? m_frame->isMainFrame() : false);
         frameLoader()->client().dispatchDidChangeProvisionalURL();
+    }
 }
 
 void DocumentLoader::setMainDocumentError(const ResourceError& error)
@@ -570,6 +574,10 @@ void DocumentLoader::willSendRequest(ResourceRequest&& newRequest, const Resourc
     // callbacks is meant to prevent.
     ASSERT(!newRequest.isNull());
 
+    // Logging for <rdar://problem/54830233>.
+    if (!frameLoader() || !frameLoader()->provisionalDocumentLoader())
+        RELEASE_LOG_IF_ALLOWED("willSendRequest: With no provisional document loader (frame = %p, main = %d)", m_frame, m_frame ? m_frame->isMainFrame() : false);
+
     bool didReceiveRedirectResponse = !redirectResponse.isNull();
     if (!frameLoader()->checkIfFormActionAllowedByCSP(newRequest.url(), didReceiveRedirectResponse)) {
         RELEASE_LOG_IF_ALLOWED("willSendRequest: canceling - form action not allowed by CSP (frame = %p, main = %d)", m_frame, m_frame->isMainFrame());
index 26246f7..101669e 100644 (file)
@@ -1237,6 +1237,7 @@ void FrameLoader::setupForReplace()
     m_client.revertToProvisionalState(m_documentLoader.get());
     setState(FrameStateProvisional);
     m_provisionalDocumentLoader = m_documentLoader;
+    RELEASE_LOG_IF_ALLOWED("setupForReplace: Setting provisional document loader (frame = %p, main = %d m_provisionalDocumentLoader=%p)", &m_frame, m_frame.isMainFrame(), m_provisionalDocumentLoader.get());
     m_documentLoader = nullptr;
     detachChildren();
 }
@@ -1651,6 +1652,7 @@ void FrameLoader::clearProvisionalLoadForPolicyCheck()
 
     SetForScope<bool> change(m_inClearProvisionalLoadForPolicyCheck, true);
     m_provisionalDocumentLoader->stopLoading();
+    RELEASE_LOG_IF_ALLOWED("clearProvisionalLoadForPolicyCheck: Clearing provisional document loader (frame = %p, main = %d m_provisionalDocumentLoader=%p)", &m_frame, m_frame.isMainFrame(), m_provisionalDocumentLoader.get());
     setProvisionalDocumentLoader(nullptr);
 }
 
@@ -1839,6 +1841,7 @@ void FrameLoader::stopAllLoaders(ClearProvisionalItemPolicy clearProvisionalItem
     if (m_documentLoader)
         m_documentLoader->stopLoading();
 
+    RELEASE_LOG_IF_ALLOWED("allAllLoaders: Clearing provisional document loader (frame = %p, main = %d m_provisionalDocumentLoader=%p)", &m_frame, m_frame.isMainFrame(), m_provisionalDocumentLoader.get());
     setProvisionalDocumentLoader(nullptr);
 
     m_inStopAllLoaders = false;    
@@ -1850,6 +1853,7 @@ void FrameLoader::stopForBackForwardCache()
     if (!m_frame.isMainFrame()) {
         if (m_provisionalDocumentLoader)
             m_provisionalDocumentLoader->stopLoading();
+        RELEASE_LOG_IF_ALLOWED("stopForBackForwardCache: Clearing provisional document loader (frame = %p, main = %d m_provisionalDocumentLoader=%p)", &m_frame, m_frame.isMainFrame(), m_provisionalDocumentLoader.get());
         setProvisionalDocumentLoader(nullptr);
     }
 
@@ -1996,6 +2000,7 @@ void FrameLoader::setState(FrameState newState)
 
 void FrameLoader::clearProvisionalLoad()
 {
+    RELEASE_LOG_IF_ALLOWED("clearProvisionalLoad: Clearing provisional document loader (frame = %p, main = %d m_provisionalDocumentLoader=%p)", &m_frame, m_frame.isMainFrame(), m_provisionalDocumentLoader.get());
     setProvisionalDocumentLoader(nullptr);
     m_progressTracker->progressCompleted();
     setState(FrameStateComplete);
@@ -2161,6 +2166,7 @@ void FrameLoader::transitionToCommitted(CachedPage* cachedPage)
     setDocumentLoader(m_provisionalDocumentLoader.get());
     if (pdl != m_provisionalDocumentLoader)
         return;
+    RELEASE_LOG_IF_ALLOWED("transitionToCommitted: Clearing provisional document loader (frame = %p, main = %d m_provisionalDocumentLoader=%p)", &m_frame, m_frame.isMainFrame(), m_provisionalDocumentLoader.get());
     setProvisionalDocumentLoader(nullptr);
 
     // Nothing else can interrupt this commit - set the Provisional->Committed transition in stone
@@ -3205,6 +3211,7 @@ void FrameLoader::continueFragmentScrollAfterNavigationPolicy(const ResourceRequ
     // If we have a provisional request for a different document, a fragment scroll should cancel it.
     if (m_provisionalDocumentLoader && !equalIgnoringFragmentIdentifier(m_provisionalDocumentLoader->request().url(), request.url())) {
         m_provisionalDocumentLoader->stopLoading();
+        RELEASE_LOG_IF_ALLOWED("continueFragmentScrollAfterNavigationPolicy: Clearing provisional document loader (frame = %p, main = %d m_provisionalDocumentLoader=%p)", &m_frame, m_frame.isMainFrame(), m_provisionalDocumentLoader.get());
         setProvisionalDocumentLoader(nullptr);
     }
 
@@ -3490,6 +3497,7 @@ void FrameLoader::continueLoadAfterNavigationPolicy(const ResourceRequest& reque
     }
 
     setProvisionalDocumentLoader(m_policyDocumentLoader.get());
+    RELEASE_LOG_IF_ALLOWED("continueLoadAfterNavigationPolicy: Setting provisional document loader (frame = %p, main = %d m_provisionalDocumentLoader=%p)", &m_frame, m_frame.isMainFrame(), m_provisionalDocumentLoader.get());
     m_loadType = type;
     setState(FrameStateProvisional);