Closed Bug 1275548 Opened 4 years ago Closed 4 years ago

eliminate races in OS X shared memory shutdown

Categories

(Core :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

Attachments

(2 files)

No description provided.
The patches being posted here are my attempts at solving the intermittent
shutdown crashes seen when trying to land bug 1246743.  We only need one of
these patches, but I'm not entirely sure which one is preferred.  Feedback
and/or pushback greatly appreciated.

Shared memory is currently shutdown via XRE_ShutdownChildProcess, but
ImageBridge threads may still be running at that point and requesting
new shared memory channels.  To synchronize properly, we need to record
that the shared memory manager has been shutdown and reject requests
after that point.

This is the conservative patch that's obviously correct, as we'll just reject
requests for shared memory at an appropriate point in shutdown.  However, this
doesn't do anything to actually try and untangle our shutdown dependencies,
which seems mildly unfortunate.
Attachment #8756309 - Flags: review?(wmccloskey)
As mentioned previously, we only need one of these two patches (though we could
do both for extra safety).  This is the more adventurous patch.

OS X shared memory is currently set up in XRE_InitChildProcess and torn
down in XRE_ShutdownChildProcess.  The current location of shared memory
shutdown is vulnerable to races from outstanding shared memory requests
from ImageBridge threads; if we request a new shared memory segment from
an image bridge thread after shared memory has been shutdown, we will
crash.

Despite its name, XRE_ShutdownChildProcess doesn't actually perform all
shutdown steps; it merely notifies the main run loop that it's OK to
exit, which eventually returns control to XRE_InitChildProcess.  Inside
XRE_InitChildProcess, then, is where cleanup of the content process
actually happens, and where shared memory shutdown needs to be.

I'm *pretty* sure we'll have shutdown all the IPC threads at this point (I
believe we're past XPCOM shutdown via ContentProcess::CleanUp), and any
shutdown messages getting sent by SharedMemoryBasic::Shutdown will be going via
mach, so there shouldn't be any races here.

r? to khuey as well, since he likes shutdown issues.
Attachment #8756312 - Flags: review?(wmccloskey)
Attachment #8756312 - Flags: review?(khuey)
Blocks: 1246743
Comment on attachment 8756312 [details] [diff] [review]
move OS X SharedMemoryBasic shutdown

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

This actually feels like the safer patch to me. We should definitely take this one.
Attachment #8756312 - Flags: review?(wmccloskey) → review+
Comment on attachment 8756309 [details] [diff] [review]
prevent further requests for shared memory after shared memory shutdown on OS X

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

I'd kinda rather turn this into some sort of fatal assertion.
Attachment #8756309 - Flags: review?(wmccloskey)
Attachment #8756312 - Flags: review?(khuey)
https://hg.mozilla.org/mozilla-central/rev/8585521af418
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.