449822 - Service manager can create two instances of a service

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

Found working on the testcase for bug 443874, the service manager can create two instances of a service. In that case it was two instances of the proxy object manager and that causes a fun deadlock. Patch attached which makes the service manager 1) wait for creation of a single instance of the service if called from two different threads at the same time, and 2) return failure for recursive getservice.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1 [-w version]

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I think this should obsolete bug 448021.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Try server builds with talos numbers are on their way.

Comment 4

[Shawn Wilsher :sdwilsh]

We hit this in storage at one point too and it was a royal pain to debug.  We ended up adding our own protection against it, but I'd really like to see a general solution if we could.

Comment 5

[Benjamin Smedberg]

Comment on attachment 332990 [details] [diff] [review]
Patch, v1

I'm a little skeptical of the hashtable... the constant cost of doing hashtable lookups on every getservice is going to show up, I'd think.

The common case is that there are *not* pending CIDs, which is why the voidarray works so well... if there are no entries, you short-circuit the code entirely.

I suggest if we're going to do this (and need the extra nsIThread*), we should use a nsTArray and optimize for the no-pending-cid case.

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v2

Ok, this uses nsTArray.

Comment 7

[Benjamin Smedberg]

Comment on attachment 333799 [details] [diff] [review]
Patch, v2

>diff --git a/xpcom/components/nsComponentManager.cpp b/xpcom/components/nsComponentManager.cpp

>+// This should only ever be called within the monitor!
>+nsIThread*
>+nsComponentManagerImpl::GetPendingServiceThread(const nsCID& aServiceCID) const
>+{
>+  PRUint32 pendingCount = mPendingServices.Length();
>+  for (PRUint32 index = 0; index < pendingCount; index++) {

nit, use ++index

> NS_IMETHODIMP
> nsComponentManagerImpl::GetService(const nsCID& aClass,
>                                    const nsIID& aIID,
>                                    void* *result)
> {

>+    nsIThread* currentThread = NS_GetCurrentThread();
>+    NS_ASSERTION(currentThread, "This should never be null!");

Is it possible to avoid the NS_GetCurrentThread call if there are no pending CIDs?

>+#ifdef SHOW_CI_ON_EXISTING_SERVICE
>+        // XXX Do we want to continue supporting this rarely-built behavior?
>+        if (!pendingThread)
>+            pendingThread = currentThread;
>+#endif

I don't understand this hunk

>+        if (!NS_ProcessNextEvent(currentThread, PR_FALSE))
>+            PR_Sleep(PR_INTERVAL_NO_WAIT);


Please add a comment that this is equivalent to yielding, because that wasn't obvious until I went and looked up the API docs for PR_Sleep.

I tend to think that it should be possible to create a unit test for this behavior... e.g. a service that, while it's being created, sleeps for a couple seconds while a second thread also tried to create the service.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #7)
> Is it possible to avoid the NS_GetCurrentThread call if there are no pending
> CIDs?

Yes, but only if we're ok calling that function within the monitor. Is that oK?

> I don't understand this hunk

It's to support the old behavior of the 'abusedContracts' list... Basically it's there to help you find the bad caller that is CI'ing your service. Like I said in the comments I'm not really sure how useful it is or whether or not we care to continue supporting it.

> I tend to think that it should be possible to create a unit test for this
> behavior...

I think that is doable, yes. I'll whip one up.

Comment 9

[Benjamin Smedberg]

Instead of storing an nsIThread*, can you store a PRThread* for the current thread and in the pending table? PR_GetCurrentThread is quite a bit faster than the NS_GetCurrentThread wrapper IIRC, and doesn't have any refcounting overhead. I also think it's safe to call from within the monitor.

Ditch the old abusedContracts behavior... I prefer code readability.

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v3

This addresses all of bsmedberg's comments and moves the GetThread calls until after we've already established that the service does not yet exist. Also includes a unit test that hits both the ClassID and ContractID code paths.

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Pushed changeset 7f5dbe89e781 to mozilla-central.