Closed Bug 449822 Opened 13 years ago Closed 13 years ago

Service manager can create two instances of a service

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: bent.mozilla, Assigned: bent.mozilla)

References

Details

Attachments

(1 file, 3 obsolete files)

Attached patch Patch, v1 (obsolete) — Splinter Review
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.
Attachment #332990 - Flags: review?(benjamin)
I think this should obsolete bug 448021.
Try server builds with talos numbers are on their way.
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 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.
Attachment #332990 - Flags: review?(benjamin) → review-
Attached patch Patch, v2 (obsolete) — Splinter Review
Ok, this uses nsTArray.
Attachment #332990 - Attachment is obsolete: true
Attachment #332991 - Attachment is obsolete: true
Attachment #333799 - Flags: review?(benjamin)
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.
Attachment #333799 - Flags: review?(benjamin) → review-
(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.
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.
Attached patch Patch, v3Splinter Review
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.
Attachment #333799 - Attachment is obsolete: true
Attachment #336688 - Flags: review?(benjamin)
Attachment #336688 - Flags: review?(benjamin) → review+
Pushed changeset 7f5dbe89e781 to mozilla-central.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Blocks: 453722
You need to log in before you can comment on or make changes to this bug.