1366208 - Crash in mozilla::MP4Demuxer::NotifyDataArrived. Part 2

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

hi, there are still crashes with this signature in 54.0b8 after the patch landed there:
https://crash-stats.mozilla.com/signature/?product=Firefox&version=54.0b8&signature=mozilla%3A%3AMP4Demuxer%3A%3ANotifyDataArrived#reports

Comment 1

[Jean-Yves Avenard [:jya]]

This is all on 32 bits.. I think it's due to an earlier OOM in MP4Demuxer::Init()

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1366208: Make sure no empty RefPtr are ever stored.

Should any error occurred while checking the tracks, we could end up with a null pointer stored in the list of track demuxers.

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

Comment 3

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

Comment on attachment 8869374 [details]
Bug 1366208: Make sure no empty RefPtr are ever stored.

https://reviewboard.mozilla.org/r/141026/#review144554

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a2ed738274c1
Make sure no empty RefPtr are ever stored. r=gerald

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a2ed738274c1

Comment 6

[Jean-Yves Avenard [:jya]]

Comment on attachment 8869374 [details]
Bug 1366208: Make sure no empty RefPtr are ever stored.

Approval Request Comment
[Feature/Bug causing the regression]: 1343437
[User impact if declined]: crashes
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: won't know for sure until a few days, as it's an intermittent failure
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: we ensure that if an error occurred while attempting to read a mp4, we don't later use null pointers.
[String changes made/needed]: none

Comment 7

[Gerry Chang [:gchang]]

Track 54+ as the crashes still show in 54.0b8.

Comment 8

[Gerry Chang [:gchang]]

Hi :jya, 
I'm not sure if bug 1343437 in the uplift request template is correct because bug 1343437 is fixed in 55. Why will the fix of bug 1343437 cause the issue?

Comment 9

[Jean-Yves Avenard [:jya]]

Actually, the bug was first introduced with bug 1341454, but made worse with bug 1343437 as there's more cases that could put a null pointer in the array.

Comment 10

[Gerry Chang [:gchang]]

Comment on attachment 8869374 [details]
Bug 1366208: Make sure no empty RefPtr are ever stored.

Fix a crash. Let's see how it goes. Beta54+. Should be in 54 beta 10.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5de3b1528ca0

Comment 12

[Andrei Vaida [:avaida]]

(In reply to Jean-Yves Avenard [:jya] from comment #6)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: won't know for sure until a few
> days, as it's an intermittent failure
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on jya's assessment on manual testing needs.

Comment 13

[Jean-Yves Avenard [:jya]]

No crash since May 15th... I guess we got it \o/

Comment 14

[Blake Wu [:bwu][:blakewu]]

Congrats!