Closed
Bug 725552
Opened 12 years ago
Closed 12 years ago
Add Cross-Process Mutex to ipc/glue
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
mozilla13
People
(Reporter: bas.schouten, Assigned: bas.schouten)
References
Details
Attachments
(2 files, 3 obsolete files)
3.48 KB,
patch
|
cjones
:
review+
|
Details | Diff | Splinter Review |
12.41 KB,
patch
|
cjones
:
review+
|
Details | Diff | Splinter Review |
For the new NPAPI Async drawing model we need a proper cross-process mutex implemented, at least on windows for now, and hopefully soon after that for other platforms as well.
Assignee | ||
Comment 1•12 years ago
|
||
This is the first part, it prepares us for easily sharing code when implementing CrossProcessAutoMutexLock. It removes a needless forward declaration that would otherwise become a little more complicated.
Assignee: nobody → bas.schouten
Attachment #595614 -
Flags: review?(jones.chris.g)
Comment on attachment 595614 [details] [diff] [review] Part 1: Turn MutexAutoLock into a template with specializations. >+typedef BaseAutoLock<mozilla::Mutex> MutexAutoLock; > mozilla:: qualification isn't needed here. >+typedef BaseAutoUnlock<mozilla::Mutex> MutexAutoUnlock; > (same)
Attachment #595614 -
Flags: review?(jones.chris.g) → review+
Assignee | ||
Comment 3•12 years ago
|
||
Alright! I actually just included it because it was there in the original AutoLock code.
Assignee | ||
Comment 4•12 years ago
|
||
This ended up being done in IPC/glue for dependency reasons, this patch seems to work for the cross-process mutex.
Attachment #596794 -
Flags: review?(jones.chris.g)
Assignee | ||
Comment 5•12 years ago
|
||
Altering bug name to reflect updated reality.
Summary: Add Cross-Process Mutex to XPCOM → Add Cross-Process Mutex to ipc/glue
Comment on attachment 596794 [details] [diff] [review] Part 2: Add cross-process mutex code. Use the new license header in new files you add. Keep the modelines. >diff --git a/ipc/glue/CrossProcessMutex.h b/ipc/glue/CrossProcessMutex.h >+// - CrossProcessMutex, a recursive mutex that can be shared across processes Recursive mutexes are bad ju-ju. Don't go there unless you absolutely have to. From what I understand of the usage of this class, you don't need a reentrant mutex. I know the underlying mutex object returned by windows is reentrant, but let's make the spec for CrossProcessMutex non-recursive. I won't ask you add checks for that since the POSIX impl will be non-reentrant. >+typedef HANDLE CrossProcessMutexHandle; >+ Nope! >+#ifdef XP_WIN >+ /* Handle is defined as void*, this prevents us from including windows.h */ >+ HANDLE mMutex; >+#endif Nope! >diff --git a/ipc/glue/CrossProcessMutex_windows.cpp b/ipc/glue/CrossProcessMutex_windows.cpp >+#include "CrossProcessMutex.h" >+ >+#include <windows.h> >+#include "nsTraceRefcnt.h" >+#include "nsDebug.h" >+#include "base/process_util.h" >+ I'm a bit anal about include order. Please use #include <windows.h> #include "base/process_util.h" #include "CrossProcessMutex.h" #include "nsDebug.h" #include "nsTraceRefcnt.h" >+CrossProcessMutex::CrossProcessMutex(const char*) >+{ >+ // We explicitly share this using DuplicateHandle, so no security >+ // attributes are given. And in fact, we *don't* want this to be inherited by child processes by default. So this is actually somewhat important. Please note that. >+ mMutex = ::CreateMutexA(NULL, FALSE, NULL); >+ NS_ASSERTION(mMutex, "This shouldn't happen - failed to create mutex!"); In Mutex.h, we RUNTIMEABORT() when we fail to create mutexes. There's really no way to recover from that kind of failure. Trying to do so is a good way to get epically pwned by a security exploit. >+ >+CrossProcessMutex::CrossProcessMutex(CrossProcessMutexHandle aHandle) >+{ >+ mMutex = aHandle; Is there some kind of validation we can do here that this is actually a valid cross-process mutex object with the right ACL? Mostly looks good. Would like to see a v2 that will compile on non-win32 and has fixes for above.
Attachment #596794 -
Flags: review?(jones.chris.g)
Assignee | ||
Comment 7•12 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #6) > Comment on attachment 596794 [details] [diff] [review] > Part 2: Add cross-process mutex code. > > Use the new license header in new files you add. Keep the modelines. > > >diff --git a/ipc/glue/CrossProcessMutex.h b/ipc/glue/CrossProcessMutex.h > > >+// - CrossProcessMutex, a recursive mutex that can be shared across processes > > Recursive mutexes are bad ju-ju. Don't go there unless you absolutely > have to. From what I understand of the usage of this class, you don't > need a reentrant mutex. > > I know the underlying mutex object returned by windows is reentrant, > but let's make the spec for CrossProcessMutex non-recursive. I won't > ask you add checks for that since the POSIX impl will be > non-reentrant. Sure, I kind of like reentrant(I prefer the higher code-reuse it allows by allowing functions holding the mutex to call a function that also grabs the mutex because it may still be called by functions not holding the mutex). But non-reentrant might be more widely supported, making it more attractive. You're also right that I do not have any need for a reentrant mutex at this time anyway. > > >+typedef HANDLE CrossProcessMutexHandle; > >+ > > Nope! You mean #ifdef this? :) > > >+#ifdef XP_WIN > >+ /* Handle is defined as void*, this prevents us from including windows.h */ > >+ HANDLE mMutex; > >+#endif > > Nope! You mean fix the comment? :) > > >+ > >+CrossProcessMutex::CrossProcessMutex(CrossProcessMutexHandle aHandle) > >+{ > >+ mMutex = aHandle; > > Is there some kind of validation we can do here that this is actually > a valid cross-process mutex object with the right ACL? We can do a call to GetHandleInformation? Do you want to #ifdef DEBUG it (or put it inside an NS_ASSERTION)? Or would you rather RUNTIMEABORT if fails. (And take the 'overhead' of the call.
(In reply to Bas Schouten (:bas) from comment #7) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #6) > > Comment on attachment 596794 [details] [diff] [review] > > > > >+typedef HANDLE CrossProcessMutexHandle; > > >+ > > > > Nope! > > You mean #ifdef this? :) > Yes, it needs to compile on non-windows. > > > > >+#ifdef XP_WIN > > >+ /* Handle is defined as void*, this prevents us from including windows.h */ > > >+ HANDLE mMutex; > > >+#endif > > > > Nope! > > You mean fix the comment? :) > Yep. > > > > >+ > > >+CrossProcessMutex::CrossProcessMutex(CrossProcessMutexHandle aHandle) > > >+{ > > >+ mMutex = aHandle; > > > > Is there some kind of validation we can do here that this is actually > > a valid cross-process mutex object with the right ACL? > > We can do a call to GetHandleInformation? Do you want to #ifdef DEBUG it (or > put it inside an NS_ASSERTION)? Or would you rather RUNTIMEABORT if fails. > (And take the 'overhead' of the call. That doesn't seem to allow us to check much except that the handle is valid. I guess that's OK. It should be very cheap, and this function shouldn't be called much, so we can do it always.
Assignee | ||
Comment 9•12 years ago
|
||
Attachment #596794 -
Attachment is obsolete: true
Attachment #597053 -
Flags: review?(jones.chris.g)
Assignee | ||
Comment 10•12 years ago
|
||
Updated.
Attachment #597053 -
Attachment is obsolete: true
Attachment #597053 -
Flags: review?(jones.chris.g)
Attachment #597679 -
Flags: review?(jones.chris.g)
Assignee | ||
Comment 11•12 years ago
|
||
Corrected ProcessHandle namespace in unimplemented.cpp
Attachment #597679 -
Attachment is obsolete: true
Attachment #597679 -
Flags: review?(jones.chris.g)
Attachment #597687 -
Flags: review?(jones.chris.g)
Comment on attachment 597687 [details] [diff] [review] Part 2: Add cross-process mutex code. v3 (In reply to Chris Jones [:cjones] [:warhammer] from comment #6) > Comment on attachment 596794 [details] [diff] [review] > Part 2: Add cross-process mutex code. > > Use the new license header in new files you add. Keep the modelines. > Didn't fix this. Otherwise looks ok. r=me with that.
Attachment #597687 -
Flags: review?(jones.chris.g) → review+
Comment 13•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/12c2ab926f8e https://hg.mozilla.org/mozilla-central/rev/54d4849c2c94
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in
before you can comment on or make changes to this bug.
Description
•