1271169 - [EME] Unit test for NodeId generation

Status: RESOLVED FIXED

(Core :: Audio/Video: GMP, defect, P2)

Description

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

We should have a unit test to ensure that node Id generation doesn't regress. This broke Netflix in Bug 1264497.

Comment 1

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3d64783956f

Comment 2

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=58ec1a82be43

Comment 3

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

Attachment: MozReview Request: Bug 1271169 - Move EME/GMP device binding code into GMPDeviceBinding.h/cpp. r=gerald

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/

Comment 4

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

Attachment: MozReview Request: Bug 1271169 - Move all device binding code into librlz and remove static runtime requirement. r=glandium

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/

Comment 5

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

Attachment: MozReview Request: Bug 1271169 - Add gtest to ensure EME device binding works. r=gerald

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 6

[Gerald Squelart [:gerald] (he/him) PTO until 2022-05-31]

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

Comment 7

[Gerald Squelart [:gerald] (he/him) PTO until 2022-05-31]

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 8

[Mike Hommey [:glandium]]

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.

Comment 9

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

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

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

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/

Comment 11

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

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 12

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

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/

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/200726fdfb53
https://hg.mozilla.org/integration/mozilla-inbound/rev/4da6cece7b58
https://hg.mozilla.org/integration/mozilla-inbound/rev/57fa89168a44

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/200726fdfb53
https://hg.mozilla.org/mozilla-central/rev/4da6cece7b58
https://hg.mozilla.org/mozilla-central/rev/57fa89168a44