Closed Bug 1149278 Opened 8 years ago Closed 8 years ago

[EME] Crash in demuxer at end of EME playback

Categories

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

Product:
Core
Component:
Audio/Video
Platform:
x86_64
Windows 8.1
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla40
Tracking Flags:
Tracking Status
firefox38 --- fixed
firefox39 --- fixed
firefox40 --- fixed

People

(Reporter: cpearce, Assigned: jya)

References

(Blocks 1 open bug)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file, 2 obsolete files)

Limit box reads to resource length
3.36 KB, patch
ajones
: review+
Sylvestre
: approval-mozilla-aurora+
Sylvestre
: approval-mozilla-beta+
Details | Diff | Splinter Review 
We're crashing when playback on Adobe's EME+MSE test page reaches end of stream in this morning's Nightly build.

https://crash-stats.mozilla.com/report/index/0da48627-387b-4f16-b54c-749112150330

STR:
1. Update your 32bit Nightly build on Windows.
2. Open http://drmtest2.adobe.com/HTML5AAXSPlayer/2/mse-access/index.html
3. Press "Play" button
3. Playback to end of stream (you can seek to near EOS and play from there)
4. Observe crash
Looks similar to bug 1147448.
jya: can you take a look?
Flags: needinfo?(jyavenard)
Do not attempt to read the entire box content to determine if we have the whole box. Should the box contains an invalid size we would crash with an OOM. We can't simply use the length of the provided stream as we want the read to be blocking and not mess with MP4 Invoke & Retry hack. In any case, we can safely assume that if we have the last byte, we must have the entire content.
Attachment #8585970 - Flags: review?(ajones)
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
looking at the segment, I can't see anything wrong with them, yet we read a size here > 1GB. So I'm suspecting something wrong on how we read the size or elsewhere. will look at this tomorrow
Flags: needinfo?(jyavenard)
Doh...

resource.js ends with:
  "media/video/Alice25.m4s",
,

note the trailing ,

This cause appendBuffer to be called with:
"<html><head><title>Web Server X - Error report</title><style><!--H1 {font-family:Tahoma,Arial,sans-serif;color:white;background-color:#525D76;font-size:22px;} H2 {font-family:Tahoma,Arial,sans-serif;color:white;background-color:#525D76;font-size:16px;} H3 {font-family:Tahoma,Arial,sans-serif;color:white;background-color:#525D76;font-size:14px;} BODY {font-family:Tahoma,Arial,sans-serif;color:black;background-color:white;} B {font-family:Tahoma,Arial,sans-serif;color:white;background-color:#525D76;} P {font-family:Tahoma,Arial,sans-serif;background:white;color:black;font-size:12px;}A {color : black;}A.name {color : black;}HR {color : #525D76;}--></style> </head><body><h1>HTTP Status 404 - /HTML5AAXSPlayer/2/mse-access/undefined</h1><HR size="1" noshade="noshade"><p><b>type</b> Status report</p><p><b>message</b> <u>/HTML5AAXSPlayer/2/mse-access/undefined</u></p><p><b>description</b> <u>The requested resource (/HTML5AAXSPlayer/2/mse-access/undefined) is not available.</u></p><HR size="1" noshade="noshade"><h3>Web Server X</h3></body></html>"

That caused the demuxer to attempt to parse this HTML header as if it was a proper fragmented mp4.

Only thing more I can suggest we do on top of this patch, is to check if the fourcc mType is valid and abort if not.
Comment on attachment 8585970 [details] [diff] [review]
Only attempt to read last byte of box content

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

::: media/libstagefright/binding/Box.cpp
@@ +101,1 @@
>      return;

I don't think calling Read() inside the constructor is the right approach at all. It would be better to get rid of the read call altogether and put the logic to handle this case inside MoofParser::BlockingReadNextMoof().
Attachment #8585970 - Flags: review?(ajones) → review+
making change as discussed on IRC (using mediaresource length if known)
Attachment #8585970 - Attachment is obsolete: true
Attachment #8586578 - Flags: review?(ajones)
Assignee: jyavenard → ajones
Attachment #8586578 - Attachment is obsolete: true
Attachment #8586578 - Flags: review?(ajones)
3rd time lucky?
Attachment #8587141 - Flags: review?(ajones)
Assignee: ajones → jyavenard
Attachment #8587141 - Flags: review?(ajones) → review+
Needs uplift.
Flags: needinfo?(cpearce)
With the patch in as-is, it regressed YouTube jumping straight to a given position like:
https://youtu.be/HvRypx1lbR4?t=413
Depends on: 1151299
https://hg.mozilla.org/mozilla-central/rev/cd65aead5419
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment on attachment 8587141 [details] [diff] [review]
Limit box reads to resource length

Approval Request Comment
[Feature/regressing bug #]: EME+MSE
[User impact if declined]: If bad input is sent to a MediaSource, Firefox can crash. This affects EME (issue was discovered in an EME test case) and also affects MSE.
[Describe test coverage new/current, TreeHerder]: We have lots of MSE and EME mochittests
[Risks and why]: Seems low; makes us smarter about reading data.
[String/UUID change made/needed]: None.
Attachment #8587141 - Flags: approval-mozilla-beta?
Attachment #8587141 - Flags: approval-mozilla-aurora?
Comment on attachment 8587141 [details] [diff] [review]
Limit box reads to resource length

Should be in 38 beta 3.
Attachment #8587141 - Flags: approval-mozilla-beta?
Attachment #8587141 - Flags: approval-mozilla-beta+
Attachment #8587141 - Flags: approval-mozilla-aurora?
Attachment #8587141 - Flags: approval-mozilla-aurora+
Flags: needinfo?(cpearce)
Crash Signature: [@ OOM | large | mozalloc_abort(char const* const) | mozalloc_handle_oom(unsigned int) | moz_xmalloc | nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_CopyWithMemutils>::EnsureCapacity(unsigned int, unsigned int) | nsTArray_base<nsTArrayInfallibleAllo…
Keywords: crash
You need to log in before you can comment on or make changes to this bug.