Crash when a WKHTTPCookieStore outlives its owning WKWebsiteDataStore.
authorbeidson@apple.com <beidson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 17 Jul 2017 01:43:14 +0000 (01:43 +0000)
committerbeidson@apple.com <beidson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 17 Jul 2017 01:43:14 +0000 (01:43 +0000)
<rdar://problem/33341730> and https://bugs.webkit.org/show_bug.cgi?id=174574

Reviewed by Tim Horton.

Source/WebKit:

Instead of holding a weak reference to its owning API::WebsiteDataStore,
API::HTTPCookieStore can hold a strong reference to the owner's implementation
WebKit::WebsiteDataStore.

* UIProcess/API/APIHTTPCookieStore.cpp:
(API::HTTPCookieStore::HTTPCookieStore):
(API::HTTPCookieStore::cookies):
(API::HTTPCookieStore::setCookie):
(API::HTTPCookieStore::deleteCookie):
(API::HTTPCookieStore::registerObserver):
(API::HTTPCookieStore::unregisterObserver):
(API::HTTPCookieStore::cookieManagerDestroyed):
(API::HTTPCookieStore::registerForNewProcessPoolNotifications):
* UIProcess/API/APIHTTPCookieStore.h:

Tools:

* TestWebKitAPI/Tests/WebKit2Cocoa/WKHTTPCookieStore.mm:
(-[CookieNavigationDelegate webView:didFinishNavigation:]):
(TEST):

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

Source/WebKit/ChangeLog
Source/WebKit/UIProcess/API/APIHTTPCookieStore.cpp
Source/WebKit/UIProcess/API/APIHTTPCookieStore.h
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKHTTPCookieStore.mm

index ab15036..06ab7bd 100644 (file)
@@ -1,3 +1,25 @@
+2017-07-16  Brady Eidson  <beidson@apple.com>
+
+        Crash when a WKHTTPCookieStore outlives its owning WKWebsiteDataStore.
+        <rdar://problem/33341730> and https://bugs.webkit.org/show_bug.cgi?id=174574
+
+        Reviewed by Tim Horton.
+
+        Instead of holding a weak reference to its owning API::WebsiteDataStore,
+        API::HTTPCookieStore can hold a strong reference to the owner's implementation
+        WebKit::WebsiteDataStore.
+        
+        * UIProcess/API/APIHTTPCookieStore.cpp:
+        (API::HTTPCookieStore::HTTPCookieStore):
+        (API::HTTPCookieStore::cookies):
+        (API::HTTPCookieStore::setCookie):
+        (API::HTTPCookieStore::deleteCookie):
+        (API::HTTPCookieStore::registerObserver):
+        (API::HTTPCookieStore::unregisterObserver):
+        (API::HTTPCookieStore::cookieManagerDestroyed):
+        (API::HTTPCookieStore::registerForNewProcessPoolNotifications):
+        * UIProcess/API/APIHTTPCookieStore.h:
+
 2017-07-15  Brady Eidson  <beidson@apple.com>
 
         Make sure all CFHTTPCookieStorageRefs we create are scheduled.
index 5a91593..5767455 100644 (file)
@@ -38,7 +38,7 @@ using namespace WebKit;
 namespace API {
 
 HTTPCookieStore::HTTPCookieStore(WebsiteDataStore& websiteDataStore)
-    : m_owningDataStore(websiteDataStore)
+    : m_owningDataStore(websiteDataStore.websiteDataStore())
 {
 }
 
@@ -54,27 +54,25 @@ HTTPCookieStore::~HTTPCookieStore()
 
 void HTTPCookieStore::cookies(Function<void (const Vector<WebCore::Cookie>&)>&& completionHandler)
 {
-    auto& dataStore = m_owningDataStore.websiteDataStore();
-    auto* pool = dataStore.processPoolForCookieStorageOperations();
+    auto* pool = m_owningDataStore->processPoolForCookieStorageOperations();
     if (!pool) {
-        callOnMainThread([completionHandler = WTFMove(completionHandler), allCookies = dataStore.pendingCookies()]() {
+        callOnMainThread([completionHandler = WTFMove(completionHandler), allCookies = m_owningDataStore->pendingCookies()]() {
             completionHandler(allCookies);
         });
         return;
     }
 
     auto* cookieManager = pool->supplement<WebKit::WebCookieManagerProxy>();
-    cookieManager->getAllCookies(dataStore.sessionID(), [pool = WTFMove(pool), completionHandler = WTFMove(completionHandler)](const Vector<WebCore::Cookie>& cookies, CallbackBase::Error error) {
+    cookieManager->getAllCookies(m_owningDataStore->sessionID(), [pool = WTFMove(pool), completionHandler = WTFMove(completionHandler)](const Vector<WebCore::Cookie>& cookies, CallbackBase::Error error) {
         completionHandler(cookies);
     });
 }
 
 void HTTPCookieStore::setCookie(const WebCore::Cookie& cookie, Function<void ()>&& completionHandler)
 {
-    auto& dataStore = m_owningDataStore.websiteDataStore();
-    auto* pool = dataStore.processPoolForCookieStorageOperations();
+    auto* pool = m_owningDataStore->processPoolForCookieStorageOperations();
     if (!pool) {
-        dataStore.addPendingCookie(cookie);
+        m_owningDataStore->addPendingCookie(cookie);
         callOnMainThread([completionHandler = WTFMove(completionHandler)]() {
             completionHandler();
         });
@@ -82,17 +80,16 @@ void HTTPCookieStore::setCookie(const WebCore::Cookie& cookie, Function<void ()>
     }
 
     auto* cookieManager = pool->supplement<WebKit::WebCookieManagerProxy>();
-    cookieManager->setCookie(dataStore.sessionID(), cookie, [pool = WTFMove(pool), completionHandler = WTFMove(completionHandler)](CallbackBase::Error error) {
+    cookieManager->setCookie(m_owningDataStore->sessionID(), cookie, [pool = WTFMove(pool), completionHandler = WTFMove(completionHandler)](CallbackBase::Error error) {
         completionHandler();
     });
 }
 
 void HTTPCookieStore::deleteCookie(const WebCore::Cookie& cookie, Function<void ()>&& completionHandler)
 {
-    auto& dataStore = m_owningDataStore.websiteDataStore();
-    auto* pool = dataStore.processPoolForCookieStorageOperations();
+    auto* pool = m_owningDataStore->processPoolForCookieStorageOperations();
     if (!pool) {
-        dataStore.removePendingCookie(cookie);
+        m_owningDataStore->removePendingCookie(cookie);
         callOnMainThread([completionHandler = WTFMove(completionHandler)]() {
             completionHandler();
         });
@@ -100,7 +97,7 @@ void HTTPCookieStore::deleteCookie(const WebCore::Cookie& cookie, Function<void
     }
 
     auto* cookieManager = pool->supplement<WebKit::WebCookieManagerProxy>();
-    cookieManager->deleteCookie(dataStore.sessionID(), cookie, [pool = WTFMove(pool), completionHandler = WTFMove(completionHandler)](CallbackBase::Error error) {
+    cookieManager->deleteCookie(m_owningDataStore->sessionID(), cookie, [pool = WTFMove(pool), completionHandler = WTFMove(completionHandler)](CallbackBase::Error error) {
         completionHandler();
     });
 }
@@ -137,8 +134,7 @@ void HTTPCookieStore::registerObserver(Observer& observer)
 
     m_cookieManagerProxyObserver = std::make_unique<APIWebCookieManagerProxyObserver>(*this);
 
-    auto& dataStore = m_owningDataStore.websiteDataStore();
-    auto* pool = dataStore.processPoolForCookieStorageOperations();
+    auto* pool = m_owningDataStore->processPoolForCookieStorageOperations();
 
     if (!pool) {
         registerForNewProcessPoolNotifications();
@@ -155,7 +151,7 @@ void HTTPCookieStore::registerObserver(Observer& observer)
     }
 
     m_observedCookieManagerProxy = pool->supplement<WebKit::WebCookieManagerProxy>();
-    m_observedCookieManagerProxy->registerObserver(dataStore.sessionID(), *m_cookieManagerProxyObserver);
+    m_observedCookieManagerProxy->registerObserver(m_owningDataStore->sessionID(), *m_cookieManagerProxyObserver);
 }
 
 void HTTPCookieStore::unregisterObserver(Observer& observer)
@@ -166,7 +162,7 @@ void HTTPCookieStore::unregisterObserver(Observer& observer)
         return;
 
     if (m_observedCookieManagerProxy)
-        m_observedCookieManagerProxy->unregisterObserver(m_owningDataStore.websiteDataStore().sessionID(), *m_cookieManagerProxyObserver);
+        m_observedCookieManagerProxy->unregisterObserver(m_owningDataStore->sessionID(), *m_cookieManagerProxyObserver);
 
     if (m_observingUIProcessCookies)
         WebCore::stopObservingCookieChanges(WebCore::NetworkStorageSession::defaultStorageSession());
@@ -188,12 +184,10 @@ void HTTPCookieStore::cookiesDidChange()
 
 void HTTPCookieStore::cookieManagerDestroyed()
 {
-    auto& dataStore = m_owningDataStore.websiteDataStore();
-
-    m_observedCookieManagerProxy->unregisterObserver(dataStore.sessionID(), *m_cookieManagerProxyObserver);
+    m_observedCookieManagerProxy->unregisterObserver(m_owningDataStore->sessionID(), *m_cookieManagerProxyObserver);
     m_observedCookieManagerProxy = nullptr;
 
-    auto* pool = dataStore.processPoolForCookieStorageOperations();
+    auto* pool = m_owningDataStore->processPoolForCookieStorageOperations();
 
     if (!pool) {
         registerForNewProcessPoolNotifications();
@@ -201,7 +195,7 @@ void HTTPCookieStore::cookieManagerDestroyed()
     }
 
     m_observedCookieManagerProxy = pool->supplement<WebKit::WebCookieManagerProxy>();
-    m_observedCookieManagerProxy->registerObserver(dataStore.sessionID(), *m_cookieManagerProxyObserver);
+    m_observedCookieManagerProxy->registerObserver(m_owningDataStore->sessionID(), *m_cookieManagerProxyObserver);
 }
 
 void HTTPCookieStore::registerForNewProcessPoolNotifications()
@@ -211,7 +205,7 @@ void HTTPCookieStore::registerForNewProcessPoolNotifications()
     m_processPoolCreationListenerIdentifier = WebProcessPool::registerProcessPoolCreationListener([this](WebProcessPool& newProcessPool) {
         ASSERT(m_cookieManagerProxyObserver);
 
-        if (!m_owningDataStore.websiteDataStore().isAssociatedProcessPool(newProcessPool))
+        if (!m_owningDataStore->isAssociatedProcessPool(newProcessPool))
             return;
 
         // Now that an associated process pool exists, we need to flush the UI process cookie store
@@ -221,7 +215,7 @@ void HTTPCookieStore::registerForNewProcessPoolNotifications()
 
 
         m_observedCookieManagerProxy = newProcessPool.supplement<WebKit::WebCookieManagerProxy>();
-        m_observedCookieManagerProxy->registerObserver(m_owningDataStore.websiteDataStore().sessionID(), *m_cookieManagerProxyObserver);
+        m_observedCookieManagerProxy->registerObserver(m_owningDataStore->sessionID(), *m_cookieManagerProxyObserver);
         unregisterForNewProcessPoolNotifications();
     });
 }
index 45fde85..89e8dc5 100644 (file)
@@ -39,6 +39,7 @@ struct Cookie;
 
 namespace WebKit {
 class WebCookieManagerProxy;
+class WebsiteDataStore;
 }
 
 namespace API {
@@ -77,7 +78,7 @@ private:
     void registerForNewProcessPoolNotifications();
     void unregisterForNewProcessPoolNotifications();
 
-    WebsiteDataStore& m_owningDataStore;
+    Ref<WebKit::WebsiteDataStore> m_owningDataStore;
     HashSet<Observer*> m_observers;
 
     WebKit::WebCookieManagerProxy* m_observedCookieManagerProxy { nullptr };
index 913f09e..68a7c7f 100644 (file)
@@ -1,3 +1,14 @@
+2017-07-16  Brady Eidson  <beidson@apple.com>
+
+        Crash when a WKHTTPCookieStore outlives its owning WKWebsiteDataStore.
+        <rdar://problem/33341730> and https://bugs.webkit.org/show_bug.cgi?id=174574
+
+        Reviewed by Tim Horton.
+
+        * TestWebKitAPI/Tests/WebKit2Cocoa/WKHTTPCookieStore.mm:
+        (-[CookieNavigationDelegate webView:didFinishNavigation:]):
+        (TEST):
+
 2017-07-16  Bernhard M. Wiedemann  <bwiedemann@suse.de>
 
         [GTK] Sort inspector GResource manifest to ensure reproducible builds
index 0453b2b..673bdd3 100644 (file)
@@ -26,6 +26,7 @@
 #include "config.h"
 
 #import "PlatformUtilities.h"
+#import "TestNavigationDelegate.h"
 #import <WebKit/WKFoundation.h>
 #import <WebKit/WKHTTPCookieStore.h>
 #import <WebKit/WKWebsiteDataStorePrivate.h>
@@ -174,6 +175,34 @@ TEST(WebKit2, WKHTTPCookieStore)
     [globalCookieStore removeObserver:observer2.get()];
 }
 
+static RetainPtr<WKHTTPCookieStore> staticCookieStore;
+
+TEST(WebKit2, CookieObserverCrash)
+{
+    RetainPtr<WKWebsiteDataStore> nonPersistentDataStore;
+    @autoreleasepool {
+        nonPersistentDataStore = [WKWebsiteDataStore nonPersistentDataStore];
+    }
+
+    auto configuration = adoptNS([[WKWebViewConfiguration alloc] init]);
+    configuration.get().websiteDataStore = nonPersistentDataStore.get();
+
+    auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:configuration.get()]);
+
+    [webView loadHTMLString:@"Oh hello" baseURL:[NSURL URLWithString:@"http://webkit.org"]];
+    [webView _test_waitForDidFinishNavigation];
+
+    staticCookieStore = nonPersistentDataStore.get().httpCookieStore;
+    RetainPtr<CookieObserver> observer = adoptNS([[CookieObserver alloc] init]);
+    [staticCookieStore addObserver:observer.get()];
+
+    [staticCookieStore getAllCookies:[](NSArray<NSHTTPCookie *> *) {
+        gotFlag = true;
+    }];
+
+    TestWebKitAPI::Util::run(&gotFlag);
+}
+
 static bool finished;
 
 @interface CookieUIDelegate : NSObject <WKUIDelegate>