Closed Bug 1387798 Opened 7 years ago Closed 7 years ago

Assertion failure: false in [@ mp4_demuxer::Sbgp::Sbgp]

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Version:
54 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: tsmith, Assigned: ayang)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(6 files, 1 obsolete file)

test_case.mp4
148.57 KB, video/mp4
Details
Bug 1387798 - copy ByteReader.
59 bytes, text/x-review-board-request
kinetik
: review+
Details
Bug 1387798 - rename to BufferReader.
59 bytes, text/x-review-board-request
kinetik
: review+
Details
Bug 1387798 - move initialized codes out of box constructor to another function which could be able to return error.
59 bytes, text/x-review-board-request
kinetik
: review+
Details
Bug 1387798 - use Result as return value in BufferReader.
59 bytes, text/x-review-board-request
kinetik
: review+
Details
Bug 1387798 - remove uncessary check for remaining data and add error log when parsing fails.
59 bytes, text/x-review-board-request
kinetik
: review+
Details
Attached video test_case.mp4 — 
Assertion failure: false, at src/media/libstagefright/binding/include/mp4_demuxer/ByteReader.h:129

Marking s-s because failure is triggered by mp4_demuxer::ByteReader::ReadU32.

#0 0x7f443d96c1e9 in mp4_demuxer::ByteReader::ReadU32() src/media/libstagefright/binding/include/mp4_demuxer/ByteReader.h:129:7
#1 0x7f443d99124f in mp4_demuxer::Sbgp::Sbgp(mp4_demuxer::Box&) src/media/libstagefright/binding/MoofParser.cpp:1007:36
#2 0x7f443d98bde8 in mp4_demuxer::Moof::ParseTraf(mp4_demuxer::Box&, mp4_demuxer::Trex&, mp4_demuxer::Mvhd&, mp4_demuxer::Mdhd&, mp4_demuxer::Edts&, mp4_demuxer::Sinf&, unsigned long*, bool) src/media/libstagefright/binding/MoofParser.cpp:544:14
#3 0x7f443d98b1ca in mp4_demuxer::Moof::Moof(mp4_demuxer::Box&, mp4_demuxer::Trex&, mp4_demuxer::Mvhd&, mp4_demuxer::Mdhd&, mp4_demuxer::Edts&, mp4_demuxer::Sinf&, unsigned long*, bool) src/media/libstagefright/binding/MoofParser.cpp:399:7
#4 0x7f443d9875f5 in mp4_demuxer::MoofParser::RebuildFragmentedIndex(mp4_demuxer::BoxContext&) src/media/libstagefright/binding/MoofParser.cpp:66:12
#5 0x7f443d9871b3 in mp4_demuxer::MoofParser::RebuildFragmentedIndex(mozilla::media::IntervalSet<long> const&) src/media/libstagefright/binding/MoofParser.cpp:36:10
#6 0x7f443d97b0e6 in mp4_demuxer::Index::UpdateMoofIndex(mozilla::media::IntervalSet<long> const&, bool) src/media/libstagefright/binding/Index.cpp:433:16
#7 0x7f444247c25b in mozilla::MP4TrackDemuxer::EnsureUpToDateIndex() src/dom/media/fmp4/MP4Demuxer.cpp:407:11
#8 0x7f444247baf4 in mozilla::MP4TrackDemuxer::MP4TrackDemuxer(mozilla::MP4Demuxer*, mozilla::UniquePtr<mozilla::TrackInfo, mozilla::DefaultDelete<mozilla::TrackInfo> >&&, mp4_demuxer::IndiceWrapper const&) src/dom/media/fmp4/MP4Demuxer.cpp:364:3
#9 0x7f4442479dbc in mozilla::MP4Demuxer::Init() src/dom/media/fmp4/MP4Demuxer.cpp:255:13
#10 0x7f4441f4c465 in mozilla::MediaFormatReader::DemuxerProxy::Init()::$_10::operator()() const src/dom/media/MediaFormatReader.cpp:1027:47
#11 0x7f4441f4c116 in mozilla::detail::ProxyFunctionRunnable<mozilla::MediaFormatReader::DemuxerProxy::Init()::$_10, mozilla::MozPromise<mozilla::MediaResult, mozilla::MediaResult, true> >::Run() src/obj-firefox/dist/include/mozilla/MozPromise.h:1510:29
#12 0x7f443dbab645 in mozilla::TaskQueue::Runner::Run() src/xpcom/threads/TaskQueue.cpp:246:12
#13 0x7f443dbe795e in nsThreadPool::Run() src/xpcom/threads/nsThreadPool.cpp:225:14
#14 0x7f443dbe7ddc in non-virtual thunk to nsThreadPool::Run() src/xpcom/threads/nsThreadPool.cpp:154:15
#15 0x7f443dbdf4b0 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1446:14
#16 0x7f443dbe50f0 in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:480:10
#17 0x7f443e747f64 in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:339:20
#18 0x7f443e6996e7 in MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:326:10
#19 0x7f443e699579 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:299:3
#20 0x7f443dbd761b in nsThread::ThreadFunc(void*) src/xpcom/threads/nsThread.cpp:506:11
#21 0x7f44599985ed in _pt_root src/nsprpub/pr/src/pthreads/ptthread.c:216:5
#22 0x7f445cfa26b9 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76b9)
#23 0x7f445c02b3dc in clone /build/glibc-bfm8X4/glibc-2.23/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:109
Flags: in-testsuite?
Flags: needinfo?(ayang)
Assignee: nobody → ayang
Flags: needinfo?(ayang)
Looks like this will be fixed in bug 1387793
Group: media-core-security
Depends on: 1387793
ByteReader doesn't return an error, so it keeps spinning in MoofParser. I can rewrite it with Result [1] so the error can be handled gracefully.
Unfortunately, ByteReader has been used everywhere. It takes a while to replace them all in short time. So I'll write a new ByteReader and fix this bug first. And deals others later.

[1] https://dxr.mozilla.org/mozilla-central/source/mfbt/Result.h
See Also: → 1392177
I'll refactory others in bug 1392177.
Comment on attachment 8899341 [details]
Bug 1387798 - refactory MoofParser so it gets error when reading malformat video.

https://reviewboard.mozilla.org/r/170558/#review176102

DataReader needs a better name.

It'd be better to have DataReader return Result<T> instead of a bool.  I saw you mention that earlier in the bug, is there a reason you decided against that approach?

I don't think the deprecation comment on ByteReader will stop anyone using it since there's a good chance they won't notice it.  Might be better to mark it with MOZ_DEPRECATED instead.

For ease of review, I'd prefer to see several changesets where you copy ByteReader and only rename it.  Then add the error handling changes in a second changeset.  Then, to reduce duplication, you could replace the internals of the old ByteReader with calls to the new one, but ignoring error results.

::: commit-message-b95b1:1
(Diff revision 1)
> +Bug 1387798 - refactory MoofParser so it gets error when reading malformat video. r?kinetik

Maybe "Return error from MoofParser when underlying ByteReader returns error."?
Attachment #8899341 - Flags: review?(kinetik) → review-
Mass change P1->P2 to align with new Mozilla triage process
Priority: P1 → P2
Blocks: 1405841
INFO: Last good revision: 42cd21f10830c493837d80facdfd0234124f9948
INFO: First bad revision: 4d18b49a5b315020619bd3051964c9dc762232f4
INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=42cd21f10830c493837d80facdfd0234124f9948&tochange=4d18b49a5b315020619bd3051964c9dc762232f4

Note that on trunk, this no longer asserts for. I just gets stuck in an infinite loop of:
WARNING: Failed to read data: file /builds/worker/workspace/build/src/media/libstagefright/binding/include/mp4_demuxer/ByteReader.h, line 124

Would be really nice if we could get this fixed for 58 still especially given bug 1405841.
Blocks: 1318792
Has Regression Range: --- → yes
status-firefox56: --- → wontfix
status-firefox57: affected → wontfix
status-firefox58: --- → affected
status-firefox-esr52: --- → unaffected
Version: Trunk → 54 Branch
Attachment #8899341 - Attachment is obsolete: true
(In reply to Matthew Gregan [:kinetik] from comment #5)
> Comment on attachment 8899341 [details]
> Bug 1387798 - refactory MoofParser so it gets error when reading malformat
> video.
> 
> https://reviewboard.mozilla.org/r/170558/#review176102
> 
> DataReader needs a better name.
> 
> It'd be better to have DataReader return Result<T> instead of a bool.  I saw
> you mention that earlier in the bug, is there a reason you decided against
> that approach?
> 
> I don't think the deprecation comment on ByteReader will stop anyone using
> it since there's a good chance they won't notice it.  Might be better to
> mark it with MOZ_DEPRECATED instead.
> 
> For ease of review, I'd prefer to see several changesets where you copy
> ByteReader and only rename it.  Then add the error handling changes in a
> second changeset.  Then, to reduce duplication, you could replace the
> internals of the old ByteReader with calls to the new one, but ignoring
> error results.
> 
> ::: commit-message-b95b1:1
> (Diff revision 1)
> > +Bug 1387798 - refactory MoofParser so it gets error when reading malformat video. r?kinetik
> 
> Maybe "Return error from MoofParser when underlying ByteReader returns
> error."?

I've separated it into several patches, I hope it'd be easier for you to review. :)

ByteReader is still there now because it is used by other modules. I'll remove it once I fix them.
Comment on attachment 8918171 [details]
Bug 1387798 - copy ByteReader.

https://reviewboard.mozilla.org/r/189032/#review194342
Attachment #8918171 - Flags: review?(kinetik) → review+
Comment on attachment 8918172 [details]
Bug 1387798 - rename to BufferReader.

https://reviewboard.mozilla.org/r/189034/#review194346
Attachment #8918172 - Flags: review?(kinetik) → review+
Comment on attachment 8918173 [details]
Bug 1387798 - move initialized codes out of box constructor to another function which could be able to return error.

https://reviewboard.mozilla.org/r/189036/#review194568
Attachment #8918173 - Flags: review?(kinetik) → review+
Comment on attachment 8918174 [details]
Bug 1387798 - use Result as return value in BufferReader.

https://reviewboard.mozilla.org/r/189038/#review194572

OK with comment addressed

::: media/libstagefright/binding/include/mp4_demuxer/BufferReader.h:107
(Diff revision 1)
> -  uint32_t ReadU24()
> +  Result<uint32_t, bool> ReadU24()
>    {
>      auto ptr = Read(3);
>      if (!ptr) {
>        NS_WARNING("Failed to read data");
>        return 0;

Does this need to return false?
Attachment #8918174 - Flags: review?(kinetik) → review+
Comment on attachment 8918175 [details]
Bug 1387798 - remove uncessary check for remaining data and add error log when parsing fails.

https://reviewboard.mozilla.org/r/189040/#review194576
Attachment #8918175 - Flags: review?(kinetik) → review+
(In reply to Matthew Gregan [:kinetik] from comment #17)
> Comment on attachment 8918174 [details]
> Bug 1387798 - use Result as return value in BufferReader.
> 
> https://reviewboard.mozilla.org/r/189038/#review194572
> 
> OK with comment addressed
> 
> ::: media/libstagefright/binding/include/mp4_demuxer/BufferReader.h:107
> (Diff revision 1)
> > -  uint32_t ReadU24()
> > +  Result<uint32_t, bool> ReadU24()
> >    {
> >      auto ptr = Read(3);
> >      if (!ptr) {
> >        NS_WARNING("Failed to read data");
> >        return 0;
> 
> Does this need to return false?

Yes, thanks for catching this!
Pushed by ayang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6690f01be1bf
copy ByteReader. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/70b9d82b683f
rename to BufferReader. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/bc6312a22923
move initialized codes out of box constructor to another function which could be able to return error. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/12fd12cae6e3
use Result as return value in BufferReader. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/4e892b27c2f1
remove uncessary check for remaining data and add error log when parsing fails. r=kinetik
See Also: → 1409298
Depends on: 1410565
Depends on: 1414444
No longer depends on: 1414444
Regressions: 1414444
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: