1307019 - Crash in mozilla::EMEDecoderModule::SupportsMimeType

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P1)

Description

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

Attachment: missing-pssh.zip

This bug was filed from the Socorro interface and is 
report bp-c37f65e1-7a6f-472c-89a3-b2af22161003.
=============================================================

Crash with attached testcase, which tries to play an encrypted file which doesn't have a PSSH without first negotiating a ClearKey license.

We're doing a null pointer deref.

Comment 1

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

Turns out this was fixed by bug 1300069. Given that I was trying to write a test case for bug 1300069, so I guess I've succeeded!

Comment 2

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

Not quite true. This is actually a regression from bug 1300069, which seems to be timing dependent. The patch in that bug introduced an inconsistency between what the MediaDecoderStateMachine and the MediaFormatReader consider an encrypted stream. The MDSM considered a stream encrypted if mInfo.IsEncrypted() is true, and that only takes into account the PSSH. Whereas the MFR only considers the presence of a TENC box to indicate encryptedness. This would cause the MDSM to not wait for the CDM before trying to start decoding. So if you setup the MediaSource before setting the MediaKeys on the MediaElement, you'll hit this crash.

Comment 3

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

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

Comment 4

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

Attachment: Bug 1307019 - Ensure MDSM and MFR have consistent view of what counts as encrypted.

The patch in bug 1300069 introduced an inconsistency between what the
MediaDecoderStateMachine and the MediaFormatReader consider an encrypted
stream. The MDSM considered a stream encrypted if mInfo.IsEncrypted() is true,
and that only takes into account the PSSH. Whereas the MFR only considers the
presence of a TENC box to indicate encryptedness. This would cause the MDSM
to not wait for the CDM before trying to start decoding. So if you setup the
MediaSource before setting the MediaKeys on the MediaElement, you'll end up
trying to create an EME decoder without a CDMProxy, and that causes a null
pointer deref and crash.

This patch ensures that the MDSM and the MFR use the same logic to determine
whether a stream is encrypted.

Review commit: https://reviewboard.mozilla.org/r/82666/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/82666/

Comment 5

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

Attachment: Bug 1307019 - Testcase for encrypted MP4 without PSSH and MDSM waiting-for-CDM.

Tests that a fragmented MP4 file without a PSSH, but with encrypted valid
tracks with valid TENC boxes, is able to load with EME. This is a test for
the code path added in bug 1300069.

We setup MSE before starting up EME, so that we exercise the "waiting for
cdm" step in the MediaDecoderStateMachine, which was regressed in bug 1300069.

Review commit: https://reviewboard.mozilla.org/r/82668/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/82668/

Comment 6

[Jean-Yves Avenard [:jya]]

Comment on attachment 8797030 [details]
Bug 1307019 - Ensure MDSM and MFR have consistent view of what counts as encrypted.

https://reviewboard.mozilla.org/r/82666/#review81376

Comment 7

[Jean-Yves Avenard [:jya]]

Comment on attachment 8797031 [details]
Bug 1307019 - Testcase for encrypted MP4 without PSSH and MDSM waiting-for-CDM.

https://reviewboard.mozilla.org/r/82668/#review81378

::: dom/media/test/test_eme_missing_pssh.html:75
(Diff revision 1)
> +          sourceBuffer = mediaSource.addSourceBuffer(type);
> +          var req = new XMLHttpRequest();
> +          req.open("GET", url);
> +          req.responseType = "arraybuffer";
> +          req.addEventListener("load", function() {
> +            sourceBuffer.addEventListener("updateend", resolve);

You should use a promise (like with the once utility found in mediasource.js) here rather than an event listnener.. or at least remove the event handler as the later call to endOfStream will cause updateend to be fired again.

otherwise you may get intermittent errors due to an invalid state

::: dom/media/test/test_eme_missing_pssh.html:84
(Diff revision 1)
> +        });
> +      }
> +
> +      function LoadMSE() {
> +        var ms = new MediaSource();
> +        audio.src = URL.createObjectURL(ms);

You could have made this mochitest must simpler if it was possible to share some of the scripts found in the mediasource/test directory.

Comment 8

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

(In reply to Jean-Yves Avenard [:jya] from comment #7)
> Comment on attachment 8797031 [details]
> Bug 1307019 - Testcase for encrypted MP4 without PSSH and MDSM
> waiting-for-CDM.
> [...]
> You could have made this mochitest must simpler if it was possible to share
> some of the scripts found in the mediasource/test directory.

I manually imported once() (with a minor change to pass the event argument to the callback) and fetchWithXHR() into eme.js. Using the dom/media/mediasource/test/mediasource.js script wasn't working, or at least, it isn't obvious how to make it work from the normal dom/media/test directory.

Comment 9

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

Comment on attachment 8797030 [details]
Bug 1307019 - Ensure MDSM and MFR have consistent view of what counts as encrypted.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82666/diff/1-2/

Comment 10

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

Comment on attachment 8797031 [details]
Bug 1307019 - Testcase for encrypted MP4 without PSSH and MDSM waiting-for-CDM.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82668/diff/1-2/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/61d63bbd649f
Ensure MDSM and MFR have consistent view of what counts as encrypted. r=jya
https://hg.mozilla.org/integration/autoland/rev/8f201cbda968
Testcase for encrypted MP4 without PSSH and MDSM waiting-for-CDM. r=jya

Comment 12

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

Must remember to uplift...

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/61d63bbd649f
https://hg.mozilla.org/mozilla-central/rev/8f201cbda968

Comment 14

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

Comment on attachment 8797030 [details]
Bug 1307019 - Ensure MDSM and MFR have consistent view of what counts as encrypted.

Approval Request Comment
[Feature/regressing bug #]: bug 1300069, EME Video Playback
[User impact if declined]: Crashes if sites setup streaming sources before setting up EME. I missed a case in my patch for bug 1300069, and we crash in that case. :(
[Describe test coverage new/current, TreeHerder]: Testcase included in next patch in this bug. That testcase also covers bug 1300069 which was uplifted a few days ago.
[Risks and why]: Low, fairly simple fix, with testcase.
[String/UUID change made/needed]: None.

Comment 15

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

Comment on attachment 8797031 [details]
Bug 1307019 - Testcase for encrypted MP4 without PSSH and MDSM waiting-for-CDM.

Approval Request Comment
[Feature/regressing bug #]: bug 1300069, EME Video Playback
[User impact if declined]: Test coverage for crash fix, and bug 1300069.
[Describe test coverage new/current, TreeHerder]: This is the testcase.
[Risks and why]: Test, so should be no user risk!
[String/UUID change made/needed]: None.

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8797030 [details]
Bug 1307019 - Ensure MDSM and MFR have consistent view of what counts as encrypted.

Fix a crash, taking it
Shoud be in 50 beta 5

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8104e34db545
https://hg.mozilla.org/releases/mozilla-aurora/rev/abd9db31f1e2

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1aac04b95158
https://hg.mozilla.org/releases/mozilla-beta/rev/5350691a386a