WebKitLegacy: Can trigger recursive loads triggering debug assertions
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 29 Jun 2018 22:56:29 +0000 (22:56 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 29 Jun 2018 22:56:29 +0000 (22:56 +0000)
https://bugs.webkit.org/show_bug.cgi?id=187121
<rdar://problem/41259430>

Reviewed by Brent Fulgham.

Source/WebCore:

In order to support asynchronous policy delegates, r229722 added a call to
FrameLoader::clearProvisionalLoadForPolicyCheck() when starting a navigation
policy decision in PolicyChecker::checkNavigationPolicy(). This calls
stopLoading() on the current provisional loader if there is one, and potentially
calls the didFailProvisionalLoadWithError cleint delegate. This delegate call
is synchronous on WebKit1, so the client may start a new load from this delegate
and re-enter Webcore. This happens in practive with Quickens 2017 / 2018 on Mac.

Before r229722, this was not an issue because pending loads were canceled after
the (asynchronous) navigation policy decision, via FrameLoader::stopAllLoaders().
FrameLoader::stopAllLoaders() sets a m_inStopAllLoaders flag and we return early
in FrameLoader::loadRequest() when this flag is set to prevent recursive loads.

To maintain shipping behavior as much as possible, this patch introduces a similar
inClearProvisionalLoadForPolicyCheck which gets set during
FrameLoader::clearProvisionalLoadForPolicyCheck() and we prevent new loads while
this flag is set.

I have verified that Quickens 2017 / 2018 works again after this change and I added
API test coverage for this behavior.

* loader/FrameLoader.cpp:
(WebCore::FrameLoader::loadURL):
(WebCore::FrameLoader::load):
(WebCore::FrameLoader::clearProvisionalLoadForPolicyCheck):
* loader/FrameLoader.h:

Tools:

Add API test coverage.

* TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
* TestWebKitAPI/Tests/mac/StartLoadInDidFailProvisionalLoad.mm: Added.
(-[StartLoadInDidFailProvisionalLoadDelegate webView:didFailProvisionalLoadWithError:forFrame:]):
(-[StartLoadInDidFailProvisionalLoadDelegate webView:didFinishLoadForFrame:]):
(TestWebKitAPI::TEST):

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

Source/WebCore/ChangeLog
Source/WebCore/loader/FrameLoader.cpp
Source/WebCore/loader/FrameLoader.h
Tools/ChangeLog
Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
Tools/TestWebKitAPI/Tests/mac/StartLoadInDidFailProvisionalLoad.mm [new file with mode: 0644]

index fd52945..7175663 100644 (file)
@@ -1,3 +1,38 @@
+2018-06-29  Chris Dumez  <cdumez@apple.com>
+
+        WebKitLegacy: Can trigger recursive loads triggering debug assertions
+        https://bugs.webkit.org/show_bug.cgi?id=187121
+        <rdar://problem/41259430>
+
+        Reviewed by Brent Fulgham.
+
+        In order to support asynchronous policy delegates, r229722 added a call to
+        FrameLoader::clearProvisionalLoadForPolicyCheck() when starting a navigation
+        policy decision in PolicyChecker::checkNavigationPolicy(). This calls
+        stopLoading() on the current provisional loader if there is one, and potentially
+        calls the didFailProvisionalLoadWithError cleint delegate. This delegate call
+        is synchronous on WebKit1, so the client may start a new load from this delegate
+        and re-enter Webcore. This happens in practive with Quickens 2017 / 2018 on Mac.
+
+        Before r229722, this was not an issue because pending loads were canceled after
+        the (asynchronous) navigation policy decision, via FrameLoader::stopAllLoaders().
+        FrameLoader::stopAllLoaders() sets a m_inStopAllLoaders flag and we return early
+        in FrameLoader::loadRequest() when this flag is set to prevent recursive loads.
+
+        To maintain shipping behavior as much as possible, this patch introduces a similar
+        inClearProvisionalLoadForPolicyCheck which gets set during
+        FrameLoader::clearProvisionalLoadForPolicyCheck() and we prevent new loads while
+        this flag is set.
+
+        I have verified that Quickens 2017 / 2018 works again after this change and I added
+        API test coverage for this behavior.
+
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::loadURL):
+        (WebCore::FrameLoader::load):
+        (WebCore::FrameLoader::clearProvisionalLoadForPolicyCheck):
+        * loader/FrameLoader.h:
+
 2018-06-25  Said Abou-Hallawa  <sabouhallawa@apple.com>
 
         Infinite loop if a <use> element references its ancestor and the DOMNodeInserted event handler of one its ancestor's descents updates the document style
index 4ee5246..61ac48c 100644 (file)
@@ -1316,7 +1316,7 @@ bool FrameLoader::isStopLoadingAllowed() const
 void FrameLoader::loadURL(FrameLoadRequest&& frameLoadRequest, const String& referrer, FrameLoadType newLoadType, Event* event, RefPtr<FormState>&& formState, CompletionHandler<void()>&& completionHandler)
 {
     CompletionHandlerCallingScope completionHandlerCaller(WTFMove(completionHandler));
-    if (m_inStopAllLoaders)
+    if (m_inStopAllLoaders || m_inClearProvisionalLoadForPolicyCheck)
         return;
 
     Ref<Frame> protect(m_frame);
@@ -1431,7 +1431,7 @@ SubstituteData FrameLoader::defaultSubstituteDataForURL(const URL& url)
 
 void FrameLoader::load(FrameLoadRequest&& request)
 {
-    if (m_inStopAllLoaders)
+    if (m_inStopAllLoaders || m_inClearProvisionalLoadForPolicyCheck)
         return;
 
     if (!request.frameName().isEmpty()) {
@@ -1603,9 +1603,10 @@ void FrameLoader::loadWithDocumentLoader(DocumentLoader* loader, FrameLoadType t
 
 void FrameLoader::clearProvisionalLoadForPolicyCheck()
 {
-    if (!m_policyDocumentLoader || !m_provisionalDocumentLoader)
+    if (!m_policyDocumentLoader || !m_provisionalDocumentLoader || m_inClearProvisionalLoadForPolicyCheck)
         return;
 
+    SetForScope<bool> change(m_inClearProvisionalLoadForPolicyCheck, true);
     m_provisionalDocumentLoader->stopLoading();
     setProvisionalDocumentLoader(nullptr);
 }
index 2588fca..815f659 100644 (file)
@@ -430,6 +430,7 @@ private:
     bool m_quickRedirectComing;
     bool m_sentRedirectNotification;
     bool m_inStopAllLoaders;
+    bool m_inClearProvisionalLoadForPolicyCheck { false };
     bool m_shouldReportResourceTimingToParentFrame { true };
 
     String m_outgoingReferrer;
index 0b19079..09ddef7 100644 (file)
@@ -1,3 +1,19 @@
+2018-06-29  Chris Dumez  <cdumez@apple.com>
+
+        WebKitLegacy: Can trigger recursive loads triggering debug assertions
+        https://bugs.webkit.org/show_bug.cgi?id=187121
+        <rdar://problem/41259430>
+
+        Reviewed by Brent Fulgham.
+
+        Add API test coverage.
+
+        * TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
+        * TestWebKitAPI/Tests/mac/StartLoadInDidFailProvisionalLoad.mm: Added.
+        (-[StartLoadInDidFailProvisionalLoadDelegate webView:didFailProvisionalLoadWithError:forFrame:]):
+        (-[StartLoadInDidFailProvisionalLoadDelegate webView:didFinishLoadForFrame:]):
+        (TestWebKitAPI::TEST):
+
 2018-06-29  Lucas Forschler  <lforschler@apple.com>
 
         Teach bisect-builds to retrieve supported platforms from the rest api.
index ba6b541..482778d 100644 (file)
                837A35F11D9A1E7D00663C57 /* DownloadRequestBlobURL.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = 837A35F01D9A1E6400663C57 /* DownloadRequestBlobURL.html */; };
                83B6DE6F1EE75221001E792F /* RestoreSessionState.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 83B6DE6E1EE7520F001E792F /* RestoreSessionState.cpp */; };
                83BAEE8D1EF4625500DDE894 /* PluginLoadClientPolicies.mm in Sources */ = {isa = PBXBuildFile; fileRef = 83BAEE8C1EF4625500DDE894 /* PluginLoadClientPolicies.mm */; };
+               83BC5AC020E6C0DF00F5879F /* StartLoadInDidFailProvisionalLoad.mm in Sources */ = {isa = PBXBuildFile; fileRef = 83BC5ABF20E6C0D300F5879F /* StartLoadInDidFailProvisionalLoad.mm */; };
                83DB79691EF63B3C00BFA5E5 /* Function.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 83DB79671EF63B3C00BFA5E5 /* Function.cpp */; };
                83DE134D1EF1C50800C1B355 /* ResponsivenessTimer.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 83DE134C1EF1C4FE00C1B355 /* ResponsivenessTimer.cpp */; };
                83F22C6420B355F80034277E /* NoPolicyDelegateResponse.mm in Sources */ = {isa = PBXBuildFile; fileRef = 83F22C6320B355EB0034277E /* NoPolicyDelegateResponse.mm */; };
                83B6DE6E1EE7520F001E792F /* RestoreSessionState.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = RestoreSessionState.cpp; sourceTree = "<group>"; };
                83B88A331C80056D00BB2418 /* HTMLParserIdioms.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = HTMLParserIdioms.cpp; sourceTree = "<group>"; };
                83BAEE8C1EF4625500DDE894 /* PluginLoadClientPolicies.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = PluginLoadClientPolicies.mm; sourceTree = "<group>"; };
+               83BC5ABF20E6C0D300F5879F /* StartLoadInDidFailProvisionalLoad.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = StartLoadInDidFailProvisionalLoad.mm; sourceTree = "<group>"; };
                83DB79671EF63B3C00BFA5E5 /* Function.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = Function.cpp; sourceTree = "<group>"; };
                83DE134C1EF1C4FE00C1B355 /* ResponsivenessTimer.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = ResponsivenessTimer.cpp; sourceTree = "<group>"; };
                83F22C6320B355EB0034277E /* NoPolicyDelegateResponse.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = NoPolicyDelegateResponse.mm; sourceTree = "<group>"; };
                                52B8CF9515868CF000281053 /* SetDocumentURI.mm */,
                                C540F775152E4DA000A40C8C /* SimplifyMarkup.mm */,
                                57F4AA9F208FA83D00A68E9E /* SSLKeyGenerator.mm */,
+                               83BC5ABF20E6C0D300F5879F /* StartLoadInDidFailProvisionalLoad.mm */,
                                291861FD17BD4DC700D4E41E /* StopLoadingFromDidFinishLoading.mm */,
                                E194E1BA177E5145009C4D4E /* StopLoadingFromDidReceiveResponse.mm */,
                                3799AD3914120A43005EB0C6 /* StringByEvaluatingJavaScriptFromString.mm */,
                                0F4FFA9E1ED3AA8500F7111F /* SnapshotViaRenderInContext.mm in Sources */,
                                7CCE7F151A411AE600447C4C /* SpacebarScrolling.cpp in Sources */,
                                57F4AAA0208FAEF000A68E9E /* SSLKeyGenerator.mm in Sources */,
+                               83BC5AC020E6C0DF00F5879F /* StartLoadInDidFailProvisionalLoad.mm in Sources */,
                                7CCE7EF21A411AE600447C4C /* StopLoadingDuringDidFailProvisionalLoad.cpp in Sources */,
                                7CCE7ECE1A411A7E00447C4C /* StopLoadingFromDidFinishLoading.mm in Sources */,
                                7CCE7ECF1A411A7E00447C4C /* StopLoadingFromDidReceiveResponse.mm in Sources */,
diff --git a/Tools/TestWebKitAPI/Tests/mac/StartLoadInDidFailProvisionalLoad.mm b/Tools/TestWebKitAPI/Tests/mac/StartLoadInDidFailProvisionalLoad.mm
new file mode 100644 (file)
index 0000000..612f5e4
--- /dev/null
@@ -0,0 +1,72 @@
+/*
+ * Copyright (C) 2013 Apple Inc. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR ITS CONTRIBUTORS
+ * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
+ * THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#import "config.h"
+#import "PlatformUtilities.h"
+#import <wtf/RetainPtr.h>
+
+static bool finished = false;
+static bool didFailProvisionalLoad = false;
+static WebView *testView = nullptr;
+
+@interface StartLoadInDidFailProvisionalLoadDelegate : NSObject <WebFrameLoadDelegate>
+@end
+
+@implementation StartLoadInDidFailProvisionalLoadDelegate
+
+- (void)webView:(WebView *)sender didFailProvisionalLoadWithError:(NSError *)error forFrame:(WebFrame *)frame
+{
+    EXPECT_FALSE(didFailProvisionalLoad);
+    didFailProvisionalLoad = true;
+
+    [[testView mainFrame] loadRequest:[NSURLRequest requestWithURL:[[NSBundle mainBundle] URLForResource:@"simple3" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"]]];
+}
+
+- (void)webView:(WebView *)sender didFinishLoadForFrame:(WebFrame *)frame
+{
+    finished = true;
+}
+
+@end
+
+namespace TestWebKitAPI {
+
+TEST(WebKitLegacy, StartLoadInDidFailProvisionalLoad)
+{
+    auto webView = adoptNS([[WebView alloc] init]);
+    testView = webView.get();
+    auto frameLoadDelegate = adoptNS([[StartLoadInDidFailProvisionalLoadDelegate alloc] init]);
+    webView.get().frameLoadDelegate = frameLoadDelegate.get();
+    [[webView mainFrame] loadRequest:[NSURLRequest requestWithURL:[[NSBundle mainBundle] URLForResource:@"simple" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"]]];
+
+    // Start another load before the first one has a chance to complete. This should cancel the previous load and call didFailProvisionalLoadWithError.
+    [[webView mainFrame] loadRequest:[NSURLRequest requestWithURL:[[NSBundle mainBundle] URLForResource:@"simple2" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"]]];
+    Util::run(&finished);
+    EXPECT_TRUE(didFailProvisionalLoad);
+
+    EXPECT_WK_STREQ([[[NSBundle mainBundle] URLForResource:@"simple2" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"] absoluteString], webView.get().mainFrameURL);
+}
+
+} // namespace TestWebKitAPI