Changeset 246043 in webkit

Timestamp:

Jun 3, 2019 12:39:03 PM (5 years ago)

Author:

aestes@apple.com

Message:

Memory-cached main resources continue to load after the client decides a content policy of PolicyAction::Download
​https://bugs.webkit.org/show_bug.cgi?id=198469
<rdar://problem/50512713>

Reviewed by Youenn Fablet.

Source/WebCore:

When a document is loaded from the memory cache it does not have a main resource loader, but
DocumentLoader::continueAfterContentPolicy relies on being able to call
ResourceLoader::didFail on the main resource loader to cancel the provisional navigation
when the client decides a content policy of PolicyAction::Download.

This means that memory-cached main resources continue to load even after WebKit has started
to download the main resource. The expected behavior is for the provisional navigation to
fail once the download starts, like what happens when there is a main resource loader.

This patch teaches DocumentLoader::continueAfterContentPolicy to call
stopLoadingForPolicyChange() in the case of a null main resource loader. This will dispatch
didFailProvisionalNavigation and remove the DocumentLoader as a client of its
CachedRawResource to prevent it from delivering any cached data.

Added a new API test.

• loader/DocumentLoader.cpp:

(WebCore::DocumentLoader::continueAfterContentPolicy):

Tools:

• TestWebKitAPI/Tests/WebKitCocoa/Download.mm:

(-[TestDownloadNavigationResponseFromMemoryCacheDelegate webView:didStartProvisionalNavigation:]):
(-[TestDownloadNavigationResponseFromMemoryCacheDelegate webView:didFailProvisionalNavigation:withError:]):
(-[TestDownloadNavigationResponseFromMemoryCacheDelegate webView:didFinishNavigation:]):
(-[TestDownloadNavigationResponseFromMemoryCacheDelegate _downloadDidStart:]):
(-[TestDownloadNavigationResponseFromMemoryCacheDelegate webView:decidePolicyForNavigationResponse:decisionHandler:]):
(TEST):

• TestWebKitAPI/cocoa/TestProtocol.h:
• TestWebKitAPI/cocoa/TestProtocol.mm:

(+[TestProtocol additionalResponseHeaders]):
(+[TestProtocol setAdditionalResponseHeaders:]):
(-[TestProtocol startLoading]):

Location:

trunk

Files:

• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/loader/DocumentLoader.cpp (modified) (1 diff)
• Tools/ChangeLog (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/WebKitCocoa/Download.mm (modified) (2 diffs)
• Tools/TestWebKitAPI/cocoa/TestProtocol.h (modified) (1 diff)
• Tools/TestWebKitAPI/cocoa/TestProtocol.mm (modified) (3 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2019-06-02  Andy Estes  <aestes@apple.com>
+
+        Memory-cached main resources continue to load after the client decides a content policy of PolicyAction::Download
+        https://bugs.webkit.org/show_bug.cgi?id=198469
+        <rdar://problem/50512713>
+
+        Reviewed by Youenn Fablet.
+
+        When a document is loaded from the memory cache it does not have a main resource loader, but
+        DocumentLoader::continueAfterContentPolicy relies on being able to call
+        ResourceLoader::didFail on the main resource loader to cancel the provisional navigation
+        when the client decides a content policy of PolicyAction::Download.
+
+        This means that memory-cached main resources continue to load even after WebKit has started
+        to download the main resource. The expected behavior is for the provisional navigation to
+        fail once the download starts, like what happens when there is a main resource loader.
+
+        This patch teaches DocumentLoader::continueAfterContentPolicy to call
+        stopLoadingForPolicyChange() in the case of a null main resource loader. This will dispatch
+        didFailProvisionalNavigation and remove the DocumentLoader as a client of its
+        CachedRawResource to prevent it from delivering any cached data.
+
+        Added a new API test.
+
+        * loader/DocumentLoader.cpp:
+        (WebCore::DocumentLoader::continueAfterContentPolicy):
+
 2019-06-03  Timothy Hatcher  <timothy@apple.com>
 

trunk/Source/WebCore/loader/DocumentLoader.cpp

@@ -943 +943 @@
             frameLoader()->client().convertMainResourceLoadToDownload(this, sessionID, m_request, m_response);
 
-        // It might have gone missing
-        if (mainResourceLoader())
+        // The main resource might be loading from the memory cache, or its loader might have gone missing.
+        if (mainResourceLoader()) {
             static_cast<ResourceLoader*>(mainResourceLoader())->didFail(interruptedForPolicyChangeError());
+            return;
+        }
+
+        // We must stop loading even if there is no main resource loader. Otherwise, we might remain
+        // the client of a CachedRawResource that will continue to send us data.
+        stopLoadingForPolicyChange();
         return;
     }

trunk/Tools/ChangeLog

@@ +1 @@
+2019-06-02  Andy Estes  <aestes@apple.com>
+
+        Memory-cached main resources continue to load after the client decides a content policy of PolicyAction::Download
+        https://bugs.webkit.org/show_bug.cgi?id=198469
+        <rdar://problem/50512713>
+
+        Reviewed by Youenn Fablet.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/Download.mm:
+        (-[TestDownloadNavigationResponseFromMemoryCacheDelegate webView:didStartProvisionalNavigation:]):
+        (-[TestDownloadNavigationResponseFromMemoryCacheDelegate webView:didFailProvisionalNavigation:withError:]):
+        (-[TestDownloadNavigationResponseFromMemoryCacheDelegate webView:didFinishNavigation:]):
+        (-[TestDownloadNavigationResponseFromMemoryCacheDelegate _downloadDidStart:]):
+        (-[TestDownloadNavigationResponseFromMemoryCacheDelegate webView:decidePolicyForNavigationResponse:decisionHandler:]):
+        (TEST):
+        * TestWebKitAPI/cocoa/TestProtocol.h:
+        * TestWebKitAPI/cocoa/TestProtocol.mm:
+        (+[TestProtocol additionalResponseHeaders]):
+        (+[TestProtocol setAdditionalResponseHeaders:]):
+        (-[TestProtocol startLoading]):
+
 2019-06-03  Timothy Hatcher  <timothy@apple.com>
 

trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/Download.mm

@@ -34 +34 @@
 #import "TestProtocol.h"
 #import "TestWKWebView.h"
+#import <WebKit/WKErrorPrivate.h>
 #import <WebKit/WKNavigationDelegatePrivate.h>
 #import <WebKit/WKProcessPoolPrivate.h>
@@ -915 +916 @@
 } // namespace TestWebKitAPI
 
+@interface TestDownloadNavigationResponseFromMemoryCacheDelegate : NSObject <WKNavigationDelegate, _WKDownloadDelegate>
+@property (nonatomic) WKNavigationResponsePolicy responsePolicy;
+@property (nonatomic, readonly) BOOL didFailProvisionalNavigation;
+@property (nonatomic, readonly) BOOL didFinishNavigation;
+@property (nonatomic, readonly) BOOL didStartDownload;
+@end
+
+@implementation TestDownloadNavigationResponseFromMemoryCacheDelegate
+
+- (void)webView:(WKWebView *)webView didStartProvisionalNavigation:(WKNavigation *)navigation
+{
+    _didFailProvisionalNavigation = NO;
+    _didFinishNavigation = NO;
+    _didStartDownload = NO;
+}
+
+- (void)webView:(WKWebView *)webView didFailProvisionalNavigation:(WKNavigation *)navigation withError:(NSError *)error
+{
+    EXPECT_EQ(_WKErrorCodeFrameLoadInterruptedByPolicyChange, error.code);
+    EXPECT_FALSE(_didFinishNavigation);
+    EXPECT_WK_STREQ(_WKLegacyErrorDomain, error.domain);
+    _didFailProvisionalNavigation = YES;
+    if (_responsePolicy != _WKNavigationResponsePolicyBecomeDownload || _didStartDownload)
+        isDone = true;
+}
+
+- (void)webView:(WKWebView *)webView didFinishNavigation:(WKNavigation *)navigation
+{
+    EXPECT_FALSE(_didFailProvisionalNavigation);
+    _didFinishNavigation = YES;
+    if (_responsePolicy != _WKNavigationResponsePolicyBecomeDownload || _didStartDownload)
+        isDone = true;
+}
+
+- (void)_downloadDidStart:(_WKDownload *)download
+{
+    _didStartDownload = YES;
+    if (_didFailProvisionalNavigation || _didFinishNavigation)
+        isDone = true;
+}
+
+- (void)webView:(WKWebView *)webView decidePolicyForNavigationResponse:(WKNavigationResponse *)navigationResponse decisionHandler:(void (^)(WKNavigationResponsePolicy))decisionHandler
+{
+    decisionHandler(_responsePolicy);
+}
+
+@end
+
+TEST(WebKit, DownloadNavigationResponseFromMemoryCache)
+{
+    [TestProtocol registerWithScheme:@"http"];
+    TestProtocol.additionalResponseHeaders = @{ @"Cache-Control" : @"max-age=3600" };
+
+    auto delegate = adoptNS([[TestDownloadNavigationResponseFromMemoryCacheDelegate alloc] init]);
+    auto webView = adoptNS([[WKWebView alloc] init]);
+    [webView setNavigationDelegate:delegate.get()];
+    [webView configuration].processPool._downloadDelegate = delegate.get();
+
+    NSURL *firstURL = [NSURL URLWithString:@"http://bundle-html-file/simple"];
+    [delegate setResponsePolicy:WKNavigationResponsePolicyAllow];
+    [webView loadRequest:[NSURLRequest requestWithURL:firstURL]];
+    isDone = false;
+    TestWebKitAPI::Util::run(&isDone);
+    EXPECT_FALSE([delegate didFailProvisionalNavigation]);
+    EXPECT_FALSE([delegate didStartDownload]);
+    EXPECT_TRUE([delegate didFinishNavigation]);
+    EXPECT_WK_STREQ(firstURL.absoluteString, [webView URL].absoluteString);
+
+    NSURL *secondURL = [NSURL URLWithString:@"http://bundle-html-file/simple2"];
+    [delegate setResponsePolicy:_WKNavigationResponsePolicyBecomeDownload];
+    [webView loadRequest:[NSURLRequest requestWithURL:secondURL]];
+    isDone = false;
+    TestWebKitAPI::Util::run(&isDone);
+    EXPECT_FALSE([delegate didFinishNavigation]);
+    EXPECT_TRUE([delegate didFailProvisionalNavigation]);
+    EXPECT_TRUE([delegate didStartDownload]);
+    EXPECT_WK_STREQ(firstURL.absoluteString, [webView URL].absoluteString);
+
+    [delegate setResponsePolicy:WKNavigationResponsePolicyAllow];
+    [webView loadRequest:[NSURLRequest requestWithURL:secondURL]];
+    isDone = false;
+    TestWebKitAPI::Util::run(&isDone);
+    EXPECT_FALSE([delegate didFailProvisionalNavigation]);
+    EXPECT_FALSE([delegate didStartDownload]);
+    EXPECT_TRUE([delegate didFinishNavigation]);
+    EXPECT_WK_STREQ(secondURL.absoluteString, [webView URL].absoluteString);
+
+    [delegate setResponsePolicy:WKNavigationResponsePolicyAllow];
+    [webView goBack];
+    isDone = false;
+    TestWebKitAPI::Util::run(&isDone);
+    EXPECT_FALSE([delegate didFailProvisionalNavigation]);
+    EXPECT_FALSE([delegate didStartDownload]);
+    EXPECT_TRUE([delegate didFinishNavigation]);
+    EXPECT_WK_STREQ(firstURL.absoluteString, [webView URL].absoluteString);
+
+    [delegate setResponsePolicy:_WKNavigationResponsePolicyBecomeDownload];
+    [webView loadRequest:[NSURLRequest requestWithURL:secondURL]];
+    isDone = false;
+    TestWebKitAPI::Util::run(&isDone);
+    EXPECT_FALSE([delegate didFinishNavigation]);
+    EXPECT_TRUE([delegate didFailProvisionalNavigation]);
+    EXPECT_TRUE([delegate didStartDownload]);
+    EXPECT_WK_STREQ(firstURL.absoluteString, [webView URL].absoluteString);
+
+    TestProtocol.additionalResponseHeaders = nil;
+    [TestProtocol unregister];
+}
+
 #endif // PLATFORM(MAC) || PLATFORM(IOS)

trunk/Tools/TestWebKitAPI/cocoa/TestProtocol.h

@@ -32 +32 @@
 + (void)unregister;
 + (NSString *)scheme;
+@property (nonatomic, class, copy) NSDictionary<NSString *, NSString *> *additionalResponseHeaders;
 @end
 

trunk/Tools/TestWebKitAPI/cocoa/TestProtocol.mm

@@ -69 +69 @@
 }
 
+static NSDictionary<NSString *, NSString *> *additionalResponseHeaders;
+
++ (NSDictionary<NSString *, NSString *> *)additionalResponseHeaders
+{
+    return additionalResponseHeaders;
+}
+
++ (void)setAdditionalResponseHeaders:(NSDictionary<NSString *, NSString *> *)additionalHeaders
+{
+    [additionalResponseHeaders autorelease];
+    additionalResponseHeaders = [additionalHeaders copy];
+}
+
 static NSURL *createRedirectURL(NSString *query)
 {
@@ -87 +100 @@
     }
 
+    if ([requestURL.host isEqualToString:@"redirect"]) {
+        NSData *data = [@"PASS" dataUsingEncoding:NSASCIIStringEncoding];
+        auto response = adoptNS([[NSURLResponse alloc] initWithURL:requestURL MIMEType:@"text/html" expectedContentLength:data.length textEncodingName:nil]);
+        [self.client URLProtocol:self wasRedirectedToRequest:[NSURLRequest requestWithURL:createRedirectURL(requestURL.query)] redirectResponse:response.get()];
+        return;
+    }
+
     NSData *data;
     if ([requestURL.host isEqualToString:@"bundle-html-file"])
@@ -93 +113 @@
         data = [@"PASS" dataUsingEncoding:NSASCIIStringEncoding];
 
-    RetainPtr<NSURLResponse> response = adoptNS([[NSURLResponse alloc] initWithURL:requestURL MIMEType:@"text/html" expectedContentLength:data.length textEncodingName:nil]);
+    NSMutableDictionary *responseHeaders = [NSMutableDictionary dictionaryWithCapacity:2];
+    responseHeaders[@"Content-Type"] = @"text/html";
+    responseHeaders[@"Content-Length"] = [NSString stringWithFormat:@"%tu", data.length];
+    if (additionalResponseHeaders)
+        [responseHeaders addEntriesFromDictionary:additionalResponseHeaders];
 
-    if ([requestURL.host isEqualToString:@"redirect"]) {
-        [self.client URLProtocol:self wasRedirectedToRequest:[NSURLRequest requestWithURL:createRedirectURL(requestURL.query)] redirectResponse:response.get()];
-        return;
-    }
+    auto response = adoptNS([[NSHTTPURLResponse alloc] initWithURL:requestURL statusCode:200 HTTPVersion:@"HTTP/1.1" headerFields:responseHeaders]);
 
     [self.client URLProtocol:self didReceiveResponse:response.get() cacheStoragePolicy:NSURLCacheStorageNotAllowed];