Last Comment Bug 323412 - Proxy object lock and service manager lock are intertwined
: Proxy object lock and service manager lock are intertwined
Status: RESOLVED FIXED
: fixed1.8.0.5, fixed1.8.1
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All Windows XP
: -- normal (vote)
: ---
Assigned To: Matthew Gertner
:
Mentors:
http://plasticmillion.pastebin.com/48...
Depends on: 342784
Blocks:
  Show dependency treegraph
 
Reported: 2006-01-14 07:17 PST by Matthew Gertner
Modified: 2006-07-05 14:15 PDT (History)
6 users (show)
matthew.gertner: blocking1.8.1?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Moves retrieval of the event queue service out of the proxy map lock (12.38 KB, patch)
2006-01-14 07:18 PST, Matthew Gertner
dougt: review+
darin.moz: approval‑branch‑1.8.1+
dveditz: approval1.8.0.5+
Details | Diff | Splinter Review
Additional patch for nsProxyEvent.h (missing from original) (1.72 KB, patch)
2006-01-28 08:23 PST, Matthew Gertner
dveditz: approval1.8.0.5+
Details | Diff | Splinter Review

Description Matthew Gertner 2006-01-14 07:17:21 PST
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 2006-01-14 07:18:25 PST
Created attachment 208478 [details] [diff] [review]
Moves retrieval of the event queue service out of the proxy map lock
Comment 2 Doug Turner (:dougt) 2006-01-14 15:30:56 PST
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) 2006-01-28 07:46:39 PST
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 2006-01-28 08:23:38 PST
Created attachment 209965 [details] [diff] [review]
Additional patch for nsProxyEvent.h (missing from original)
Comment 5 Christian :Biesinger (don't email me, ping me on IRC) 2006-01-28 08:25:55 PST
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) 2006-02-01 07:07:55 PST
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 2006-06-13 09:39:04 PDT
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] 2006-06-14 15:02:06 PDT
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] 2006-06-14 15:02:13 PDT
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) 2006-06-15 06:59:40 PDT
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] 2006-06-20 14:57:40 PDT
reporter please verify if this is fixed. Thanks
Comment 12 Matthew Gertner 2006-06-23 07:32:01 PDT
Yes, it looks fine.
Comment 13 Scott MacGregor 2006-07-05 13:17:01 PDT
We think this change caused 342784.
Comment 14 Brendan Eich [:brendan] 2006-07-05 13:36:21 PDT
(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 2006-07-05 14:15:01 PDT
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. 

Note You need to log in before you can comment on or make changes to this bug.