[webkit-changes] [219550] trunk

Sun, 16 Jul 2017 18:44:11 -0700

Title: [219550] trunk
Revision
219550
Author
beid...@apple.com
Date
2017-07-16 18:43:14 -0700 (Sun, 16 Jul 2017)

Log Message

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.

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):

Modified Paths

Diff

Modified: trunk/Source/WebKit/ChangeLog (219549 => 219550)


--- trunk/Source/WebKit/ChangeLog	2017-07-16 22:23:52 UTC (rev 219549)
+++ trunk/Source/WebKit/ChangeLog	2017-07-17 01:43:14 UTC (rev 219550)
@@ -1,3 +1,25 @@
+2017-07-16  Brady Eidson  <beid...@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  <beid...@apple.com>
 
         Make sure all CFHTTPCookieStorageRefs we create are scheduled.

Modified: trunk/Source/WebKit/UIProcess/API/APIHTTPCookieStore.cpp (219549 => 219550)


--- trunk/Source/WebKit/UIProcess/API/APIHTTPCookieStore.cpp	2017-07-16 22:23:52 UTC (rev 219549)
+++ trunk/Source/WebKit/UIProcess/API/APIHTTPCookieStore.cpp	2017-07-17 01:43:14 UTC (rev 219550)
@@ -38,7 +38,7 @@
 namespace API {
 
 HTTPCookieStore::HTTPCookieStore(WebsiteDataStore& websiteDataStore)
-    : m_owningDataStore(websiteDataStore)
+    : m_owningDataStore(websiteDataStore.websiteDataStore())
 {
 }
 
@@ -54,10 +54,9 @@
 
 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;
@@ -64,7 +63,7 @@
     }
 
     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);
     });
 }
@@ -71,10 +70,9 @@
 
 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,7 +80,7 @@
     }
 
     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();
     });
 }
@@ -89,10 +87,9 @@
 
 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 @@
     }
 
     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 @@
 
     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 @@
     }
 
     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 @@
         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::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 @@
     }
 
     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 @@
     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 @@
 
 
         m_observedCookieManagerProxy = newProcessPool.supplement<WebKit::WebCookieManagerProxy>();
-        m_observedCookieManagerProxy->registerObserver(m_owningDataStore.websiteDataStore().sessionID(), *m_cookieManagerProxyObserver);
+        m_observedCookieManagerProxy->registerObserver(m_owningDataStore->sessionID(), *m_cookieManagerProxyObserver);
         unregisterForNewProcessPoolNotifications();
     });
 }

Modified: trunk/Source/WebKit/UIProcess/API/APIHTTPCookieStore.h (219549 => 219550)


--- trunk/Source/WebKit/UIProcess/API/APIHTTPCookieStore.h	2017-07-16 22:23:52 UTC (rev 219549)
+++ trunk/Source/WebKit/UIProcess/API/APIHTTPCookieStore.h	2017-07-17 01:43:14 UTC (rev 219550)
@@ -39,6 +39,7 @@
 
 namespace WebKit {
 class WebCookieManagerProxy;
+class WebsiteDataStore;
 }
 
 namespace API {
@@ -77,7 +78,7 @@
     void registerForNewProcessPoolNotifications();
     void unregisterForNewProcessPoolNotifications();
 
-    WebsiteDataStore& m_owningDataStore;
+    Ref<WebKit::WebsiteDataStore> m_owningDataStore;
     HashSet<Observer*> m_observers;
 
     WebKit::WebCookieManagerProxy* m_observedCookieManagerProxy { nullptr };

Modified: trunk/Tools/ChangeLog (219549 => 219550)


--- trunk/Tools/ChangeLog	2017-07-16 22:23:52 UTC (rev 219549)
+++ trunk/Tools/ChangeLog	2017-07-17 01:43:14 UTC (rev 219550)
@@ -1,3 +1,14 @@
+2017-07-16  Brady Eidson  <beid...@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  <bwiedem...@suse.de>
 
         [GTK] Sort inspector GResource manifest to ensure reproducible builds

Modified: trunk/Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKHTTPCookieStore.mm (219549 => 219550)


--- trunk/Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKHTTPCookieStore.mm	2017-07-16 22:23:52 UTC (rev 219549)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKHTTPCookieStore.mm	2017-07-17 01:43:14 UTC (rev 219550)
@@ -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 @@
     [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>

_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to