[EME] Unit test for NodeId generation

RESOLVED FIXED in Firefox 49

Status

()

defect
P2
normal
Rank:
25
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

unspecified
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(3 attachments)

We should have a unit test to ensure that node Id generation doesn't regress. This broke Netflix in Bug 1264497.
Rank: 25
Priority: -- → P2
I want the EME device binding/nodeId code to be callable from gtests, as well
as from in plugin-container.

First step is to move the device binding code into a discrete file, so I can
also link that into gtests, and call it from there to compare the result with
what's in the GMP process.

Review commit: https://reviewboard.mozilla.org/r/54116/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54116/
Attachment #8755227 - Flags: review?(gsquelart)
Attachment #8755228 - Flags: review?(mh+mozilla)
Attachment #8755229 - Flags: review?(gsquelart)
I want the EME device binding/nodeId code to be callable from gtests, as well
as in plugin-container. I need this because I want to add a gtest that ensures
that we don't regress the EME/GMP device binding code. I want to call the GMP
device binding code in the gtest and in the GMP process, and compare the
result.

So we need to make it possible to link the device binding code into the gtests
as well as plugin-container. So move all code that device binding calls into
librlz, to make it easier to link against all the code required.

Unfortunately the gtests are linked against a dynamic MSVCRT, but librlz, where
the device binding code lives, is linked with static MSVCRT. So in order to
link into gtests, we must remove the force static runtime directive from
librlz's moz.build.

Note: the device binding code needs to be statically linked into
plugin-container so that it's covered by the Adobe CDM's voucher tool.

Review commit: https://reviewboard.mozilla.org/r/54118/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54118/
Call the device binding code in gtests, and have a test GMP which returns the
device ID it was passed in a message, so that we can verify that the id the GMP
was passed is the same as the id generated in the parent.

Review commit: https://reviewboard.mozilla.org/r/54120/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54120/
Comment on attachment 8755227 [details]
MozReview Request: Bug 1271169 - Move EME/GMP device binding code into GMPDeviceBinding.h/cpp. r=gerald

https://reviewboard.mozilla.org/r/54116/#review51134
Attachment #8755227 - Flags: review?(gsquelart) → review+
Attachment #8755229 - Flags: review?(gsquelart) → review+
Comment on attachment 8755229 [details]
MozReview Request: Bug 1271169 - Add gtest to ensure EME device binding works. r=gerald

https://reviewboard.mozilla.org/r/54120/#review51140

r+ for your changes.
Noticed this nearby, if you want to improve it:

::: dom/media/gtest/TestGMPCrossOrigin.cpp:639
(Diff revision 1)
>      Updates(GMPStorageTest* aRunner, nsTArray<nsCString>&& aUpdates)
>        : mRunner(aRunner),
>          mUpdates(aUpdates)

The last initializer should probably be 'mUpdates(Move(aUpdates))'
Comment on attachment 8755228 [details]
MozReview Request: Bug 1271169 - Move all device binding code into librlz and remove static runtime requirement. r=glandium

https://reviewboard.mozilla.org/r/54118/#review51332

Nothing to say about the patch, but I have something to say about the commit message:

> Unfortunately the gtests are linked against a dynamic MSVCRT, but librlz, where
> the device binding code lives, is linked with static MSVCRT. So in order to
> link into gtests, we must remove the force static runtime directive from
> librlz's moz.build.

That's actually not true since bug 1035125, which removed the USE_STATIC_LIBS = True in rlz/moz.build. The FORCE_STATIC_LIB you're removing in this patch is actually a no-op because it's the default when you have a Library(), and is not related to the static CRT.
Attachment #8755228 - Flags: review?(mh+mozilla) → review+
(In reply to Mike Hommey [:glandium] from comment #8)
> That's actually not true since bug 1035125, which removed the
> USE_STATIC_LIBS = True in rlz/moz.build. The FORCE_STATIC_LIB you're
> removing in this patch is actually a no-op because it's the default when you
> have a Library(), and is not related to the static CRT.

Ha! I was sure the behaviour changed around the 18th, so that would explain why! Thanks.
Comment on attachment 8755227 [details]
MozReview Request: Bug 1271169 - Move EME/GMP device binding code into GMPDeviceBinding.h/cpp. r=gerald

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54116/diff/1-2/
Attachment #8755227 - Attachment description: MozReview Request: Bug 1271169 - Move EME/GMP device binding code into GMPDeviceBinding.h/cpp. r?gerald → MozReview Request: Bug 1271169 - Move EME/GMP device binding code into GMPDeviceBinding.h/cpp. r=gerald
Attachment #8755228 - Attachment description: MozReview Request: Bug 1271169 - Move all device binding code into librlz and remove static runtime requirement. r?glandium → MozReview Request: Bug 1271169 - Move all device binding code into librlz and remove static runtime requirement. r=glandium
Attachment #8755229 - Attachment description: MozReview Request: Bug 1271169 - Add gtest to ensure EME device binding works. r?gerald → MozReview Request: Bug 1271169 - Add gtest to ensure EME device binding works. r=gerald
Comment on attachment 8755228 [details]
MozReview Request: Bug 1271169 - Move all device binding code into librlz and remove static runtime requirement. r=glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54118/diff/1-2/
Comment on attachment 8755229 [details]
MozReview Request: Bug 1271169 - Add gtest to ensure EME device binding works. r=gerald

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54120/diff/1-2/
You need to log in before you can comment on or make changes to this bug.