323412 - Proxy object lock and service manager lock are intertwined

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Matthew Gertner]

nsProxyEventObject::GetNewOrUsedProxy locks the proxy map before instantiating a new proxy object (if necessary). The constructor of nsProxyEventObject gets the event queue service and assigns it to a member variable, which requires locking the service map in the service manager.

It's theoretically possible to lock the service map and then the proxy map, causing a possible deadlock condition (picked up by the deadlock detection code). I managed to make this happen by overloading the constructor of an XPCOM service to return a proxy to that service instead (I wanted to ensure that the service always runs on the UI thread, regardless of the calling thread). See the URL for the two stacks.

I can't think of other use cases offhand, but since there isn't any obvious need to lock the service map inside the proxy map lock, it makes sense to fix this.

Comment 1

[Matthew Gertner]

Attachment: Moves retrieval of the event queue service out of the proxy map lock

Comment 2

[Doug Turner (:dougt)]

Comment on attachment 208478 [details] [diff] [review]
Moves retrieval of the event queue service out of the proxy map lock

nice find.  r=me

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

checked in on trunk

Checking in xpcom/proxy/src/nsProxyEvent.cpp;
/cvsroot/mozilla/xpcom/proxy/src/nsProxyEvent.cpp,v  <--  nsProxyEvent.cpp
new revision: 1.80; previous revision: 1.79
done
Checking in xpcom/proxy/src/nsProxyEventObject.cpp;
/cvsroot/mozilla/xpcom/proxy/src/nsProxyEventObject.cpp,v  <--  nsProxyEventObject.cpp
new revision: 1.50; previous revision: 1.49
done
Checking in xpcom/proxy/src/nsProxyEventPrivate.h;
/cvsroot/mozilla/xpcom/proxy/src/nsProxyEventPrivate.h,v  <--  nsProxyEventPrivate.h
new revision: 1.40; previous revision: 1.39
done

Comment 4

[Matthew Gertner]

Attachment: Additional patch for nsProxyEvent.h (missing from original)

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

that's now also checked in:
Checking in xpcom/proxy/public/nsProxyEvent.h;
/cvsroot/mozilla/xpcom/proxy/public/nsProxyEvent.h,v  <--  nsProxyEvent.h
new revision: 1.35; previous revision: 1.34
done

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

checked in on MOZILLA_1_8_BRANCH, including bustage fix
Checking in xpcom/proxy/public/nsProxyEvent.h;
/cvsroot/mozilla/xpcom/proxy/public/nsProxyEvent.h,v  <--  nsProxyEvent.h
new revision: 1.34.30.1; previous revision: 1.34
done
Checking in xpcom/proxy/src/nsProxyEvent.cpp;
/cvsroot/mozilla/xpcom/proxy/src/nsProxyEvent.cpp,v  <--  nsProxyEvent.cpp
new revision: 1.79.10.1; previous revision: 1.79
done
Checking in xpcom/proxy/src/nsProxyEventObject.cpp;
/cvsroot/mozilla/xpcom/proxy/src/nsProxyEventObject.cpp,v  <--  nsProxyEventObject.cpp
new revision: 1.49.30.1; previous revision: 1.49
done
Checking in xpcom/proxy/src/nsProxyEventPrivate.h;
/cvsroot/mozilla/xpcom/proxy/src/nsProxyEventPrivate.h,v  <--  nsProxyEventPrivate.h
new revision: 1.38.30.1; previous revision: 1.38
done

Comment 7

[Matthew Gertner]

Comment on attachment 208478 [details] [diff] [review]
Moves retrieval of the event queue service out of the proxy map lock

We seem to be actually running into this problem with FF 1.5.

Comment 8

[Daniel Veditz [:dveditz]]

Comment on attachment 208478 [details] [diff] [review]
Moves retrieval of the event queue service out of the proxy map lock

approved for 1.8.0 branch, a=dveditz for drivers

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 209965 [details] [diff] [review]
Additional patch for nsProxyEvent.h (missing from original)

approved for 1.8.0 branch, a=dveditz for drivers

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

Both patches checked in on MOZILLA_1_8_0_BRANCH

Checking in xpcom/proxy/public/nsProxyEvent.h;
/cvsroot/mozilla/xpcom/proxy/public/nsProxyEvent.h,v  <--  nsProxyEvent.h
new revision: 1.34.38.1; previous revision: 1.34
done
Checking in xpcom/proxy/src/nsProxyEvent.cpp;
/cvsroot/mozilla/xpcom/proxy/src/nsProxyEvent.cpp,v  <--  nsProxyEvent.cpp
new revision: 1.79.18.1; previous revision: 1.79
done
Checking in xpcom/proxy/src/nsProxyEventObject.cpp;
/cvsroot/mozilla/xpcom/proxy/src/nsProxyEventObject.cpp,v  <--  nsProxyEventObject.cpp
new revision: 1.49.38.1; previous revision: 1.49
done
Checking in xpcom/proxy/src/nsProxyEventPrivate.h;
/cvsroot/mozilla/xpcom/proxy/src/nsProxyEventPrivate.h,v  <--  nsProxyEventPrivate.h
new revision: 1.38.38.1; previous revision: 1.38
done

Comment 11

[Tracy Walker [:tracy]]

reporter please verify if this is fixed. Thanks

Comment 12

[Matthew Gertner]

Yes, it looks fine.

Comment 13

[Scott MacGregor]

We think this change caused 342784.

Comment 14

[Brendan Eich [:brendan]]

(In reply to comment #13)
> We think this change caused 342784.

How did this bug's patch cause bug 342784 ?  I didn't see any diagnosis here or in that bug.

/be

Comment 15

[Scott MacGregor]

Backing this out, and David and I still hang. It must be caused by something else that was checked into the 1.8.0 branch on the 15th of june.