- Revision
- 233374
- Author
- [email protected]
- Date
- 2018-06-29 15:56:29 -0700 (Fri, 29 Jun 2018)
Log Message
WebKitLegacy: Can trigger recursive loads triggering debug assertions
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):
Modified Paths
Added Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (233373 => 233374)
--- trunk/Source/WebCore/ChangeLog 2018-06-29 22:51:59 UTC (rev 233373)
+++ trunk/Source/WebCore/ChangeLog 2018-06-29 22:56:29 UTC (rev 233374)
@@ -1,3 +1,38 @@
+2018-06-29 Chris Dumez <[email protected]>
+
+ 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 <[email protected]>
Infinite loop if a <use> element references its ancestor and the DOMNodeInserted event handler of one its ancestor's descents updates the document style
Modified: trunk/Source/WebCore/loader/FrameLoader.cpp (233373 => 233374)
--- trunk/Source/WebCore/loader/FrameLoader.cpp 2018-06-29 22:51:59 UTC (rev 233373)
+++ trunk/Source/WebCore/loader/FrameLoader.cpp 2018-06-29 22:56:29 UTC (rev 233374)
@@ -1316,7 +1316,7 @@
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 @@
void FrameLoader::load(FrameLoadRequest&& request)
{
- if (m_inStopAllLoaders)
+ if (m_inStopAllLoaders || m_inClearProvisionalLoadForPolicyCheck)
return;
if (!request.frameName().isEmpty()) {
@@ -1603,9 +1603,10 @@
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);
}
Modified: trunk/Source/WebCore/loader/FrameLoader.h (233373 => 233374)
--- trunk/Source/WebCore/loader/FrameLoader.h 2018-06-29 22:51:59 UTC (rev 233373)
+++ trunk/Source/WebCore/loader/FrameLoader.h 2018-06-29 22:56:29 UTC (rev 233374)
@@ -430,6 +430,7 @@
bool m_quickRedirectComing;
bool m_sentRedirectNotification;
bool m_inStopAllLoaders;
+ bool m_inClearProvisionalLoadForPolicyCheck { false };
bool m_shouldReportResourceTimingToParentFrame { true };
String m_outgoingReferrer;
Modified: trunk/Tools/ChangeLog (233373 => 233374)
--- trunk/Tools/ChangeLog 2018-06-29 22:51:59 UTC (rev 233373)
+++ trunk/Tools/ChangeLog 2018-06-29 22:56:29 UTC (rev 233374)
@@ -1,3 +1,19 @@
+2018-06-29 Chris Dumez <[email protected]>
+
+ 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 <[email protected]>
Teach bisect-builds to retrieve supported platforms from the rest api.
Modified: trunk/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj (233373 => 233374)
--- trunk/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj 2018-06-29 22:51:59 UTC (rev 233373)
+++ trunk/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj 2018-06-29 22:56:29 UTC (rev 233374)
@@ -571,6 +571,7 @@
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 */; };
@@ -1629,6 +1630,7 @@
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>"; };
@@ -3095,6 +3097,7 @@
52B8CF9515868CF000281053 /* SetDocumentURI.mm */,
C540F775152E4DA000A40C8C /* SimplifyMarkup.mm */,
57F4AA9F208FA83D00A68E9E /* SSLKeyGenerator.mm */,
+ 83BC5ABF20E6C0D300F5879F /* StartLoadInDidFailProvisionalLoad.mm */,
291861FD17BD4DC700D4E41E /* StopLoadingFromDidFinishLoading.mm */,
E194E1BA177E5145009C4D4E /* StopLoadingFromDidReceiveResponse.mm */,
3799AD3914120A43005EB0C6 /* StringByEvaluatingJavaScriptFromString.mm */,
@@ -3805,6 +3808,7 @@
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 */,
Added: trunk/Tools/TestWebKitAPI/Tests/mac/StartLoadInDidFailProvisionalLoad.mm (0 => 233374)
--- trunk/Tools/TestWebKitAPI/Tests/mac/StartLoadInDidFailProvisionalLoad.mm (rev 0)
+++ trunk/Tools/TestWebKitAPI/Tests/mac/StartLoadInDidFailProvisionalLoad.mm 2018-06-29 22:56:29 UTC (rev 233374)
@@ -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
