Closed Bug 1098607 Opened 5 years ago Closed 5 years ago

ipc/glue/CrossProcessMutex_unimplemented.cpp compile issue

Categories

(Core :: IPC, defect)

36 Branch
x86_64
NetBSD
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: spz, Assigned: jbeich)

References

Details

Attachments

(2 files)

User Agent: Mozilla/5.0 (X11; NetBSD amd64; rv:36.0) Gecko/20100101 Firefox/36.0
Build ID: 20141113094658

Steps to reproduce:

I'm on a OS_POSIX system that is neither Linux nor Darwin and tried to compile firefox.
I hit a snag compiling ipc/glue/CrossProcessMutex_unimplemented.cpp 



Actual results:

complaints about 0 being incompatible with the CrossProcessMutexHandle type


Expected results:

a snagless build :)

Instead of the patch, the real fix may be to use CrossProcessMutex__posix.cpp for all OS_POSIX systems.
Attachment #8522540 - Flags: review?(benjamin)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment on attachment 8522540 [details] [diff] [review]
CrossProcessMutex__unimplemented.cpp.patch

jbeich touched this most recently, I'm going to ask him for feedback because I don't even really know what this code does.
Attachment #8522540 - Attachment is patch: true
Attachment #8522540 - Attachment mime type: text/x-patch → text/plain
Attachment #8522540 - Flags: review?(benjamin) → review?(jbeich)
PTHREAD_PROCESS_SHARED isn't implemented on any BSD for ipc/glue/CrossProcessMutex_posix.cpp to be usable. One could try using sem_open(3) instead after finding simple tests for CrossProcessMutex to check it works correctly. Bug 1072093 case for OS X 10.6 suggests only progressive-paint may need an actual implementation.

http://mail-index.netbsd.org/current-users/2013/11/29/msg023791.html
http://lists.freebsd.org/pipermail/freebsd-threads/2014-February/005531.html
Blocks: 1072093
Attached patch fixSplinter Review
ipc/chromium code is confusing. Do we support platforms without SharedMemoryBasic ?

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=eba07acf108b
Attachment #8523083 - Flags: review?(benjamin)
Comment on attachment 8523083 [details] [diff] [review]
fix

Sorry, should have passed to whoever introduced the regression.
Attachment #8523083 - Flags: review?(benjamin) → review?(bugmail.mozilla)
Comment on attachment 8522540 [details] [diff] [review]
CrossProcessMutex__unimplemented.cpp.patch

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

Not a peer or know enough about non-POSIX case. Back to Benjamin if he wants to continue.

::: ipc/glue/CrossProcessMutex_unimplemented.cpp.orig
@@ +43,2 @@
>    NS_RUNTIMEABORT("Cross-process mutices not allowed on this platform - woah! We should've aborted by now!");
> +  return result; /* uninitialized, but we abort before getting here anyway */

If the above works on non-POSIX systems as well because SharedMemoryBasic exists then uintptr_t in CrossProcessMutex.h can be removed:

  #if defined(OS_WIN)
  typedef HANDLE CrossProcessMutexHandle;
  #else
  typedef mozilla::ipc::SharedMemoryBasic::Handle CrossProcessMutexHandle;
  #endif
Attachment #8522540 - Flags: review?(jbeich) → review?(benjamin)
Comment on attachment 8523083 [details] [diff] [review]
fix

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

Looks fine to me, thanks!
Attachment #8523083 - Flags: review?(bugmail.mozilla) → review+
Stumbled upon the same issue on OpenBSD, patch fixes it, so pushed to m-i:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c130981cdab
https://hg.mozilla.org/mozilla-central/rev/2c130981cdab
Assignee: nobody → jbeich
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment on attachment 8522540 [details] [diff] [review]
CrossProcessMutex__unimplemented.cpp.patch

This patch isn't necessary any more, right?
Attachment #8522540 - Flags: review?(benjamin)
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #9)
> Comment on attachment 8522540 [details] [diff] [review]
> CrossProcessMutex__unimplemented.cpp.patch
> 
> This patch isn't necessary any more, right?

correct
Blocks: 1252246
You need to log in before you can comment on or make changes to this bug.