Closed Bug 725552 Opened 8 years ago Closed 8 years ago

Add Cross-Process Mutex to ipc/glue

Categories

(Core :: XPCOM, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: bas.schouten, Assigned: bas.schouten)

References

Details

Attachments

(2 files, 3 obsolete files)

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.
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+
Alright! I actually just included it because it was there in the original AutoLock code.
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)
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)
(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.
Attachment #596794 - Attachment is obsolete: true
Attachment #597053 - Flags: review?(jones.chris.g)
Updated.
Attachment #597053 - Attachment is obsolete: true
Attachment #597053 - Flags: review?(jones.chris.g)
Attachment #597679 - Flags: review?(jones.chris.g)
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+
https://hg.mozilla.org/mozilla-central/rev/12c2ab926f8e
https://hg.mozilla.org/mozilla-central/rev/54d4849c2c94
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.