Closed Bug 1072093 Opened 5 years ago Closed 5 years ago

Enable the CrossProcessMutex code for use on OS X

Categories

(Core :: IPC, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(2 files, 2 obsolete files)

The CrossProcessMutex is used in the APZ/tiling code for implementing the progressive tiling behavior. Now that we have tiling enabled on OS X by default it would be nice to be able to turn on progressive tiling at the same time as APZ. To do this we need to make sure we use a working version of CrossProcessMutex on OS X, rather than the _unimplemented version we're using now.
A local build with this seems to work fine. Try push at https://tbpl.mozilla.org/?tree=Try&rev=e73bfc8edaf7

Will request review if that goes green.
Attached patch Part 2 - Fiddle with prefs (obsolete) — Splinter Review
This should be good assuming part 1 goes well (which I expect it to).
Attachment #8494241 - Flags: review?(rbarker)
Attachment #8494239 - Attachment description: Use CrossProcessMutex_posix on OS X → Part 1 - Use CrossProcessMutex_posix on OS X
Technically for non b2g/e10s we only need a cross-thread (regular) Mutex but doing this can't hurt.
Comment on attachment 8494239 [details] [diff] [review]
Part 1 - Use CrossProcessMutex_posix on OS X

Try looks good!
Attachment #8494239 - Flags: review?(rbarker)
Attachment #8494239 - Flags: review?(benjamin)
Comment on attachment 8494239 [details] [diff] [review]
Part 1 - Use CrossProcessMutex_posix on OS X

Knowing nothing about CrossProcessMutex other than I wish it didn't exist because it represents the possibility of the chrome process deadlocking on some other process, I'm going to punt this to a mac expert.
Attachment #8494239 - Flags: review?(benjamin) → review?(smichaud)
Comment on attachment 8494239 [details] [diff] [review]
Part 1 - Use CrossProcessMutex_posix on OS X

Review of attachment 8494239 [details] [diff] [review]:
-----------------------------------------------------------------

The reason cross process mutexes can not be enabled for MacOS X is that pthread_mutexattr_setpshared is not supported on the platform. Maybe it would be better to have a Mac version that only returns the mutex handle and not a handle to shared memory containing the mutex handle, that way it should work cross thread but would fail if someone tried to use it cross process on a Mac? Maybe add some comments in the header stating it won't actually work cross process on a Mac?
Attachment #8494239 - Flags: review?(rbarker) → review-
Attachment #8494241 - Flags: review?(rbarker) → review+
Comment on attachment 8494239 [details] [diff] [review]
Part 1 - Use CrossProcessMutex_posix on OS X

I know almost nothing about cross-process mutexes, and I'm currently very busy with something else.  Maybe sometime next week?

One thing, though:

pthread_mutexattr_setpshared *does* seem to be supported on OS X, though it's undocumented there (and doesn't have a man page).  On OS X 10.9 (at least), I see an implementation in /usr/lib/system/libsystem_pthread.dylib.

Once I have more time, I can check for its presence on other versions of OS X, and see how widely it's used.  It's entirely possible that "Apple's own stuff" relies on it, but that they don't want to officially support it for "outsiders".  If so, it certainly won't be the first time that's happened :-(
(In reply to Steven Michaud from comment #7)
> Comment on attachment 8494239 [details] [diff] [review]
> Part 1 - Use CrossProcessMutex_posix on OS X
> 
> I know almost nothing about cross-process mutexes, and I'm currently very
> busy with something else.  Maybe sometime next week?
> 
> One thing, though:
> 
> pthread_mutexattr_setpshared *does* seem to be supported on OS X, though
> it's undocumented there (and doesn't have a man page).  On OS X 10.9 (at
> least), I see an implementation in /usr/lib/system/libsystem_pthread.dylib.
> 
> Once I have more time, I can check for its presence on other versions of OS
> X, and see how widely it's used.  It's entirely possible that "Apple's own
> stuff" relies on it, but that they don't want to officially support it for
> "outsiders".  If so, it certainly won't be the first time that's happened :-(

My understanding is pthread_mutexattr_setpshared may have a symbol, but isn't actually implemented. On the man page for pthread_rwlockattr_setpshared it specifically states that: "The PTHREAD_PROCESS_SHARED attribute is not supported."

https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPages/man3/pthread_rwlockattr_setpshared.3.html
The pthread_mutexattr_setpshared implementation I looked at (in assembly code) didn't look like a no-op.  And this wouldn't be the first time Apple has "deprecated" something they themselves rely heavily on.

I'll still try to find time to check whether Apple's stuff does use this method.

But in any case, it will be more difficult to use pthread_mutexattr_setpshared, even if it *is* fully supported, as long as Apple doesn't document exactly how it should be used.  For example, Apple's implementation may have Apple-specific quirks.
(In reply to Steven Michaud from comment #9)
> The pthread_mutexattr_setpshared implementation I looked at (in assembly
> code) didn't look like a no-op.  And this wouldn't be the first time Apple
> has "deprecated" something they themselves rely heavily on.
> 
> I'll still try to find time to check whether Apple's stuff does use this
> method.
> 
> But in any case, it will be more difficult to use
> pthread_mutexattr_setpshared, even if it *is* fully supported, as long as
> Apple doesn't document exactly how it should be used.  For example, Apple's
> implementation may have Apple-specific quirks.

Here is the source for 10.8 (looks like some one forgot to include system_pthread in the 10.9 source drops)
http://www.opensource.apple.com/source/Libc/Libc-825.40.1/pthreads/pthread_mutex.c

/*
 * Temp: till pshared is fixed correctly
 */
int
pthread_mutexattr_setpshared(pthread_mutexattr_t *attr, int pshared)
{
#if __DARWIN_UNIX03
	if (__unix_conforming == 0)
		__unix_conforming = 1;
#endif /* __DARWIN_UNIX03 */

        if (attr->sig == _PTHREAD_MUTEX_ATTR_SIG)
        {
#if __DARWIN_UNIX03
                if (( pshared == PTHREAD_PROCESS_PRIVATE) || (pshared == PTHREAD_PROCESS_SHARED))
#else /* __DARWIN_UNIX03 */
                if ( pshared == PTHREAD_PROCESS_PRIVATE)
#endif /* __DARWIN_UNIX03 */
	  	{
                         attr->pshared = pshared; 
                        return (0);
                } else {
                        return (EINVAL); /* Invalid parameter */
                }
        } else
        {
                return (EINVAL); /* Not an initialized 'attribute' structure */
        }
}

That is the only occurrence of PTHREAD_PROCESS_SHARED in the entire file. So it could be implemented in 10.9 but does not appear to be as of 10.8.
I grepped in the /System/Libraries directory on OS X 10.8.5, and found the following two apps that call pthread_mutexattr_setpshared (imported from /usr/lib/libsystem_c.dylib).  (They actually contain methods that call pthread_mutexattr_setpshared.)

/System/Library/Frameworks/OpenGL.framework/Libraries/libLLVMContainer.dylib
/System/Library/PrivateFrameworks/LLDB.framework/LLDB

I don't know what's up with this.  But in any case I don't think it counts as "heavy use".  So yes, let's just drop the idea of using pthread_mutexattr_setpshared on OS X.
By the way, I've been using Hopper Disassembler (http://www.hopperapp.com/) to load these apps/libraries and check their implementations and/or use of pthread_mutexattr_setpshared.  It's an excellent program!  Though unfortunately it's not open source.
(Following up comment #11)

I get the same results on OS X 10.7.5, and the following two users on OS X 10.9.5:

/System/Library/Frameworks/OpenGL.framework/Libraries/libLLVMContainer.dylib
/System/Library/StagedFrameworks/Safari/JavaScriptCore.framework/Libraries/libllvmForJSC.dylib
The code pasted in comment 10 should return EINVAL in the case where PTHREAD_PROCESS_SHARED is not supported. A return of EINVAL should cause a MOZ_CRASH at http://mxr.mozilla.org/mozilla-central/source/ipc/glue/CrossProcessMutex_posix.cpp?rev=74034d6b6ae6#32 which I didn't see happen when I ran this code. So my bet is that it is implemented, at least on 10.9.4 where I ran it.
(In reply to comment #14)

OK, then, let's keep open the possibility of doing this on OS X.

I won't have time for a thorough review before next week.  And seeing as we might not be able to fully trust Apple's implementation of pthread_mutexattr_setpshared, I need a fairly thorough set of manual tests I can perform.  Someone please provide them.  Once we have them, I'll test on all our supported (major) versions of OS X, back to 10.6.
I also wrote a standalone test program to verify that a cross-process mutex works, and indeed it does work fine on OS X 10.9.4. My test program and instructions are at https://gist.github.com/staktrace/44f2a9e55bc4d18d64fb (it may contain errors since I'm pretty unfamiliar with some of these posix APIs).
(In reply to Steven Michaud from comment #15)
> I need a fairly thorough set of manual tests I
> can perform.  Someone please provide them.  Once we have them, I'll test on
> all our supported (major) versions of OS X, back to 10.6.

Is the standalone test case I linked to in comment 16 sufficient? I'm not sure if you need a more comprehensive set of tests or not. Also FWIW I'm fine with disabling progressive painting on older versions of OS X that don't support this, if it comes to that.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #16)
> I also wrote a standalone test program to verify that a cross-process mutex
> works, and indeed it does work fine on OS X 10.9.4. My test program and
> instructions are at https://gist.github.com/staktrace/44f2a9e55bc4d18d64fb
> (it may contain errors since I'm pretty unfamiliar with some of these posix
> APIs).

I tested on 10.8.5 and it seems to work. Looking at the source, it appears that 10.4.x contains last version of pthread_mutexattr_setpshared to not support PTHREAD_PROCESS_SHARED. The function returns an error when PTHREAD_PROCESS_SHARED is passed in. Having said that, the fact that it isn't documented and the comment in the code calls it a temp fix (for the past four or five major releases) makes me nervous using it.
I ran my test app on a OS X machine running 10.6.8 and the call to setpshared failed :(
Do either of you have access to a machine running 10.7 where we can test if this works? Also, are there any other alternatives for cross-process mutexes that will work on all our supported OS X versions? I'm not really sure what the next step here is if we want to have a CrossProcessMutex implementation on OS X.
I can test on all major versions of OS X back to 10.6.8.  But right now I simply don't have the time.  I hope that by next week things will be a little better.
I've finally got some time to spend on this.

> I ran my test app on a OS X machine running 10.6.8 and the call to setpshared failed :(

Actually it seems to work fine, if you first play a sly trick.  You need to set the TET_EXECUTE environment variable before you call the "creator"!  E.g. TET_EXECUTE=1 ./a.out foo.

(I found this out by looking at the machine code for pthread_mutexattr_setpshared on 10.6.8.)

I'm not entirely sure what the significance of this is.  Google does find a number of hits for TET_EXECUTE, none of them having to do with Apple.  But I expect Apple's using it as a way to limit access to pthread_mutexattr_setpshared to it's "own" apps.
Kartikaya, your testcase from comment #16 works just fine on OS X 10.7.5, without the TET_EXECUTE trick.
That testcase also works fine on OS X 10.8.5, 10.9.5 and 10.10.
Comment on attachment 8494239 [details] [diff] [review]
Part 1 - Use CrossProcessMutex_posix on OS X

I'm fine with this, in principle.

We may, of course, later discover problems that will make it difficult or even impossible to use cross process mutexes on OS X.  But I don't think that's likely, and we can cross (or burn) that bridge when we come to it.

I don't know how important it is to support cross process mutexes on OS X 10.6.  I suspect the answer is "not very", even though a fifth to a quarter of our users are still on that version of OS X.  Whether you support it or not, though, you'll need to add code that determines the OS X version at runtime, and then act accordingly -- either by setting the TET_EXECUTE environment variable or by somehow indicating that cross process mutexes aren't supported.

The methods you should use to determine the OS X version are here:
https://hg.mozilla.org/mozilla-central/annotate/04a87b6ff211/widget/cocoa/nsCocoaFeatures.h#l11
Attachment #8494239 - Flags: review?(smichaud) → review+
I looked a bit harder and found an Apple-related reference to TET_EXECUTE, here:

#ifdef PR_5243343
	/* 5243343 - temporary hack to detect if we are running the conformance test */
	if(getenv("TET_EXECUTE"))
		PR_5243343_flag = 1;
#endif /* PR_5243343 */

http://www.opensource.apple.com/source/Libc/Libc-825.40.1/sys/__libc_init.c?txt

This doesn't inspire a whole lot of confidence that pthread_mutexattr_setpshared will work correctly on OS X 10.6 :-(
Thanks for looking into this! I don't really want to propagate the TET_EXECUTE hack so I'll just do a MOZ_CRASH on pre-Lion versions. The code that I want to use this with is the progressive painting code which we can also disable prior to Lion. This makes sense to do anyway because progressive painting doesn't make sense without tiling, and tiling is disabled by default before Lion.
Updated to MOZ_CRASH on OS X 10.6. Carrying r+ from smichaud.
Attachment #8494239 - Attachment is obsolete: true
Attachment #8515986 - Flags: review+
Attachment #8515993 - Flags: review?(bas) → review?(bgirard)
Attachment #8515993 - Flags: review?(bgirard) → review+
https://hg.mozilla.org/mozilla-central/rev/33b7cccd5e2b
https://hg.mozilla.org/mozilla-central/rev/6c2fb5df16ba
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Depends on: 1098607
You need to log in before you can comment on or make changes to this bug.