Attachment 362393 Details for Bug 194820 – Fixes the bug

[patch] Fixes the bug

bug-194820-20190219104724.patch (text/plain), 7.52 KB, created by Ryosuke Niwa on 2019-02-19 10:47:25 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Ryosuke Niwa

Created: 2019-02-19 10:47:25 PST

Size: 7.52 KB

patch

obsolete

>Index: Source/WebCore/ChangeLog
>===================================================================
>--- Source/WebCore/ChangeLog	(revision 241761)
>+++ 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 NOBODY (OOPS!).
>+
>+        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  Commit Queue  <commit-queue@webkit.org>
> 
>         Unreviewed, rolling out r241722.
>Index: Source/WebCore/loader/FrameLoader.cpp
>===================================================================
>--- Source/WebCore/loader/FrameLoader.cpp	(revision 241755)
>+++ Source/WebCore/loader/FrameLoader.cpp	(working copy)
>@@ -1381,8 +1381,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();
>@@ -1467,7 +1465,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);
>         });
>@@ -3022,7 +3019,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: LayoutTests/ChangeLog
>===================================================================
>--- LayoutTests/ChangeLog	(revision 241755)
>+++ 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 NOBODY (OOPS!).
>+
>+        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-18  Alex Christensen  <achristensen@webkit.org>
> 
>         Adjust test expectations after r241754
>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