Proxy object lock and service manager lock are intertwined

RESOLVED FIXED

Status

()

Core
XPCOM
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Matthew Gertner, Assigned: Matthew Gertner)

Tracking

({fixed1.8.0.5, fixed1.8.1})

Trunk
All
Windows XP
fixed1.8.0.5, fixed1.8.1
Points:
---
Bug Flags:
blocking1.8.1 ?

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments)

(Assignee)

Description

11 years ago
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.
(Assignee)

Comment 1

11 years ago
Created attachment 208478 [details] [diff] [review]
Moves retrieval of the event queue service out of the proxy map lock
Attachment #208478 - Flags: review?
(Assignee)

Updated

11 years ago
Attachment #208478 - Flags: review? → review?(dougt)

Comment 2

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

nice find.  r=me
Attachment #208478 - Flags: review?(dougt) → review+
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
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Updated

11 years ago
Flags: blocking1.8.1?
Assignee: dougt → matthew
(Assignee)

Updated

11 years ago
Attachment #208478 - Flags: approval1.8.1?
(Assignee)

Comment 4

11 years ago
Created attachment 209965 [details] [diff] [review]
Additional patch for nsProxyEvent.h (missing from original)
Attachment #209965 - Flags: approval1.8.1?
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
Attachment #208478 - Flags: approval1.8.1? → branch-1.8.1?(darin)
Attachment #209965 - Flags: approval1.8.1?

Updated

11 years ago
Attachment #208478 - Flags: branch-1.8.1?(darin) → branch-1.8.1+
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
Keywords: fixed1.8.1
(Assignee)

Comment 7

11 years ago
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.
Attachment #208478 - Flags: approval1.8.0.5?
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
Attachment #208478 - Flags: approval1.8.0.5? → approval1.8.0.5+
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
Attachment #209965 - Flags: approval1.8.0.5+
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
Keywords: fixed1.8.0.5
reporter please verify if this is fixed. Thanks
(Assignee)

Comment 12

11 years ago
Yes, it looks fine.

Comment 13

11 years ago
We think this change caused 342784.
Depends on: 342784
(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

11 years ago
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. 
You need to log in before you can comment on or make changes to this bug.