1055395 - [EME] Unit tests for GMPAsyncShutdown

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: 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.

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b74c4a23f70b
https://tbpl.mozilla.org/?tree=Try&rev=b74c4a23f70b

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 1 v2: GMPAsyncShutdown gtests

Fixed some build warnings...

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: 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...

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d902d50c8d53
https://tbpl.mozilla.org/?tree=Try&rev=d902d50c8d53

Comment 6

[Steven Michaud [:smichaud] (Retired)]

> 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 7

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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?

Comment 9

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 11

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

Steven: Also note that bug 1060179 changed the control flow of starting up the child process significantly.

Comment 14

[Steven Michaud [:smichaud] (Retired)]

(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().

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

Testing gtests with work around:
https://tbpl.mozilla.org/?tree=Try&rev=d1506e2fe699

Comment 16

[Chris Pearce [:cpearce (Not reading bugmail)]]

Landed test with work around:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fcf48b5ce92d

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

I filed Bug 1087901 to track fixing the MacOSX issue.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fcf48b5ce92d

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/95e70cc80f82

Comment 20

[Steven Michaud [:smichaud] (Retired)]

> I filed Bug 1087901 to track fixing the MacOSX issue.

We've resolved this issue.

Comment 21

[Chris Pearce [:cpearce (Not reading bugmail)]]

Mass update firefox-status to track EME uplift.