REGRESSION (r240909): Release assert in FrameLoader::loadURL when navigating with...
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 6 Feb 2019 09:42:49 +0000 (09:42 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 6 Feb 2019 09:42:49 +0000 (09:42 +0000)
https://bugs.webkit.org/show_bug.cgi?id=194329

Reviewed by Geoffrey Garen.

Source/WebCore:

The bug was caused by the code path for when navigating with a specific target frame name that does not exist
never setting the load type of PolicyChecker. As a result, we would use whatever load type used in the previous
navigation, resulting in this release assertion.

Updating the load type here should in theory fix the underlying bug r240909 was meant to catch & fix.

Test: fast/loader/navigate-with-new-target-after-back-forward-navigation.html

* loader/FrameLoader.cpp:
(WebCore::FrameLoader::loadURL):

LayoutTests:

Added a regression test.

* fast/loader/navigate-with-new-target-after-back-forward-navigation-expected.txt: Added.
* fast/loader/navigate-with-new-target-after-back-forward-navigation.html: Added.

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

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

index 4754f1b..f71a17d 100644 (file)
@@ -1,3 +1,15 @@
+2019-02-05  Ryosuke Niwa  <rniwa@webkit.org>
+
+        REGRESSION (r240909): Release assert in FrameLoader::loadURL when navigating with a non-existent target name
+        https://bugs.webkit.org/show_bug.cgi?id=194329
+
+        Reviewed by Geoffrey Garen.
+
+        Added a regression test.
+
+        * fast/loader/navigate-with-new-target-after-back-forward-navigation-expected.txt: Added.
+        * fast/loader/navigate-with-new-target-after-back-forward-navigation.html: Added.
+
 2019-02-05  Nikita Vasilyev  <nvasilyev@apple.com>
 
         Web Inspector: Styles: PropertiesChanged shouldn't fire when old and new text are both empty
index fe88082..e87c76a 100644 (file)
@@ -6,6 +6,8 @@
 # Platform-specific tests. Skipped here, then re-enabled on the appropriate platform.
 #//////////////////////////////////////////////////////////////////////////////////////////
 
+webgl [ Skip ]
+
 compositing/ios [ Skip ]
 css3/touch-action [ Skip ]
 accessibility/ios-simulator [ Skip ]
diff --git a/LayoutTests/fast/loader/navigate-with-new-target-after-back-forward-navigation-expected.txt b/LayoutTests/fast/loader/navigate-with-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-new-target-after-back-forward-navigation.html b/LayoutTests/fast/loader/navigate-with-new-target-after-back-forward-navigation.html
new file mode 100644 (file)
index 0000000..35ccb34
--- /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('a').click(), 0);
+            }
+        } else
+            localStorage.setItem('loaded', 'true');
+    }
+    document.write(location.search);
+    document.write(` <a href="?third" target="unknownTarget">go</a>`);
+}
+
+</script>
+</body>
+</html>
index aa190ab..76da75c 100644 (file)
@@ -1,3 +1,21 @@
+2019-02-05  Ryosuke Niwa  <rniwa@webkit.org>
+
+        REGRESSION (r240909): Release assert in FrameLoader::loadURL when navigating with a non-existent target name
+        https://bugs.webkit.org/show_bug.cgi?id=194329
+
+        Reviewed by Geoffrey Garen.
+
+        The bug was caused by the code path for when navigating with a specific target frame name that does not exist
+        never setting the load type of PolicyChecker. As a result, we would use whatever load type used in the previous
+        navigation, resulting in this release assertion.
+
+        Updating the load type here should in theory fix the underlying bug r240909 was meant to catch & fix.
+
+        Test: fast/loader/navigate-with-new-target-after-back-forward-navigation.html
+
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::loadURL):
+
 2019-02-05  Claudio Saavedra  <csaavedra@igalia.com>
 
         [FreeType] Build fix for Debian stable
index 1f552bd..462ee48 100644 (file)
@@ -1379,6 +1379,7 @@ 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);