Last Comment Bug 725552 - Add Cross-Process Mutex to ipc/glue
: Add Cross-Process Mutex to ipc/glue
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: unspecified
: x86_64 Windows 7
: -- normal (vote)
: mozilla13
Assigned To: Bas Schouten (:bas.schouten)
:
:
Mentors:
Depends on:
Blocks: 651192
  Show dependency treegraph
 
Reported: 2012-02-08 18:50 PST by Bas Schouten (:bas.schouten)
Modified: 2012-03-02 06:10 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Turn MutexAutoLock into a template with specializations. (3.48 KB, patch)
2012-02-08 18:59 PST, Bas Schouten (:bas.schouten)
cjones.bugs: review+
Details | Diff | Splinter Review
Part 2: Add cross-process mutex code. (12.02 KB, patch)
2012-02-13 14:41 PST, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Part 2: Add cross-process mutex code. (12.29 KB, patch)
2012-02-14 09:26 PST, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Part 2: Add cross-process mutex code. v2 (12.41 KB, patch)
2012-02-15 20:50 PST, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Part 2: Add cross-process mutex code. v3 (12.41 KB, patch)
2012-02-15 21:54 PST, Bas Schouten (:bas.schouten)
cjones.bugs: review+
Details | Diff | Splinter Review

Description Bas Schouten (:bas.schouten) 2012-02-08 18:50:04 PST
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) 2012-02-08 18:59:09 PST
Created attachment 595614 [details] [diff] [review]
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 2012-02-08 22:30:11 PST
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) 2012-02-09 07:22:46 PST
Alright! I actually just included it because it was there in the original AutoLock code.
Comment 4 Bas Schouten (:bas.schouten) 2012-02-13 14:41:07 PST
Created attachment 596794 [details] [diff] [review]
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) 2012-02-13 14:41:29 PST
Altering bug name to reflect updated reality.
Comment 6 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-02-13 16:30:19 PST
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) 2012-02-13 23:16:56 PST
(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 2012-02-14 06:53:19 PST
(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) 2012-02-14 09:26:46 PST
Created attachment 597053 [details] [diff] [review]
Part 2: Add cross-process mutex code.
Comment 10 Bas Schouten (:bas.schouten) 2012-02-15 20:50:26 PST
Created attachment 597679 [details] [diff] [review]
Part 2: Add cross-process mutex code. v2

Updated.
Comment 11 Bas Schouten (:bas.schouten) 2012-02-15 21:54:22 PST
Created attachment 597687 [details] [diff] [review]
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 2012-02-16 00:15:33 PST
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.

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