493080 - Rewrite: Upgrade XPCOM to new synchronization API

Status: RESOLVED DUPLICATE of bug 645263

(Core :: XPCOM, defect)

Comment 1

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

Attachment: Backup

Testing on try.  I decided to punt on rewriting xptinfo, because it used nsAutoLock.lock/unlock extensively, which means it needs manual rewrite, and I don't feel comfortable doing that.

Comment 2

[Benjamin Smedberg]

Comment on attachment 377554 [details] [diff] [review]
Backup

>diff --git a/xpcom/base/nsConsoleService.h b/xpcom/base/nsConsoleService.h

>     // To serialize interesting methods.
>-    PRLock *mLock;
>+    mozilla::Mutex * mLock;

Now that lock creation is infallible, this should be a direct member, not a pointer.

>diff --git a/xpcom/base/nsExceptionService.cpp b/xpcom/base/nsExceptionService.cpp

> PRUintn nsExceptionService::tlsIndex = BAD_TLS_INDEX;
>-PRLock *nsExceptionService::lock = nsnull;
>+Mutex * nsExceptionService::lock = nsnull;

In this and other places, you are introducing an extra space. Any reason why? I'd prefer to avoid it.

>diff --git a/xpcom/base/nsExceptionService.h b/xpcom/base/nsExceptionService.h


>   /* single lock protects both providers hashtable
>      and thread list */
>-  static PRLock* lock;
>+  static mozilla::Mutex* lock;

roc typedefs to avoid explicit class scopes throughout (see m.d.platform):

typedef mozilla::Mutex Mutex;
static Mutex* lock;

>diff --git a/xpcom/base/nsUUIDGenerator.h b/xpcom/base/nsUUIDGenerator.h

> class nsUUIDGenerator : public nsIUUIDGenerator {

>-    PRLock* mLock;
>+    mozilla::Mutex* mLock;

This should be a direct member as well.


>diff --git a/xpcom/io/nsFastLoadService.cpp b/xpcom/io/nsFastLoadService.cpp

>     if (mLock)
>-        PR_DestroyLock(mLock);
>+        delete mLock;

ditch the `if`, delete is null-safe.

r=me with those changes, or feel free to ask me to review a followup patch if you're unsure about something.

Comment 3

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

(In reply to comment #2)
> (From update of attachment 377554 [details] [diff] [review])
> >diff --git a/xpcom/base/nsConsoleService.h b/xpcom/base/nsConsoleService.h
> 
> >     // To serialize interesting methods.
> >-    PRLock *mLock;
> >+    mozilla::Mutex * mLock;
> 
> Now that lock creation is infallible, this should be a direct member, not a
> pointer.
> 

No problem.  I want humans to tell me when this won't break things.

> >diff --git a/xpcom/base/nsExceptionService.cpp b/xpcom/base/nsExceptionService.cpp
> 
> > PRUintn nsExceptionService::tlsIndex = BAD_TLS_INDEX;
> >-PRLock *nsExceptionService::lock = nsnull;
> >+Mutex * nsExceptionService::lock = nsnull;
> 
> In this and other places, you are introducing an extra space. Any reason why?
> I'd prefer to avoid it.
> 

Just a side effect of using an automated rewrite tool.  I didn't notice this.  BTW, this is caused by clashing pointer styles: the old code used |Foo *f|, and I was trying to rewrite it with the style |Bar* b|.

> >diff --git a/xpcom/base/nsUUIDGenerator.h b/xpcom/base/nsUUIDGenerator.h
> 
> > class nsUUIDGenerator : public nsIUUIDGenerator {
> 
> >-    PRLock* mLock;
> >+    mozilla::Mutex* mLock;
> 
> This should be a direct member as well.
> 

OK.

Comment 4

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

Attachment: v2, comments addressed

Comment 5

[Josh Matthews [:jdm]]

With a straight-up rebased version (plus massaging to get it to build) of the most recent patch, I hit a segfault in plugin-container on shutdown with --enable-trace-malloc turned on.  The final NS_LogTerm ends up triggering BlockingResourceBase::Shutdown which cleans up the deadlock detector, and is followed by a call to nsTraceRefcntImpl::ResetStatistics which then locks the nsTraceRefcntImpl mutex.  Yay.

Comment 6

[Josh Matthews [:jdm]]

And of course I get it in the main firefox process as well.

Comment 7

[Josh Matthews [:jdm]]

In fact, this is worse than I thought.  The interaction with --enable-trace-malloc is completely broken right now, and in an ironic twist the deadlock detector ends up causing deadlocks.  This stack tells all:

#3  0x008bcf23 in pthread_mutex_lock () from /lib/libpthread.so.0
#4  0x0059cb04 in PR_Lock (lock=0x98dd810) at /home/t_mattjo/src/firefox/mozilla-central/nsprpub/pr/src/pthreads/ptsynch.c:206
#5  0x0210b151 in mozilla::DeadlockDetector<mozilla::BlockingResourceBase::DeadlockDetectorEntry>::PRAutoLock::PRAutoLock (this=0xbfe8ea64, aLock=0x98dd810) at ../../dist/include/mozilla/DeadlockDetector.h:343
#6  0x0210ace0 in mozilla::DeadlockDetector<mozilla::BlockingResourceBase::DeadlockDetectorEntry>::CheckAcquisition (this=0x98dd6d0, aLast=0x0, aProposed=0x98dd870, aCallContext=...)
    at ../../dist/include/mozilla/DeadlockDetector.h:433
#7  0x02109d81 in mozilla::BlockingResourceBase::CheckAcquire (this=0x98dd6c0, aCallContext=...) at BlockingResourceBase.cpp:143
#8  0x0210a40c in mozilla::Mutex::Lock (this=0x98dd6c0) at BlockingResourceBase.cpp:263
#9  0x0218c96e in NS_LogCtor_P (aPtr=0x997f7e4, aType=0x2865309 "nsTArray_base", aInstanceSize=4) at /home/t_mattjo/src/firefox/mozilla-central/xpcom/base/nsTraceRefcntImpl.cpp:1122
#10 0x00c0cb1f in nsTArray_base<nsTArrayDefaultAllocator>::nsTArray_base (this=0x997f7e4) at ../../../../dist/include/nsTArray-inl.h:46
#11 0x0210b79b in nsTArray<PLHashEntry*, nsTArrayDefaultAllocator>::nsTArray (this=0x997f7e4) at ../../dist/include/nsTArray.h:379
#12 0x0210b501 in mozilla::DeadlockDetector<mozilla::BlockingResourceBase::DeadlockDetectorEntry>::OrderingEntry::OrderingEntry (this=0x997f7e0) at ../../dist/include/mozilla/DeadlockDetector.h:226
#13 0x0210b1b5 in mozilla::DeadlockDetector<mozilla::BlockingResourceBase::DeadlockDetectorEntry>::PutEntry (this=0x98dd6d0, aKey=0x997f7d0) at ../../dist/include/mozilla/DeadlockDetector.h:272
#14 0x0210ac73 in mozilla::DeadlockDetector<mozilla::BlockingResourceBase::DeadlockDetectorEntry>::Add (this=0x98dd6d0, aResource=0x997f7d0) at ../../dist/include/mozilla/DeadlockDetector.h:396
#15 0x02109cbb in mozilla::BlockingResourceBase::BlockingResourceBase (this=0x997bbb8, aName=0x2d177a0 "nsThreadManager.mLock", aType=eMutex) at BlockingResourceBase.cpp:112
#16 0x00c3d372 in mozilla::Mutex::Mutex (this=0x997bbb8, name=0x2d177a0 "nsThreadManager.mLock") at ../../../dist/include/mozilla/Mutex.h:78
#17 0x0217ae69 in nsThreadManager::Init (this=0x2fc6b00) at /home/t_mattjo/src/firefox/mozilla-central/xpcom/threads/nsThreadManager.cpp:93
#18 0x02111bf1 in NS_InitXPCOM2_P (result=0xbfe8f0f4, binDirectory=0x98f19d0, appFileLocationProvider=0xbfe8f094) at /home/t_mattjo/src/firefox/mozilla-central/xpcom/build/nsXPComInit.cpp:408
#19 0x00c18370 in ScopedXPCOMStartup::Initialize (this=0xbfe8f0f4) at /home/t_mattjo/src/firefox/mozilla-central/toolkit/xre/nsAppRunner.cpp:1186
#20 0x00c1f1ef in XRE_main (argc=5, argv=0xbfe8f414, aAppData=0x98efd58) at /home/t_mattjo/src/firefox/mozilla-central/toolkit/xre/nsAppRunner.cpp:3546
#21 0x08048df7 in main (argc=6, argv=0xbfe8f414) at /home/t_mattjo/src/firefox/mozilla-central/browser/app/nsBrowserApp.cpp:158

The DeadlockDetector::Add call locks the deadlock detector lock, which DeadlockDetector::CheckAcquisition attempts to use later.  Bleah.

Comment 8

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

It's not 100% clear to me what's happening in that stack; my eyes are glazing over somewhat.  Is nsTraceRefcntImpl using a mozilla::Mutex?  If so, that's bad juju, needs to not do that.

Comment 9

[Josh Matthews [:jdm]]

Yeah, I changed nsTraceRefcntImpl to use a PR_Lock and everything Just Worked.