58904 - XPCOM should have nsLock strong type to match nsAutoLock

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P3)

Description

[Brendan Eich [:brendan]]

That way, we can manage the deadlock detection state (OrderTable and its 
entries) in xpcom/threads/nsAutoLock.cpp better.  nsMonitor for nsAutoMonitor 
too.  I'd like to get rid of nsAutoCMonitor and abolish PR_CEnterMonitor and 
PR_CExitMonitor usage from Mozilla.  Comments?

/be

Comment 1

[Darin Fisher]

I agree... willing to do whatever leg work I can in necko to make the shift.

Comment 2

[Brendan Eich [:brendan]]

Sorry.

Comment 3

[Brendan Eich [:brendan]]

Load-balancing.

/be

Comment 4

[Brendan Eich [:brendan]]

I'm busy elsewhere, won't get to this in 0.9.4.

/be

Comment 5

[Doug Turner (:dougt)]

well, agree  that all cache'ed locks and monitors should be looked at as a
source of performance loss.  (I also think that monitor usage should die.  All
code can be rewritten to use a non-reentrant lock.  We should just a lock and a
condVar if we need monitor functionality).

So, what exactly is a NsLock.  How is it different than a PRLock?  Yes, the
first two chars, but what else? :-)

Comment 6

[Brendan Eich [:brendan]]

dougt: nsLock is a veneer around PRLock that lets us track the lifetime of a
lock so that the deadlock-detection code in xpcom/threads/nsAutoLock.cpp can
remove the address of the PRLock from its OrderTable.  See the comment in
http://lxr.mozilla.org/mozilla/source/xpcom/threads/nsAutoLock.cpp#75.

/be

Comment 7

[John Bandhauer]

> I also think that monitor usage should die. 
> All code can be rewritten to use a non-reentrant lock...

I wonder how serious you are. All digital circuits can be constructed using 
NAND gates too, but that is not always the best way to get the job done. I sort 
of agree. I prefer non-reentrant locks. But, I've also run into situations that 
would require serious bending over backwards that are fixed easily by a 
re-entrant lock. Is anyone planning a crusade? Or is this armchair talk?

Comment 8

[Brendan Eich [:brendan]]

Sometimes you risk livelock with optimistic strategies that do not nest locks. 
Sometimes the code to implement such a strategy is too big and hairy to be worth
avoiding a monitor.  Never say never, I always say :-).  But it's best to avoid
nesting locks, if you can.

Ok, jband was addressing dougt, but I had to say my peace.

/be

Comment 9

[Doug Turner (:dougt)]

sorry to greate your cheese, john.  no plans to anything about it, just being a
monday morning qb.

Comment 10

[Brendan Eich [:brendan]]

Minor, punting beyond 1.0 -- this is not a feature, it's a safety mechanism, but
we aren't hurting for want of it, much.  I hope it makes it under the wire, and
welcome feedback, but I can't justify working on it before other 1.0 stuff.

/be

Comment 11

[Brendan Eich [:brendan]]

I'm missing 1.1alpha.  I'd rather move to 1.2alpha than try to rationalize this
kind of change as a "beta" fix.

/be

Comment 12

[Brendan Eich [:brendan]]

Moving out, some of these may move to 1.3alpha shortly.

/be

Comment 13

[Brendan Eich [:brendan]]

dougt, you interested in this?  Has anyone seen a false positive from the
deadlock detecting code in nsAutoLock.cpp?

/be

Comment 14

[Doug Turner (:dougt)]

Sorry.  Right now, I am focusing on freezing part of XPCOM for clients and
building out our SDK.  I may have time in 1.3 to work on it.  

Comment 15

[Brendan Eich [:brendan]]

Fixing TM.

/be

Comment 16

[Brendan Eich [:brendan]]

Maybe for mozilla2.0, but only as part of a more coordinated plan.

/be

Comment 17

[John Gaunt (redfive)]

Songbird sees a lot of the warning from http://lxr.mozilla.org/mozilla1.8/source/xpcom/threads/nsAutoLock.cpp#286 that seem spurious to me. We do have nested locks in some places.

Also, and I'm not sure if this bug would address the point, we see in debug mode a major thrashing of the OrderTable. We currently use nsAutoLocks in our db code and make a TON of queries. The symptom we saw is CPU spiking ( 50% on a 2 CPU machine meaning one cpu is always pegged ) and then the app becomes very un-responsive (much like kerz after a charger loss). Of course in release this symptom goes away.

I have been able to make the symptom go away a couple of ways, 1) change nsAutoLock back to PRLock/PRUnlock (duh) 2) hack nsAutoLock to have static methods as does nsAutoMonitor - NewLock() and DestroyLock() that operate identically to the nsAutoMonitor calls.

Comment 18

[Brendan Eich [:brendan]]

I'm a bad owner for this. John, anyone: feel free to claim this and I will review any patch.

/be

Comment 19

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I'd like to kill several birds with this one stone.  Proposed changes follow.  Since they're nearly identical for both Locks and Monitors, I'll only mention the Lock changes.

1. Implement nsLock as a light wrapper around PR_Lock, keeping the same semantics from nspr: non-recursive mutex.

2. nsLocks are created and destroyed with |new nsLock()| and |delete lock|.  nsAutoLock::NewLock and nsAutoLock::DestroyLock can be kept around, but they will be changed to return nsLock* during implementation of (3).

3. Move the deadlock detection machinery to nsLock.

4. Deprecate direct usage of PR_NewLock(), fix offending code.

Anyone strongly want an nsCMonitor?  I see no reason to keep it around.

Comment 20

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

In 2, why allow 'new nsLock'? The only reason you'd want to create an nsLock (instead of a PRLock) is to use it with nsAutoLock, so making nsAutoLock::NewLock be the only way to make one seems reasonable.

I think the axe has come down on CMonitors.

Comment 21

[Benjamin Smedberg]

If we're going to do this, we might as well avoid PRLock altogether and use the XPCOM API exclusively... that way we could even avoid function call and perhaps allocation overhead of NSPR locks.

Comment 22

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

> If we're going to do this, we might as well avoid PRLock altogether and use the
> XPCOM API exclusively... that way we could even avoid function call and perhaps
> allocation overhead of NSPR locks.
> 

Yes, I heartily agree with this point --- (2) and (4) in comment 19 are designed to bring this about.  A fairly significant amount of work is required to bypass NSPR locks/monitors, so that will remain a TODO.

Comment 23

[Brendan Eich [:brendan]]

Chris, great to see this old bug I filed (mid-five digits!) getting attention. Your plan sounds good tome.

/be

Comment 24

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Proposed API changes (not compilable)

This patch shows the proposed changes to the API, to further the discussion.  It not meant to be reviewed for checkin.  The full implementation awaits approval of this approach.

The approach chosen has been changed from that described in comment 19, after discussion with Ben T.  (The original wasn't feasible without C++ exceptions.)

Comment 25

[Shawn Wilsher :sdwilsh]

Is is possible to also make nsLock live on the stack (or heap I guess)?  Granted, iIt won't be terribly useful until we fix bug 476540 and change nsLock to use a stack based PRLock, but I can make cycles to fix that pretty quickly.

Comment 26

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

nsLock can't be entirely statically allocated until the underlying lock can :).

The reason is that because PR_Lock is currently dynamically allocated, if that allocation fails, there's no way for us to report to the calling code that something exceptional has happened.

Comment 27

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: New synchronization API, v2

It's nice to be settled in and back to work!

r? for the API only.  I'm holding back posting the implementation until the API is settled.  The implementation is basically finished, though.  (And there were several potential problems with the old one ... we may catch several new deadlocks with the new one, in addition to the ones caught by switching to nsLock only.)

The only substantial change from the earlier proposal is the addition of nsCondVar.  If we discourage direct use of PRLock, then we need to provide nsCondVar, since in our API they need to mated with a lock.  (I would rather only use monitors, but I understand condvars are already sprinkled throughout the code.)  This has the added advantage of allowing us to integrate condvars into the deadlock detector, although that's work for another day because it's less than trivial.  Detecting deadlock on the underlying locks suits me for now.

A style issue I want to call out in the new patch is the Destroy*() family of functions.  They have been modified to take a reference to a pointer, and they null out the reference after deletion.  I prefer this, because I think it helps catch more bugs, but I don't know if it's the "Mozilla way."  Please advise.

Comment 28

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: In-progress synchronization API implementation

This is a mostly working implementation.  The biggest change is that it's fully backwards compatible with the old API.  I plan to leave compatibility for a week or so, to give people time to update their patches to the new API.  (And to give me more time to work on refactoring existing code.)

One issue with this patch is that it creates two deadlock detectors: one for the old code, and one for the new.  Probably not much of an issue, but this means it may miss deadlocks that it otherwise would if there were a unified detector.  I tried to change the existing detector as little as possible, but there were several bugs and assumptions that weren't true for the new API.  I plan on doing the detector overhaul for bug 456272, but for now, the "new" detector is uglier than it should be.

Comment 29

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: In-progress impl, v2

The last patch lacked two files I forgot to hg add.  This one has those, and a few small bugfixes.  Passes my unit tests, including detecting simple deadlocks.

Next up is testing on Minefield opt/debug, and then I'll ask for review.

Comment 30

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: New synchronization API and impl

Summary;
  * wraps PRLock, PRMonitor, and PRCondvar in nsXXX variants
  * makes deadlock detector threadsafe (for new impl), and fixes a couple of other detector bugs
  * fully backwards compatible with old API, but will change
  * kills off nsAutoCMonitor

Notes:
  * there are now two deadlock detectors (old and new), each maintaining its own poset.  so some deadlocks may be missed while backwards compatibility is enabled.
  * the detector for the new impl will never be freed
  * the nsXXX::DestroyXXX functions null out pointers after freeing them
  * the detector is ugly, but will be overhauled for bug 456272

Comment 31

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Sync API + Impl, new and improved

Not substantially different from the last patch.  The biggest difference is that the new classes CondVar, Monitor, and Mutex are now in the mozilla namespace.  (Why nsLock was renamed to Mutex is left as an exercise for the reader.)

Since nsLock went away, I also took the opportunity to give each sync primitive its own header file.

Comment 32

[Benjamin Smedberg]

Comment on attachment 371556 [details] [diff] [review]
Sync API + Impl, new and improved

>diff --git a/intl/strres/src/nsStringBundle.h b/intl/strres/src/nsStringBundle.h

>+#include "nsAutoLock.h"

We're going with #include "mozilla/Mutex.h" for the new code, right? Or is this just fixing to the old API first, and we'll move to that style later?

>diff --git a/xpcom/glue/BlockingResourceBase.cpp b/xpcom/glue/BlockingResourceBase.cpp

>+#include "nsVoidArray.h"

nsVoidArray is dying. New code should use nsTArray<>

>+// XXX/TODO not kosher to access by enum value
>+static const char* const ResourceTypeNames[] = {"Mutex", "Monitor", "CondVar"};

I'd prefer this declaration to be static char const *const kResourceTypeNames.

>+/*
>+ TODO re-enable this after API change.  with backwards compatibility
>+ enabled, it conflicts with the impl in nsAutoLock.cpp.  Not freeing
>+ this stuff will "leak" memory that is cleanup up when the process
>+ exits.

use an #if 0 to disable this code, instead of extending the comment

>+mozilla::BlockingResourceBase::BlockingResourceBase(

start namespace mozilla { here?

>diff --git a/xpcom/glue/BlockingResourceBase.h b/xpcom/glue/BlockingResourceBase.h

>+namespace mozilla {
>+#if 0
>+} // for emacs
>+#endif

Oh, that's ugly. What does emacs do which provoked you to do this?

>diff --git a/xpcom/glue/Monitor.h b/xpcom/glue/Monitor.h

>+//--------------------------------------------------

Please remove this line unless it has some special purpose I don't know about.

>+    void Enter() {
>+        // NSPR does consistency checks foor us

typo

>+private:
>+    Monitor(void) { }

>+    Monitor(Monitor& /*aMonitor*/) { }
>+    Monitor& operator =(Monitor& /*aMonitor*/) {
>+        return *this;
>+    }

I don't understand the need for a copy constructor or operator=... can we leave them both unimplemented?

I'm more than a little saddened by the NewMonitor API. In my opinion it would be better for clients to use Monitor() directly and either

1) check the state of the monitor for allocation failure using an mon.IsValid() API
or
2) abort if creating a monitor failed

>+class NS_COM_GLUE MonitorAutoEnter {
>+public:
>+    /**
>+     * Constructor
>+     * The constructor aquires the given lock.  The destructor
>+     * releases the lock.
>+     * 
>+     * @param aMonitor A valid mozilla::Monitor* returned by 
>+     *                 mozilla::Monitor::NewMonitor. 
>+     **/
>+    MonitorAutoEnter(mozilla::Monitor* aMonitor)

Any particular reason this should take a pointer instead of a reference? A reference seems more natural here.

>+private:
>+    // Not meant to be implemented. This makes it a compiler error to
>+    // construct or assign an MonitorAutoEnter object incorrectly.
>+    MonitorAutoEnter() {}
>+    MonitorAutoEnter(MonitorAutoEnter& /*aMonitor*/) {}
>+    MonitorAutoEnter& operator =(MonitorAutoEnter& /*aMonitor*/) {
>+        return *this;
>+    }

If they are not meant to be implemented, just don't implement them. Link errors are good indicators of doing something wrong.

>+    // Not meant to be implemented. This makes it a compiler error to
>+    // attempt to create an MonitorAutoEnter object on the heap.
>+    static void* operator new(size_t /*size*/) CPP_THROW_NEW {
>+        return nsnull;
>+    }
>+    static void operator delete(void* /*memory*/) {}

You should also annotate this class with NS_STACK_CLASS, which will cause better error messages from the tinderbox which builds with static checking enabled.

>diff --git a/xpcom/glue/Mutex.h b/xpcom/glue/Mutex.h

>+class NS_COM_GLUE Mutex : BlockingResourceBase {

>+    static Mutex* NewMutex(const char* name) {

Again, the NewMutex helper seems ugly as a permanent API, and forces this class to be heap-allocated instead of being a direct member.

>+class NS_COM_GLUE MutexAutoLock {

And NS_STACK_CLASS

Also style nit, class decls should start the brace on the following line, e.g.

class Foo
{
};

>+/**
>+ * MutexAutoLock

This is the doccomment for MutexAutoUnlock ;-)

>diff --git a/xpcom/tests/TestSynchronization.cpp b/xpcom/tests/TestSynchronization.cpp

>+/*
>+    // fun with deadlocks!
>+    // TODO separate deadlock checker unit tests

Use #if 0

Not sure that we need a deadlock-checker unit test... it would only be useful in debug builds right?

Comment 33

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #32)
> (From update of attachment 371556 [details] [diff] [review])
> >diff --git a/intl/strres/src/nsStringBundle.h b/intl/strres/src/nsStringBundle.h
> 
> >+#include "nsAutoLock.h"
> 
> Or is this
> just fixing to the old API first, and we'll move to that style later?
>

This.  See bug 486606 and upcoming friends.
 
> >diff --git a/xpcom/glue/BlockingResourceBase.cpp b/xpcom/glue/BlockingResourceBase.cpp
> 
> >+#include "nsVoidArray.h"
> 
> nsVoidArray is dying. New code should use nsTArray<>
> 

:).  See bug 456272.  This code is copied from the old deadlock detector, and I want do the rewrite in another pass so that nothing breaks in the interim.

> >+mozilla::BlockingResourceBase::BlockingResourceBase(
> 
> start namespace mozilla { here?
> 

Why?

> >diff --git a/xpcom/glue/BlockingResourceBase.h b/xpcom/glue/BlockingResourceBase.h
> 
> >+namespace mozilla {
> >+#if 0
> >+} // for emacs
> >+#endif
> 
> Oh, that's ugly. What does emacs do which provoked you to do this?
> 

It wants to keep indenting under "namespace mozilla {."  That hack prevents it from doing that.  If there's a saint of the Church of Emacs who knows a better way, I'm all ears.

> >diff --git a/xpcom/glue/Monitor.h b/xpcom/glue/Monitor.h
> 
> >+//--------------------------------------------------
> 
> Please remove this line unless it has some special purpose I don't know about.
> 

It helps me visually scan different sections of the code.  I like to put it between the implementations of classes.  But I'm not wedded to it.

> >+private:
> >+    Monitor(void) { }
> 
> >+    Monitor(Monitor& /*aMonitor*/) { }
> >+    Monitor& operator =(Monitor& /*aMonitor*/) {
> >+        return *this;
> >+    }
> 
> I don't understand the need for a copy constructor or operator=... can we leave
> them both unimplemented?
> 

These are "private" to prevent people from calling them.  The defaults would copy the underling PRMonitor and thereby violate an invariant of the deadlock detector.

> I'm more than a little saddened by the NewMonitor API. In my opinion it would
> be better for clients to use Monitor() directly and either
> 

I don't like it either.  See bug 476540.  I'm trying to do this upgrade one step at a time, and throwing in the removal of heap allocation was more than I wanted take on at once.

> >+class NS_COM_GLUE MonitorAutoEnter {
> >+public:
> >+    /**
> >+     * Constructor
> >+     * The constructor aquires the given lock.  The destructor
> >+     * releases the lock.
> >+     * 
> >+     * @param aMonitor A valid mozilla::Monitor* returned by 
> >+     *                 mozilla::Monitor::NewMonitor. 
> >+     **/
> >+    MonitorAutoEnter(mozilla::Monitor* aMonitor)
> 
> Any particular reason this should take a pointer instead of a reference? A
> reference seems more natural here.
> 

No, good point.  Ben Turner suggested it also, and I seem not to have gotten around to it.

> Not sure that we need a deadlock-checker unit test... it would only be useful
> in debug builds right?

Yes, it would only be useful in debug builds.  However, I want it to be a good safety net, which to me means (i) fast enough it's not noticed; and (ii) catches exactly the deadlocks it was intended to (no more, no less).  I'd like to verify that with unit tests.

Maybe xpcom/tests is not the place for that; is there a better location?

Comment 34

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

> > I don't understand the need for a copy constructor or operator=... can we leave
> > them both unimplemented?
> > 
> 
> These are "private" to prevent people from calling them.  The defaults would
> copy the underling PRMonitor and thereby violate an invariant of the deadlock
> detector.
> 

Sorry, please ignore this.  I misunderstood your point on my first pass through your comments and forgot to delete this ignorant reply.

Comment 35

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Sync API, now with non-heap allocation!

Oh, being able to abort on failed malloc brings tears of joy to my eyes.

Comment 36

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Sync API, now with non-heap allocation! And no emacs hacks!

Crap, missed an emacs namespace hack.

Comment 37

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 372122 [details] [diff] [review]
 Sync API, now with non-heap allocation!   And no emacs hacks!

Adding a vim modeline.

Comment 38

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Sync API, now with non-heap allocation! And no emacs hacks! And a vim modeline!

Benjamin, sorry for all the spam.

Comment 39

[Benjamin Smedberg]

Comment on attachment 372129 [details] [diff] [review]
Sync API, now with non-heap allocation! And no emacs hacks!  And a vim modeline!

>diff --git a/xpcom/glue/BlockingResourceBase.cpp b/xpcom/glue/BlockingResourceBase.cpp

>+        if (this == chainFront) {
>+            // acceptable reentry, and nothing to update.
>+            return;
>+        } else if (chainFront) {

Style nit, use

}
else if (cond) {

>+    } else {

ditto

>+class NS_COM_GLUE BlockingResourceBase {

style nit, brace on following line

>+    enum BlockingResourceType {eMutex, eMonitor, eCondVar/*, eSemaphore??*/};

semaphore? leave it out until we need it. Also it might be worth adding a comment that this needs to be kept in sync with kResourceTypeNames

>+    void Init(void* aResource, 
>+              const char* aName,
>+              BlockingResourceType aType);

global nit, did you give up on my Type *identifier snuggly-preference, or are you just waiting on the conclusion of the newsgroup thread?

>+    void*           mResource;

whitespace nit, this seems to have extra spaces which don't line up with anything around it. Either make all the fields line up, or don't do any lining-up and use a single space.

>diff --git a/xpcom/glue/Monitor.h b/xpcom/glue/Monitor.h

>+/**
>+ * Monitor
>+ * Java-like monitor.  Using the MonitorAutoEnter wrapper is much preferred to
>+ * using this class without it; do so only when you have a compelling reason.
>+ **/

How about this text: """
When possible, use MonitorAutoEnter to hold this monitor within a scope, instead of calling Enter/Exit/Wait directly.
"""

>+/**
>+ * MonitorAutoEnter
>+ * Enters the Monitor when it enters scope, and exits it when it leaves 
>+ * scope.
>+ *
>+ * MUCH PREFERRED to bare Monitor.
>+ */ 

s/bare Monitor/bare calls to Monitor.Enter and Exit/

ditto these comments for Mutex

>diff --git a/xpcom/glue/Mutex.h b/xpcom/glue/Mutex.h

>+    Mutex(const char* name) {
>+        mLock = PR_NewLock();
>+        if (!mLock)
>+            NS_DebugBreak(NS_DEBUG_ABORT,
>+                          "Can't allocate mozilla::Mutex",
>+                          "Pr_NewLock()",
>+                          __FILE__, __LINE__);
>+        Init(mLock, name, eMutex);

I'm sorry I was unclear about this... what I actually meant was to use a macro to make this code more readable, so that this could be:

if (!mLock)
  NS_RUNTIMEABORT("Can't allocate mozilla::Mutex");

Also, please fix up the Abort() function in nsDebugImpl.cpp to do what we discussed on IRC:

1) dereference NULL
2) _exit(127)

Otherwise this is good... I'd like to see a followup patch before this gets checked in.

Comment 40

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Updated sync api

Waiting for review/checkin until it passes tryserver.

Comment 41

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Last patch, rebased

Tryserver-ing again before review.

Comment 42

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #39)
> (From update of attachment 372129 [details] [diff] [review])
> >+    void Init(void* aResource, 
> >+              const char* aName,
> >+              BlockingResourceType aType);
> 
> global nit, did you give up on my Type *identifier snuggly-preference, or are
> you just waiting on the conclusion of the newsgroup thread?
> 

Yeah, I'm just waiting.  Once that's concluded, I can do an auto-rewrite.

> >+    void*           mResource;
> 
> whitespace nit, this seems to have extra spaces which don't line up with
> anything around it. Either make all the fields line up, or don't do any
> lining-up and use a single space.
> 
> >diff --git a/xpcom/glue/Monitor.h b/xpcom/glue/Monitor.h
> 
> >+/**
> >+ * Monitor
> >+ * Java-like monitor.  Using the MonitorAutoEnter wrapper is much preferred to
> >+ * using this class without it; do so only when you have a compelling reason.
> >+ **/
> 
> How about this text: """
> When possible, use MonitorAutoEnter to hold this monitor within a scope,
> instead of calling Enter/Exit/Wait directly.
> """
> 
> >+/**
> >+ * MonitorAutoEnter
> >+ * Enters the Monitor when it enters scope, and exits it when it leaves 
> >+ * scope.
> >+ *
> >+ * MUCH PREFERRED to bare Monitor.
> >+ */ 
> 
> s/bare Monitor/bare calls to Monitor.Enter and Exit/
> 
> ditto these comments for Mutex
> 
> >diff --git a/xpcom/glue/Mutex.h b/xpcom/glue/Mutex.h
> 
> >+    Mutex(const char* name) {
> >+        mLock = PR_NewLock();
> >+        if (!mLock)
> >+            NS_DebugBreak(NS_DEBUG_ABORT,
> >+                          "Can't allocate mozilla::Mutex",
> >+                          "Pr_NewLock()",
> >+                          __FILE__, __LINE__);
> >+        Init(mLock, name, eMutex);
> 
> I'm sorry I was unclear about this... what I actually meant was to use a macro
> to make this code more readable, so that this could be:
> 
> if (!mLock)
>   NS_RUNTIMEABORT("Can't allocate mozilla::Mutex");
> 
> Also, please fix up the Abort() function in nsDebugImpl.cpp to do what we
> discussed on IRC:
> 
> 1) dereference NULL
> 2) _exit(127)
> 
> Otherwise this is good... I'd like to see a followup patch before this gets
> checked in.

Comment 43

[Brendan Eich [:brendan]]

I have a tear in my eye upon seeing this old bug on the verge of being fixed, with plain Mutex.h header files and 'namespace mozilla {...}' even... *snif*.

Huzzah!

/be

Comment 44

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #43)
> I have a tear in my eye upon seeing this old bug on the verge of being fixed,
> with plain Mutex.h header files and 'namespace mozilla {...}' even... *snif*.
> 

The second time this bug has brought tears to someone's eyes!  Mozilla 2.0, ho!

Comment 45

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: pushable changeset

r=bsmedberg

Comment 46

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

After working on bug 456272, I've realized that the current deadlock detector, is, to put it nicely, crap.  (It's used in this patch.)  But that's a checkin for another day...

I don't know when I'll get commit access, so marking checkin-needed.

Comment 47

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

Pushed changeset 58c8f67ce6f6 to mozilla-central.

Comment 48

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Thanks, Ben!

Comment 49

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://developer.mozilla.org/En/XPCOM_Thread_Synchronization