1357244 - Crash in mozilla::MediaCacheStream::BlockList::RemoveBlock

Status: RESOLVED FIXED

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

Description

[JW Wang [:jwwang] [:jw_wang]]

This bug was filed from the Socorro interface and is 
report bp-8774867c-4001-4581-baec-403592170417.
=============================================================

https://hg.mozilla.org/mozilla-central/annotate/ce69b6e1773e/dom/media/MediaCache.cpp#l456
Looks like a null-deref on |entry|.

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1357244. P1 - use stronger assertions to debug non-debug builds.

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

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1357244. P2 - enforce the invariant that a block is in mFreeBlocks iff it has no owners.

http://searchfox.org/mozilla-central/rev/4bd7a206dea5382c97a8a0c30beef668cc449f5b/dom/media/MediaCache.cpp#1529
Note we might return here with mOwners is empty and break the invariant.

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

Comment 3

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

Comment on attachment 8859022 [details]
Bug 1357244. P1 - use stronger assertions to debug non-debug builds.

https://reviewboard.mozilla.org/r/131040/#review134190

Comment 4

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

P2 will take me a little while to review...

Comment 5

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

Comment on attachment 8859023 [details]
Bug 1357244. P2 - enforce the invariant that a block is in mFreeBlocks iff it has no owners.

https://reviewboard.mozilla.org/r/131042/#review134234

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

Thanks!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ae7086408b80
P1 - use stronger assertions to debug non-debug builds. r=gerald
https://hg.mozilla.org/integration/autoland/rev/96e7a9cb5588
P2 - enforce the invariant that a block is in mFreeBlocks iff it has no owners. r=gerald

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/ae7086408b80
https://hg.mozilla.org/mozilla-central/rev/96e7a9cb5588

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

Attachment: 1357244_fix_p2_54_beta.patch

Comment 10

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8859022 [details]
Bug 1357244. P1 - use stronger assertions to debug non-debug builds.

Approval Request Comment
[Feature/Bug causing the regression]:unknown
[User impact if declined]:crash might happen when memory is low.
[Is this code covered by automated tests?]:yes
[Has the fix been verified in Nightly?]:yes
[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?]:low
[Why is the change risky/not risky?]:the change is simple and tested on Try.
[String changes made/needed]:none

Comment 11

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8859834 [details] [diff] [review]
1357244_fix_p2_54_beta.patch

Approval Request Comment
[Feature/Bug causing the regression]:unknown
[User impact if declined]: crash might happen when memory is low.
[Is this code covered by automated tests?]:yes
[Has the fix been verified in Nightly?]:yes
[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?]:low
[Why is the change risky/not risky?]:the change is simple and tested on Try.
[String changes made/needed]:none

Comment 12

[Gerry Chang [:gchang]]

Comment on attachment 8859022 [details]
Bug 1357244. P1 - use stronger assertions to debug non-debug builds.

The volume of crashes in 53/54 is very low. Let's let it ride the train on 55. Beta54-.

Comment 13

[Gerry Chang [:gchang]]

From the crash report, the volume of crashes in 54 is 1 from the past month. Mark 54 won't fix. Let it ride the train on 55 and bake more time.

Comment 14

[Andrei Vaida [:avaida]]

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #11)
> [Is this code covered by automated tests?]:yes
> [Has the fix been verified in Nightly?]:yes
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on JW Wang's assessment on manual testing needs and the fact that this fix has automated coverage.