We should have unit tests for GMPAsyncShutdown. Test that: * GMPAsyncShutdown works * GMPStorage works while in async shutdown. * GMPAsyncShutdown times out * GMPAsyncShutdown works if the plugin crashes while shutting down. * GMPTimers don't work once shutdown has started.
Created attachment 8508553 [details] [diff] [review] Patch: GMPAsyncShutdown gtests gTests that test that GMPStorage works during async shutdown, and that we abort shutdown and kill the the GMP after a timeout. Based on top of patch in bug 1085168.
Attachment #8508553 - Flags: review?(edwin)
Attachment #8508553 - Flags: review?(edwin) → review+
Created attachment 8509143 [details] [diff] [review] Patch 1 v2: GMPAsyncShutdown gtests Fixed some build warnings...
Created attachment 8509144 [details] [diff] [review] Patch 2: Don't load the GMP dylib twice on MacOSX While writing the tests in this bug I discovered (because my tests were failing) that we're loading the GMP dylib twice on MacOSX. This is because GMPChild overrides OnChannelConnected() and loads the GMP dylib there and initializes the sandbox, but we're also doing this in GMPChild::RecvStartPlugin() (which is called later). I added the call to RecvStartPlugin in bug 1060179, but I didn't realise the plugin was already being loaded on MacOSX. So this patch moves the init of the GMP and the sandbox on MacOSX to the same call site as on other platforms. I don't know why the sandbox was being started in the OnChannelConnected() override, but it seems to work with this patch...
Attachment #8509144 - Flags: review?(smichaud)
> I don't know why the sandbox was being started in the OnChannelConnected() override This is the best way to ensure that sandbox is only started after IPC has finished being set up. I'll look at your patch ... but I'm very skeptical about it from the start.
Comment on attachment 8509144 [details] [diff] [review] Patch 2: Don't load the GMP dylib twice on MacOSX I've explained my objections in my previous comment. Let me see if I can come up with an alternative patch.
Attachment #8509144 - Flags: review?(smichaud) → review-
(In reply to Steven Michaud from comment #6) > > I don't know why the sandbox was being started in the OnChannelConnected() override > > This is the best way to ensure that sandbox is only started after IPC has > finished being set up. > > I'll look at your patch ... but I'm very skeptical about it from the start. The IPC messages that we send to the child process during startup are here: http://mxr.mozilla.org/mozilla-central/source/content/media/gmp/GMPParent.cpp#163 This happens in the StartPlugin message handler, that's the second message we receive (other than protocol setup). Can we really have not finished setting up the IPC connection before we've started to process received messages in the child process? If so, is this a Mac only thing, or could other platforms be affected too?
I've been using OnChannelConnected() (on the Mac) since the beginning, because I understand that using it *guarantees* that IPC is already fully set up. I did this after noticing all kinds of problems on other platforms caused by IPC-startup activity happening (at least sometimes) after the sandbox had been launched. Those problems may have been eliminated, at least for the time being, by hacks and workarounds. But yes, I suspect all platforms would benefit by doing this (at least in the long run). However, I do realize that other platforms have been doing things differently, and (on Windows at least) we're close to an unmovable release date. So I don't suggest that we change things on Windows and Linux, at least for now. On the other hand, and for the same reasons, I *really* don't want to change how we've handled this on the Mac since the beginning, this close to getting into a release version of Firefox.
We have to change this on Mac. We can't start the sandbox until we've received a message from the parent process which gives us some salt with which to hash the machine ID with. Adobe require a device-specific ID, and we need the salt to be delivered from the parent process via a message before we can generate it. So we *must not* start the sandbox until after we've received that message (the GMPChild::RecvSetNodeId() call/message), as we need to do privileged operations in order to generate the device specific ID. Honestly, I'm having a hard time believing that OnChannelConnected() is called after the GMPChild::RecvSetNodeId and RecvStartPlugin messages/calls. I tested on my 10.7 Mac Mini, and the OnChannelConnected() call definitely happens *before* any of the Recv$Message calls occur, so I think this patch above is fine.
I will need to look through your patch, and its context. I'll also have to do some testing. This will take some time -- at least several days. What I said in comment #9 still stands.
OK run your tests. We still have several weeks until the uplift, so we have ample time to fix this, and we'll have ample time to back it out if our fix proves bad. I will stick a workaround in my tests here, land them, and file a follow up bug to address this issue.
Steven: Also note that bug 1060179 changed the control flow of starting up the child process significantly.
(In reply to comment #13) So I see. Thanks for the note. That makes things easier. So now I just need to prove to myself that GMPChild::RecvStartPlugin() *must* happen after GMPChild::OnChannelConnected().
Testing gtests with work around: https://tbpl.mozilla.org/?tree=Try&rev=d1506e2fe699
Landed test with work around: https://hg.mozilla.org/integration/mozilla-inbound/rev/fcf48b5ce92d
I filed Bug 1087901 to track fixing the MacOSX issue.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
> I filed Bug 1087901 to track fixing the MacOSX issue. We've resolved this issue.
Mass update firefox-status to track EME uplift.
status-firefox37: --- → fixed
status-firefox38: --- → fixed
status-firefox39: --- → fixed
You need to log in before you can comment on or make changes to this bug.