725552 - Add Cross-Process Mutex to ipc/glue

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Bas Schouten (:bas.schouten)]

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.

Comment 1

[Bas Schouten (:bas.schouten)]

Attachment: Part 1: Turn MutexAutoLock into a template with specializations.

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.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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)

Comment 3

[Bas Schouten (:bas.schouten)]

Alright! I actually just included it because it was there in the original AutoLock code.

Comment 4

[Bas Schouten (:bas.schouten)]

Attachment: Part 2: Add cross-process mutex code.

This ended up being done in IPC/glue for dependency reasons, this patch seems to work for the cross-process mutex.

Comment 5

[Bas Schouten (:bas.schouten)]

Altering bug name to reflect updated reality.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 7

[Bas Schouten (:bas.schouten)]

(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.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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.

Comment 9

[Bas Schouten (:bas.schouten)]

Attachment: Part 2: Add cross-process mutex code.

Comment 10

[Bas Schouten (:bas.schouten)]

Attachment: Part 2: Add cross-process mutex code. v2

Updated.

Comment 11

[Bas Schouten (:bas.schouten)]

Attachment: Part 2: Add cross-process mutex code. v3

Corrected ProcessHandle namespace in unimplemented.cpp

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 13

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/12c2ab926f8e
https://hg.mozilla.org/mozilla-central/rev/54d4849c2c94