Closed Bug 1024728 Opened 5 years ago Closed 5 years ago

CrossProcessMutex::Lock can hang if called with a stale handle


(Core :: IPC, defect)

Gonk (Firefox OS)
Not set



Tracking Status
firefox31 --- wontfix
firefox32 --- fixed
firefox33 --- fixed
b2g-v1.4 --- unaffected
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed


(Reporter: kats, Assigned: kats)



(1 file)

While investigating a failure in bug 1023882 I found that a call to CrossProcessMutex::Lock was hanging. The reason for this was that the parent process created one of these mutexes, and sent a "create" message along with the handle over to the child process. The parent process then destroyed the mutex and send over a "destroy" message to the child process. All of this happened before the child process got around to processing the "create" message, so when it did eventually call the constructor, the mMutex object at [1] had already been destroyed by pthread_mutex_destroy at [2]. A subsequent call to Lock the mutex (before the child process got around to deal with the "destroy" message) caused it to hang, because the mMutex internal state was wrong.

At [3] Randall suggested re-initializing the mMutex based on the count, and this does seem like a good solution.

Attached patch PatchSplinter Review
Not sure if the windows version has this problem too.
(In reply to Kartikaya Gupta ( from comment #1)
> Created attachment 8439497 [details] [diff] [review]
> Patch
> Not sure if the windows version has this problem too.

A quick glance at the Windows version may have an issue. If the mutex handle is closed before the child process opens the mutex, it should fail and call NS_RUNTIMEABORT("Attempt to construct a mutex from an invalid handle!");

Right now the Windows Cross Process Mutex is only used in plugin code from what I can tell.
I don't see any issue on Windows. Once you've called DuplicateHandle, the other side has a valid handle even if you call ::CloseHandle immediately on the local side.
Comment on attachment 8439497 [details] [diff] [review]

Review of attachment 8439497 [details] [diff] [review]:

Sounds good then. Flagging khuey for review since he reviewed the original code, but feel free to steal.
Attachment #8439497 - Flags: review?(khuey)
Try push includes this change (and the lack of a failing M-4 test there shows that it does fix the problem I was seeing in bug 1023882).
Marking affected on b2g 2.0 and up because that's where progressive painting is enabled and uses this code. Technically the bug exists in older B2G as well but the code isn't exercised there.
Comment on attachment 8439497 [details] [diff] [review]

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 895358 (initial landing of this code)
User impact if declined: If progressive painting is enabled in a cross-process compositor environment (only B2G at the moment) then there is a potential for content processes to hang indefinitely. Not easy to hit in the wild but still possible.
Testing completed (on m-c, etc.): locally, on tryserver where the problem was found.
Risk to taking this patch (and alternatives if risky): pretty low risk. the code is not widely used.
String or IDL/UUID changes made by this patch: none
Attachment #8439497 - Flags: approval-mozilla-aurora?
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Comment on attachment 8439497 [details] [diff] [review]

Aurora approval granted.
Attachment #8439497 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.