Attachment 362467 Details for Bug 194820 – Patch for safari-607 branch with release build fix

[patch] Patch for safari-607 branch with release build fix

fix194820-for-safari-607c.patch (text/plain), 13.22 KB, created by Ryosuke Niwa on 2019-02-19 18:21:31 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Ryosuke Niwa

Created: 2019-02-19 18:21:31 PST

Size: 13.22 KB

patch

obsolete

>Index: Source/WebCore/ChangeLog
>===================================================================
>--- Source/WebCore/ChangeLog	(revision 241780)
>+++ Source/WebCore/ChangeLog	(working copy)
>@@ -1,3 +1,27 @@
>+2019-02-19  Ryosuke Niwa  <rniwa@webkit.org>
>+
>+        REGRESSION(r240909): Release assertion in FrameLoader::loadPostRequest when opening new window
>+        https://bugs.webkit.org/show_bug.cgi?id=194820
>+
>+        Reviewed by Geoffrey Garen and Brady Eidson.
>+
>+        This release assertion was wrong. The invocation of PolicyChecker::checkNewWindowPolicy in FrameLoader
>+        doesnât require PolicyChecker's load type to be set in PolicyChecker because FrameLoader's
>+        continueLoadAfterNewWindowPolicy invokes loadWithNavigationAction which sets the load type later,
>+        and we don't rely on PolicyChecker's load type until then.
>+
>+        Fixed the crash by removing relese asserts before invoking checkNewWindowPolicy accordingly.
>+
>+        This patch reverts r241015 since it too was asserting that PolicyChecker's load type is set before
>+        invoking checkNewWindowPolicy which is not the right assumption.
>+
>+        Test: fast/loader/navigate-with-post-to-new-target-after-back-forward-navigation.html
>+
>+        * loader/FrameLoader.cpp:
>+        (WebCore::FrameLoader::loadURL):
>+        (WebCore::FrameLoader::load):
>+        (WebCore::FrameLoader::loadPostRequest):
>+
> 2019-02-18  Babak Shafiei  <bshafiei@apple.com>
> 
>         Apply patch. rdar://problem/48122553
>Index: Source/WebCore/loader/DocumentLoader.cpp
>===================================================================
>--- Source/WebCore/loader/DocumentLoader.cpp	(revision 241780)
>+++ Source/WebCore/loader/DocumentLoader.cpp	(working copy)
>@@ -664,7 +664,7 @@ void DocumentLoader::willSendRequest(Res
> 
>     // FIXME: Add a load type check.
>     auto& policyChecker = frameLoader()->policyChecker();
>-    RELEASE_ASSERT(!isBackForwardLoadType(policyChecker.loadType()) || frameLoader()->history().provisionalItem());
>+    ASSERT(!isBackForwardLoadType(policyChecker.loadType()) || frameLoader()->history().provisionalItem());
>     policyChecker.checkNavigationPolicy(WTFMove(newRequest), redirectResponse, WTFMove(navigationPolicyCompletionHandler));
> }
> 
>@@ -845,7 +845,11 @@ void DocumentLoader::responseReceived(co
>     auto requestIdentifier = PolicyCheckIdentifier::create();
>     frameLoader()->checkContentPolicy(m_response, requestIdentifier, [this, protectedThis = makeRef(*this), mainResourceLoader = WTFMove(mainResourceLoader),
>         completionHandler = completionHandlerCaller.release(), requestIdentifier] (PolicyAction policy, PolicyCheckIdentifier responseIdentifeir) mutable {
>-        RELEASE_ASSERT(responseIdentifeir.isValidFor(requestIdentifier));
>+#if ASSERT_DISABLED
>+        UNUSED_PARAM(requestIdentifier);
>+        UNUSED_PARAM(responseIdentifeir);
>+#endif
>+        ASSERT(responseIdentifeir.isValidFor(requestIdentifier));
>         continueAfterContentPolicy(policy);
>         if (mainResourceLoader)
>             mainResourceLoader->didReceiveResponsePolicy();
>Index: Source/WebCore/loader/FrameLoader.cpp
>===================================================================
>--- Source/WebCore/loader/FrameLoader.cpp	(revision 241780)
>+++ Source/WebCore/loader/FrameLoader.cpp	(working copy)
>@@ -1379,8 +1379,6 @@ void FrameLoader::loadURL(FrameLoadReque
> 
>     if (!targetFrame && !effectiveFrameName.isEmpty()) {
>         action = action.copyWithShouldOpenExternalURLsPolicy(shouldOpenExternalURLsPolicyToApply(m_frame, frameLoadRequest));
>-        policyChecker().setLoadType(newLoadType);
>-        RELEASE_ASSERT(!isBackForwardLoadType(policyChecker().loadType()));
>         policyChecker().checkNewWindowPolicy(WTFMove(action), WTFMove(request), WTFMove(formState), effectiveFrameName, [this, allowNavigationToInvalidURL, openerPolicy, completionHandler = completionHandlerCaller.release()] (const ResourceRequest& request, WeakPtr<FormState>&& formState, const String& frameName, const NavigationAction& action, ShouldContinue shouldContinue) mutable {
>             continueLoadAfterNewWindowPolicy(request, formState.get(), frameName, action, shouldContinue, allowNavigationToInvalidURL, openerPolicy);
>             completionHandler();
>@@ -1401,7 +1399,7 @@ void FrameLoader::loadURL(FrameLoadReque
>         oldDocumentLoader->setLastCheckedRequest(ResourceRequest());
>         policyChecker().stopCheck();
>         policyChecker().setLoadType(newLoadType);
>-        RELEASE_ASSERT(!isBackForwardLoadType(newLoadType) || history().provisionalItem());
>+        ASSERT(!isBackForwardLoadType(newLoadType) || history().provisionalItem());
>         policyChecker().checkNavigationPolicy(WTFMove(request), ResourceResponse { } /* redirectResponse */, oldDocumentLoader.get(), WTFMove(formState), [this, protectedFrame = makeRef(m_frame)] (const ResourceRequest& request, WeakPtr<FormState>&&, NavigationPolicyDecision navigationPolicyDecision) {
>             continueFragmentScrollAfterNavigationPolicy(request, navigationPolicyDecision == NavigationPolicyDecision::ContinueLoad);
>         }, PolicyDecisionMode::Synchronous);
>@@ -1465,7 +1463,6 @@ void FrameLoader::load(FrameLoadRequest&
> 
>     if (request.shouldCheckNewWindowPolicy()) {
>         NavigationAction action { request.requester(), request.resourceRequest(), InitiatedByMainFrame::Unknown, NavigationType::Other, request.shouldOpenExternalURLsPolicy() };
>-        RELEASE_ASSERT(!isBackForwardLoadType(policyChecker().loadType()));
>         policyChecker().checkNewWindowPolicy(WTFMove(action), WTFMove(request.resourceRequest()), { }, request.frameName(), [this] (const ResourceRequest& request, WeakPtr<FormState>&& formState, const String& frameName, const NavigationAction& action, ShouldContinue shouldContinue) {
>             continueLoadAfterNewWindowPolicy(request, formState.get(), frameName, action, shouldContinue, AllowNavigationToInvalidURL::Yes, NewFrameOpenerPolicy::Suppress);
>         });
>@@ -1590,7 +1587,7 @@ void FrameLoader::loadWithDocumentLoader
>     }
> 
>     policyChecker().setLoadType(type);
>-    RELEASE_ASSERT(!isBackForwardLoadType(type) || history().provisionalItem());
>+    ASSERT(!isBackForwardLoadType(type) || history().provisionalItem());
>     bool isFormSubmission = formState;
> 
>     const String& httpMethod = loader->request().httpMethod();
>@@ -1602,7 +1599,7 @@ void FrameLoader::loadWithDocumentLoader
>         oldDocumentLoader->setTriggeringAction(WTFMove(action));
>         oldDocumentLoader->setLastCheckedRequest(ResourceRequest());
>         policyChecker().stopCheck();
>-        RELEASE_ASSERT(!isBackForwardLoadType(policyChecker().loadType()) || history().provisionalItem());
>+        ASSERT(!isBackForwardLoadType(policyChecker().loadType()) || history().provisionalItem());
>         policyChecker().checkNavigationPolicy(ResourceRequest(loader->request()), ResourceResponse { }  /* redirectResponse */, oldDocumentLoader.get(), WTFMove(formState), [this, protectedFrame = makeRef(m_frame)] (const ResourceRequest& request, WeakPtr<FormState>&&, NavigationPolicyDecision navigationPolicyDecision) {
>             continueFragmentScrollAfterNavigationPolicy(request, navigationPolicyDecision == NavigationPolicyDecision::ContinueLoad);
>         }, PolicyDecisionMode::Synchronous);
>@@ -1637,7 +1634,7 @@ void FrameLoader::loadWithDocumentLoader
>         return;
>     }
> 
>-    RELEASE_ASSERT(!isBackForwardLoadType(policyChecker().loadType()) || history().provisionalItem());
>+    ASSERT(!isBackForwardLoadType(policyChecker().loadType()) || history().provisionalItem());
>     policyChecker().checkNavigationPolicy(ResourceRequest(loader->request()), ResourceResponse { } /* redirectResponse */, loader, WTFMove(formState), [this, protectedFrame = makeRef(m_frame), allowNavigationToInvalidURL, completionHandler = completionHandlerCaller.release()] (const ResourceRequest& request, WeakPtr<FormState>&& formState, NavigationPolicyDecision navigationPolicyDecision) mutable {
>         continueLoadAfterNavigationPolicy(request, formState.get(), navigationPolicyDecision, allowNavigationToInvalidURL);
>         completionHandler();
>@@ -2997,7 +2994,6 @@ void FrameLoader::loadPostRequest(FrameL
>             return;
>         }
> 
>-        RELEASE_ASSERT(!isBackForwardLoadType(policyChecker().loadType()));
>         policyChecker().checkNewWindowPolicy(WTFMove(action), WTFMove(workingResourceRequest), WTFMove(formState), frameName, [this, allowNavigationToInvalidURL, openerPolicy, completionHandler = WTFMove(completionHandler)] (const ResourceRequest& request, WeakPtr<FormState>&& formState, const String& frameName, const NavigationAction& action, ShouldContinue shouldContinue) mutable {
>             continueLoadAfterNewWindowPolicy(request, formState.get(), frameName, action, shouldContinue, allowNavigationToInvalidURL, openerPolicy);
>             completionHandler();
>Index: Source/WebCore/loader/PolicyChecker.cpp
>===================================================================
>--- Source/WebCore/loader/PolicyChecker.cpp	(revision 241780)
>+++ Source/WebCore/loader/PolicyChecker.cpp	(working copy)
>@@ -80,9 +80,9 @@ PolicyCheckIdentifier PolicyCheckIdentif
> 
> bool PolicyCheckIdentifier::isValidFor(PolicyCheckIdentifier expectedIdentifier)
> {
>-    RELEASE_ASSERT_WITH_MESSAGE(m_policyCheck, "Received 0 as the policy check identifier");
>-    RELEASE_ASSERT_WITH_MESSAGE(m_process == expectedIdentifier.m_process, "Received a policy check response for a wrong process");
>-    RELEASE_ASSERT_WITH_MESSAGE(m_policyCheck <= expectedIdentifier.m_policyCheck, "Received a policy check response from the future");
>+    ASSERT_WITH_MESSAGE(m_policyCheck, "Received 0 as the policy check identifier");
>+    ASSERT_WITH_MESSAGE(m_process == expectedIdentifier.m_process, "Received a policy check response for a wrong process");
>+    ASSERT_WITH_MESSAGE(m_policyCheck <= expectedIdentifier.m_policyCheck, "Received a policy check response from the future");
>     return m_policyCheck == expectedIdentifier.m_policyCheck;
> }
> 
>Index: LayoutTests/ChangeLog
>===================================================================
>--- LayoutTests/ChangeLog	(revision 241780)
>+++ LayoutTests/ChangeLog	(working copy)
>@@ -1,3 +1,15 @@
>+2019-02-19  Ryosuke Niwa  <rniwa@webkit.org>
>+
>+        REGRESSION(r240909): Release assertion in FrameLoader::loadPostRequest when opening new window
>+        https://bugs.webkit.org/show_bug.cgi?id=194820
>+
>+        Reviewed by Geoffrey Garen and Brady Eidson.
>+
>+        Added a regression test.
>+
>+        * fast/loader/navigate-with-post-to-new-target-after-back-forward-navigation-expected.txt: Added.
>+        * fast/loader/navigate-with-post-to-new-target-after-back-forward-navigation.html: Added.
>+
> 2019-02-13  Babak Shafiei  <bshafiei@apple.com>
> 
>         Cherry-pick r241499. rdar://problem/48065634
>Index: LayoutTests/fast/loader/navigate-with-post-to-new-target-after-back-forward-navigation-expected.txt
>===================================================================
>--- LayoutTests/fast/loader/navigate-with-post-to-new-target-after-back-forward-navigation-expected.txt	(nonexistent)
>+++ LayoutTests/fast/loader/navigate-with-post-to-new-target-after-back-forward-navigation-expected.txt	(working copy)
>@@ -0,0 +1,5 @@
>+ALERT: PASS
>+This tests navigating via an anchor element with a non-existent target name, which should create a new window.
>+WebKit should not hit any assertions and alert "PASS".
>+
>+
>Index: LayoutTests/fast/loader/navigate-with-post-to-new-target-after-back-forward-navigation.html
>===================================================================
>--- LayoutTests/fast/loader/navigate-with-post-to-new-target-after-back-forward-navigation.html	(nonexistent)
>+++ LayoutTests/fast/loader/navigate-with-post-to-new-target-after-back-forward-navigation.html	(working copy)
>@@ -0,0 +1,51 @@
>+<!DOCTYPE html>
>+<html>
>+<body>
>+<script>
>+
>+if (window.testRunner) {
>+    testRunner.dumpAsText();
>+    testRunner.waitUntilDone();
>+    testRunner.setCanOpenWindows();
>+    testRunner.setCloseRemainingWindowsWhenComplete();
>+    testRunner.overridePreference("WebKitUsesPageCachePreferenceKey", 1);
>+}
>+
>+if (location.search == '?third') {
>+    alert('PASS');
>+    if (window.testRunner)
>+        testRunner.notifyDone();
>+} else if (self == top) {
>+    document.write(`<p>This tests navigating via an anchor element with a non-existent target name, which should create a new window.<br>
>+WebKit should not hit any assertions and alert "PASS".</p>`);
>+    const frame = document.createElement('iframe');
>+    frame.src = '?first';
>+    let step = 0;
>+    frame.onload = () => {
>+        switch (step++) {
>+        case 0:
>+            setTimeout(() => frame.src = '?second', 0);
>+            break;
>+        case 1:
>+            setTimeout(() => history.back(), 0);
>+            break;
>+        }
>+    }
>+    document.body.appendChild(frame);
>+} else {
>+    if (location.search == '?first') {
>+        if (localStorage.getItem('loaded')) {
>+            localStorage.removeItem('loaded');
>+            window.onload = () => {
>+                setTimeout(() => document.querySelector('form').submit(), 0);
>+            }
>+        } else
>+            localStorage.setItem('loaded', 'true');
>+    }
>+    document.write(location.search);
>+    document.write(` <form action="?third" method="post" target="unknownTarget"><input type="text" name="query"><input type="submit"></a>`);
>+}
>+
>+</script>
>+</body>
>+</html>

Actions: View | Formatted Diff | Diff

Attachments on bug 194820: 362393 | 362413 | 362442 | 362467