[webkit-reviews] review requested: [Bug 200920] Unsafe usage of CookieStorageObserver from a background thread : [Attachment 376770] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has asked  for review:
Bug 200920: Unsafe usage of CookieStorageObserver from a background thread
https://bugs.webkit.org/show_bug.cgi?id=200920

Attachment 376770: Patch

https://bugs.webkit.org/attachment.cgi?id=376770&action=review




--- Comment #5 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 376770
  --> https://bugs.webkit.org/attachment.cgi?id=376770
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.