1378507 - Crash in InvalidArrayIndex_CRASH | nsTArray_Impl<T>::ElementAt | mp4_demuxer::H264::ExtractExtraData

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

This bug was filed from the Socorro interface and is 
report bp-7dd0f191-82e0-4908-adda-a92f90170705.
=============================================================

Index is 4 in all of these crashes, and the array length is 0.

20 of these crashes, all on Nightly, starting on the 6-29 build.

Comment 1

[Jean-Yves Avenard [:jya]]

This would normally be caught with the MOZ_ASSERT(AnnexB::IsAVCC(aSample)); above.

How we get there with non AVCC sample is beyond me at this stage.

Comment 2

[Andrew McCreight [:mccr8]]

What is the needinfo for? I came across this on crash stats. Looking over the crash reports, it is possible this is just one user, so maybe something is just weird with their configuration or machine. Feel free to close this if you think it isn't actionable.

Comment 3

[Jean-Yves Avenard [:jya]]

VP9 in MP4 would definitely cause this to crash.

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1378507: Add extra check to ensure data is valid.

MP4TrackDemuxer::GetNextSamples set mExtraData and it should be valid. This sample will later be rejected by the H264Converter.
The case would also fail if the video codec was VP9.

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

Comment 5

[Jean-Yves Avenard [:jya]]

This isn't a valid webm, it has a h264 track.

Per spec, webm is to contain only opus, vorbis, vp8, vp9 (and now av1)
http://www.webmproject.org/docs/container/

Comment 6

[Jean-Yves Avenard [:jya]]

wrong bug

Comment 7

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8883857 [details]
Bug 1378507: Add extra check to ensure data is valid.

https://reviewboard.mozilla.org/r/154808/#review160084

::: commit-message-835c7:1
(Diff revision 1)
> +Bug 1378507: Add extra check to ensure data is valid. r?gerald

Traditionally commit messages start with "Bug 1378507 - Add ...", i.e., dash instead of colon.

Comment 8

[Jean-Yves Avenard [:jya]]

Comment on attachment 8883857 [details]
Bug 1378507: Add extra check to ensure data is valid.

https://reviewboard.mozilla.org/r/154808/#review160084

> Traditionally commit messages start with "Bug 1378507 - Add ...", i.e., dash instead of colon.

what the... you're only telling me this now?

I my 1800ish history of commits, not once have I used a dash next to the bug number :(

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities
only states that the commit message "should include the bug number, the names of the reviewers, and a clear explanation of the fix."

Will change from now on

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/951d6b30f1f6
Add extra check to ensure data is valid. r=gerald

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/951d6b30f1f6

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Untracked and 55 unaffected as I don't see any crash signatures on 55 in the past one month. Please let me know if I misread the data.

Comment 12

[Jean-Yves Avenard [:jya]]

Comment on attachment 8883857 [details]
Bug 1378507: Add extra check to ensure data is valid.

Approval Request Comment
[Feature/Bug causing the regression]: 1374774
[User impact if declined]: crashes. Bug 1374774 is about to be uplifted, this change must go in to avoid the downfall.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: hard to say..
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: adding extra check to avoid entering crash condition
[String changes made/needed]: none

Comment 13

[Julien Cristau [:jcristau]]

Comment on attachment 8883857 [details]
Bug 1378507: Add extra check to ensure data is valid.

regression from 1374774, they should go in beta together

I guess you're going to rebase the patches from 1374774?

Comment 14

[Jean-Yves Avenard [:jya]]

Yes. I have a patch stack ready to be pushed. Was just waiting on this r+

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/aae9a3c05e65