128646 - GetService threadsafety problem allows creation of multiple instances

Status: RESOLVED DUPLICATE of bug 5795

(Core :: XPCOM, defect)

Description

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

waterson and I ran into a threadsafety problem with GetService the summer before
last.  See bug 21650 and revision 3.46 of xpcom/components/nsServiceManager.cpp.
 The problem was never really fixed.

Here's part of a discussion on threadsafety from IRC, with some bits not
relevant to this specific bug left in, for fear of removing something that is
relevant:

[20:33:53] <dbaron> Is the service manager threadsafety problem of
simultaneously creating a service on multiple threads fixed?
[0:34:16] <dmose> no, i think that's the same problem as createinstance
[20:34:25] <dmose> at least that's what dougt seemed to say to me yesterday
[20:34:30] <shaver> why would that be the same problem?
[20:34:32] <dbaron> no, there was a service manager hashtable issue
[20:34:37] <dbaron> waterson and I ran into it the summer before last
[20:34:42] <brendan> bugs, we need bugs
[20:34:44] <bryner> dbaron: it seems like to solve that you would need
per-service locking
[20:34:44] <shaver> there are no singleton issues in createinstance
[20:34:55] <dmose> yeah, we need bugs
[20:35:04] <dmose> i'll poke dougt to show up here next time i talk to him
[20:35:08] <shaver> not here
[20:35:10] <shaver> in bugzilla
[20:35:42] <dbaron> The problem is this:
[20:35:50] <dbaron> In nsComponentManagerImpl::GetService
[20:35:55] <dmose> shaver: my impression is that all these bugs are already filed
[20:36:10] <dbaron> to avoid AB-BA deadlock, we do mon.exit() and mon.enter()
around the CreateInstance call
[20:36:40] <dbaron> but mon.exit() allows another thread to enter the monitor at
the top of GetService and work its way to the CreateInstance, creating 2 instances.
[20:36:42] <dbaron> (or 3)
[20:36:48] <bryner> dbaron: but by that point you've already decided an existing
instance doesn't exist?
[20:36:51] <dbaron> This used to happen on viewer startup with the MIME
service.[20:37:04] <dbaron> bryner: by which point?
[20:37:25] <dbaron> It could be fixed by creating a sort of dummy entry before
exiting the monitor.
[20:37:42] <dbaron> at least partly.
[20:37:50] <dbaron> I'm not sure how to handle the waiting on the other thread,
though.
[20:37:56] <dbaron> (without the risk of deadlock)
[20:37:56] <dmose> http://bugzilla.mozilla.org/show_bug.cgi?id=101976 is the
tracking bug
[20:37:58] <bryner> dbaron: basically what you said
[20:38:02] <dmose> it has about 10 dependents
[20:38:33] <dmose> it's targetted at 1.1
[20:38:40] <bryner> dbaron: the first thread exits the monitor with no
indication that it has already decided to create an instance
[20:42:01] <brendan> dbaron: what's the deadlock hazard if you make a dummy
entry and notify a condvar to wake up any racers waiting for that dummy entry to
become real?
[20:42:33] <shaver> the only createinstance bug in that list is a WONTFIX
[20:42:53] <shaver> if you need singleton semantics under createinstance, do the
locking yourself in the leaf constructor
[20:43:11] <dawn> bryner: i've just started doing laundry
[20:43:12] <shaver> the component manager must not hold locks when calling out
to factories, which might nest and deadlock
[20:43:16] <bryner> ah, ok.
[20:43:36] <dbaron> brendan: Could the creation require a lock that's held on
the thread waiting on the condvar?
[20:43:37] <dmose> shaver: not true; see bug 5795
[20:43:48] <shaver> dmose: that's the bug I'm talking about
[20:43:51] <shaver> dmose: I read that bug
[20:44:12] <dmose> and the category mgr bug
[20:44:17] <dmose> is also about createinstance 
[20:44:39] <brendan> dbaron: the thread waiting on the condvar must not hold a
lock -- no nested locks
[20:44:52] <brendan> dbaron: if threads nest locks in arbitrary order, they
*will* deadlock
[20:44:57] <dbaron> ok
[20:45:05] <brendan> dbaron: condvar's lock is released during the wait, by
PR_WaitCondVar

Comment 1

[Doug Turner (:dougt)]

This is a known issue and a possible dup of on of the other bugs that block 101976.

I will address this for mozilla 1.1.

Comment 2

[Doug Turner (:dougt)]

moving out based on my workload.  Yell, if you have a problem.

Comment 3

[Doug Turner (:dougt)]

This is a dup.  (component manager == service manager)

*** This bug has been marked as a duplicate of 5795 ***

Comment 4

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

It seems valid to me.

Comment 5

[Doug Turner (:dougt)]

possibly.  see dp's comments in 5795, or shaver's comments in this bug.  it
would be much more risky to hold a lock while calling out to a factory.  The
factory implementation can protect themselves this is a problem.