Expand WebCore policy concept of "shouldContinue" to allow for more than true/false
authorbeidson@apple.com <beidson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 10 Apr 2018 00:31:02 +0000 (00:31 +0000)
committerbeidson@apple.com <beidson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 10 Apr 2018 00:31:02 +0000 (00:31 +0000)
https://bugs.webkit.org/show_bug.cgi?id=184424

Reviewed by Alex Christensen.

No new tests (No behavior change, refactor only)

Specifically this expands the "shouldContinue" bool to be an enum class with:
-Yes
-No
-ForSuspension

* loader/DocumentLoader.cpp:
(WebCore::DocumentLoader::willSendRequest):
* loader/FrameLoader.cpp:
(WebCore::FrameLoader::loadURL):
(WebCore::FrameLoader::load):
(WebCore::FrameLoader::loadWithDocumentLoader):
(WebCore::FrameLoader::loadPostRequest):
(WebCore::FrameLoader::continueLoadAfterNavigationPolicy):
(WebCore::FrameLoader::continueLoadAfterNewWindowPolicy):
* loader/FrameLoader.h:
* loader/PolicyChecker.cpp:
(WebCore::PolicyChecker::checkNavigationPolicy):
(WebCore::PolicyChecker::checkNewWindowPolicy):
* loader/PolicyChecker.h:

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

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

index d0892fa..c243160 100644 (file)
@@ -1,3 +1,32 @@
+2018-04-09  Brady Eidson  <beidson@apple.com>
+
+        Expand WebCore policy concept of "shouldContinue" to allow for more than true/false
+        https://bugs.webkit.org/show_bug.cgi?id=184424
+
+        Reviewed by Alex Christensen.
+
+        No new tests (No behavior change, refactor only)
+
+        Specifically this expands the "shouldContinue" bool to be an enum class with:
+        -Yes
+        -No
+        -ForSuspension
+
+        * loader/DocumentLoader.cpp:
+        (WebCore::DocumentLoader::willSendRequest):
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::loadURL):
+        (WebCore::FrameLoader::load):
+        (WebCore::FrameLoader::loadWithDocumentLoader):
+        (WebCore::FrameLoader::loadPostRequest):
+        (WebCore::FrameLoader::continueLoadAfterNavigationPolicy):
+        (WebCore::FrameLoader::continueLoadAfterNewWindowPolicy):
+        * loader/FrameLoader.h:
+        * loader/PolicyChecker.cpp:
+        (WebCore::PolicyChecker::checkNavigationPolicy):
+        (WebCore::PolicyChecker::checkNewWindowPolicy):
+        * loader/PolicyChecker.h:
+
 2018-04-09  Sihui Liu  <sihui_liu@apple.com>
 
         REGRESSION(r229929): localStorage is broken for WebInspector
index 376d56a..80dbf13 100644 (file)
@@ -639,10 +639,19 @@ void DocumentLoader::willSendRequest(ResourceRequest&& newRequest, const Resourc
 
     ASSERT(!m_waitingForNavigationPolicy);
     m_waitingForNavigationPolicy = true;
-    frameLoader()->policyChecker().checkNavigationPolicy(ResourceRequest(newRequest), didReceiveRedirectResponse, [this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler)] (ResourceRequest&& request, FormState*, bool shouldContinue) mutable {
+    frameLoader()->policyChecker().checkNavigationPolicy(ResourceRequest(newRequest), didReceiveRedirectResponse, [this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler)] (ResourceRequest&& request, FormState*, ShouldContinue shouldContinue) mutable {
         m_waitingForNavigationPolicy = false;
-        if (!shouldContinue)
+        switch (shouldContinue) {
+        case ShouldContinue::ForSuspension:
+            FALLTHROUGH;
+            // FIXME: Setup this page for suspension (e.g. Page Cache) here.
+        case ShouldContinue::No:
             stopLoadingForPolicyChange();
+            break;
+        case ShouldContinue::Yes:
+            break;
+        }
+
         completionHandler(WTFMove(request));
     });
 }
index d55d0a2..fe0558d 100644 (file)
@@ -1316,7 +1316,7 @@ void FrameLoader::loadURL(FrameLoadRequest&& frameLoadRequest, const String& ref
 
     if (!targetFrame && !frameName.isEmpty()) {
         action = action.copyWithShouldOpenExternalURLsPolicy(shouldOpenExternalURLsPolicyToApply(m_frame, frameLoadRequest));
-        policyChecker().checkNewWindowPolicy(WTFMove(action), request, formState, frameName, [this, allowNavigationToInvalidURL, openerPolicy, completionHandler = completionHandlerCaller.release()] (const ResourceRequest& request, FormState* formState, const String& frameName, const NavigationAction& action, bool shouldContinue) {
+        policyChecker().checkNewWindowPolicy(WTFMove(action), request, formState, frameName, [this, allowNavigationToInvalidURL, openerPolicy, completionHandler = completionHandlerCaller.release()] (const ResourceRequest& request, FormState* formState, const String& frameName, const NavigationAction& action, ShouldContinue shouldContinue) {
             continueLoadAfterNewWindowPolicy(request, formState, frameName, action, shouldContinue, allowNavigationToInvalidURL, openerPolicy);
             completionHandler();
         });
@@ -1337,10 +1337,10 @@ void FrameLoader::loadURL(FrameLoadRequest&& frameLoadRequest, const String& ref
         policyChecker().stopCheck();
         policyChecker().setLoadType(newLoadType);
         auto completionHandlerCalled = adoptRef(*new SharedBool);
-        policyChecker().checkNavigationPolicy(ResourceRequest(request), false /* didReceiveRedirectResponse */, oldDocumentLoader.get(), formState, [this, protectedFrame = makeRef(m_frame), completionHandlerCalled = completionHandlerCalled.copyRef()] (const ResourceRequest& request, FormState*, bool shouldContinue) {
+        policyChecker().checkNavigationPolicy(ResourceRequest(request), false /* didReceiveRedirectResponse */, oldDocumentLoader.get(), formState, [this, protectedFrame = makeRef(m_frame), completionHandlerCalled = completionHandlerCalled.copyRef()] (const ResourceRequest& request, FormState*, ShouldContinue shouldContinue) {
             if (!completionHandlerCalled->value) {
                 completionHandlerCalled->value = true;
-                continueFragmentScrollAfterNavigationPolicy(request, shouldContinue);
+                continueFragmentScrollAfterNavigationPolicy(request, shouldContinue == ShouldContinue::Yes);
             }
         });
             if (!completionHandlerCalled->value) {
@@ -1399,7 +1399,7 @@ void FrameLoader::load(FrameLoadRequest&& request)
 
     if (request.shouldCheckNewWindowPolicy()) {
         NavigationAction action { request.requester(), request.resourceRequest(), InitiatedByMainFrame::Unknown, NavigationType::Other, request.shouldOpenExternalURLsPolicy() };
-        policyChecker().checkNewWindowPolicy(WTFMove(action), request.resourceRequest(), nullptr, request.frameName(), [this] (const ResourceRequest& request, FormState* formState, const String& frameName, const NavigationAction& action, bool shouldContinue) {
+        policyChecker().checkNewWindowPolicy(WTFMove(action), request.resourceRequest(), nullptr, request.frameName(), [this] (const ResourceRequest& request, FormState* formState, const String& frameName, const NavigationAction& action, ShouldContinue shouldContinue) {
             continueLoadAfterNewWindowPolicy(request, formState, frameName, action, shouldContinue, AllowNavigationToInvalidURL::Yes, NewFrameOpenerPolicy::Suppress);
         });
 
@@ -1513,10 +1513,10 @@ void FrameLoader::loadWithDocumentLoader(DocumentLoader* loader, FrameLoadType t
         oldDocumentLoader->setLastCheckedRequest(ResourceRequest());
         policyChecker().stopCheck();
         auto completionHandlerCalled = adoptRef(*new SharedBool);
-        policyChecker().checkNavigationPolicy(ResourceRequest(loader->request()), false /* didReceiveRedirectResponse */, oldDocumentLoader.get(), formState, [this, protectedFrame = makeRef(m_frame), completionHandlerCalled = completionHandlerCalled.copyRef()] (const ResourceRequest& request, FormState*, bool shouldContinue) {
+        policyChecker().checkNavigationPolicy(ResourceRequest(loader->request()), false /* didReceiveRedirectResponse */, oldDocumentLoader.get(), formState, [this, protectedFrame = makeRef(m_frame), completionHandlerCalled = completionHandlerCalled.copyRef()] (const ResourceRequest& request, FormState*, ShouldContinue shouldContinue) {
             if (!completionHandlerCalled->value) {
                 completionHandlerCalled->value = true;
-                continueFragmentScrollAfterNavigationPolicy(request, shouldContinue);
+                continueFragmentScrollAfterNavigationPolicy(request, shouldContinue == ShouldContinue::Yes);
             }
         });
         if (!completionHandlerCalled->value) {
@@ -1542,7 +1542,7 @@ void FrameLoader::loadWithDocumentLoader(DocumentLoader* loader, FrameLoadType t
         // a new URL, the parent frame shouldn't learn the URL.
         if (!m_stateMachine.committedFirstRealDocumentLoad()
             && !ownerElement->dispatchBeforeLoadEvent(loader->request().url().string())) {
-            continueLoadAfterNavigationPolicy(loader->request(), formState, false, allowNavigationToInvalidURL);
+            continueLoadAfterNavigationPolicy(loader->request(), formState, ShouldContinue::No, allowNavigationToInvalidURL);
             return;
         }
     }
@@ -1550,11 +1550,11 @@ void FrameLoader::loadWithDocumentLoader(DocumentLoader* loader, FrameLoadType t
     m_frame.navigationScheduler().cancel(true);
 
     if (!m_currentLoadShouldCheckNavigationPolicy) {
-        continueLoadAfterNavigationPolicy(loader->request(), formState, true, allowNavigationToInvalidURL);
+        continueLoadAfterNavigationPolicy(loader->request(), formState, ShouldContinue::Yes, allowNavigationToInvalidURL);
         return;
     }
 
-    policyChecker().checkNavigationPolicy(ResourceRequest(loader->request()), false /* didReceiveRedirectResponse */, loader, formState, [this, protectedFrame = makeRef(m_frame), allowNavigationToInvalidURL, completionHandler = completionHandlerCaller.release()] (const ResourceRequest& request, FormState* formState, bool shouldContinue) {
+    policyChecker().checkNavigationPolicy(ResourceRequest(loader->request()), false /* didReceiveRedirectResponse */, loader, formState, [this, protectedFrame = makeRef(m_frame), allowNavigationToInvalidURL, completionHandler = completionHandlerCaller.release()] (const ResourceRequest& request, FormState* formState, ShouldContinue shouldContinue) {
         continueLoadAfterNavigationPolicy(request, formState, shouldContinue, allowNavigationToInvalidURL);
         completionHandler();
     });
@@ -2814,7 +2814,7 @@ void FrameLoader::loadPostRequest(FrameLoadRequest&& request, const String& refe
             return;
         }
 
-        policyChecker().checkNewWindowPolicy(WTFMove(action), workingResourceRequest, WTFMove(formState), frameName, [this, allowNavigationToInvalidURL, openerPolicy, completionHandler = WTFMove(completionHandler)] (const ResourceRequest& request, FormState* formState, const String& frameName, const NavigationAction& action, bool shouldContinue) {
+        policyChecker().checkNewWindowPolicy(WTFMove(action), workingResourceRequest, WTFMove(formState), frameName, [this, allowNavigationToInvalidURL, openerPolicy, completionHandler = WTFMove(completionHandler)] (const ResourceRequest& request, FormState* formState, const String& frameName, const NavigationAction& action, ShouldContinue shouldContinue) {
             continueLoadAfterNewWindowPolicy(request, formState, frameName, action, shouldContinue, allowNavigationToInvalidURL, openerPolicy);
             completionHandler();
         });
@@ -3169,7 +3169,7 @@ bool FrameLoader::dispatchBeforeUnloadEvent(Chrome& chrome, FrameLoader* frameLo
     return chrome.runBeforeUnloadConfirmPanel(text, m_frame);
 }
 
-void FrameLoader::continueLoadAfterNavigationPolicy(const ResourceRequest& request, FormState* formState, bool shouldContinue, AllowNavigationToInvalidURL allowNavigationToInvalidURL)
+void FrameLoader::continueLoadAfterNavigationPolicy(const ResourceRequest& request, FormState* formState, ShouldContinue shouldContinue, AllowNavigationToInvalidURL allowNavigationToInvalidURL)
 {
     // If we loaded an alternate page to replace an unreachableURL, we'll get in here with a
     // nil policyDataSource because loading the alternate page will have passed
@@ -3179,13 +3179,7 @@ void FrameLoader::continueLoadAfterNavigationPolicy(const ResourceRequest& reque
     bool isTargetItem = history().provisionalItem() ? history().provisionalItem()->isTargetItem() : false;
 
     bool urlIsDisallowed = allowNavigationToInvalidURL == AllowNavigationToInvalidURL::No && !request.url().isValid();
-
-    // Three reasons we can't continue:
-    //    1) Navigation policy delegate said we can't so request is nil. A primary case of this 
-    //       is the user responding Cancel to the form repost nag sheet.
-    //    2) User responded Cancel to an alert popped up by the before unload event handler.
-    //    3) The request's URL is invalid and navigation to invalid URLs is disallowed.
-    bool canContinue = shouldContinue && shouldClose() && !urlIsDisallowed;
+    bool canContinue = shouldContinue != ShouldContinue::No && shouldClose() && !urlIsDisallowed;
 
     if (!canContinue) {
         // If we were waiting for a quick redirect, but the policy delegate decided to ignore it, then we 
@@ -3269,9 +3263,10 @@ void FrameLoader::continueLoadAfterNavigationPolicy(const ResourceRequest& reque
 }
 
 void FrameLoader::continueLoadAfterNewWindowPolicy(const ResourceRequest& request,
-    FormState* formState, const String& frameName, const NavigationAction& action, bool shouldContinue, AllowNavigationToInvalidURL allowNavigationToInvalidURL, NewFrameOpenerPolicy openerPolicy)
+    FormState* formState, const String& frameName, const NavigationAction& action, ShouldContinue shouldContinue, AllowNavigationToInvalidURL allowNavigationToInvalidURL, NewFrameOpenerPolicy openerPolicy)
 {
-    if (!shouldContinue)
+    ASSERT(shouldContinue != ShouldContinue::ForSuspension);
+    if (shouldContinue != ShouldContinue::Yes)
         return;
 
     Ref<Frame> frame(m_frame);
index 7e5ed5d..129a83c 100644 (file)
@@ -81,6 +81,7 @@ class SubframeLoader;
 class SubstituteData;
 
 enum class NavigationPolicyCheck;
+enum class ShouldContinue;
 
 struct WindowFeatures;
 
@@ -338,8 +339,8 @@ private:
     bool dispatchBeforeUnloadEvent(Chrome&, FrameLoader* frameLoaderBeingNavigated);
     void dispatchUnloadEvents(UnloadEventPolicy);
 
-    void continueLoadAfterNavigationPolicy(const ResourceRequest&, FormState*, bool shouldContinue, AllowNavigationToInvalidURL);
-    void continueLoadAfterNewWindowPolicy(const ResourceRequest&, FormState*, const String& frameName, const NavigationAction&, bool shouldContinue, AllowNavigationToInvalidURL, NewFrameOpenerPolicy);
+    void continueLoadAfterNavigationPolicy(const ResourceRequest&, FormState*, ShouldContinue, AllowNavigationToInvalidURL);
+    void continueLoadAfterNewWindowPolicy(const ResourceRequest&, FormState*, const String& frameName, const NavigationAction&, ShouldContinue, AllowNavigationToInvalidURL, NewFrameOpenerPolicy);
     void continueFragmentScrollAfterNavigationPolicy(const ResourceRequest&, bool shouldContinue);
 
     bool shouldPerformFragmentNavigation(bool isFormSubmission, const String& httpMethod, FrameLoadType, const URL&);
index 42d52c1..3702580 100644 (file)
@@ -92,7 +92,7 @@ void PolicyChecker::checkNavigationPolicy(ResourceRequest&& request, bool didRec
     // Don't ask more than once for the same request or if we are loading an empty URL.
     // This avoids confusion on the part of the client.
     if (equalIgnoringHeaderFields(request, loader->lastCheckedRequest()) || (!request.isNull() && request.url().isEmpty())) {
-        function(ResourceRequest(request), nullptr, true);
+        function(ResourceRequest(request), nullptr, ShouldContinue::Yes);
         loader->setLastCheckedRequest(WTFMove(request));
         return;
     }
@@ -107,7 +107,7 @@ void PolicyChecker::checkNavigationPolicy(ResourceRequest&& request, bool didRec
 #endif
         if (isBackForwardLoadType(m_loadType))
             m_loadType = FrameLoadType::Reload;
-        function(WTFMove(request), nullptr, shouldContinue);
+        function(WTFMove(request), nullptr, shouldContinue ? ShouldContinue::Yes : ShouldContinue::No);
         return;
     }
 
@@ -117,19 +117,19 @@ void PolicyChecker::checkNavigationPolicy(ResourceRequest&& request, bool didRec
             // reveal that the frame was blocked. This way, it looks like any other cross-origin page load.
             m_frame.ownerElement()->dispatchEvent(Event::create(eventNames().loadEvent, false, false));
         }
-        function(WTFMove(request), nullptr, false);
+        function(WTFMove(request), nullptr, ShouldContinue::No);
         return;
     }
 
     loader->setLastCheckedRequest(ResourceRequest(request));
 
     if (request.url() == blankURL())
-        return function(WTFMove(request), formState, true);
+        return function(WTFMove(request), formState, ShouldContinue::Yes);
 
 #if USE(QUICK_LOOK)
     // Always allow QuickLook-generated URLs based on the protocol scheme.
     if (!request.isNull() && isQuickLookPreviewURL(request.url()))
-        return function(WTFMove(request), formState, true);
+        return function(WTFMove(request), formState, ShouldContinue::Yes);
 #endif
 
 #if ENABLE(CONTENT_FILTERING)
@@ -139,7 +139,7 @@ void PolicyChecker::checkNavigationPolicy(ResourceRequest&& request, bool didRec
             if (unblocked)
                 frame->loader().reload();
         });
-        return function({ }, nullptr, false);
+        return function({ }, nullptr, ShouldContinue::No);
     }
     m_contentFilterUnblockHandler = { };
 #endif
@@ -160,16 +160,15 @@ void PolicyChecker::checkNavigationPolicy(ResourceRequest&& request, bool didRec
             m_frame.loader().client().startDownload(request, suggestedFilename);
             FALLTHROUGH;
         case PolicyAction::Ignore:
-            return function({ }, nullptr, false);
+            return function({ }, nullptr, ShouldContinue::No);
         case PolicyAction::Suspend:
-            LOG(Loading, "PolicyAction::Suspend encountered - Treating as PolicyAction::Ignore for now");
-            return function({ }, nullptr, false);
+            return function({ }, nullptr, ShouldContinue::ForSuspension);
         case PolicyAction::Use:
             if (!m_frame.loader().client().canHandleRequest(request)) {
                 handleUnimplementablePolicy(m_frame.loader().client().cannotShowURLError(request));
-                return function({ }, nullptr, false);
+                return function({ }, nullptr, ShouldContinue::No);
             }
-            return function(WTFMove(request), formState.get(), true);
+            return function(WTFMove(request), formState.get(), ShouldContinue::Yes);
         }
         ASSERT_NOT_REACHED();
     });
@@ -178,10 +177,10 @@ void PolicyChecker::checkNavigationPolicy(ResourceRequest&& request, bool didRec
 void PolicyChecker::checkNewWindowPolicy(NavigationAction&& navigationAction, const ResourceRequest& request, FormState* formState, const String& frameName, NewWindowPolicyDecisionFunction&& function)
 {
     if (m_frame.document() && m_frame.document()->isSandboxed(SandboxPopups))
-        return function({ }, nullptr, { }, { }, false);
+        return function({ }, nullptr, { }, { }, ShouldContinue::No);
 
     if (!DOMWindow::allowPopUp(m_frame))
-        return function({ }, nullptr, { }, { }, false);
+        return function({ }, nullptr, { }, { }, ShouldContinue::No);
 
     m_frame.loader().client().dispatchDecidePolicyForNewWindowAction(navigationAction, request, formState, frameName, [frame = makeRef(m_frame), request, formState = makeRefPtr(formState), frameName, navigationAction, function = WTFMove(function)](PolicyAction policyAction) mutable {
         switch (policyAction) {
@@ -189,13 +188,13 @@ void PolicyChecker::checkNewWindowPolicy(NavigationAction&& navigationAction, co
             frame->loader().client().startDownload(request);
             FALLTHROUGH;
         case PolicyAction::Ignore:
-            function({ }, nullptr, { }, { }, false);
+            function({ }, nullptr, { }, { }, ShouldContinue::No);
             return;
         case PolicyAction::Suspend:
             // It is invalid to get a "Suspend" policy for new windows, as the old document is not going away.
             RELEASE_ASSERT_NOT_REACHED();
         case PolicyAction::Use:
-            function(request, formState.get(), frameName, navigationAction, true);
+            function(request, formState.get(), frameName, navigationAction, ShouldContinue::Yes);
             return;
         }
         ASSERT_NOT_REACHED();
index d0f0bc2..2fdb250 100644 (file)
@@ -50,8 +50,14 @@ class NavigationAction;
 class ResourceError;
 class ResourceResponse;
 
-using NewWindowPolicyDecisionFunction = WTF::CompletionHandler<void(const ResourceRequest&, FormState*, const String& frameName, const NavigationAction&, bool shouldContinue)>;
-using NavigationPolicyDecisionFunction = WTF::CompletionHandler<void(ResourceRequest&&, FormState*, bool shouldContinue)>;
+enum class ShouldContinue {
+    Yes,
+    No,
+    ForSuspension
+};
+
+using NewWindowPolicyDecisionFunction = CompletionHandler<void(const ResourceRequest&, FormState*, const String& frameName, const NavigationAction&, ShouldContinue)>;
+using NavigationPolicyDecisionFunction = CompletionHandler<void(ResourceRequest&&, FormState*, ShouldContinue)>;
 
 class PolicyChecker {
     WTF_MAKE_NONCOPYABLE(PolicyChecker);