279852 - nsWeakReference not threadsafe assertions due to TimerThread::Init

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

In bug 234858, sicking made nsWeakReference use macros for its refcounting
rather than writing AddRef and Release by hand.  This caused some
nsWeakReference not threadsafe assertions.  The observers in question are
registered in TimerThread::Init on a non-main thread:

          observerService->AddObserver(this, "sleep_notification", PR_TRUE);
          observerService->AddObserver(this, "wake_notification", PR_TRUE);

at the stack:

TimerThread::Init() (/builds/trunk/mozilla/xpcom/threads/TimerThread.cpp:134)
nsTimerImpl::InitCommon(unsigned int, unsigned int)
(/builds/trunk/mozilla/xpcom/threads/nsTimerImpl.cpp:215)
nsTimerImpl::InitWithFuncCallback(void (*)(nsITimer*, void*), void*, unsigned
int, unsigned int) (/builds/trunk/mozilla/xpcom/threads/nsTimerImpl.cpp:253)
NS_NewTimer(nsITimer**, void (*)(nsITimer*, void*), void*, unsigned int,
unsigned int) (/builds/trunk/mozilla/xpcom/threads/nsTimerImpl.cpp:633)
nsRecyclingAllocator::Malloc(unsigned int, int)
(/builds/trunk/mozilla/xpcom/ds/nsRecyclingAllocator.cpp:200)
nsRecyclingAllocatorImpl::Alloc(unsigned int)
(/builds/trunk/mozilla/xpcom/ds/nsRecyclingAllocator.cpp:385)
nsSegmentedBuffer::AppendNewSegment()
(/builds/trunk/mozilla/xpcom/io/nsSegmentedBuffer.cpp:103)
nsPipe::GetWriteSegment(char*&, unsigned int&)
(/builds/trunk/mozilla/xpcom/io/nsPipe3.cpp:482)
nsPipeOutputStream::WriteSegments(unsigned int (*)(nsIOutputStream*, void*,
char*, unsigned int, unsigned int, unsigned int*), void*, unsigned int, unsigned
int*) (/builds/trunk/mozilla/xpcom/io/nsPipe3.cpp:1071)
nsStreamCopierOB::DoCopy(unsigned int*, unsigned int*)
(/builds/trunk/mozilla/xpcom/io/nsStreamUtils.cpp:546)
nsAStreamCopier::Process() (/builds/trunk/mozilla/xpcom/io/nsStreamUtils.cpp:320)
nsAStreamCopier::HandleContinuationEvent(PLEvent*)
(/builds/trunk/mozilla/xpcom/io/nsStreamUtils.cpp:390)
PL_HandleEvent (/builds/trunk/mozilla/xpcom/threads/plevent.c:698)
nsIOThreadPool::ThreadFunc(void*)
(/builds/trunk/mozilla/netwerk/base/src/nsIOThreadPool.cpp:279)
_pt_root (/builds/trunk/mozilla/nsprpub/pr/src/pthreads/ptthread.c:217)


I don't think *all* weak references should have to use threadsafe refcounting. 
It shouldn't be hard to make a subclass that did (nsMTWeakReference).  That
said, there may be other problems with using the observer service like this
(darin?).

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: work in progress

Actually, it's not so trivial, but this is partway there. 
nsSupportsThreadsafeWeakReference still needs a bit more work to be threadsafe.
 And I'm not crazy about the names.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch (insufficient)

I think this should work, but I haven't thought it through too carefully.

It would be nice to be able to do this with atomic operations rather than
wasting space on a lock, though.  I'm not sure whether that's possible, though.

Comment 3

[timeless]

Comment on attachment 172416 [details] [diff] [review]
patch (insufficient)

>+nsSupportsThreadsafeWeakReference::nsSupportsThreadsafeWeakReference()
>+  {
>+    mLock = PR_NewLock();
>+  }
>+
>+nsSupportsThreadsafeWeakReference::~nsSupportsThreadsafeWeakReference()
>+  {
>+    PR_DestroyLock(mLock);
>+  }

you need to handle PR_NewLock failing.

Comment 4

[Darin Fisher]

Why does TimerThread need to implement nsISupportsWeakReference?  It seems to me
that it has to do with the AddObserver calls, but I don't understand why those
need to be weak references.  The RemoveObserver calls happen in the
TimerThread's destructor, and the TimerThread is meant to stick around for the
lifetime of the application, so it would seem to me that we could make those be
strong references and then just not bother calling RemoveObserver.  That would
mean that the observer service would keep the TimerThread object around for the
lifetime of the XPCOM session (i.e., until the observer service is shutdown). 
We use the same technique for other observers that need to stick around for the
entire XPCOM session.  So, perhaps that would be an alternative (and simpler)
solution here?

One more thing: the nsIObserverService has very ill-defined behavior in the
multithreaded use case.  it is troubling to me that this code is attempting to
use nsIObserverService across threads.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I was thinking it might be better to just copy & paste both classes instead of
using inheritance (the only extra codesize would be the copied QueryReferent
implementation, which is small) to save the virtual function call and make
things a little cleaner.

However, there are still some problems.  This patch doesn't handle the case of
QueryReferent on one thread racing with the destructor of
nsSupportsWeakReference on another thread, and I'm not sure how to handle that.
 In particular, the ~nsSupportsWeakReference happens well into the destruction
process of the object that inherits from it, and we really need QueryReferent
not to work much sooner.  If we didn't have (or need) refcount stabilization
we'd be able to detect this case by calling AddRef and Release on mReferent in
QueryReferent and looking at the return values, but we do.

Comment 6

[Darin Fisher]

another issue based on the stack trace in comment #0:

it looks like the recycling allocator is trying to create a timer on a thread
that does not have a nsIEventQueue associated with it.  that means that the
timer would never fire.  it's not a major bug since we use the cached i/o
buffers frequently, and the pool size is relatively small, but nonetheless it
seems like a bug we ought to fix.

long term i think we should have a way to associate a nsIEventTarget with the
current thread and have an API to expose it.  since the i/o threads do implement
nsIEventTarget and Timers only use the nsIEventTarget portion of nsIEventQueue,
this would be fairly trivial to fix.

there may be value in a threadsafe weakreference implementation, but it sounds
like a complicated thing to implement, and until we have a real consumer, i
think we should just take the short path to solving this bug.

Comment 7

[timeless]

fwiw, i'm fairly certain i'm such a customer. i have code which runs on threads
and might want to be a weak reference to either prefservice or observerservice.
but i'm in no hurry (it's not like xpcom or xpconnect are really threadsafe atm).

Comment 8

[David :Bienvenu]

this affects imap as well, whenever a connection is closed...

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The TimerThread::Init observer service usage was introduced by 197863.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

...by bug 197863.

Comment 11

[Simon Fraser [no longer active]]

The TimerThread::Init case should be fairly easy to fix by moving the
AddObserver calls to the main thread.

Comment 12

[Darin Fisher]

I'm testing a patch for this.

Comment 13

[Darin Fisher]

Attachment: v1 patch

dbaron is away on vacation, or otherwise i would ask him for review on this. 
instead, i'll ask the folks who reviewed the patch i'm changing ;-)

Comment 14

[Simon Fraser [no longer active]]

Comment on attachment 176961 [details] [diff] [review]
v1 patch

I'm a little worried that this might cause the TimerThread to be destroyed
later, therefore delaying the release of all the timers. Is this an issue?

Comment 15

[Darin Fisher]

It is not an issue.  If you read NS_ShutdownXPCOM you will see that
xpcom-shutdown happens before nsTimerImpl::Shutdown, which calls
TimerThread::Shutdown.  So, this should be ok :)

Comment 16

[Simon Fraser [no longer active]]

Comment on attachment 176961 [details] [diff] [review]
v1 patch

I'll take your word for it.

Comment 17

[Darin Fisher]

fixed-on-trunk