REGRESSION(r229722): WebKitLegacy clients can crash when loading alternate page
authorbfulgham@apple.com <bfulgham@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 25 Jun 2018 21:29:50 +0000 (21:29 +0000)
committerbfulgham@apple.com <bfulgham@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 25 Jun 2018 21:29:50 +0000 (21:29 +0000)
https://bugs.webkit.org/show_bug.cgi?id=187008

Reviewed by Chris Dumez.

The new call to 'clearProvisionalLoadForPolicyCheck' added in r229722 broke loading
behavior in WebKitLegacy.

1. We can now enter 'cancelPolicyCheckIfNeeded' without a Frame loader, in what appears
   to be a recursive call during the load cancellation (the 'm_waitingForContentPolicy'
   and 'm_waitingForNavigationPolicy' have already been nulled). It seems like we should
   return early here, or perhaps just move the RELEASE_ASSERT inside the case where we
   have an active policy check happening.

2. We also enter FrameLoader::checkContentPolicy without an active document loader. We
   should recognize this case and handle it, rather than trying to dereference a nullptr
   document loader.

* loader/DocumentLoader.cpp:
(WebCore::DocumentLoader::cancelPolicyCheckIfNeeded): Move the RELEASE_ASSERT inside the
conditional where the frameLoader is actually used.
* loader/FrameLoader.cpp:
(WebCore::FrameLoader::checkContentPolicy): Recognize that the activeDocumentLoader may
be nullptr at this point, and take appropriate action (rather than crashing).

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

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

index 21f283e..4cb3fc5 100644 (file)
@@ -1,3 +1,30 @@
+2018-06-25  Brent Fulgham  <bfulgham@apple.com>
+
+        REGRESSION(r229722): WebKitLegacy clients can crash when loading alternate page
+        https://bugs.webkit.org/show_bug.cgi?id=187008
+        
+        Reviewed by Chris Dumez.
+
+        The new call to 'clearProvisionalLoadForPolicyCheck' added in r229722 broke loading
+        behavior in WebKitLegacy.
+
+        1. We can now enter 'cancelPolicyCheckIfNeeded' without a Frame loader, in what appears
+           to be a recursive call during the load cancellation (the 'm_waitingForContentPolicy'
+           and 'm_waitingForNavigationPolicy' have already been nulled). It seems like we should
+           return early here, or perhaps just move the RELEASE_ASSERT inside the case where we
+           have an active policy check happening.
+
+        2. We also enter FrameLoader::checkContentPolicy without an active document loader. We
+           should recognize this case and handle it, rather than trying to dereference a nullptr
+           document loader.
+
+        * loader/DocumentLoader.cpp:
+        (WebCore::DocumentLoader::cancelPolicyCheckIfNeeded): Move the RELEASE_ASSERT inside the
+        conditional where the frameLoader is actually used.
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::checkContentPolicy): Recognize that the activeDocumentLoader may
+        be nullptr at this point, and take appropriate action (rather than crashing).
+
 2018-06-25  Simon Fraser  <simon.fraser@apple.com>
 
         MatchedPropertiesCacheItem wastes 388KB of vector capacity on nytimes.com
index ad4d01f..b51cf80 100644 (file)
@@ -1813,9 +1813,8 @@ void DocumentLoader::loadMainResource(ResourceRequest&& request)
 
 void DocumentLoader::cancelPolicyCheckIfNeeded()
 {
-    RELEASE_ASSERT(frameLoader());
-
     if (m_waitingForContentPolicy || m_waitingForNavigationPolicy) {
+        RELEASE_ASSERT(frameLoader());
         frameLoader()->policyChecker().stopCheck();
         m_waitingForContentPolicy = false;
         m_waitingForNavigationPolicy = false;
index 3f20375..4ee5246 100644 (file)
@@ -362,6 +362,12 @@ void FrameLoader::setDefersLoading(bool defers)
 
 void FrameLoader::checkContentPolicy(const ResourceResponse& response, ContentPolicyDecisionFunction&& function)
 {
+    if (!activeDocumentLoader()) {
+        // Load was cancelled
+        function(PolicyAction::Ignore);
+        return;
+    }
+
     client().dispatchDecidePolicyForResponse(response, activeDocumentLoader()->request(), WTFMove(function));
 }