Unsafe usage of CookieStorageObserver from a background thread
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 20 Aug 2019 16:53:21 +0000 (16:53 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 20 Aug 2019 16:53:21 +0000 (16:53 +0000)
https://bugs.webkit.org/show_bug.cgi?id=200920

Reviewed by Alex Christensen.

Source/WebCore:

Unsafe usage of CookieStorageObserver from a background thread. CookieStorageObserver gets
constructed / destructed on the main thread. However, CookieStorageObserver::cookiesDidChange()
gets called on a background thread and tries to ref |this|. Even though CookieStorageObserver
is ThreadSafeRefCounted, this is still unsafe because the CookieStorageObserver destructor may
already be running on the main thread when CookieStorageObserver::cookiesDidChange() gets called
on the background thread.

* platform/network/NetworkStorageSession.h:
* platform/network/cocoa/CookieStorageObserver.h:
* platform/network/cocoa/CookieStorageObserver.mm:
(WebCore::CookieStorageObserver::CookieStorageObserver):
(WebCore::CookieStorageObserver::cookiesDidChange):
(WebCore::CookieStorageObserver::create): Deleted.
* platform/network/cocoa/NetworkStorageSessionCocoa.mm:
(WebCore::NetworkStorageSession::cookieStorageObserver const):

Source/WebKit:

* UIProcess/API/APIHTTPCookieStore.h:
* UIProcess/API/Cocoa/APIHTTPCookieStoreCocoa.mm:
(API::HTTPCookieStore::startObservingChangesToDefaultUIProcessCookieStore):

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

Source/WebCore/ChangeLog
Source/WebCore/platform/network/NetworkStorageSession.h
Source/WebCore/platform/network/cocoa/CookieStorageObserver.h
Source/WebCore/platform/network/cocoa/CookieStorageObserver.mm
Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm
Source/WebKit/ChangeLog
Source/WebKit/UIProcess/API/APIHTTPCookieStore.h
Source/WebKit/UIProcess/API/Cocoa/APIHTTPCookieStoreCocoa.mm

index 100c2f5..205f031 100644 (file)
@@ -1,5 +1,28 @@
 2019-08-20  Chris Dumez  <cdumez@apple.com>
 
+        Unsafe usage of CookieStorageObserver from a background thread
+        https://bugs.webkit.org/show_bug.cgi?id=200920
+
+        Reviewed by Alex Christensen.
+
+        Unsafe usage of CookieStorageObserver from a background thread. CookieStorageObserver gets
+        constructed / destructed on the main thread. However, CookieStorageObserver::cookiesDidChange()
+        gets called on a background thread and tries to ref |this|. Even though CookieStorageObserver
+        is ThreadSafeRefCounted, this is still unsafe because the CookieStorageObserver destructor may
+        already be running on the main thread when CookieStorageObserver::cookiesDidChange() gets called
+        on the background thread.
+
+        * platform/network/NetworkStorageSession.h:
+        * platform/network/cocoa/CookieStorageObserver.h:
+        * platform/network/cocoa/CookieStorageObserver.mm:
+        (WebCore::CookieStorageObserver::CookieStorageObserver):
+        (WebCore::CookieStorageObserver::cookiesDidChange):
+        (WebCore::CookieStorageObserver::create): Deleted.
+        * platform/network/cocoa/NetworkStorageSessionCocoa.mm:
+        (WebCore::NetworkStorageSession::cookieStorageObserver const):
+
+2019-08-20  Chris Dumez  <cdumez@apple.com>
+
         Use a strongly typed identifier for StorageNamespace's identifier
         https://bugs.webkit.org/show_bug.cgi?id=200895
 
index 8ddf55b..54ff45d 100644 (file)
@@ -200,7 +200,7 @@ public:
     CookieStorageObserver& cookieStorageObserver() const;
 
 private:
-    mutable RefPtr<CookieStorageObserver> m_cookieStorageObserver;
+    mutable std::unique_ptr<CookieStorageObserver> m_cookieStorageObserver;
 #endif
     static bool m_processMayUseCookieAPI;
 };
index 0278cee..7d4747a 100644 (file)
 #include <pal/spi/cf/CFNetworkSPI.h>
 #include <wtf/Function.h>
 #include <wtf/RetainPtr.h>
-#include <wtf/ThreadSafeRefCounted.h>
+#include <wtf/WeakPtr.h>
 
 OBJC_CLASS NSHTTPCookieStorage;
 OBJC_CLASS WebCookieObserverAdapter;
 
 namespace WebCore {
 
-class WEBCORE_EXPORT CookieStorageObserver : public ThreadSafeRefCounted<CookieStorageObserver> {
+class WEBCORE_EXPORT CookieStorageObserver : public CanMakeWeakPtr<CookieStorageObserver> {
+    WTF_MAKE_FAST_ALLOCATED;
+    WTF_MAKE_NONCOPYABLE(CookieStorageObserver);
 public:
-    static Ref<CookieStorageObserver> create(NSHTTPCookieStorage *);
+    explicit CookieStorageObserver(NSHTTPCookieStorage *);
     ~CookieStorageObserver();
 
     void startObserving(Function<void()>&& callback);
@@ -46,8 +48,8 @@ public:
     void cookiesDidChange();
 
 private:
-    CookieStorageObserver(NSHTTPCookieStorage *);
 
+    WeakPtr<CookieStorageObserver> m_weakThis;
     RetainPtr<NSHTTPCookieStorage> m_cookieStorage;
     bool m_hasRegisteredInternalsForNotifications { false };
     RetainPtr<WebCookieObserverAdapter> m_observerAdapter;
index 5d25ae1..c4d48eb 100644 (file)
 
 namespace WebCore {
 
-Ref<CookieStorageObserver> CookieStorageObserver::create(NSHTTPCookieStorage *cookieStorage)
-{
-    return adoptRef(*new CookieStorageObserver(cookieStorage));
-}
-
 CookieStorageObserver::CookieStorageObserver(NSHTTPCookieStorage *cookieStorage)
-    : m_cookieStorage(cookieStorage)
+    : m_weakThis(makeWeakPtr(*this))
+    , m_cookieStorage(cookieStorage)
 {
     ASSERT(isMainThread());
     ASSERT(m_cookieStorage);
@@ -134,9 +130,9 @@ void CookieStorageObserver::stopObserving()
 
 void CookieStorageObserver::cookiesDidChange()
 {
-    callOnMainThread([protectedThis = makeRef(*this), this] {
-        if (m_cookieChangeCallback)
-            m_cookieChangeCallback();
+    callOnMainThread([weakThis = m_weakThis] {
+        if (weakThis && weakThis->m_cookieChangeCallback)
+            weakThis->m_cookieChangeCallback();
     });
 }
 
index 85f4c62..3276192 100644 (file)
@@ -121,7 +121,7 @@ NSHTTPCookieStorage *NetworkStorageSession::nsCookieStorage() const
 CookieStorageObserver& NetworkStorageSession::cookieStorageObserver() const
 {
     if (!m_cookieStorageObserver)
-        m_cookieStorageObserver = CookieStorageObserver::create(nsCookieStorage());
+        m_cookieStorageObserver = makeUnique<CookieStorageObserver>(nsCookieStorage());
 
     return *m_cookieStorageObserver;
 }
index b07176c..f3533ce 100644 (file)
@@ -1,5 +1,16 @@
 2019-08-20  Chris Dumez  <cdumez@apple.com>
 
+        Unsafe usage of CookieStorageObserver from a background thread
+        https://bugs.webkit.org/show_bug.cgi?id=200920
+
+        Reviewed by Alex Christensen.
+
+        * UIProcess/API/APIHTTPCookieStore.h:
+        * UIProcess/API/Cocoa/APIHTTPCookieStoreCocoa.mm:
+        (API::HTTPCookieStore::startObservingChangesToDefaultUIProcessCookieStore):
+
+2019-08-20  Chris Dumez  <cdumez@apple.com>
+
         Use a strongly typed identifier for StorageNamespace's identifier
         https://bugs.webkit.org/show_bug.cgi?id=200895
 
index a67647e..ebf96c8 100644 (file)
@@ -98,7 +98,7 @@ private:
     uint64_t m_processPoolCreationListenerIdentifier { 0 };
 
 #if PLATFORM(COCOA)
-    RefPtr<WebCore::CookieStorageObserver> m_defaultUIProcessObserver;
+    std::unique_ptr<WebCore::CookieStorageObserver> m_defaultUIProcessObserver;
 #endif
 };
 
index a72d729..880775c 100644 (file)
@@ -60,7 +60,7 @@ void HTTPCookieStore::deleteCookieFromDefaultUIProcessCookieStore(const WebCore:
 void HTTPCookieStore::startObservingChangesToDefaultUIProcessCookieStore(Function<void()>&& function)
 {
     stopObservingChangesToDefaultUIProcessCookieStore();
-    m_defaultUIProcessObserver = WebCore::CookieStorageObserver::create([NSHTTPCookieStorage sharedHTTPCookieStorage]);
+    m_defaultUIProcessObserver = makeUnique<WebCore::CookieStorageObserver>([NSHTTPCookieStorage sharedHTTPCookieStorage]);
     m_defaultUIProcessObserver->startObserving(WTFMove(function));
 }