1116056 - MSE: MOZ_ASSERT(false) in Box::Read(nsTArray<uint8_t>* aDest)

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

Occurred twice today while investigating with 1113924 and playing: https://www.youtube.com/watch?v=M6RfU9eVaFE

I'm on a 3G data modem, not super fast speed. Default video resolution is 244p, I change to 720p following by 1080p.

Caused a crash.

(lldb) bt
* thread #209: tid = 0x12a03c, 0x0000000103596197 XUL`mp4_demuxer::Box::Read(this=0x0000000141f40010, aDest=0x0000000141f3ff28) + 231 at Box.cpp:101, name = 'Media Decode #8', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x0000000103596197 XUL`mp4_demuxer::Box::Read(this=0x0000000141f40010, aDest=0x0000000141f3ff28) + 231 at Box.cpp:101
    frame #1: 0x00000001035c29b7 XUL`mp4_demuxer::BoxReader::BoxReader(this=0x0000000141f3ff10, aBox=0x0000000141f40010) + 71 at Box.h:67
    frame #2: 0x00000001035b39fd XUL`mp4_demuxer::BoxReader::BoxReader(this=0x0000000141f3ff10, aBox=0x0000000141f40010) + 29 at Box.h:69
    frame #3: 0x000000010359b551 XUL`mp4_demuxer::Moof::ParseTrun(this=0x0000000141f401e0, aBox=0x0000000141f40010, aTfhd=0x0000000141f40048, aTfdt=0x0000000141f40040, aMdhd=0x000000012acea9a8, aEdts=0x000000012acea9e8) + 81 at MoofParser.cpp:293
    frame #4: 0x000000010359adb3 XUL`mp4_demuxer::Moof::ParseTraf(this=0x0000000141f401e0, aBox=0x0000000141f400e8, aTrex=0x000000012acea9c8, aMdhd=0x000000012acea9a8, aEdts=0x000000012acea9e8) + 355 at MoofParser.cpp:254
    frame #5: 0x000000010359abf6 XUL`mp4_demuxer::Moof::Moof(this=0x0000000141f401e0, aBox=0x0000000141f40240, aTrex=0x000000012acea9c8, aMdhd=0x000000012acea9a8, aEdts=0x000000012acea9e8) + 278 at MoofParser.cpp:172
    frame #6: 0x000000010359a4f5 XUL`mp4_demuxer::Moof::Moof(this=0x0000000141f401e0, aBox=0x0000000141f40240, aTrex=0x000000012acea9c8, aMdhd=0x000000012acea9a8, aEdts=0x000000012acea9e8) + 53 at MoofParser.cpp:176
    frame #7: 0x000000010359a30e XUL`mp4_demuxer::MoofParser::RebuildFragmentedIndex(this=0x000000012acea980, aContext=0x0000000141f40348) + 302 at MoofParser.cpp:31
    frame #8: 0x0000000103599571 XUL`mp4_demuxer::MoofParser::BlockingReadNextMoof(this=0x000000012acea980) + 401 at MoofParser.cpp:85
    frame #9: 0x0000000103599287 XUL`mp4_demuxer::SampleIterator::Get(this=0x0000000150f381e0) + 167 at Index.cpp:146
    frame #10: 0x0000000103598b96 XUL`mp4_demuxer::SampleIterator::GetNext(this=0x0000000150f381e0) + 54 at Index.cpp:88
    frame #11: 0x000000010359d8aa XUL`mp4_demuxer::MP4Demuxer::DemuxAudioSample(this=0x0000000148da39c0) + 122 at mp4_demuxer.cpp:196
    frame #12: 0x00000001062a09cf XUL`mozilla::MP4Reader::PopSampleLocked(this=0x00000001483b6000, aTrack=kAudio) + 111 at MP4Reader.cpp:648
    frame #13: 0x00000001062a093c XUL`mozilla::MP4Reader::PopSample(this=0x00000001483b6000, aTrack=kAudio) + 60 at MP4Reader.cpp:639
    frame #14: 0x00000001062a04dc XUL`mozilla::MP4Reader::Update(this=0x00000001483b6000, aTrack=kAudio) + 700 at MP4Reader.cpp:589
    frame #15: 0x00000001062a2915 XUL`nsRunnableMethodImpl<void (this=0x000000012922c180)(mp4_demuxer::TrackType), mp4_demuxer::TrackType, true>::Run() + 197 at nsThreadUtils.h:361
    frame #16: 0x00000001060eee49 XUL`mozilla::MediaTaskQueue::Runner::Run(this=0x0000000129f9cca0) + 633 at MediaTaskQueue.cpp:230
    frame #17: 0x00000001037057a7 XUL`nsThreadPool::Run(this=0x000000011b079060) + 967 at nsThreadPool.cpp:225
    frame #18: 0x000000010370589c XUL`non-virtual thunk to nsThreadPool::Run(this=0x000000011b079068) + 28 at nsThreadPool.cpp:239
    frame #19: 0x0000000103702208 XUL`nsThread::ProcessNextEvent(this=0x0000000114b337a0, aMayWait=false, aResult=0x0000000141f40c4e) + 2088 at nsThread.cpp:855
    frame #20: 0x000000010375ba47 XUL`NS_ProcessNextEvent(aThread=0x0000000114b337a0, aMayWait=false) + 151 at nsThreadUtils.cpp:265
    frame #21: 0x0000000103d87e03 XUL`mozilla::ipc::MessagePumpForNonMainThreads::Run(this=0x0000000114bcc940, aDelegate=0x000000010042d5c0) + 691 at MessagePump.cpp:339
    frame #22: 0x0000000103d1f285 XUL`MessageLoop::RunInternal(this=0x000000010042d5c0) + 117 at message_loop.cc:233
    frame #23: 0x0000000103d1f195 XUL`MessageLoop::RunHandler(this=0x000000010042d5c0) + 21 at message_loop.cc:226
    frame #24: 0x0000000103d1f13d XUL`MessageLoop::Run(this=0x000000010042d5c0) + 45 at message_loop.cc:200
    frame #25: 0x0000000103700696 XUL`nsThread::ThreadFunc(aArg=0x0000000114b337a0) + 358 at nsThread.cpp:356
    frame #26: 0x0000000103371d0f libnss3.dylib`_pt_root(arg=0x000000011b04bc60) + 463 at ptthread.c:212
    frame #27: 0x00007fff8dbec2fc libsystem_pthread.dylib`_pthread_body + 131
    frame #28: 0x00007fff8dbec279 libsystem_pthread.dylib`_pthread_start + 176
    frame #29: 0x00007fff8dbea4b1 libsystem_pthread.dylib`thread_start + 13
(lldb)

Comment 1

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

This probably means that the read is failing. It may be due to YouTube attempting to start at a higher rate and aborting. We can probably just change this to an NS_ASSERTION().

Comment 2

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Change MOZ_ASSERT() to NS_ASSERTION() in Box::Read()

Comment 3

[Jean-Yves Avenard [:jya]]

Comment on attachment 8547911 [details] [diff] [review]
Change MOZ_ASSERT() to NS_ASSERTION() in Box::Read()

Review of attachment 8547911 [details] [diff] [review]:
-----------------------------------------------------------------

why not make it just a warning with an informative message ?

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 8547911 [details] [diff] [review]
Change MOZ_ASSERT() to NS_ASSERTION() in Box::Read()

Review of attachment 8547911 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/libstagefright/binding/Box.cpp
@@ +97,5 @@
>    if (!mContext->mSource->CachedReadAt(mChildOffset, &(*aDest)[0],
>                                         aDest->Length(), &bytes) ||
>        bytes != aDest->Length()) {
>      // Byte ranges are being reported incorrectly
> +    NS_ASSERTION(false, "Read filed in mp4_demuxer::Box::Read()");

Nit: "failed". And yes, this should just be NS_WARNING.

Comment 5

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2896167fae14

Comment 6

[Jean-Yves Avenard [:jya]]

Comment on attachment 8547911 [details] [diff] [review]
Change MOZ_ASSERT() to NS_ASSERTION() in Box::Read()

Review of attachment 8547911 [details] [diff] [review]:
-----------------------------------------------------------------

changing my review.

By removing the assert there, the Box constructor will now call ByteReader with an empty array, which will assert there instead.

assert needs to be removed, and array not cleared

Comment 7

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Jean-Yves Avenard [:jya] from comment #6)
> changing my review.

To quote swiper the fox "you're too late". The issue is that we aren't propagating the error properly. This probably counts as EOS.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2896167fae14

Comment 9

[Ralph Giles (:rillian)]

Comment on attachment 8547911 [details] [diff] [review]
Change MOZ_ASSERT() to NS_ASSERTION() in Box::Read()

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: More crashes with Youtube.
[Describe test coverage new/current, TBPL]: Landed on m-c.
[Risks and why]: Risk is low. At worst it could crash somewhere else instead. Does affect non-MSE mp4 playback.
[String/UUID change made/needed]: None.

Comment 10

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

I won't have time to fix this today. Leaving for someone else to pick up. The read error condition needs to be propagated and handled properly. The code was originally written for only parsing buffered data. Fixing it by pre-reading the Moof in BlockingReadNextMoof may be the best approach.

Comment 11

[Jean-Yves Avenard [:jya]]

Attachment: Ensure all atoms read are valid

Ensure all atoms read are valid. We can now check if the moof atom is valid, and that should be checked upon TrackBuffer::AppendData

Comment 12

[Jean-Yves Avenard [:jya]]

Attachment: Ensure all atoms read are valid

treat edts as being fully optional and ignore a malformed one.

Comment 13

[Jean-Yves Avenard [:jya]]

Attachment: Ensure all atoms read are valid

Asking Matt Woodrow to review as Anthony is away for the week

Comment 14

[Jean-Yves Avenard [:jya]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c9d2b07a108d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1a1a66376625

Comment 15

[Ralph Giles (:rillian)]

Comment on attachment 8550995 [details] [diff] [review]
Ensure all atoms read are valid

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Less consistent testing. Missing dependency for subsequent crash fixes.
[Describe test coverage new/current, TBPL]: Presuming green on inbound.
[Risks and why]: This does affect normal mp4 playback, but just attempts to rewrite the parse code to be more safe. We will reject more invalid files.
[String/UUID change made/needed]: None

Comment 16

[Ralph Giles (:rillian)]

To clarify: the second uplift request is for the second patch. The first already landed, approved and uplifted.

Comment 17

[Jean-Yves Avenard [:jya]]

FWIW: it only affects Fragmented MP4 (so MSE only)

Comment 18

[Carsten Book [:Tomcat]]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0cbf191913ec
https://hg.mozilla.org/releases/mozilla-aurora/rev/00a3cf2000a7

https://hg.mozilla.org/releases/mozilla-beta/rev/54386fba64a7
https://hg.mozilla.org/releases/mozilla-beta/rev/1f392909ff1f