1665308 - WebSocketImpl may clear nsWeakPtr members on different thread where they were created.

Status: RESOLVED DUPLICATE of bug 1660307

(Core :: DOM: Networking, defect)

Description

[Jens Stutte [:jstutte]]

+++ This bug was initially created as a clone of Bug #1611094 +++

nsWeakPtr is not thread safe. It seems, we can destroy ourself off the thread that created the referenced objects.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

NS_DECL_THREADSAFE_ISUPPORTS means just that refcounting is threadsafe, nothing else.
Having WeakPtrs is fine as long as they are created and cleared on the same thread. But looks like that isn't the case.

Comment 2

[Jens Stutte [:jstutte]]

Just for my understanding: if an object is NS_DECL_THREADSAFE_ISUPPORTS, we expect it to be refcounted and thus potentially deleted on multiple threads. It then sounds very difficult and fragile to me to ensure, that members of such an object are not deleted off-thread by the release function. Am I missing something?

Edit: We would need to ensure, that the last reference to ourself is hold on the thread that created the members, yes? Or that there are other references that keep alive those members? It sounds all a bit out-of-control of the class itself...

Comment 3

[Jens Stutte [:jstutte]]

In this particular case the WebSocketImpl lives in a timer event which means until the timer fired, anything could happen to the referenced objects such that we hold the last (weak) reference here during the timer event.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

https://searchfox.org/mozilla-central/rev/30e70f2fe80c97bfbfcd975e68538cefd7f58b2a/xpcom/threads/nsProxyRelease.h#110-127
is what is used often to release member variables on the correct thread. (or the main thread variant if we care about main thread release only)

Comment 6

[Jens Stutte [:jstutte]]

I assume we should hide the other bug then, too? IIUC, in release builds this might lead to a UAF.

Comment 7

[Dragana Damjanovic [:dragana]]

Done