Closed Bug 1116056 Opened 5 years ago Closed 5 years ago

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

Categories

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

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox36 --- fixed
firefox37 --- fixed
firefox38 --- fixed

People

(Reporter: jya, Assigned: jya)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

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)
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().
Assignee: nobody → ajones
Status: NEW → ASSIGNED
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 ?
Attachment #8547911 - Flags: review?(jyavenard) → review+
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 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
Attachment #8547911 - Flags: review+ → review-
(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.
Keywords: leave-open
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.
Attachment #8547911 - Flags: approval-mozilla-beta?
Attachment #8547911 - Flags: approval-mozilla-aurora?
Target Milestone: --- → mozilla38
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.
Assignee: ajones → nobody
Assignee: nobody → jyavenard
Attached patch Ensure all atoms read are valid (obsolete) — Splinter Review
Ensure all atoms read are valid. We can now check if the moof atom is valid, and that should be checked upon TrackBuffer::AppendData
Attachment #8550665 - Flags: review?(ajones)
Blocks: 1122873
Attached patch Ensure all atoms read are valid (obsolete) — Splinter Review
treat edts as being fully optional and ignore a malformed one.
Attachment #8550666 - Flags: review?(ajones)
Attachment #8550665 - Attachment is obsolete: true
Attachment #8550665 - Flags: review?(ajones)
Attachment #8547911 - Flags: approval-mozilla-beta?
Attachment #8547911 - Flags: approval-mozilla-beta+
Attachment #8547911 - Flags: approval-mozilla-aurora?
Attachment #8547911 - Flags: approval-mozilla-aurora+
Asking Matt Woodrow to review as Anthony is away for the week
Attachment #8550995 - Flags: review?(matt.woodrow)
Attachment #8550666 - Attachment is obsolete: true
Attachment #8550666 - Flags: review?(ajones)
Blocks: 1123507
Attachment #8550995 - Flags: review?(matt.woodrow) → review+
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
Attachment #8550995 - Flags: approval-mozilla-beta?
Attachment #8550995 - Flags: approval-mozilla-aurora?
To clarify: the second uplift request is for the second patch. The first already landed, approved and uplifted.
FWIW: it only affects Fragmented MP4 (so MSE only)
Attachment #8550995 - Flags: approval-mozilla-beta?
Attachment #8550995 - Flags: approval-mozilla-beta+
Attachment #8550995 - Flags: approval-mozilla-aurora?
Attachment #8550995 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.