makeWeakPtr isn't thread-safe even though it is called in non main threads. This can be problem if multiple threads would call makeWeakPtr simultaneously. WeakPtrFactory::createWeakPtr reads/writes 'm_ref' member variable without locking mutex. https://trac.webkit.org/browser/webkit/trunk/Source/WTF/wtf/WeakPtr.h?rev=242387#L109 > WeakPtr<T> createWeakPtr(T& ptr) const > { > if (!m_ref) > m_ref = WeakReference<T>::create(&ptr); > return { makeRef(*m_ref) }; > } The simple solution is making WeakPtrFactory::m_ref always have a reference to a instance of WeakReference by calling createWeakPtr in WeakPtrFactory ctor.
(In reply to Fujii Hironori from comment #0) > makeWeakPtr isn't thread-safe even though it is called in non main threads. > This can be problem if multiple threads would call makeWeakPtr > simultaneously. > > WeakPtrFactory::createWeakPtr reads/writes 'm_ref' member variable without > locking mutex. > https://trac.webkit.org/browser/webkit/trunk/Source/WTF/wtf/WeakPtr. > h?rev=242387#L109 > > > WeakPtr<T> createWeakPtr(T& ptr) const > > { > > if (!m_ref) > > m_ref = WeakReference<T>::create(&ptr); > > return { makeRef(*m_ref) }; > > } > > The simple solution is making WeakPtrFactory::m_ref always have a reference > to a instance of WeakReference by calling createWeakPtr in WeakPtrFactory > ctor. It's important that WeakReference<T> is constructed lazily. I think we should do make the assignment to m_ref atomic. Maybe we can use std::atomic<WeakReference<T>*> m_ref then manually call ref() / deref()? I'm sure others have a better idea than this.
Is this a problem with actual code right now? Can you provide an example? WeakPtr is normally not used across thread since you could check on one line that the weak pointer is not null and then dereference it on the next line, by which time your object may be dead already.
(In reply to Chris Dumez from comment #2) > Is this a problem with actual code right now? Can you provide an example? > WeakPtr is normally not used across thread since you could check on one line > that the weak pointer is not null and then dereference it on the next line, > by which time your object may be dead already. WTF::WeakReference is a sub-class of ThreadSafeRefCounted. > class WeakReference : public ThreadSafeRefCounted<WeakReference<T>> { And, WTF::WeakPtr doesn't have a method like weak_ptr::lock. std::weak_ptr::lock https://en.cppreference.com/w/cpp/memory/weak_ptr/lock These two facts mean WTF::WeakPtr should be used in a single thread, but its WTF::WeakReference can be held in multiple threads by design. And, it's easy to find code looks like the following under WebCore/platform. > callOnMainThread([weakPtr = makeWeakPtr(*this)] {
(In reply to Fujii Hironori from comment #3) > (In reply to Chris Dumez from comment #2) > > Is this a problem with actual code right now? Can you provide an example? > > WeakPtr is normally not used across thread since you could check on one line > > that the weak pointer is not null and then dereference it on the next line, > > by which time your object may be dead already. > > WTF::WeakReference is a sub-class of ThreadSafeRefCounted. > > > class WeakReference : public ThreadSafeRefCounted<WeakReference<T>> { > > And, WTF::WeakPtr doesn't have a method like weak_ptr::lock. > > std::weak_ptr::lock > https://en.cppreference.com/w/cpp/memory/weak_ptr/lock > > These two facts mean WTF::WeakPtr should be used in a single thread, but its > WTF::WeakReference can be held in multiple threads by design. > > And, it's easy to find code looks like the following under WebCore/platform. > > > callOnMainThread([weakPtr = makeWeakPtr(*this)] { So no concrete example where there is an actual problem? CallonMainThread() is frequently used on the main thread to call something asynchronously and does not imply multithreading. Also, it is ok to temporarily pass a weakptr to another thread (since ThreadSafeRefcounted) as long as it is created and used on the thread where the object lives (which may or may not be the main thread).
Honestly, it's probably a bug that WeakReference is ThreadSafeRefCounted. (It should just be RefCounted.) None of the data in WeakPtr, WeakReference, or WeakPtrFactory is thread-safe. I left WeakReference ThreadSafeRefCounted out of an abundance of caution, because it was originally written that way and I didn't know if there any clients needed the atomic reference count. If we wanted a thread-safe approach to weak references, we could create one, and it would be a distinct class with a distinct implementation. (All operations would need to be atomic, get() and operator->() would need to return RefPtr, and operator*() would need to return Ref.)
(In reply to Geoffrey Garen from comment #5) > Honestly, it's probably a bug that WeakReference is ThreadSafeRefCounted. > (It should just be RefCounted.) None of the data in WeakPtr, WeakReference, > or WeakPtrFactory is thread-safe. > > I left WeakReference ThreadSafeRefCounted out of an abundance of caution, > because it was originally written that way and I didn't know if there any > clients needed the atomic reference count. > > If we wanted a thread-safe approach to weak references, we could create one, > and it would be a distinct class with a distinct implementation. (All > operations would need to be atomic, get() and operator->() would need to > return RefPtr, and operator*() would need to return Ref.) I have leveraged the fact that it is ThreadSafeRefcounted in my code for cases where I jump to a background thread and then back to the main thread. I only create the WeakPtr and use it on the main thread, which is where my object live, so this is safe. It is only safe though because it is ThreadSafeRefCounted. That said, I agree with the closing on this bug and I do not think we should try and promote usage of WeakPtr from different threads. Just because an object is ThreadSafeRefCounted does not imply its methods are thread safe and that you use it from different threads.
Should we then make WeakReference RefCounted instead of ThreadSafeRefCounted and add assertions that WeakPtrs are made, used, and destroyed on the same thread? Those both sound like improvements to me. We'll need the move constructor to be able to be called from any thread for cases where we hop to a different thread then back.
(In reply to Alex Christensen from comment #7) > Should we then make WeakReference RefCounted instead of ThreadSafeRefCounted > and add assertions that WeakPtrs are made, used, and destroyed on the same > thread? Those both sound like improvements to me. We'll need the move > constructor to be able to be called from any thread for cases where we hop > to a different thread then back. What about the very valid use case I mentioned earlier?
It does need to be ThreadSafeRefCounted, but why don't we add assertions that all important use happens on the same thread? I could imagine that preventing some strange crashes.
(In reply to Alex Christensen from comment #9) > It does need to be ThreadSafeRefCounted, but why don't we add assertions > that all important use happens on the same thread? I could imagine that > preventing some strange crashes. I'd argue that we should do this for RefCounted: https://bugs.webkit.org/show_bug.cgi?id=195656
Let's continue the more general per-object / per-type assertion discussion in bug 195656.
Bug 199517 – Add threading assertion to WTF::WeakPtr Bug 199639 – Add threading assertion to WeakPtrFactory::createWeakPtr()