Unsafe usage of CookieStorageObserver from a background thread

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 376770 [details]
Patch

[Alex Christensen]

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?

[Chris Dumez]

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.

[Alex Christensen]

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.

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 376774 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/54513840>

[WebKit Commit Bot]

Comment on attachment 376774 [details]
Patch

Clearing flags on attachment: 376774

Committed r248902: <https://trac.webkit.org/changeset/248902>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Geoffrey Garen]

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

[Chris Dumez]

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...

[Chris Dumez]

(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.