Attachment 371543 Details for Bug 198635 – Patch

[patch] Patch

bug-198635-20190606172928.patch (text/plain), 9.50 KB, created by Chris Dumez on 2019-06-06 17:29:30 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2019-06-06 17:29:30 PDT

Size: 9.50 KB

patch

obsolete

>Subversion Revision: 246173
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index f308a3645f5fd5e156f27032326357d07c9acb23..19431204cc401bd344848f77e050a30d2d2c3d71 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,29 @@
>+2019-06-06  Chris Dumez  <cdumez@apple.com>
>+
>+        [WKHTTPCookieStore getAllCookies:] may return duplicate cookies
>+        https://bugs.webkit.org/show_bug.cgi?id=198635
>+        <rdar://problem/46010232>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        [WKHTTPCookieStore getAllCookies:] could return duplicates when there is no process pool
>+        yet and there are pending cookies on the data store. Even though,
>+        WebsiteDataStore::m_pendingCookies was a HashSet, it could contain several cookies with
>+        the same name, as long as at least of of their properties (e.g. value) differs.
>+
>+        We should never have more than one cookie with the same name so this patches switches
>+        to using a HashMap<String, Cookie> where the key is the cookie name.
>+
>+        * UIProcess/API/APIHTTPCookieStore.cpp:
>+        (API::HTTPCookieStore::cookies):
>+        * UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:
>+        (WebKit::WebsiteDataStore::parameters):
>+        * UIProcess/WebsiteData/WebsiteDataStore.cpp:
>+        (WebKit::WebsiteDataStore::pendingCookies const):
>+        (WebKit::WebsiteDataStore::addPendingCookie):
>+        (WebKit::WebsiteDataStore::removePendingCookie):
>+        * UIProcess/WebsiteData/WebsiteDataStore.h:
>+
> 2019-06-06  Antoine Quint  <graouts@apple.com>
> 
>         Remove duplicated websiteDataStoreParameters code from WebsitePoliciesData::applyToDocumentLoader()
>diff --git a/Source/WebKit/UIProcess/API/APIHTTPCookieStore.cpp b/Source/WebKit/UIProcess/API/APIHTTPCookieStore.cpp
>index ec1083868f32d6629201c4cc29ed470304fa11a6..eb479fcdb2a174ea114ab2cd70ebfedb1c35d21c 100644
>--- a/Source/WebKit/UIProcess/API/APIHTTPCookieStore.cpp
>+++ b/Source/WebKit/UIProcess/API/APIHTTPCookieStore.cpp
>@@ -60,7 +60,13 @@ void HTTPCookieStore::cookies(CompletionHandler<void(const Vector<WebCore::Cooki
>         Vector<WebCore::Cookie> allCookies;
>         if (m_owningDataStore->sessionID() == PAL::SessionID::defaultSessionID())
>             allCookies = getAllDefaultUIProcessCookieStoreCookies();
>-        allCookies.appendVector(m_owningDataStore->pendingCookies());
>+
>+        for (auto& pendingCookie : m_owningDataStore->pendingCookies()) {
>+            allCookies.removeFirstMatching([&](auto& cookie) {
>+                return cookie.name == pendingCookie.name;
>+            });
>+            allCookies.append(pendingCookie);
>+        }
> 
>         callOnMainThread([completionHandler = WTFMove(completionHandler), allCookies] () mutable {
>             completionHandler(allCookies);
>diff --git a/Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm b/Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm
>index 2a56a7436e8a00860a5c0bd1120533392319c225..58f9ad0d4dae5977c9a895858044ef893b922370 100644
>--- a/Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm
>+++ b/Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm
>@@ -152,7 +152,7 @@ WebsiteDataStoreParameters WebsiteDataStore::parameters()
>     parameters.networkSessionParameters.sourceApplicationBundleIdentifier = m_sourceApplicationBundleIdentifier;
>     parameters.networkSessionParameters.sourceApplicationSecondaryIdentifier = m_sourceApplicationSecondaryIdentifier;
> 
>-    parameters.pendingCookies = copyToVector(m_pendingCookies);
>+    parameters.pendingCookies = copyToVector(m_pendingCookies.values());
> 
>     if (!cookieFile.isEmpty())
>         SandboxExtension::createHandleForReadWriteDirectory(FileSystem::directoryName(cookieFile), parameters.cookieStoragePathExtensionHandle);
>diff --git a/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp b/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp
>index 3ba29b31e0f3e89ba154587c1521a8aea7869a4f..a760a85226a8b07034f934f1c711143f35aa626c 100644
>--- a/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp
>+++ b/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp
>@@ -1934,19 +1934,24 @@ void WebsiteDataStore::clearResourceLoadStatisticsInWebProcesses(CompletionHandl
> }
> #endif
> 
>-Vector<WebCore::Cookie> WebsiteDataStore::pendingCookies() const
>+typename HashMap<String, WebCore::Cookie>::ValuesConstIteratorRange WebsiteDataStore::pendingCookies() const
> {
>-    return copyToVector(m_pendingCookies);
>+    return m_pendingCookies.values();
> }
> 
> void WebsiteDataStore::addPendingCookie(const WebCore::Cookie& cookie)
> {
>-    m_pendingCookies.add(cookie);
>+    m_pendingCookies.set(cookie.name, cookie);
> }
> 
> void WebsiteDataStore::removePendingCookie(const WebCore::Cookie& cookie)
> {
>-    m_pendingCookies.remove(cookie);
>+    auto it = m_pendingCookies.find(cookie.name);
>+    if (it == m_pendingCookies.end())
>+        return;
>+
>+    if (it->value == cookie)
>+        m_pendingCookies.remove(it);
> }
>     
> void WebsiteDataStore::clearPendingCookies()
>diff --git a/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h b/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h
>index 822c3fc7637d7c5eb003c69ce94745bd382593f2..63864120cb929267f0b31e94d7e09547eebeca27 100644
>--- a/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h
>+++ b/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h
>@@ -197,7 +197,7 @@ public:
> 
>     WebsiteDataStoreParameters parameters();
> 
>-    Vector<WebCore::Cookie> pendingCookies() const;
>+    typename HashMap<String, WebCore::Cookie>::ValuesConstIteratorRange pendingCookies() const;
>     void addPendingCookie(const WebCore::Cookie&);
>     void removePendingCookie(const WebCore::Cookie&);
>     void clearPendingCookies();
>@@ -312,7 +312,7 @@ private:
>     WebCore::CurlProxySettings m_proxySettings;
> #endif
> 
>-    HashSet<WebCore::Cookie> m_pendingCookies;
>+    HashMap<String, WebCore::Cookie> m_pendingCookies;
> 
>     String m_boundInterfaceIdentifier;
>     AllowsCellularAccess m_allowsCellularAccess { AllowsCellularAccess::Yes };
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index 7ff805d0138e7bcedf0c2899e96b187dae9bdbae..807507b642e683c0bfbcdb3652655b858aa0f68a 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,18 @@
>+2019-06-06  Chris Dumez  <cdumez@apple.com>
>+
>+        [WKHTTPCookieStore getAllCookies:] may return duplicate cookies
>+        https://bugs.webkit.org/show_bug.cgi?id=198635
>+        <rdar://problem/46010232>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Add API test coverage.
>+
>+        * TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm:
>+        (randomCookie):
>+        (areCookiesEqual):
>+        (TEST):
>+
> 2019-06-06  Keith Rollin  <krollin@apple.com>
> 
>         Remove SSL-based tests from TestWebKitAPI on some configurations
>diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm b/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm
>index 0f1e79ee16029dcb1ca8651e48efbe697b04020c..66b33d115b5312bab3652bfd9fea215772207ebd 100644
>--- a/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm
>+++ b/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm
>@@ -627,3 +627,56 @@ TEST(WebKit, WKHTTPCookieStoreWithoutProcessPoolEphemeralSession)
>     [webView loadHTMLString:alertCookieHTML baseURL:[NSURL URLWithString:@"http://127.0.0.1"]];
>     TestWebKitAPI::Util::run(&finished);
> }
>+
>+static RetainPtr<NSHTTPCookie> randomCookie()
>+{
>+    return adoptNS([[NSHTTPCookie alloc] initWithProperties:@{
>+        NSHTTPCookieName : [NSUUID UUID].UUIDString,
>+        NSHTTPCookieValue : [NSUUID UUID].UUIDString,
>+        NSHTTPCookieOriginURL : [@"http://" stringByAppendingString:[NSUUID UUID].UUIDString],
>+        NSHTTPCookieVersion : [NSUUID UUID].UUIDString,
>+        NSHTTPCookieDomain : [NSUUID UUID].UUIDString,
>+        NSHTTPCookiePath : [NSUUID UUID].UUIDString,
>+        NSHTTPCookieSecure : [NSUUID UUID].UUIDString
>+    }]);
>+}
>+
>+static bool areCookiesEqual(NSHTTPCookie *first, NSHTTPCookie *second)
>+{
>+    return [first.name isEqual:second.name] && [first.value isEqual:second.value];
>+}
>+
>+TEST(WebKit, WKHTTPCookieStoreWithoutProcessPoolDuplicates)
>+{
>+    auto randomCookie1 = randomCookie();
>+    auto properties = adoptNS([randomCookie1.get().properties mutableCopy]);
>+    properties.get()[NSHTTPCookieValue] = @"some_other_cookie_value";
>+    auto randomCookie2 = adoptNS([[NSHTTPCookie alloc] initWithProperties:properties.get()]);
>+
>+    [[WKWebsiteDataStore defaultDataStore].httpCookieStore setCookie:randomCookie1.get() completionHandler:^{
>+        [[WKWebsiteDataStore defaultDataStore].httpCookieStore setCookie:randomCookie2.get() completionHandler:^{
>+            dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(1 * NSEC_PER_SEC)), dispatch_get_main_queue(), ^{
>+                [[WKWebsiteDataStore defaultDataStore].httpCookieStore getAllCookies:^(NSArray<NSHTTPCookie *> * cookies) {
>+                    bool found = false;
>+                    for (NSHTTPCookie *cookie in cookies) {
>+                        if (areCookiesEqual(cookie, randomCookie2.get())) {
>+                            found = true;
>+                            break;
>+                        }
>+                    }
>+                    EXPECT_TRUE(found);
>+
>+                    for (NSHTTPCookie *cookie in cookies) {
>+                        if (areCookiesEqual(cookie, randomCookie1.get()))
>+                            EXPECT_TRUE(false);
>+                    }
>+
>+                    finished = true;
>+                }];
>+            });
>+        }];
>+    }];
>+    
>+    TestWebKitAPI::Util::run(&finished);
>+    finished = false;
>+}

Actions: View | Formatted Diff | Diff

Attachments on bug 198635: 371543 | 371554 | 371606 | 371641 | 371643 | 371749