323195 - XPCAutoUnlock messes with detection of potential deadlocks

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Matthew Gertner]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5

The deadlock detection code in nsAutoLock depends on the setting of thread-local data to keep track of the current chain of locks. The correct management of the thread-local data can be assured by deriving from nsAutoLockBase, which does the right thing in its constructor and destructor. But XPCAutoUnlock derives from the class although it should have the reverse behavior with respect to thread-local data. The result is that spurious "potential deadlock" warnings occur where XPCAutoUnlock is used (e.g. XPCNativeWrapper::InitTearOff).

Reproducible: Always

Comment 1

[Matthew Gertner]

Attachment: Creates an nsAutoUnlockBase class and derives XPCAutoUnlock from it

Created a new base class for "unlocking" classes with correct management for thread-local data.

Comment 2

[Brendan Eich [:brendan]]

Matthew, I gave you canconfirm and editbugs bugzilla privs.  Thanks for finding and patching this.

/be

Comment 3

[Matthew Gertner]

Thanks, Brendan. BTW: I should have mentioned that I don't think will work unless the XPCAutoUnlock occurs directly under the relevant XPCAutoLock. In the case of

XPCAutoLock lock1(foo);
XPCAutoLock lock2(bar);
XPCAutoLock unlock(foo);

...it is bar that will be unlocked as far as the deadlock detection code is concerned. I could fix this by walking up the chain and finding the matching lock, removing it from the chain (remembering where it was) and then inserting it back in in the nsAutoUnlockBase destructor. If you can confirm this analysis, I'll implement a more complex version that handles the general case.

Also, I'm assuming that if I implement this more complex version, I still don't have to run the deadlock detection code (from the nsAutoLockBase constructor) in the nsAutoUnlockBase destructor because in all cases the relevant stack trace will already have been handled. I'm not sure though. What do you think?

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 208284 [details] [diff] [review]
Creates an nsAutoUnlockBase class and derives XPCAutoUnlock from it

>+nsAutoUnlockBase::nsAutoUnlockBase(void* addr)
>+{
>+   mAddr = addr;
>+   if (mAddr)
>+   {

Nit: local style I've asserted (file-local) puts left-brace for compound statement at end of control-structure first line.

>+       mUp = (nsAutoLockBase *) PR_GetThreadPrivate(LockStackTPI);

Might mTop or mSaveTop be a better name than mUp?  This isn't a request to change anything unless you happen to agree ;-).

>+/**
>+* nsAutoUnlockBase
>+* This is the base class for the stack-based unlocking objects.
>+**/

Nit: asterisks stack under one another -- that's prevailing style here, in doc-comments in Java, and in lots of C code.

/be

Comment 5

[Brendan Eich [:brendan]]

Might it be better to make XPCAutoUnlock not use its base ctor and dtor, rather just use nsAutoLockBase::{Hide,Show}?  Those work independent of nesting order.

/be

Comment 6

[Matthew Gertner]

(In reply to comment #5)
> Might it be better to make XPCAutoUnlock not use its base ctor and dtor, rather
> just use nsAutoLockBase::{Hide,Show}?  Those work independent of nesting order.

That makes sense. So I'm thinking I'll create a common base class for nsAutoLockBase and nsAutoUnlockBase and move the Hide and Show methods there. My biggest problem is what to call the class. The best I've come up with is nsAutoLockUnlockBase. If no one has a better idea (or disagrees with the idea of a common base class), I'll make the change and post a new patch.

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 208284 [details] [diff] [review]
Creates an nsAutoUnlockBase class and derives XPCAutoUnlock from it

Clearing to get off my queue -- feel free to request review for the new patch when it's ready.  Thanks,

/be

Comment 8

[Matthew Gertner]

Attachment: New patch that uses nsAutoLockBase::Show() and Hide()

It turns out that you don't need a common base class for nsAutoLockBase and nsAutoUnlockBase.

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 209694 [details] [diff] [review]
New patch that uses nsAutoLockBase::Show() and Hide()

Looks good to me -- if it tests for you, let me know and I'll check it in.

/be

Comment 10

[Matthew Gertner]

Seems to work fine, as far as I can see.

Comment 11

[Matthew Gertner]

Any chance to get this checked in?

Comment 12

[Mike Schroepfer]

What are the risks/rewards to getting this on the 1.8 branch?

Comment 13

[Brendan Eich [:brendan]]

Schrep, it's all #ifdef DEBUG deadlock detection code, evolved from what I wrote a long time ago.  It has been productive -- see bug 338069 and bug 240747 to name two such bugs.  Having it on the branch won't hurd any release build, and could help debugging.

/be

Comment 14

[Mike Schroepfer]

Comment on attachment 209694 [details] [diff] [review]
New patch that uses nsAutoLockBase::Show() and Hide()

Just to confirm - it is intended that this is not all in #ifdef DEBUG code?

Comment 15

[Matthew Gertner]

Yes, the nsAutoUnlockBase definition has to exist in release mode as well, but the implementation does nothing unless DEBUG is #defined.

Comment 16

[Martijn Wargers (dead)]

Checking in xpcom/threads/nsAutoLock.cpp;
/cvsroot/mozilla/xpcom/threads/nsAutoLock.cpp,v  <--  nsAutoLock.cpp
new revision: 1.17; previous revision: 1.16
done
Checking in xpcom/threads/nsAutoLock.h;
/cvsroot/mozilla/xpcom/threads/nsAutoLock.h,v  <--  nsAutoLock.h
new revision: 1.29; previous revision: 1.28
done
Checking in js/src/xpconnect/src/xpcprivate.h;
/cvsroot/mozilla/js/src/xpconnect/src/xpcprivate.h,v  <--  xpcprivate.h
new revision: 1.192; previous revision: 1.191
done

Checked in on trunk.

Comment 17

[Martijn Wargers (dead)]

Checking in xpcom/threads/nsAutoLock.cpp;
/cvsroot/mozilla/xpcom/threads/nsAutoLock.cpp,v  <--  nsAutoLock.cpp
new revision: 1.16.20.1; previous revision: 1.16
done
Checking in xpcom/threads/nsAutoLock.h;
/cvsroot/mozilla/xpcom/threads/nsAutoLock.h,v  <--  nsAutoLock.h
new revision: 1.28.28.1; previous revision: 1.28
done
Checking in js/src/xpconnect/src/xpcprivate.h;
/cvsroot/mozilla/js/src/xpconnect/src/xpcprivate.h,v  <--  xpcprivate.h
new revision: 1.162.2.14; previous revision: 1.162.2.13
done

Checked in on 1.8.1 branch.