Bug 238871 – Multiple uses of ThreadSafeRefCounted and CanMakeWeakPtr with non-thread-safe WeakPtr

• Status: NEW
• Component: Web Template Framework

[Kimmo Kinnunen]

Multiple uses of ThreadSafeRefCounted and CanMakeWeakPtr with non-thread-safe WeakPtr

ThreadSafeRefCounted means:
- Object is used in multiple threads
- Last ref can be given up in thread B

WeakPtr means:
- This object cannot change its liveliness state in other thread than in which the WeakPtr lives


Consider case:
class ImageSource : public ThreadSafeRefCounted<ImageSource>, public CanMakeWeakPtr<ImageSource> 

Thread Main checks
 WeakPtr<ImageSource> weakImageA = ...; 
 if (weakImageA) { weakImageA->doSomething(); }

Thread B gives up the last ref:
 RefPtr<ImageSource> imageA = ...;
 imageA = nullptr;
 

Consider case:
class Connection : public ThreadSafeRefCounted<Connection, WTF::DestructionThread::MainRunLoop>, public CanMakeWeakPtr<Connection>

Thread Main checks
 WeakPtr<Connection> weakConnectionA = ...; 
 if (weakConnectionA) { weakConnection->send(msg, ...); } // Connection::send will promote 0-refcount object to +1 ref, but the run loop has a callback scheduled to run the ~Connection()

Thread B gives up the last ref:
 RefPtr<Connection> connectionA = ...;
 connectionA = nullptr;  // Gives up last ref, sends the callback to main runloop to delete the instance.

Conservative list, but not exhaustive

kkinnunen@mbp3 OpenSource % git grep "ThreadSafeRefCounted.*CanMakeWeakPtr"
Source/JavaScriptCore/wasm/WasmInstance.h:class Instance : public ThreadSafeRefCounted<Instance>, public CanMakeWeakPtr<Instance> {
Source/WebCore/Modules/webaudio/AudioWorkletProcessor.h:class AudioWorkletProcessor : public ScriptWrappable, public ThreadSafeRefCounted<AudioWorkletProcessor>, public CanMakeWeakPtr<AudioWorkletProcessor> {
Source/WebCore/page/AbstractFrame.h:class AbstractFrame : public ThreadSafeRefCounted<AbstractFrame, WTF::DestructionThread::Main>, public CanMakeWeakPtr<AbstractFrame> {
Source/WebCore/platform/graphics/ImageBuffer.h:class ImageBuffer : public ThreadSafeRefCounted<ImageBuffer, WTF::DestructionThread::Main>, public CanMakeWeakPtr<ImageBuffer> {
Source/WebCore/platform/graphics/ImageSource.h:class ImageSource : public ThreadSafeRefCounted<ImageSource>, public CanMakeWeakPtr<ImageSource> {
Source/WebCore/platform/graphics/MediaPlayer.h:class WEBCORE_EXPORT MediaPlayer : public MediaPlayerEnums, public ThreadSafeRefCounted<MediaPlayer, WTF::DestructionThread::Main>, public CanMakeWeakPtr<MediaPlayer> {
Source/WebCore/platform/graphics/NativeImage.h:class NativeImage : public ThreadSafeRefCounted<NativeImage, WTF::DestructionThread::Main>, public CanMakeWeakPtr<NativeImage> {
Source/WebCore/platform/graphics/gstreamer/ImageDecoderGStreamer.h:    class InnerDecoder : public ThreadSafeRefCounted<InnerDecoder>, public CanMakeWeakPtr<InnerDecoder> {
Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.h:class WEBCORE_EXPORT MediaRecorderPrivateWriter : public ThreadSafeRefCounted<MediaRecorderPrivateWriter, WTF::DestructionThread::Main>, public CanMakeWeakPtr<MediaRecorderPrivateWriter, WeakPtrFactoryInitialization::Eager> {
Source/WebCore/platform/network/cocoa/RangeResponseGenerator.h:class RangeResponseGenerator : public ThreadSafeRefCounted<RangeResponseGenerator, WTF::DestructionThread::Main>, public CanMakeWeakPtr<RangeResponseGenerator> {
Source/WebCore/platform/xr/PlatformXR.h:class Device : public ThreadSafeRefCounted<Device>, public CanMakeWeakPtr<Device> {
Source/WebCore/storage/StorageQuotaManager.h:class StorageQuotaManager : public ThreadSafeRefCounted<StorageQuotaManager>, public CanMakeWeakPtr<StorageQuotaManager> {
Source/WebKit/NetworkProcess/NetworkDataTask.h:class NetworkDataTask : public ThreadSafeRefCounted<NetworkDataTask, WTF::DestructionThread::Main>, public CanMakeWeakPtr<NetworkDataTask> {
Source/WebKit/NetworkProcess/storage/QuotaManager.h:class QuotaManager : public ThreadSafeRefCounted<QuotaManager>, public CanMakeWeakPtr<QuotaManager> {
Source/WebKit/Platform/IPC/Connection.h:class Connection : public ThreadSafeRefCounted<Connection, WTF::DestructionThread::MainRunLoop>, public CanMakeWeakPtr<Connection> {
Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.h:class SOAuthorizationSession : public ThreadSafeRefCounted<SOAuthorizationSession, WTF::DestructionThread::MainRunLoop>, public CanMakeWeakPtr<SOAuthorizationSession> {
Source/WebKit/UIProcess/Launcher/ProcessLauncher.h:class ProcessLauncher : public ThreadSafeRefCounted<ProcessLauncher>, public CanMakeWeakPtr<ProcessLauncher> {
Source/WebKit/UIProcess/ProcessAssertion.h:class ProcessAssertion : public ThreadSafeRefCounted<ProcessAssertion>, public CanMakeWeakPtr<ProcessAssertion, WeakPtrFactoryInitialization::Eager> {
kkinnunen@mbp3 OpenSource % git grep "CanMakeWeakPtr.*ThreadSafeRefCounted"                
Source/WebCore/Modules/filesystemaccess/FileSystemHandle.h:class FileSystemHandle : public ActiveDOMObject, public CanMakeWeakPtr<FileSystemHandle>, public ThreadSafeRefCounted<FileSystemHandle> {
kkinnunen@mbp3 OpenSource %

[Chris Dumez]

We have assertions in WeakPtr to catch these things so I am surprised they are not hitting?

[Chris Dumez]

Not that in general, it is perfectly OK for a class to subclass ThreadSafeRefCounted and CanMakeWeakPtr and it is not indicative of a bug. It really depends on how you're using WeakPtr.

E.g., the following case is pretty common in WebKit:
1. We have an object A that gets constructed and destroyed on the main thread.
2. We sometimes pass object A to a background thread T and then back to the main thread.

It is OK as long as you only check the WeakPtr on the main thread and make sure that the object is always destroyed on the main thread.

[Chris Dumez]

I think it would be nice if those bug reports pointed to actual bugs in our code rather than theoretical issues.

[Kimmo Kinnunen]

(In reply to Chris Dumez from comment #2)
> Not that in general, it is perfectly OK for a class to subclass
> ThreadSafeRefCounted and CanMakeWeakPtr and it is not indicative of a bug.
> It really depends on how you're using WeakPtr.
> 
> E.g., the following case is pretty common in WebKit:
> 1. We have an object A that gets constructed and destroyed on the main
> thread.
> 2. We sometimes pass object A to a background thread T and then back to the
> main thread.
> 
> It is OK as long as you only check the WeakPtr on the main thread and make
> sure that the object is always destroyed on the main thread.

No, incorrect.
If you can guarantee that first ref is taken in main thread and last ref is dropped in main thread, then it should not be ThreadSafeRefCounted.

There's no case ThreadSafeRefCounted and CanMakeWeakPtr are correct.

[Kimmo Kinnunen]

(In reply to Chris Dumez from comment #1)
> We have assertions in WeakPtr to catch these things so I am surprised they
> are not hitting?

I don’t think you can assert !UAF?

Some cases of deletion has begun
Can be asserted

[Kimmo Kinnunen]

(In reply to Chris Dumez from comment #3)
> I think it would be nice if those bug reports pointed to actual bugs in our
> code rather than theoretical issues.

Feel free to close if you think it is not a relevant bug.

[Chris Dumez]

(In reply to Kimmo Kinnunen from comment #4)
> (In reply to Chris Dumez from comment #2)
> > Not that in general, it is perfectly OK for a class to subclass
> > ThreadSafeRefCounted and CanMakeWeakPtr and it is not indicative of a bug.
> > It really depends on how you're using WeakPtr.
> > 
> > E.g., the following case is pretty common in WebKit:
> > 1. We have an object A that gets constructed and destroyed on the main
> > thread.
> > 2. We sometimes pass object A to a background thread T and then back to the
> > main thread.
> > 
> > It is OK as long as you only check the WeakPtr on the main thread and make
> > sure that the object is always destroyed on the main thread.
> 
> No, incorrect.
> If you can guarantee that first ref is taken in main thread and last ref is
> dropped in main thread, then it should not be ThreadSafeRefCounted.

2 notes here:
1. You don't have to drop the last ref on the main thread to guarantee that the object gets destroyed on the main thread. ThreadSafeRefCounted has a template parameter to guarantee that the object gets destroyed on the main thread no matter on what thread the last ref gets dropped.
2. It isn't uncommon for people to dispatch a Ref<> to a ThreadSafeRefCounted object to a background thread and then dispatch it back to the main thread.

> 
> There's no case ThreadSafeRefCounted and CanMakeWeakPtr are correct.

I am not yet convinced, see comment above.

And again, we have threading assertions in place in WeakPtr to make sure the WeakPtr is only used on the thread the WeakPtrFactory was created on. That's not to say that we don't have bugs in our code base but I don't think it is correct to say that all usage of WeakPtr + ThreadSafeRefCounted is wrong. Maybe it would help to talk about a specific example in our code base of where you believe there is a bug? May be easier than talking in the abstract. Again, if we look I believe we can find them and fix those specific cases.

[Chris Dumez]

(In reply to Kimmo Kinnunen from comment #6)
> (In reply to Chris Dumez from comment #3)
> > I think it would be nice if those bug reports pointed to actual bugs in our
> > code rather than theoretical issues.
> 
> Feel free to close if you think it is not a relevant bug.

I don't know yet if it relevant yet or not (and I wouldn't WONTFIX another engineers bug). I don't have a clear example yet in our code base of something that it is wrong.

Right now, this bug seems to point out that we may be doing something that is not thread safe. I am willing to believe (thread safety bugs are common in WebKit and not always easy to reason about). I just think it would be better to speak about concrete examples in our code bases of something that is wrong. Then we can focus on fixing those.

While using WeakPtr and multiple threads (which is communicated by the fact that ThreadSafeRefCounted is used), it is definitely easy to write unsafe code. It doesn't mean that all those usages in our code base are currently wrong (if that makes sense).

[Fujii Hironori]

It'd be nine if we have thread-safe weak pointer.
std::weak_ptr is thread-safe. std::weak_ptr<T>::lock returns std::shared_ptr<T>.
https://en.cppreference.com/w/cpp/memory/weak_ptr/lock

[Radar WebKit Bug Importer]

<rdar://problem/91694167>

[Matt Woodrow]

(In reply to Chris Dumez from comment #8)
> 
> Right now, this bug seems to point out that we may be doing something that
> is not thread safe. I am willing to believe (thread safety bugs are common
> in WebKit and not always easy to reason about). I just think it would be
> better to speak about concrete examples in our code bases of something that
> is wrong. Then we can focus on fixing those.
> 

A counter-argument here would be that if there's a common pattern that frequently leads to security issues, we should consider blocking that pattern entirely (ideally at compile time) to prevent future bugs of that type.

[youenn fablet]

> A counter-argument here would be that if there's a common pattern that
> frequently leads to security issues, we should consider blocking that
> pattern entirely (ideally at compile time) to prevent future bugs of that
> type.

The general case does not seem too bad to me, you cannot use WeakPtr except in the right thread.

That said, https://github.com/WebKit/WebKit/pull/6078 shows at least one problematic case.
If you have the combo WeakPtr and ThreadSafeRefCounted<DestructionThread::Main>, WeakPtr might hold a valid pointer to a ThreadSafeRefCounted whose count is zero (object is scheduled to be destroyed in main thread but not yet) and we might create a Ref from the WeakPtr during that time.
It would be nice to either fix or outlaw this combo.

[Kimmo Kinnunen]

> The general case does not seem too bad to me, you cannot use WeakPtr except in the right thread.

No, this is incorrect.
You cannot use WeakPtr in *any* thread, if the object has been reffed in another thread.
This is the kind of problem why it shouldn't exist. Developers do not know what is correct and what is not.

[youenn fablet]

(In reply to Kimmo Kinnunen from comment #13)
> > The general case does not seem too bad to me, you cannot use WeakPtr except in the right thread.
> 
> No, this is incorrect.
> You cannot use WeakPtr in *any* thread, if the object has been reffed in
> another thread.
> This is the kind of problem why it shouldn't exist. Developers do not know
> what is correct and what is not.

It depends on the definition of the right thread.
In any case, I agree this is not easy to reason about and there have been identified issues in our code base.

As I said earlier, I would go with either fixing the combo or outlawing it.
Outlawing it might be easier.
I personally plan to remove this combo in code I am working on (time permitting).

[Alex Christensen]

I started fixing this in https://github.com/WebKit/WebKit/pull/6602

[Alex Christensen]

*** Bug 248051 has been marked as a duplicate of this bug. ***