Closed Bug 279852 Opened 20 years ago Closed 20 years ago

nsWeakReference not threadsafe assertions due to TimerThread::Init

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.8beta2

People

(Reporter: dbaron, Assigned: darin.moz)

References

(Blocks 1 open bug)

Details

(Whiteboard: [patch])

Attachments

(1 file, 2 obsolete files)

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?).
Attached patch work in progress (obsolete) — Splinter Review
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.
Attached patch patch (insufficient) (obsolete) — Splinter Review
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.
Assignee: dougt → dbaron
Attachment #172414 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Blocks: 279923
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.
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.
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.
Assignee: dbaron → dougt
Status: ASSIGNED → NEW
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.
Attachment #172416 - Attachment description: patch → patch (insufficient)
Attachment #172416 - Attachment is obsolete: true
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).
this affects imap as well, whenever a connection is closed...
The TimerThread::Init observer service usage was introduced by 197863.
The TimerThread::Init case should be fairly easy to fix by moving the
AddObserver calls to the main thread.
I'm testing a patch for this.
Assignee: dougt → darin
Target Milestone: --- → mozilla1.8beta2
Attached patch v1 patchSplinter Review
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 ;-)
Attachment #176961 - Flags: superreview?(bryner)
Attachment #176961 - Flags: review?(sfraser_bugs)
Attachment #176961 - Flags: superreview?(bryner) → superreview+
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?
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 on attachment 176961 [details] [diff] [review]
v1 patch

I'll take your word for it.
Attachment #176961 - Flags: review?(sfraser_bugs) → review+
fixed-on-trunk
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: