XPCOM should have nsLock strong type to match nsAutoLock

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Core
XPCOM
P3
normal
RESOLVED FIXED
18 years ago
9 years ago

People

(Reporter: brendan, Assigned: cjones)

Tracking

(Depends on: 1 bug, {dev-doc-complete})

Trunk
mozilla1.9.2a1
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 11 obsolete attachments)

57.76 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

18 years ago
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
(Reporter)

Updated

18 years ago
Status: NEW → ASSIGNED
Keywords: mozilla1.0
QA Contact: rayw → kandrot
Target Milestone: --- → mozilla0.9

Comment 1

17 years ago
I agree... willing to do whatever leg work I can in necko to make the shift.
(Reporter)

Comment 2

17 years ago
Sorry.
Target Milestone: mozilla0.9 → mozilla0.9.1
(Reporter)

Comment 3

17 years ago
Load-balancing.

/be
Target Milestone: mozilla0.9.1 → mozilla0.9.2

Updated

17 years ago
Target Milestone: mozilla0.9.2 → mozilla0.9.3
(Reporter)

Comment 4

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

/be
Target Milestone: mozilla0.9.3 → mozilla0.9.5
(Reporter)

Updated

17 years ago
Target Milestone: mozilla0.9.5 → mozilla0.9.6

Comment 5

17 years ago
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? :-)
(Reporter)

Comment 6

17 years ago
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

17 years ago
> 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?
(Reporter)

Comment 8

17 years ago
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

17 years ago
sorry to greate your cheese, john.  no plans to anything about it, just being a
monday morning qb.
(Reporter)

Comment 10

17 years ago
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
Target Milestone: mozilla0.9.6 → mozilla1.0.1
(Reporter)

Updated

16 years ago
Target Milestone: mozilla1.0.1 → mozilla1.1
(Reporter)

Updated

16 years ago
Keywords: mozilla1.0 → mozilla1.1
(Reporter)

Comment 11

16 years ago
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
Keywords: mozilla1.1 → mozilla1.2
Target Milestone: mozilla1.1alpha → mozilla1.2alpha
(Reporter)

Comment 12

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

/be
Target Milestone: mozilla1.2alpha → mozilla1.2beta
(Reporter)

Comment 13

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

/be
Keywords: mozilla1.2
Target Milestone: mozilla1.2beta → mozilla1.3alpha

Comment 14

16 years ago
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.  
(Reporter)

Comment 15

15 years ago
Fixing TM.

/be
Target Milestone: mozilla1.3alpha → mozilla1.3beta
(Reporter)

Updated

15 years ago
Target Milestone: mozilla1.3beta → mozilla1.5alpha
(Reporter)

Updated

15 years ago
Target Milestone: mozilla1.5alpha → mozilla1.6alpha
(Reporter)

Comment 16

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

/be
Target Milestone: mozilla1.6alpha → Future

Updated

12 years ago
QA Contact: kandrot → nobody
QA Contact: nobody → xpcom

Comment 17

12 years ago
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.
(Reporter)

Comment 18

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

/be
Assignee: brendan → nobody
Status: ASSIGNED → NEW
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.
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

9 years ago
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.
> 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.
(Reporter)

Comment 23

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

/be
Created attachment 367692 [details] [diff] [review]
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.)
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.
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.
Assignee: nobody → jones.chris.g
Created attachment 370357 [details] [diff] [review]
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.
Attachment #367692 - Attachment is obsolete: true
Attachment #370357 - Flags: review?
Attachment #370357 - Flags: review? → review?(benjamin)
Created attachment 370585 [details] [diff] [review]
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.
Attachment #370357 - Attachment is obsolete: true
Attachment #370357 - Flags: review?(benjamin)
Created attachment 370716 [details] [diff] [review]
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.
Attachment #370585 - Attachment is obsolete: true
Created attachment 370744 [details] [diff] [review]
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
Attachment #370716 - Attachment is obsolete: true
Attachment #370744 - Flags: review?(benjamin)
No longer blocks: 456272
Created attachment 371556 [details] [diff] [review]
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.
Attachment #370744 - Attachment is obsolete: true
Attachment #371556 - Flags: review?(benjamin)
Attachment #370744 - Flags: review?(benjamin)

Comment 32

9 years ago
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?
Attachment #371556 - Flags: review?(benjamin) → review-
(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?
> > 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.
Created attachment 372120 [details] [diff] [review]
Sync API, now with non-heap allocation!

Oh, being able to abort on failed malloc brings tears of joy to my eyes.
Attachment #371556 - Attachment is obsolete: true
Attachment #372120 - Flags: review?(benjamin)
Created attachment 372122 [details] [diff] [review]
 Sync API, now with non-heap allocation!   And no emacs hacks!

Crap, missed an emacs namespace hack.
Attachment #372120 - Attachment is obsolete: true
Attachment #372122 - Flags: review?(benjamin)
Attachment #372120 - Flags: review?(benjamin)

Updated

9 years ago
Attachment #372122 - Attachment is patch: true
Attachment #372122 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 372122 [details] [diff] [review]
 Sync API, now with non-heap allocation!   And no emacs hacks!

Adding a vim modeline.
Attachment #372122 - Attachment is obsolete: true
Attachment #372122 - Flags: review?(benjamin)
Created attachment 372129 [details] [diff] [review]
Sync API, now with non-heap allocation! And no emacs hacks!  And a vim modeline!

Benjamin, sorry for all the spam.
Attachment #372129 - Flags: review?(benjamin)

Updated

9 years ago
Attachment #372129 - Flags: review?(benjamin) → review+

Comment 39

9 years ago
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.
Created attachment 372522 [details] [diff] [review]
Updated sync api

Waiting for review/checkin until it passes tryserver.
Attachment #372129 - Attachment is obsolete: true
Created attachment 372543 [details] [diff] [review]
Last patch, rebased

Tryserver-ing again before review.
Attachment #372522 - Attachment is obsolete: true
(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.
Attachment #372543 - Flags: review?(benjamin)

Updated

9 years ago
Attachment #372543 - Flags: review?(benjamin) → review+
(Reporter)

Comment 43

9 years ago
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
(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!
Created attachment 373538 [details] [diff] [review]
pushable changeset

r=bsmedberg
Attachment #372543 - Attachment is obsolete: true
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.
Whiteboard: checkin-needed
Keywords: checkin-needed
Whiteboard: checkin-needed
Pushed changeset 58c8f67ce6f6 to mozilla-central.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: Future → mozilla1.9.2a1
Keywords: checkin-needed
Depends on: 489135

Updated

9 years ago
Keywords: dev-doc-needed
Depends on: 489239

Updated

9 years ago
Blocks: 488148

Updated

9 years ago
Keywords: dev-doc-complete

Updated

9 years ago
Blocks: 491462

Updated

9 years ago
Depends on: 491977
You need to log in before you can comment on or make changes to this bug.