WebKit.WebsitePoliciesAutoplayQuirks API test times out with async policy delegates
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 19 Mar 2018 22:30:57 +0000 (22:30 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 19 Mar 2018 22:30:57 +0000 (22:30 +0000)
https://bugs.webkit.org/show_bug.cgi?id=183702
<rdar://problem/38566060>

Reviewed by Alex Christensen.

Source/WebCore:

The issue is that the test calls loadHTMLString then loadRequest right after, without
waiting for the first load to complete first. loadHTMLString is special as it relies
on substitute data and which schedules a timer to commit the data. When doing the
navigation policy check for the following loadRequest(), the substitute data timer
would fire and commit its data and load. This would in turn cancel the pending
navigation policy check for the loadRequest().

With sync policy delegates, this is not an issue because we take care of stopping
all loaders when receiving the policy decision, which happens synchronously. However,
when the policy decision happens asynchronously, the pending substitute data load
does not get cancelled in time and it gets committed.

To address the issue, we now cancel any pending provisional load before doing the
navigation policy check.

Test: fast/loader/inner-iframe-loads-data-url-into-parent-on-unload-crash-async-delegate.html

* loader/FrameLoader.cpp:
(WebCore::FrameLoader::clearProvisionalLoadForPolicyCheck):
* loader/FrameLoader.h:
* loader/PolicyChecker.cpp:
(WebCore::PolicyChecker::checkNavigationPolicy):
Cancel any pending provisional load before starting the navigation policy check. This call
needs to be here rather than in the call site of policyChecker().checkNavigationPolicy()
because there is code in PolicyChecker::checkNavigationPolicy() which relies on
FrameLoader::activeDocumentLoader().
Also, we only cancel the provisional load if there is a policy document loader. In some
rare cases (when we receive a redirect after navigation policy has been decided for the
initial request), the provisional document loader needs to receive navigation policy
decisions so we cannot clear the provisional document loader in such case.

Tools:

Add API test coverage.

* TestWebKitAPI/Tests/WebKitCocoa/WebsitePolicies.mm:
(-[AsyncAutoplayPoliciesDelegate webView:decidePolicyForNavigationAction:decisionHandler:]):
(-[AsyncAutoplayPoliciesDelegate _webView:decidePolicyForNavigationAction:decisionHandler:]):
(-[AsyncAutoplayPoliciesDelegate _webView:handleAutoplayEvent:withFlags:]):
(TEST):

LayoutTests:

Add variant of fast/loader/inner-iframe-loads-data-url-into-parent-on-unload-crash.html with async navigation
delegate since the previous iteration of this patch broke this test case.

* fast/loader/inner-iframe-loads-data-url-into-parent-on-unload-crash-async-delegate-expected.txt: Added.
* fast/loader/inner-iframe-loads-data-url-into-parent-on-unload-crash-async-delegate.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/loader/inner-iframe-loads-data-url-into-parent-on-unload-crash-async-delegate-expected.txt [new file with mode: 0644]
LayoutTests/fast/loader/inner-iframe-loads-data-url-into-parent-on-unload-crash-async-delegate.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/loader/FrameLoader.cpp
Source/WebCore/loader/FrameLoader.h
Source/WebCore/loader/PolicyChecker.cpp
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsitePolicies.mm

index 435d87a..80e6082 100644 (file)
@@ -1,3 +1,17 @@
+2018-03-19  Chris Dumez  <cdumez@apple.com>
+
+        WebKit.WebsitePoliciesAutoplayQuirks API test times out with async policy delegates
+        https://bugs.webkit.org/show_bug.cgi?id=183702
+        <rdar://problem/38566060>
+
+        Reviewed by Alex Christensen.
+
+        Add variant of fast/loader/inner-iframe-loads-data-url-into-parent-on-unload-crash.html with async navigation
+        delegate since the previous iteration of this patch broke this test case.
+
+        * fast/loader/inner-iframe-loads-data-url-into-parent-on-unload-crash-async-delegate-expected.txt: Added.
+        * fast/loader/inner-iframe-loads-data-url-into-parent-on-unload-crash-async-delegate.html: Added.
+
 2018-03-17  Jiewen Tan  <jiewen_tan@apple.com>
 
         [WebAuthN] Implement authenticatorMakeCredential
diff --git a/LayoutTests/fast/loader/inner-iframe-loads-data-url-into-parent-on-unload-crash-async-delegate-expected.txt b/LayoutTests/fast/loader/inner-iframe-loads-data-url-into-parent-on-unload-crash-async-delegate-expected.txt
new file mode 100644 (file)
index 0000000..5221265
--- /dev/null
@@ -0,0 +1,6 @@
+
+
+--------
+Frame: '<!--framePath //<!--frame0-->-->'
+--------
+PASS did not crash.
diff --git a/LayoutTests/fast/loader/inner-iframe-loads-data-url-into-parent-on-unload-crash-async-delegate.html b/LayoutTests/fast/loader/inner-iframe-loads-data-url-into-parent-on-unload-crash-async-delegate.html
new file mode 100644 (file)
index 0000000..73e4bae
--- /dev/null
@@ -0,0 +1,23 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script>
+if (window.testRunner) {
+    testRunner.dumpAsText();
+    testRunner.dumpChildFramesAsText();
+    testRunner.waitUntilDone();
+    if (testRunner.setShouldDecideNavigationPolicyAfterDelay)
+        testRunner.setShouldDecideNavigationPolicyAfterDelay(true);
+}
+var parentFrame = document.body.appendChild(document.createElement("iframe"));
+parentFrame.src = "data:text/html,";
+
+var childFrame = parentFrame.contentDocument.body.appendChild(document.createElement("iframe"));
+childFrame.contentWindow.onunload = function () {
+    var link = parentFrame.contentDocument.createElement("a");
+    link.href = "data:text/html,PASS did not crash.<script>window.testRunner && window.testRunner.notifyDone()</" + "script>";
+    link.click(); // Navigates parentFrame
+}
+</script>
+</body>
+</html>
index 0e151aa..9c882e8 100644 (file)
@@ -1,3 +1,42 @@
+2018-03-19  Chris Dumez  <cdumez@apple.com>
+
+        WebKit.WebsitePoliciesAutoplayQuirks API test times out with async policy delegates
+        https://bugs.webkit.org/show_bug.cgi?id=183702
+        <rdar://problem/38566060>
+
+        Reviewed by Alex Christensen.
+
+        The issue is that the test calls loadHTMLString then loadRequest right after, without
+        waiting for the first load to complete first. loadHTMLString is special as it relies
+        on substitute data and which schedules a timer to commit the data. When doing the
+        navigation policy check for the following loadRequest(), the substitute data timer
+        would fire and commit its data and load. This would in turn cancel the pending
+        navigation policy check for the loadRequest().
+
+        With sync policy delegates, this is not an issue because we take care of stopping
+        all loaders when receiving the policy decision, which happens synchronously. However,
+        when the policy decision happens asynchronously, the pending substitute data load
+        does not get cancelled in time and it gets committed.
+
+        To address the issue, we now cancel any pending provisional load before doing the
+        navigation policy check.
+
+        Test: fast/loader/inner-iframe-loads-data-url-into-parent-on-unload-crash-async-delegate.html
+
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::clearProvisionalLoadForPolicyCheck):
+        * loader/FrameLoader.h:
+        * loader/PolicyChecker.cpp:
+        (WebCore::PolicyChecker::checkNavigationPolicy):
+        Cancel any pending provisional load before starting the navigation policy check. This call
+        needs to be here rather than in the call site of policyChecker().checkNavigationPolicy()
+        because there is code in PolicyChecker::checkNavigationPolicy() which relies on
+        FrameLoader::activeDocumentLoader().
+        Also, we only cancel the provisional load if there is a policy document loader. In some
+        rare cases (when we receive a redirect after navigation policy has been decided for the
+        initial request), the provisional document loader needs to receive navigation policy
+        decisions so we cannot clear the provisional document loader in such case.
+
 2018-03-19  Eric Carlson  <eric.carlson@apple.com>
 
         [Extra zoom mode] Require fullscreen for video playback
index 9a7d9ab..16b1525 100644 (file)
@@ -1544,6 +1544,15 @@ void FrameLoader::loadWithDocumentLoader(DocumentLoader* loader, FrameLoadType t
     });
 }
 
+void FrameLoader::clearProvisionalLoadForPolicyCheck()
+{
+    if (!m_policyDocumentLoader || !m_provisionalDocumentLoader)
+        return;
+
+    m_provisionalDocumentLoader->stopLoading();
+    setProvisionalDocumentLoader(nullptr);
+}
+
 void FrameLoader::reportLocalLoadFailed(Frame* frame, const String& url)
 {
     ASSERT(!url.isEmpty());
index 5be242a..23f4754 100644 (file)
@@ -141,6 +141,7 @@ public:
     void stopLoading(UnloadEventPolicy);
     bool closeURL();
     void cancelAndClear();
+    void clearProvisionalLoadForPolicyCheck();
     // FIXME: clear() is trying to do too many things. We should break it down into smaller functions (ideally with fewer raw Boolean parameters).
     void clear(Document* newDocument, bool clearWindowProperties = true, bool clearScriptObjects = true, bool clearFrameView = true);
 
index 043291e..17c0f4c 100644 (file)
@@ -144,6 +144,8 @@ void PolicyChecker::checkNavigationPolicy(ResourceRequest&& request, bool didRec
     m_contentFilterUnblockHandler = { };
 #endif
 
+    m_frame.loader().clearProvisionalLoadForPolicyCheck();
+
     m_delegateIsDecidingNavigationPolicy = true;
     String suggestedFilename = action.downloadAttribute().isEmpty() ? nullAtom() : action.downloadAttribute();
     ResourceRequest requestCopy = request;
index cb2ee91..b546dda 100644 (file)
@@ -1,3 +1,19 @@
+2018-03-19  Chris Dumez  <cdumez@apple.com>
+
+        WebKit.WebsitePoliciesAutoplayQuirks API test times out with async policy delegates
+        https://bugs.webkit.org/show_bug.cgi?id=183702
+        <rdar://problem/38566060>
+
+        Reviewed by Alex Christensen.
+
+        Add API test coverage.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/WebsitePolicies.mm:
+        (-[AsyncAutoplayPoliciesDelegate webView:decidePolicyForNavigationAction:decisionHandler:]):
+        (-[AsyncAutoplayPoliciesDelegate _webView:decidePolicyForNavigationAction:decisionHandler:]):
+        (-[AsyncAutoplayPoliciesDelegate _webView:handleAutoplayEvent:withFlags:]):
+        (TEST):
+
 2018-03-19  Daniel Bates  <dabates@apple.com>
 
         test-webkitpy no longer runs WebKit2 tests
index 749e10e..e99e90f 100644 (file)
@@ -200,6 +200,42 @@ TEST(WebKit, WebsitePoliciesContentBlockersEnabled)
 
 @end
 
+@interface AsyncAutoplayPoliciesDelegate : NSObject <WKNavigationDelegate, WKUIDelegatePrivate>
+@property (nonatomic, copy) _WKWebsiteAutoplayPolicy(^autoplayPolicyForURL)(NSURL *);
+@property (nonatomic, copy) _WKWebsiteAutoplayQuirk(^allowedAutoplayQuirksForURL)(NSURL *);
+@end
+
+@implementation AsyncAutoplayPoliciesDelegate
+
+- (void)webView:(WKWebView *)webView decidePolicyForNavigationAction:(WKNavigationAction *)navigationAction decisionHandler:(void (^)(WKNavigationActionPolicy))decisionHandler
+{
+    // _webView:decidePolicyForNavigationAction:decisionHandler: should be called instead if it is implemented.
+    EXPECT_TRUE(false);
+    decisionHandler(WKNavigationActionPolicyAllow);
+}
+
+- (void)_webView:(WKWebView *)webView decidePolicyForNavigationAction:(WKNavigationAction *)navigationAction decisionHandler:(void (^)(WKNavigationActionPolicy, _WKWebsitePolicies *))decisionHandler
+{
+    dispatch_async(dispatch_get_main_queue(), ^{
+        _WKWebsitePolicies *websitePolicies = [[[_WKWebsitePolicies alloc] init] autorelease];
+        if (_allowedAutoplayQuirksForURL)
+            websitePolicies.allowedAutoplayQuirks = _allowedAutoplayQuirksForURL(navigationAction.request.URL);
+        if (_autoplayPolicyForURL)
+            websitePolicies.autoplayPolicy = _autoplayPolicyForURL(navigationAction.request.URL);
+        decisionHandler(WKNavigationActionPolicyAllow, websitePolicies);
+    });
+}
+
+#if PLATFORM(MAC)
+- (void)_webView:(WKWebView *)webView handleAutoplayEvent:(_WKAutoplayEvent)event withFlags:(_WKAutoplayEventFlags)flags
+{
+    receivedAutoplayEventFlags = flags;
+    receivedAutoplayEvent = event;
+}
+#endif
+
+@end
+
 TEST(WebKit, WebsitePoliciesAutoplayEnabled)
 {
     auto configuration = adoptNS([[WKWebViewConfiguration alloc] init]);
@@ -658,6 +694,49 @@ TEST(WebKit, WebsitePoliciesAutoplayQuirks)
     [webView stringByEvaluatingJavaScript:@"play()"];
     [webView waitForMessage:@"playing"];
 }
+
+TEST(WebKit, WebsitePoliciesAutoplayQuirksAsyncPolicyDelegate)
+{
+    auto configuration = adoptNS([[WKWebViewConfiguration alloc] init]);
+    auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:configuration.get()]);
+
+    auto delegate = adoptNS([[AsyncAutoplayPoliciesDelegate alloc] init]);
+    [webView setNavigationDelegate:delegate.get()];
+
+    WKRetainPtr<WKPreferencesRef> preferences(AdoptWK, WKPreferencesCreate());
+    WKPreferencesSetNeedsSiteSpecificQuirks(preferences.get(), true);
+    WKPageGroupSetPreferences(WKPageGetPageGroup([webView _pageForTesting]), preferences.get());
+
+    NSURLRequest *requestWithAudio = [NSURLRequest requestWithURL:[[NSBundle mainBundle] URLForResource:@"autoplay-check" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"]];
+
+    [delegate setAllowedAutoplayQuirksForURL:^_WKWebsiteAutoplayQuirk(NSURL *url) {
+        return _WKWebsiteAutoplayQuirkSynthesizedPauseEvents;
+    }];
+    [delegate setAutoplayPolicyForURL:^(NSURL *) {
+        return _WKWebsiteAutoplayPolicyDeny;
+    }];
+    [webView loadRequest:requestWithAudio];
+    [webView waitForMessage:@"did-not-play"];
+    [webView waitForMessage:@"on-pause"];
+
+    receivedAutoplayEvent = std::nullopt;
+    [webView loadHTMLString:@"" baseURL:nil];
+
+    NSURLRequest *requestWithAudioInFrame = [NSURLRequest requestWithURL:[[NSBundle mainBundle] URLForResource:@"autoplay-check-in-iframe" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"]];
+
+    [delegate setAllowedAutoplayQuirksForURL:^_WKWebsiteAutoplayQuirk(NSURL *url) {
+        if ([url.lastPathComponent isEqualToString:@"autoplay-check-frame.html"])
+            return _WKWebsiteAutoplayQuirkInheritedUserGestures;
+
+        return _WKWebsiteAutoplayQuirkSynthesizedPauseEvents | _WKWebsiteAutoplayQuirkInheritedUserGestures;
+    }];
+    [delegate setAutoplayPolicyForURL:^(NSURL *) {
+        return _WKWebsiteAutoplayPolicyDeny;
+    }];
+    [webView loadRequest:requestWithAudioInFrame];
+    [webView waitForMessage:@"did-not-play"];
+    [webView waitForMessage:@"on-pause"];
+}
 #endif // PLATFORM(MAC)
 
 TEST(WebKit, InvalidCustomHeaders)