REGRESSION(r240909): Release assertion in FrameLoader::loadPostRequest when opening...
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 19 Feb 2019 23:07:52 +0000 (23:07 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 19 Feb 2019 23:07:52 +0000 (23:07 +0000)
https://bugs.webkit.org/show_bug.cgi?id=194820

Reviewed by Geoffrey Garen.

Source/WebCore:

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):

LayoutTests:

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.

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

LayoutTests/ChangeLog
LayoutTests/fast/loader/navigate-with-post-to-new-target-after-back-forward-navigation-expected.txt [new file with mode: 0644]
LayoutTests/fast/loader/navigate-with-post-to-new-target-after-back-forward-navigation.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/loader/FrameLoader.cpp

index 61ff95c..1753c67 100644 (file)
@@ -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.
+
+        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-19  Truitt Savell  <tsavell@apple.com>
 
         [ iOS ] Layout Tests in editing/pasteboard/data-transfer-set-data-* are flaky Timeouts
diff --git a/LayoutTests/fast/loader/navigate-with-post-to-new-target-after-back-forward-navigation-expected.txt b/LayoutTests/fast/loader/navigate-with-post-to-new-target-after-back-forward-navigation-expected.txt
new file mode 100644 (file)
index 0000000..773b172
--- /dev/null
@@ -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".
+
+
diff --git a/LayoutTests/fast/loader/navigate-with-post-to-new-target-after-back-forward-navigation.html b/LayoutTests/fast/loader/navigate-with-post-to-new-target-after-back-forward-navigation.html
new file mode 100644 (file)
index 0000000..076d9eb
--- /dev/null
@@ -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>
index ea2c31f..c25dffb 100644 (file)
@@ -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.
+
+        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-19  Zalan Bujtas  <zalan@apple.com>
 
         Fix post-commit feedback.
index 012f24f..109b7d3 100644 (file)
@@ -1381,8 +1381,6 @@ void FrameLoader::loadURL(FrameLoadRequest&& frameLoadRequest, const String& ref
 
     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();
@@ -1467,7 +1465,6 @@ void FrameLoader::load(FrameLoadRequest&& request)
 
     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);
         });
@@ -3022,7 +3019,6 @@ void FrameLoader::loadPostRequest(FrameLoadRequest&& request, const String& refe
             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();