[macOS] TestWebKitAPI.WebKit.HTTPReferer is a flaky failure
authorachristensen@apple.com <achristensen@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 21 May 2020 18:52:06 +0000 (18:52 +0000)
committerachristensen@apple.com <achristensen@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 21 May 2020 18:52:06 +0000 (18:52 +0000)
https://bugs.webkit.org/show_bug.cgi?id=212180

Reviewed by Darin Adler.

Sometimes an HTTP request takes more than one call to nw_connection_receive to receive entirely.
Add a new abstraction Connection that wraps an nw_connection_t and knows how to read an entire request.
Use strnstr instead of null terminating and using strstr.

* TestWebKitAPI/Tests/WebKitCocoa/Download.mm:
(TEST):
* TestWebKitAPI/Tests/WebKitCocoa/NetworkProcess.mm:
(TEST):
* TestWebKitAPI/Tests/WebKitCocoa/Proxy.mm:
(TestWebKitAPI::TEST):
* TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:
* TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerTCPServer.h:
* TestWebKitAPI/cocoa/HTTPServer.h:
(TestWebKitAPI::Connection::receiveHTTPRequest):
(TestWebKitAPI::Connection::Connection):
* TestWebKitAPI/cocoa/HTTPServer.mm:
(TestWebKitAPI::HTTPServer::HTTPServer):
(TestWebKitAPI::dataFromString):
(TestWebKitAPI::vectorFromData):
(TestWebKitAPI::HTTPServer::respondToRequests):
(TestWebKitAPI::HTTPServer::request const):
(TestWebKitAPI::Connection::receiveHTTPRequest const):
(TestWebKitAPI::Connection::send const):
(TestWebKitAPI::Connection::terminate const):
(TestWebKitAPI::nullTerminatedRequest): Deleted.

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

Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKitCocoa/Download.mm
Tools/TestWebKitAPI/Tests/WebKitCocoa/NetworkProcess.mm
Tools/TestWebKitAPI/Tests/WebKitCocoa/Proxy.mm
Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm
Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerTCPServer.h
Tools/TestWebKitAPI/cocoa/HTTPServer.h
Tools/TestWebKitAPI/cocoa/HTTPServer.mm

index db7a458..d40ef3b 100644 (file)
@@ -1,3 +1,36 @@
+2020-05-21  Alex Christensen  <achristensen@webkit.org>
+
+        [macOS] TestWebKitAPI.WebKit.HTTPReferer is a flaky failure
+        https://bugs.webkit.org/show_bug.cgi?id=212180
+
+        Reviewed by Darin Adler.
+
+        Sometimes an HTTP request takes more than one call to nw_connection_receive to receive entirely.
+        Add a new abstraction Connection that wraps an nw_connection_t and knows how to read an entire request.
+        Use strnstr instead of null terminating and using strstr.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/Download.mm:
+        (TEST):
+        * TestWebKitAPI/Tests/WebKitCocoa/NetworkProcess.mm:
+        (TEST):
+        * TestWebKitAPI/Tests/WebKitCocoa/Proxy.mm:
+        (TestWebKitAPI::TEST):
+        * TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:
+        * TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerTCPServer.h:
+        * TestWebKitAPI/cocoa/HTTPServer.h:
+        (TestWebKitAPI::Connection::receiveHTTPRequest):
+        (TestWebKitAPI::Connection::Connection):
+        * TestWebKitAPI/cocoa/HTTPServer.mm:
+        (TestWebKitAPI::HTTPServer::HTTPServer):
+        (TestWebKitAPI::dataFromString):
+        (TestWebKitAPI::vectorFromData):
+        (TestWebKitAPI::HTTPServer::respondToRequests):
+        (TestWebKitAPI::HTTPServer::request const):
+        (TestWebKitAPI::Connection::receiveHTTPRequest const):
+        (TestWebKitAPI::Connection::send const):
+        (TestWebKitAPI::Connection::terminate const):
+        (TestWebKitAPI::nullTerminatedRequest): Deleted.
+
 2020-05-21  Enrique Ocaña González  <eocanha@igalia.com>
 
         [GStreamer][GTK][WPE] Expose and honor the media content types requiring hardware support setting
index 2fc8b2a..120753f 100644 (file)
@@ -1172,41 +1172,30 @@ String longString(LChar c)
 TEST(_WKDownload, Resume)
 {
     using namespace TestWebKitAPI;
-    HTTPServer server([connectionCount = 0](nw_connection_t connection) mutable {
+    HTTPServer server([connectionCount = 0](Connection connection) mutable {
         switch (++connectionCount) {
         case 1:
-            nw_connection_receive(connection, 1, std::numeric_limits<uint32_t>::max(), makeBlockPtr([connection = retainPtr(connection)](dispatch_data_t content, nw_content_context_t context, bool, nw_error_t error) {
-                EXPECT_TRUE(content);
-                EXPECT_FALSE(error);
-                auto data = dataFromString(makeString(
+            connection.receiveHTTPRequest([connection](Vector<char>&&) {
+                connection.send(makeString(
                     "HTTP/1.1 200 OK\r\n"
                     "ETag: test\r\n"
                     "Content-Length: 10000\r\n"
                     "Content-Disposition: attachment; filename=\"example.txt\"\r\n"
-                    "\r\n", longString<5000>('a')));
-                nw_connection_send(connection.get(), data.get(), context, false, ^(nw_error_t error) {
-                    ASSERT(!error);
-                });
-            }).get());
+                    "\r\n", longString<5000>('a')
+                ));
+            });
             break;
         case 2:
-            nw_connection_receive(connection, 1, std::numeric_limits<uint32_t>::max(), makeBlockPtr([connection = retainPtr(connection)](dispatch_data_t content, nw_content_context_t context, bool, nw_error_t error) {
-                EXPECT_TRUE(content);
-                EXPECT_FALSE(error);
-
-                auto request = nullTerminatedRequest(content);
-                EXPECT_TRUE(strstr(request.data(), "Range: bytes=5000-\r\n"));
-
-                auto data = dataFromString(makeString(
+            connection.receiveHTTPRequest([connection](Vector<char>&& request) {
+                EXPECT_TRUE(strnstr(request.data(), "Range: bytes=5000-\r\n", request.size()));
+                connection.send(makeString(
                     "HTTP/1.1 206 Partial Content\r\n"
                     "ETag: test\r\n"
                     "Content-Length: 5000\r\n"
                     "Content-Range: bytes 5000-9999/10000\r\n"
-                    "\r\n", longString<5000>('b')));
-                nw_connection_send(connection.get(), data.get(), context, true, ^(nw_error_t error) {
-                    ASSERT(!error);
-                });
-            }).get());
+                    "\r\n", longString<5000>('b')
+                ));
+            });
             break;
         default:
             ASSERT_NOT_REACHED();
index 5c37a82..4d80c89 100644 (file)
@@ -54,17 +54,15 @@ TEST(WebKit, HTTPReferer)
     auto checkReferer = [] (NSURL *baseURL, const char* expectedReferer) {
         using namespace TestWebKitAPI;
         bool done = false;
-        HTTPServer server([done = &done, expectedReferer] (nw_connection_t connection) {
-            nw_connection_receive(connection, 1, std::numeric_limits<uint32_t>::max(), makeBlockPtr([connection = retainPtr(connection), done, expectedReferer](dispatch_data_t content, nw_content_context_t, bool, nw_error_t) {
-                EXPECT_TRUE(content);
-                auto request = nullTerminatedRequest(content);
+        HTTPServer server([&] (Connection connection) {
+            connection.receiveHTTPRequest([connection, expectedReferer, &done] (Vector<char>&& request) {
                 if (expectedReferer) {
                     auto expectedHeaderField = makeString("Referer: ", expectedReferer, "\r\n");
-                    EXPECT_TRUE(strstr(request.data(), expectedHeaderField.utf8().data()));
+                    EXPECT_TRUE(strnstr(request.data(), expectedHeaderField.utf8().data(), request.size()));
                 } else
-                    EXPECT_FALSE(strstr(request.data(), "Referer:"));
-                *done = true;
-            }).get());
+                    EXPECT_FALSE(strnstr(request.data(), "Referer:", request.size()));
+                done = true;
+            });
         });
         auto webView = adoptNS([WKWebView new]);
         [webView loadHTMLString:[NSString stringWithFormat:@"<body onload='document.getElementById(\"formID\").submit()'><form id='formID' method='post' action='http://127.0.0.1:%d/'></form></body>", server.port()] baseURL:baseURL];
index 8a4de43..7088f36 100644 (file)
 
 #if HAVE(SSL)
 
+#import "HTTPServer.h"
 #import "PlatformUtilities.h"
 #import "TCPServer.h"
+#import "TestNavigationDelegate.h"
+#import "TestUIDelegate.h"
+#import "TestWKWebView.h"
 #import "Utilities.h"
 #import <WebKit/WKWebsiteDataStorePrivate.h>
 #import <WebKit/WebKit.h>
 #import <WebKit/_WKWebsiteDataStoreConfiguration.h>
 #import <wtf/RetainPtr.h>
+#import <wtf/text/StringConcatenateNumbers.h>
 
 @interface ProxyDelegate : NSObject <WKNavigationDelegate, WKUIDelegate>
 - (NSString *)waitForAlert;
@@ -91,7 +96,7 @@ TEST(WebKit, HTTPProxyAuthentication)
 {
     TCPServer server([] (int socket) {
         auto requestShouldContain = [] (const auto& request, const char* str) {
-            EXPECT_TRUE(strstr(reinterpret_cast<const char*>(request.data()), str));
+            EXPECT_TRUE(strnstr(reinterpret_cast<const char*>(request.data()), str, request.size()));
         };
 
         auto connectRequest = TCPServer::read(socket);
index 75265d2..1072af5 100644 (file)
@@ -2070,7 +2070,7 @@ TEST(ServiceWorkers, ContentRuleList)
         TCPServer::read(socket);
         respond(contentRuleListWorkerScript, "application/javascript");
         auto lastRequest = TCPServer::read(socket);
-        EXPECT_TRUE(strstr((const char*)lastRequest.data(), "allowedsubresource"));
+        EXPECT_TRUE(strnstr((const char*)lastRequest.data(), "allowedsubresource", lastRequest.size()));
         respond("successful fetch", "application/octet-stream");
     });
 
index 39dbdfe..30e6a7c 100644 (file)
@@ -45,7 +45,7 @@ public:
                 for (auto& info : vector) {
                     auto request = TCPServer::read(socket);
                     if (!expectedUserAgent.isNull()) {
-                        EXPECT_TRUE(strstr((const char*)request.data(), makeString("User-Agent: ", expectedUserAgent).utf8().data()));
+                        EXPECT_TRUE(strnstr((const char*)request.data(), makeString("User-Agent: ", expectedUserAgent).utf8().data(), request.size()));
                         m_userAgentsChecked++;
                     }
                     NSString *format = @"HTTP/1.1 200 OK\r\n"
index d022d88..fcc5247 100644 (file)
 #if HAVE(NETWORK_FRAMEWORK)
 
 #import <Network/Network.h>
+#import <wtf/CompletionHandler.h>
 #import <wtf/Forward.h>
 #import <wtf/HashMap.h>
 #import <wtf/text/StringHash.h>
 
 namespace TestWebKitAPI {
 
+class Connection;
+
 class HTTPServer {
 public:
     struct HTTPResponse;
@@ -44,23 +47,34 @@ public:
     using CertificateVerifier = Function<void(sec_protocol_metadata_t, sec_trust_t, sec_protocol_verify_complete_t)>;
 
     HTTPServer(std::initializer_list<std::pair<String, HTTPResponse>>, Protocol = Protocol::Http, CertificateVerifier&& = nullptr);
-    HTTPServer(Function<void(nw_connection_t)>&&, Protocol = Protocol::Http);
+    HTTPServer(Function<void(Connection)>&&, Protocol = Protocol::Http);
     ~HTTPServer();
     uint16_t port() const;
-    NSURLRequest *request() const;
+    NSURLRequest *request(const String& path = "/"_str) const;
     size_t totalRequests() const;
 
 private:
     static RetainPtr<nw_parameters_t> listenerParameters(Protocol, CertificateVerifier&&);
-    static void respondToRequests(nw_connection_t, RefPtr<RequestData>);
+    static void respondToRequests(Connection, RefPtr<RequestData>);
 
     RefPtr<RequestData> m_requestData;
     RetainPtr<nw_listener_t> m_listener;
     Protocol m_protocol { Protocol::Http };
 };
 
-RetainPtr<dispatch_data_t> dataFromString(String&&);
-Vector<char> nullTerminatedRequest(dispatch_data_t);
+class Connection {
+public:
+    void send(String&&, CompletionHandler<void()>&& = nullptr) const;
+    void receiveHTTPRequest(CompletionHandler<void(Vector<char>&&)>&&, Vector<char>&& buffer = { }) const;
+    void terminate() const;
+
+private:
+    friend class HTTPServer;
+    Connection(nw_connection_t connection)
+        : m_connection(connection) { }
+
+    RetainPtr<nw_connection_t> m_connection;
+};
 
 struct HTTPServer::HTTPResponse {
     enum class TerminateConnection { No, Yes };
index 58089f2..b7636b8 100644 (file)
@@ -95,12 +95,12 @@ HTTPServer::HTTPServer(std::initializer_list<std::pair<String, HTTPResponse>> re
     nw_listener_set_new_connection_handler(m_listener.get(), makeBlockPtr([requestData = m_requestData](nw_connection_t connection) {
         nw_connection_set_queue(connection, dispatch_get_main_queue());
         nw_connection_start(connection);
-        respondToRequests(connection, requestData);
+        respondToRequests(Connection(connection), requestData);
     }).get());
     startListening(m_listener.get());
 }
 
-HTTPServer::HTTPServer(Function<void(nw_connection_t)>&& connectionHandler, Protocol protocol)
+HTTPServer::HTTPServer(Function<void(Connection)>&& connectionHandler, Protocol protocol)
     : m_listener(adoptNS(nw_listener_create(listenerParameters(protocol, nullptr).get())))
     , m_protocol(protocol)
 {
@@ -108,7 +108,7 @@ HTTPServer::HTTPServer(Function<void(nw_connection_t)>&& connectionHandler, Prot
     nw_listener_set_new_connection_handler(m_listener.get(), makeBlockPtr([connectionHandler = WTFMove(connectionHandler)] (nw_connection_t connection) {
         nw_connection_set_queue(connection, dispatch_get_main_queue());
         nw_connection_start(connection);
-        connectionHandler(connection);
+        connectionHandler(Connection(connection));
     }).get());
     startListening(m_listener.get());
 }
@@ -136,7 +136,7 @@ static String statusText(unsigned statusCode)
     return { };
 }
 
-RetainPtr<dispatch_data_t> dataFromString(String&& s)
+static RetainPtr<dispatch_data_t> dataFromString(String&& s)
 {
     auto impl = s.releaseImpl();
     ASSERT(impl->is8Bit());
@@ -145,33 +145,28 @@ RetainPtr<dispatch_data_t> dataFromString(String&& s)
     }));
 }
 
-Vector<char> nullTerminatedRequest(dispatch_data_t content)
+static Vector<char> vectorFromData(dispatch_data_t content)
 {
     __block Vector<char> request;
     dispatch_data_apply(content, ^bool(dispatch_data_t, size_t, const void* buffer, size_t size) {
         request.append(static_cast<const char*>(buffer), size);
         return true;
     });
-    request.append('\0');
     return request;
 }
 
-void HTTPServer::respondToRequests(nw_connection_t connection, RefPtr<RequestData> requestData)
+void HTTPServer::respondToRequests(Connection connection, RefPtr<RequestData> requestData)
 {
-    nw_connection_receive(connection, 1, std::numeric_limits<uint32_t>::max(), makeBlockPtr([connection = retainPtr(connection), requestData](dispatch_data_t content, nw_content_context_t context, bool complete, nw_error_t error) {
-        if (error || !content)
+    connection.receiveHTTPRequest([connection, requestData] (Vector<char>&& request) {
+        if (!request.size())
             return;
-
         requestData->requestCount++;
-        auto request = nullTerminatedRequest(content);
 
         const char* getPathPrefix = "GET ";
         const char* postPathPrefix = "POST ";
         const char* pathSuffix = " HTTP/1.1\r\n";
-        const char* pathEnd = strstr(request.data(), pathSuffix);
+        const char* pathEnd = strnstr(request.data(), pathSuffix, request.size());
         ASSERT_WITH_MESSAGE(pathEnd, "HTTPServer assumes request is HTTP 1.1");
-        const char* doubleNewline = strstr(request.data(), "\r\n\r\n");
-        ASSERT_WITH_MESSAGE(doubleNewline, "HTTPServer assumes entire HTTP request is received at once");
         size_t pathPrefixLength = 0;
         if (!memcmp(request.data(), getPathPrefix, strlen(getPathPrefix)))
             pathPrefixLength = strlen(getPathPrefix);
@@ -182,49 +177,20 @@ void HTTPServer::respondToRequests(nw_connection_t connection, RefPtr<RequestDat
         String path(request.data() + pathPrefixLength, pathLength);
         ASSERT_WITH_MESSAGE(requestData->requestMap.contains(path), "This HTTPServer does not know how to respond to a request for %s", path.utf8().data());
 
-        CompletionHandler<void()> sendResponse = [connection, context = retainPtr(context), path = WTFMove(path), requestData] {
-            auto response = requestData->requestMap.get(path);
-            if (response.terminateConnection == HTTPResponse::TerminateConnection::Yes) {
-                nw_connection_cancel(connection.get());
-                return;
-            }
-            StringBuilder responseBuilder;
-            responseBuilder.append("HTTP/1.1 ");
-            responseBuilder.appendNumber(response.statusCode);
-            responseBuilder.append(' ');
-            responseBuilder.append(statusText(response.statusCode));
-            responseBuilder.append("\r\nContent-Length: ");
-            responseBuilder.appendNumber(response.body.length());
-            responseBuilder.append("\r\n");
-            for (auto& pair : response.headerFields) {
-                responseBuilder.append(pair.key);
-                responseBuilder.append(": ");
-                responseBuilder.append(pair.value);
-                responseBuilder.append("\r\n");
-            }
-            responseBuilder.append("\r\n");
-            responseBuilder.append(response.body);
-            nw_connection_send(connection.get(), dataFromString(responseBuilder.toString()).get(), context.get(), true, ^(nw_error_t error) {
-                ASSERT(!error);
-                respondToRequests(connection.get(), requestData);
-            });
-        };
-
-        if (auto* contentLengthBegin = strstr(request.data(), "Content-Length")) {
-            size_t contentLength = atoi(contentLengthBegin + strlen("Content-Length: "));
-            size_t headerLength = doubleNewline - request.data() + strlen("\r\n\r\n");
-            constexpr size_t nullTerminationLength = 1;
-            if (request.size() - nullTerminationLength - headerLength < contentLength) {
-                nw_connection_receive(connection.get(), 1, std::numeric_limits<uint32_t>::max(), makeBlockPtr([sendResponse = WTFMove(sendResponse)] (dispatch_data_t content, nw_content_context_t context, bool complete, nw_error_t error) mutable {
-                    if (error || !content)
-                        return;
-                    sendResponse();
-                }).get());
-                return;
-            }
-        }
-        sendResponse();
-    }).get());
+        auto response = requestData->requestMap.get(path);
+        if (response.terminateConnection == HTTPResponse::TerminateConnection::Yes)
+            return connection.terminate();
+        StringBuilder responseBuilder;
+        responseBuilder.append("HTTP/1.1 ", response.statusCode, ' ', statusText(response.statusCode), "\r\n");
+        responseBuilder.append("Content-Length: ", response.body.length(), "\r\n");
+        for (auto& pair : response.headerFields)
+            responseBuilder.append(pair.key, ": ", pair.value, "\r\n");
+        responseBuilder.append("\r\n");
+        responseBuilder.append(response.body);
+        connection.send(responseBuilder.toString(), [connection, requestData] {
+            respondToRequests(connection, requestData);
+        });
+    });
 }
 
 uint16_t HTTPServer::port() const
@@ -232,19 +198,52 @@ uint16_t HTTPServer::port() const
     return nw_listener_get_port(m_listener.get());
 }
 
-NSURLRequest *HTTPServer::request() const
+NSURLRequest *HTTPServer::request(const String& path) const
 {
     NSString *format;
     switch (m_protocol) {
     case Protocol::Http:
-        format = @"http://127.0.0.1:%d/";
+        format = @"http://127.0.0.1:%d%s";
         break;
     case Protocol::Https:
     case Protocol::HttpsWithLegacyTLS:
-        format = @"https://127.0.0.1:%d/";
+        format = @"https://127.0.0.1:%d%s";
         break;
     }
-    return [NSURLRequest requestWithURL:[NSURL URLWithString:[NSString stringWithFormat:format, port()]]];
+    return [NSURLRequest requestWithURL:[NSURL URLWithString:[NSString stringWithFormat:format, port(), path.utf8().data()]]];
+}
+
+void Connection::receiveHTTPRequest(CompletionHandler<void(Vector<char>&&)>&& completionHandler, Vector<char>&& buffer) const
+{
+    nw_connection_receive(m_connection.get(), 1, std::numeric_limits<uint32_t>::max(), makeBlockPtr([connection = *this, completionHandler = WTFMove(completionHandler), buffer = WTFMove(buffer)](dispatch_data_t content, nw_content_context_t, bool, nw_error_t error) mutable {
+        if (error || !content)
+            return completionHandler({ });
+        buffer.appendVector(vectorFromData(content));
+        if (auto* doubleNewline = strnstr(buffer.data(), "\r\n\r\n", buffer.size())) {
+            if (auto* contentLengthBegin = strnstr(buffer.data(), "Content-Length", buffer.size())) {
+                size_t contentLength = atoi(contentLengthBegin + strlen("Content-Length: "));
+                size_t headerLength = doubleNewline - buffer.data() + strlen("\r\n\r\n");
+                if (buffer.size() - headerLength < contentLength)
+                    return connection.receiveHTTPRequest(WTFMove(completionHandler), WTFMove(buffer));
+            }
+            completionHandler(WTFMove(buffer));
+        } else
+            connection.receiveHTTPRequest(WTFMove(completionHandler), WTFMove(buffer));
+    }).get());
+}
+
+void Connection::send(String&& message, CompletionHandler<void()>&& completionHandler) const
+{
+    nw_connection_send(m_connection.get(), dataFromString(WTFMove(message)).get(), NW_CONNECTION_DEFAULT_MESSAGE_CONTEXT, true, makeBlockPtr([completionHandler = WTFMove(completionHandler)](nw_error_t error) mutable {
+        ASSERT(!error);
+        if (completionHandler)
+            completionHandler();
+    }).get());
+}
+
+void Connection::terminate() const
+{
+    nw_connection_cancel(m_connection.get());
 }
 
 } // namespace TestWebKitAPI