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 may already be running on the main thread when CookieStorageObserver::cookiesDidChange() gets called on the background thread. This could lead to a double free.
Created attachment 376770 [details] Patch
Comment on attachment 376770 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376770&action=review > Source/WebCore/ChangeLog:11 > + is ThreadSafeRefCounted, this is still unsafe because the CookieStorageObserver may already be Did you forget "destructor" here? I don't understand what is different about CookieStorageObserver. Couldn't this happen with any ThreadSafeRefCounted type?
Comment on attachment 376770 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376770&action=review >> Source/WebCore/ChangeLog:11 >> + is ThreadSafeRefCounted, this is still unsafe because the CookieStorageObserver may already be > > Did you forget "destructor" here? I don't understand what is different about CookieStorageObserver. Couldn't this happen with any ThreadSafeRefCounted type? Yes, I forgot destructor :/ Yes, this could happen with any ThreadSafeRefCounted type. It is almost always unsafe to create a Ref<> / RefPtr<> from a raw pointer for a ThreadSafeRefCounted type from a thread that is not the thread that owns the object. Brady suggest adding a ThreadSafeRefCounted::tryRef() for this and I think this is a nice idea but I'd rather keep this patch small (cherry-pickable). Not that usually, we do not have this problem since we pass a RefPtr<> or Ref<> from the owner thread to another thread. It is not common that we create the Ref<> / RefPtr<> from a non-owner thread.
Comment on attachment 376770 [details] Patch Sounds like we should add some assertions to ThreadSafeRefCounted, too. The name implies that you can't run into problems like this. I'll bet there are other double frees out there.
Comment on attachment 376770 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376770&action=review >>> Source/WebCore/ChangeLog:11 >>> + is ThreadSafeRefCounted, this is still unsafe because the CookieStorageObserver may already be >> >> Did you forget "destructor" here? I don't understand what is different about CookieStorageObserver. Couldn't this happen with any ThreadSafeRefCounted type? > > Yes, I forgot destructor :/ > > Yes, this could happen with any ThreadSafeRefCounted type. It is almost always unsafe to create a Ref<> / RefPtr<> from a raw pointer for a ThreadSafeRefCounted type from a thread that is not the thread that owns the object. Brady suggest adding a ThreadSafeRefCounted::tryRef() for this and I think this is a nice idea but I'd rather keep this patch small (cherry-pickable). > Not that usually, we do not have this problem since we pass a RefPtr<> or Ref<> from the owner thread to another thread. It is not common that we create the Ref<> / RefPtr<> from a non-owner thread. Also note that the CookieStorageObserver only stops observing notifications in its destructor, which is why this is an issue. I decided to fix it with a WeakPtr<> but another way to fix this would have been a 2-step destruction, requiring the caller to call a CookieStorageObserver::stopListingForNotifications() method on the main thread *before* destroying the CookieStorageObserver object. The WeakPtr approach is a bit less error-prone IMO though.
Created attachment 376774 [details] Patch
<rdar://problem/54513840>
Comment on attachment 376774 [details] Patch Clearing flags on attachment: 376774 Committed r248902: <https://trac.webkit.org/changeset/248902>
All reviewed patches have been landed. Closing bug.
Comment on attachment 376774 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376774&action=review > Source/WebCore/platform/network/cocoa/CookieStorageObserver.mm:78 > + : m_weakThis(makeWeakPtr(*this)) Similar to my comment in bug 200925, I think we should remove this data member, and just use makeWeakPtr(*this).
Comment on attachment 376774 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376774&action=review >> Source/WebCore/platform/network/cocoa/CookieStorageObserver.mm:78 >> + : m_weakThis(makeWeakPtr(*this)) > > Similar to my comment in bug 200925, I think we should remove this data member, and just use makeWeakPtr(*this). MakeWeakPtr() is not currently safe to call from another thread though...
(In reply to Chris Dumez from comment #11) > Comment on attachment 376774 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=376774&action=review > > >> Source/WebCore/platform/network/cocoa/CookieStorageObserver.mm:78 > >> + : m_weakThis(makeWeakPtr(*this)) > > > > Similar to my comment in bug 200925, I think we should remove this data member, and just use makeWeakPtr(*this). > > MakeWeakPtr() is not currently safe to call from another thread though... Oh, I now see your other comment about initializing the WeakPtrFactory in the constructor on the main thread to work around the thread-safety problem. I will look into this.