- Revision
- 248902
- Author
- cdu...@apple.com
- Date
- 2019-08-20 09:53:21 -0700 (Tue, 20 Aug 2019)
Log Message
Unsafe usage of CookieStorageObserver from a background thread
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):
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (248901 => 248902)
--- trunk/Source/WebCore/ChangeLog 2019-08-20 16:34:56 UTC (rev 248901)
+++ trunk/Source/WebCore/ChangeLog 2019-08-20 16:53:21 UTC (rev 248902)
@@ -1,5 +1,28 @@
2019-08-20 Chris Dumez <cdu...@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 <cdu...@apple.com>
+
Use a strongly typed identifier for StorageNamespace's identifier
https://bugs.webkit.org/show_bug.cgi?id=200895
Modified: trunk/Source/WebCore/platform/network/NetworkStorageSession.h (248901 => 248902)
--- trunk/Source/WebCore/platform/network/NetworkStorageSession.h 2019-08-20 16:34:56 UTC (rev 248901)
+++ trunk/Source/WebCore/platform/network/NetworkStorageSession.h 2019-08-20 16:53:21 UTC (rev 248902)
@@ -200,7 +200,7 @@
CookieStorageObserver& cookieStorageObserver() const;
private:
- mutable RefPtr<CookieStorageObserver> m_cookieStorageObserver;
+ mutable std::unique_ptr<CookieStorageObserver> m_cookieStorageObserver;
#endif
static bool m_processMayUseCookieAPI;
};
Modified: trunk/Source/WebCore/platform/network/cocoa/CookieStorageObserver.h (248901 => 248902)
--- trunk/Source/WebCore/platform/network/cocoa/CookieStorageObserver.h 2019-08-20 16:34:56 UTC (rev 248901)
+++ trunk/Source/WebCore/platform/network/cocoa/CookieStorageObserver.h 2019-08-20 16:53:21 UTC (rev 248902)
@@ -28,7 +28,7 @@
#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;
@@ -35,9 +35,11 @@
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 @@
void cookiesDidChange();
private:
- CookieStorageObserver(NSHTTPCookieStorage *);
+ WeakPtr<CookieStorageObserver> m_weakThis;
RetainPtr<NSHTTPCookieStorage> m_cookieStorage;
bool m_hasRegisteredInternalsForNotifications { false };
RetainPtr<WebCookieObserverAdapter> m_observerAdapter;
Modified: trunk/Source/WebCore/platform/network/cocoa/CookieStorageObserver.mm (248901 => 248902)
--- trunk/Source/WebCore/platform/network/cocoa/CookieStorageObserver.mm 2019-08-20 16:34:56 UTC (rev 248901)
+++ trunk/Source/WebCore/platform/network/cocoa/CookieStorageObserver.mm 2019-08-20 16:53:21 UTC (rev 248902)
@@ -74,13 +74,9 @@
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::cookiesDidChange()
{
- callOnMainThread([protectedThis = makeRef(*this), this] {
- if (m_cookieChangeCallback)
- m_cookieChangeCallback();
+ callOnMainThread([weakThis = m_weakThis] {
+ if (weakThis && weakThis->m_cookieChangeCallback)
+ weakThis->m_cookieChangeCallback();
});
}
Modified: trunk/Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm (248901 => 248902)
--- trunk/Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm 2019-08-20 16:34:56 UTC (rev 248901)
+++ trunk/Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm 2019-08-20 16:53:21 UTC (rev 248902)
@@ -121,7 +121,7 @@
CookieStorageObserver& NetworkStorageSession::cookieStorageObserver() const
{
if (!m_cookieStorageObserver)
- m_cookieStorageObserver = CookieStorageObserver::create(nsCookieStorage());
+ m_cookieStorageObserver = makeUnique<CookieStorageObserver>(nsCookieStorage());
return *m_cookieStorageObserver;
}
Modified: trunk/Source/WebKit/ChangeLog (248901 => 248902)
--- trunk/Source/WebKit/ChangeLog 2019-08-20 16:34:56 UTC (rev 248901)
+++ trunk/Source/WebKit/ChangeLog 2019-08-20 16:53:21 UTC (rev 248902)
@@ -1,5 +1,16 @@
2019-08-20 Chris Dumez <cdu...@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 <cdu...@apple.com>
+
Use a strongly typed identifier for StorageNamespace's identifier
https://bugs.webkit.org/show_bug.cgi?id=200895
Modified: trunk/Source/WebKit/UIProcess/API/APIHTTPCookieStore.h (248901 => 248902)
--- trunk/Source/WebKit/UIProcess/API/APIHTTPCookieStore.h 2019-08-20 16:34:56 UTC (rev 248901)
+++ trunk/Source/WebKit/UIProcess/API/APIHTTPCookieStore.h 2019-08-20 16:53:21 UTC (rev 248902)
@@ -98,7 +98,7 @@
uint64_t m_processPoolCreationListenerIdentifier { 0 };
#if PLATFORM(COCOA)
- RefPtr<WebCore::CookieStorageObserver> m_defaultUIProcessObserver;
+ std::unique_ptr<WebCore::CookieStorageObserver> m_defaultUIProcessObserver;
#endif
};
Modified: trunk/Source/WebKit/UIProcess/API/Cocoa/APIHTTPCookieStoreCocoa.mm (248901 => 248902)
--- trunk/Source/WebKit/UIProcess/API/Cocoa/APIHTTPCookieStoreCocoa.mm 2019-08-20 16:34:56 UTC (rev 248901)
+++ trunk/Source/WebKit/UIProcess/API/Cocoa/APIHTTPCookieStoreCocoa.mm 2019-08-20 16:53:21 UTC (rev 248902)
@@ -60,7 +60,7 @@
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));
}
