Title: [279074] trunk
Revision
279074
Author
achristen...@apple.com
Date
2021-06-21 11:50:08 -0700 (Mon, 21 Jun 2021)

Log Message

Break ref cycle between API::HTTPCookieStore and WebKit::WebsiteDataStore
https://bugs.webkit.org/show_bug.cgi?id=226992

Reviewed by Chris Dumez.

Source/WebKit:

Covered by an API test.

* UIProcess/API/APIHTTPCookieStore.cpp:
(API::HTTPCookieStore::HTTPCookieStore):
(API::HTTPCookieStore::filterAppBoundCookies):
(API::HTTPCookieStore::cookies):
(API::HTTPCookieStore::cookiesForURL):
(API::HTTPCookieStore::setCookies):
(API::HTTPCookieStore::deleteCookie):
(API::HTTPCookieStore::deleteAllCookies):
(API::HTTPCookieStore::setHTTPCookieAcceptPolicy):
(API::HTTPCookieStore::flushCookies):
(API::HTTPCookieStore::registerObserver):
(API::HTTPCookieStore::unregisterObserver):
* UIProcess/API/APIHTTPCookieStore.h:
* UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.mm:
(WebKit::SOAuthorizationSession::complete):

Tools:

* TestWebKitAPI/Tests/WebKitCocoa/WKWebsiteDatastore.mm:
(TestWebKitAPI::TEST):

Modified Paths

Diff

Modified: trunk/Source/WebKit/ChangeLog (279073 => 279074)


--- trunk/Source/WebKit/ChangeLog	2021-06-21 18:48:29 UTC (rev 279073)
+++ trunk/Source/WebKit/ChangeLog	2021-06-21 18:50:08 UTC (rev 279074)
@@ -1,3 +1,28 @@
+2021-06-21  Alex Christensen  <achristen...@webkit.org>
+
+        Break ref cycle between API::HTTPCookieStore and WebKit::WebsiteDataStore
+        https://bugs.webkit.org/show_bug.cgi?id=226992
+
+        Reviewed by Chris Dumez.
+
+        Covered by an API test.
+
+        * UIProcess/API/APIHTTPCookieStore.cpp:
+        (API::HTTPCookieStore::HTTPCookieStore):
+        (API::HTTPCookieStore::filterAppBoundCookies):
+        (API::HTTPCookieStore::cookies):
+        (API::HTTPCookieStore::cookiesForURL):
+        (API::HTTPCookieStore::setCookies):
+        (API::HTTPCookieStore::deleteCookie):
+        (API::HTTPCookieStore::deleteAllCookies):
+        (API::HTTPCookieStore::setHTTPCookieAcceptPolicy):
+        (API::HTTPCookieStore::flushCookies):
+        (API::HTTPCookieStore::registerObserver):
+        (API::HTTPCookieStore::unregisterObserver):
+        * UIProcess/API/APIHTTPCookieStore.h:
+        * UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.mm:
+        (WebKit::SOAuthorizationSession::complete):
+
 2021-06-21  James Savage  <james.sav...@apple.com>
 
         Upstream async support to Swift overlay.

Modified: trunk/Source/WebKit/UIProcess/API/APIHTTPCookieStore.cpp (279073 => 279074)


--- trunk/Source/WebKit/UIProcess/API/APIHTTPCookieStore.cpp	2021-06-21 18:48:29 UTC (rev 279073)
+++ trunk/Source/WebKit/UIProcess/API/APIHTTPCookieStore.cpp	2021-06-21 18:50:08 UTC (rev 279074)
@@ -44,7 +44,7 @@
 namespace API {
 
 HTTPCookieStore::HTTPCookieStore(WebKit::WebsiteDataStore& websiteDataStore)
-    : m_owningDataStore(websiteDataStore)
+    : m_owningDataStore(makeWeakPtr(websiteDataStore))
 {
 }
 
@@ -55,45 +55,52 @@
     ASSERT(!m_cookieManagerProxyObserver);
 }
 
-void HTTPCookieStore::filterAppBoundCookies(const Vector<WebCore::Cookie>& cookies, CompletionHandler<void(Vector<WebCore::Cookie>&&)>&& completionHandler)
+void HTTPCookieStore::filterAppBoundCookies(Vector<WebCore::Cookie>&& cookies, CompletionHandler<void(Vector<WebCore::Cookie>&&)>&& completionHandler)
 {
-    Vector<WebCore::Cookie> appBoundCookies;
 #if ENABLE(APP_BOUND_DOMAINS)
-    m_owningDataStore->getAppBoundDomains([cookies, appBoundCookies = WTFMove(appBoundCookies), completionHandler = WTFMove(completionHandler)] (auto& domains) mutable {
+    if (!m_owningDataStore)
+        return completionHandler({ });
+    m_owningDataStore->getAppBoundDomains([cookies = WTFMove(cookies), completionHandler = WTFMove(completionHandler)] (auto& domains) mutable {
+        Vector<WebCore::Cookie> appBoundCookies;
         if (!domains.isEmpty() && !isFullWebBrowser()) {
-            for (auto& cookie : cookies) {
+            for (auto& cookie : WTFMove(cookies)) {
                 if (domains.contains(WebCore::RegistrableDomain::uncheckedCreateFromHost(cookie.domain)))
-                    appBoundCookies.append(cookie);
+                    appBoundCookies.append(WTFMove(cookie));
             }
         } else
-            appBoundCookies = cookies;
+            appBoundCookies = WTFMove(cookies);
         completionHandler(WTFMove(appBoundCookies));
     });
 #else
-    appBoundCookies = cookies;
-    completionHandler(WTFMove(appBoundCookies));
+    completionHandler(WTFMove(cookies));
 #endif
 }
 
 void HTTPCookieStore::cookies(CompletionHandler<void(const Vector<WebCore::Cookie>&)>&& completionHandler)
 {
+    if (!m_owningDataStore)
+        return completionHandler({ });
     auto& cookieManager = m_owningDataStore->networkProcess().cookieManager();
-    cookieManager.getAllCookies(m_owningDataStore->sessionID(), [this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler)] (const Vector<WebCore::Cookie>& cookies) mutable {
-        filterAppBoundCookies(cookies, WTFMove(completionHandler));
+    cookieManager.getAllCookies(m_owningDataStore->sessionID(), [this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler)] (Vector<WebCore::Cookie>&& cookies) mutable {
+        filterAppBoundCookies(WTFMove(cookies), WTFMove(completionHandler));
     });
 }
 
 void HTTPCookieStore::cookiesForURL(WTF::URL&& url, CompletionHandler<void(Vector<WebCore::Cookie>&&)>&& completionHandler)
 {
+    if (!m_owningDataStore)
+        return completionHandler({ });
     auto& cookieManager = m_owningDataStore->networkProcess().cookieManager();
     cookieManager.getCookies(m_owningDataStore->sessionID(), url, [this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler)] (Vector<WebCore::Cookie>&& cookies) mutable {
-        filterAppBoundCookies(cookies, WTFMove(completionHandler));
+        filterAppBoundCookies(WTFMove(cookies), WTFMove(completionHandler));
     });
 }
 
-void HTTPCookieStore::setCookies(const Vector<WebCore::Cookie>& cookies, CompletionHandler<void()>&& completionHandler)
+void HTTPCookieStore::setCookies(Vector<WebCore::Cookie>&& cookies, CompletionHandler<void()>&& completionHandler)
 {
-    filterAppBoundCookies(cookies, [this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler)] (auto&& appBoundCookies) mutable {
+    filterAppBoundCookies(WTFMove(cookies), [this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler)] (auto&& appBoundCookies) mutable {
+        if (!m_owningDataStore)
+            return;
         auto& cookieManager = m_owningDataStore->networkProcess().cookieManager();
         cookieManager.setCookies(m_owningDataStore->sessionID(), appBoundCookies, WTFMove(completionHandler));
     });
@@ -101,6 +108,8 @@
 
 void HTTPCookieStore::deleteCookie(const WebCore::Cookie& cookie, CompletionHandler<void()>&& completionHandler)
 {
+    if (!m_owningDataStore)
+        return completionHandler();
     auto& cookieManager = m_owningDataStore->networkProcess().cookieManager();
     cookieManager.deleteCookie(m_owningDataStore->sessionID(), cookie, WTFMove(completionHandler));
 }
@@ -107,6 +116,8 @@
 
 void HTTPCookieStore::deleteAllCookies(CompletionHandler<void()>&& completionHandler)
 {
+    if (!m_owningDataStore)
+        return completionHandler();
     auto& cookieManager = m_owningDataStore->networkProcess().cookieManager();
     cookieManager.deleteAllCookies(m_owningDataStore->sessionID());
     // FIXME: The CompletionHandler should be passed to WebCookieManagerProxy::deleteAllCookies.
@@ -115,6 +126,8 @@
 
 void HTTPCookieStore::setHTTPCookieAcceptPolicy(WebCore::HTTPCookieAcceptPolicy policy, CompletionHandler<void()>&& completionHandler)
 {
+    if (!m_owningDataStore)
+        return completionHandler();
     auto& cookieManager = m_owningDataStore->networkProcess().cookieManager();
     cookieManager.setHTTPCookieAcceptPolicy(m_owningDataStore->sessionID(), policy, WTFMove(completionHandler));
 }
@@ -121,6 +134,8 @@
 
 void HTTPCookieStore::flushCookies(CompletionHandler<void()>&& completionHandler)
 {
+    if (!m_owningDataStore)
+        return completionHandler();
     m_owningDataStore->flushCookies(WTFMove(completionHandler));
 }
 
@@ -145,7 +160,7 @@
 {
     m_observers.add(&observer);
 
-    if (m_cookieManagerProxyObserver)
+    if (m_cookieManagerProxyObserver || !m_owningDataStore)
         return;
 
     ASSERT(!m_observedCookieManagerProxy);
@@ -163,7 +178,7 @@
     if (!m_observers.computesEmpty())
         return;
 
-    if (m_observedCookieManagerProxy)
+    if (m_observedCookieManagerProxy && m_owningDataStore)
         m_observedCookieManagerProxy->unregisterObserver(m_owningDataStore->sessionID(), *m_cookieManagerProxyObserver);
 
     m_observedCookieManagerProxy = nullptr;

Modified: trunk/Source/WebKit/UIProcess/API/APIHTTPCookieStore.h (279073 => 279074)


--- trunk/Source/WebKit/UIProcess/API/APIHTTPCookieStore.h	2021-06-21 18:48:29 UTC (rev 279073)
+++ trunk/Source/WebKit/UIProcess/API/APIHTTPCookieStore.h	2021-06-21 18:50:08 UTC (rev 279074)
@@ -57,7 +57,7 @@
 
     void cookies(CompletionHandler<void(const Vector<WebCore::Cookie>&)>&&);
     void cookiesForURL(WTF::URL&&, CompletionHandler<void(Vector<WebCore::Cookie>&&)>&&);
-    void setCookies(const Vector<WebCore::Cookie>&, CompletionHandler<void()>&&);
+    void setCookies(Vector<WebCore::Cookie>&&, CompletionHandler<void()>&&);
     void deleteCookie(const WebCore::Cookie&, CompletionHandler<void()>&&);
     
     void deleteAllCookies(CompletionHandler<void()>&&);
@@ -75,14 +75,12 @@
 
     void cookiesDidChange();
 
-    void filterAppBoundCookies(const Vector<WebCore::Cookie>&, CompletionHandler<void(Vector<WebCore::Cookie>&&)>&&);
+    void filterAppBoundCookies(Vector<WebCore::Cookie>&&, CompletionHandler<void(Vector<WebCore::Cookie>&&)>&&);
 
 private:
     HTTPCookieStore(WebKit::WebsiteDataStore&);
     
-    // FIXME: This is a reference cycle.
-    Ref<WebKit::WebsiteDataStore> m_owningDataStore;
-
+    WeakPtr<WebKit::WebsiteDataStore> m_owningDataStore;
     WeakHashSet<Observer> m_observers;
     WeakPtr<WebKit::WebCookieManagerProxy> m_observedCookieManagerProxy;
     std::unique_ptr<APIWebCookieManagerProxyObserver> m_cookieManagerProxyObserver;

Modified: trunk/Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.mm (279073 => 279074)


--- trunk/Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.mm	2021-06-21 18:48:29 UTC (rev 279073)
+++ trunk/Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.mm	2021-06-21 18:50:08 UTC (rev 279074)
@@ -235,7 +235,7 @@
 
     if (!m_page)
         return;
-    m_page->websiteDataStore().cookieStore().setCookies(cookies, [this, weakThis = makeWeakPtr(*this), response = WTFMove(response), data = "" alloc] initWithData:data])] () mutable {
+    m_page->websiteDataStore().cookieStore().setCookies(WTFMove(cookies), [this, weakThis = makeWeakPtr(*this), response = WTFMove(response), data = "" alloc] initWithData:data])] () mutable {
         if (!weakThis)
             return;
 

Modified: trunk/Tools/ChangeLog (279073 => 279074)


--- trunk/Tools/ChangeLog	2021-06-21 18:48:29 UTC (rev 279073)
+++ trunk/Tools/ChangeLog	2021-06-21 18:50:08 UTC (rev 279074)
@@ -1,3 +1,13 @@
+2021-06-21  Alex Christensen  <achristen...@webkit.org>
+
+        Break ref cycle between API::HTTPCookieStore and WebKit::WebsiteDataStore
+        https://bugs.webkit.org/show_bug.cgi?id=226992
+
+        Reviewed by Chris Dumez.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/WKWebsiteDatastore.mm:
+        (TestWebKitAPI::TEST):
+
 2021-06-21  Kimmo Kinnunen  <kkinnu...@apple.com>
 
         makeUnique cannot be used to instantiate function-local classes

Modified: trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebsiteDatastore.mm (279073 => 279074)


--- trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebsiteDatastore.mm	2021-06-21 18:48:29 UTC (rev 279073)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebsiteDatastore.mm	2021-06-21 18:50:08 UTC (rev 279074)
@@ -36,6 +36,7 @@
 #import <WebKit/WKWebsiteDataStorePrivate.h>
 #import <WebKit/WebKit.h>
 #import <WebKit/_WKWebsiteDataStoreConfiguration.h>
+#import <wtf/WeakObjCPtr.h>
 #import <wtf/text/WTFString.h>
 
 static bool readyToContinue;
@@ -343,6 +344,18 @@
     while (countSessionSets()) { }
 }
 
+TEST(WKWebsiteDataStore, ReferenceCycle)
+{
+    WeakObjCPtr<WKWebsiteDataStore> dataStore;
+    WeakObjCPtr<WKHTTPCookieStore> cookieStore;
+    @autoreleasepool {
+        dataStore = [WKWebsiteDataStore nonPersistentDataStore];
+        cookieStore = [dataStore httpCookieStore];
+    }
+    while (dataStore.get() || cookieStore.get())
+        TestWebKitAPI::Util::spinRunLoop();
+}
+
 TEST(WebKit, ClearCustomDataStoreNoWebViews)
 {
     HTTPServer server([connectionCount = 0] (Connection connection) mutable {
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to