[EME] Unit tests for GMPAsyncShutdown

RESOLVED FIXED in Firefox 37

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

(Blocks: 1 bug)

29 Branch
mozilla36
x86_64
All
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox37 fixed, firefox38 fixed, firefox39 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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.
(Assignee)

Updated

4 years ago
Blocks: 1032660
(Assignee)

Updated

4 years ago
Depends on: 1044667
(Assignee)

Updated

4 years ago
Assignee: nobody → cpearce
(Assignee)

Comment 1

4 years ago
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)
(Assignee)

Comment 3

4 years ago
Created attachment 8509143 [details] [diff] [review]
Patch 1 v2: GMPAsyncShutdown gtests

Fixed some build warnings...
Attachment #8508553 - Attachment is obsolete: true
Attachment #8509143 - Flags: review+
(Assignee)

Comment 4

4 years ago
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-
(Assignee)

Comment 8

4 years ago
(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.
(Assignee)

Comment 10

4 years ago
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.
(Assignee)

Updated

4 years ago
Flags: needinfo?(smichaud)
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.
(Assignee)

Comment 12

4 years ago
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.
(Assignee)

Comment 13

4 years ago
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().
(Assignee)

Comment 15

4 years ago
Testing gtests with work around:
https://tbpl.mozilla.org/?tree=Try&rev=d1506e2fe699
(Assignee)

Comment 17

4 years ago
I filed Bug 1087901 to track fixing the MacOSX issue.
https://hg.mozilla.org/mozilla-central/rev/fcf48b5ce92d
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
> I filed Bug 1087901 to track fixing the MacOSX issue.

We've resolved this issue.
Flags: needinfo?(smichaud)
(Assignee)

Comment 21

4 years ago
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.