604067 - Malformed WebM file leads to crash [@nsWebMReader::ReadMetadata]

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: testcase

Number of values: 8
Offset:  3576/0x000df8 Value: ['80', '00']
Offset: 18555/0x00487b Value: ['00', '00', '00', '00']
Offset: 19803/0x004d5b Value: ['20', '00']
Offset: 35053/0x0088ed Value: ['7f', 'ff', 'ff', 'ff', 'ff', 'ff', 'ff', 'ff']
Offset: 46018/0x00b3c2 Value: ['20', '00']
Offset: 54946/0x00d6a2 Value: ['7f', 'ff', 'ff', 'ff']
Offset: 61965/0x00f20d Value: ['ff', 'ff', 'ff', 'ff', 'ff', 'ff', 'ff', 'ff']
Offset: 96961/0x017ac1 Value: ['20', '00']

Comment 1

[Christoph Diehl [:posidron]]

Attachment: callstack

Comment 2

[timeless]

nsWebMReader::ReadMetadata 
nsBuiltinDecoderStateMachine::LoadMetadata
nsBuiltinDecoderStateMachine::Run

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Problem here is that vorbis_synthesis_init returns non-0 on failure, whereas nsWebMReader assumes it returns < 0 on failure. It actually returns 1 when the codebooks are invalid, and nsWebMReader doesn't treat that has invalid, and we're crashing when trying to use a non-initialized nsWebMReader::mVorbisDsp. Yay for lack of documentation.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Additionally, when nsWebMReader returns failure, the error isn't handled by the state machine, so we still try to use the present-but-invalid tracks anyway, and crash. When one of the streams is invalid in a WebM file is invalid, do we want to play the remaining stream? Neither Chrome nor Opeara do.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I could go either way, but it's something WebM should specify.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 1

Patch 1:
* Treat non-zero return from vorbis functions as failure in nsWebMReader.
* We still treat failure to read metadata of either of the VP8 or Vorbis stream in a WebM file as meaning it's invalid.
* In nsBuiltinDecoderStateMachine::LoadMetadata() treat the failure of the reader's ReadMetadata() as meaning the file is unplayable.
* This means we'll have the same behaviour as Chrome and Opera; we won't play the valid streams in a WebM file when there's one (or more) invalid/erroneous streams. I sent a message to the WebM-discuss mailing list to ask how we should handle such files, but no real responses yet: https://groups.google.com/a/webmproject.org/group/webm-discuss/browse_thread/thread/844629b824f6d5e5# so we may as well match the other browsers.
* Includes test files.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 2

I noticed that when opening the invalid WebM files for this bug as video documents, that we weren't correctly displaying the error UI. This is because the error events were following the same code path which we use when loading from source children (which is the 'default' code path) so the error events weren't being dispatched since when we load in a video document we don't have source children to dispatch the error events to.

This patch changes the media load algorithm to assume we're not loading from source children by default. Errors from video documents follow the same code path as errors for media elements which load from src attributes, i.e. error events get dispatched directly to the video element itself. This was a regression from the new load algorithm.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 483296 [details] [diff] [review]
Patch 2

Should be easy to add a mochitest for this. if you load a video in a same-origin <iframe> you can dig into the iframe's contentDocument to get at the video element we create.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 3: testcase

Adds mochitest. I couldn't figure out how to attach an error event listener to the media element inside an iframe at an appropriate time during the load, so I just check that the error field has the correct error value after the load completes, which is roughly equivalent.

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

http://hg.mozilla.org/mozilla-central/rev/3a1bc45d5a54
http://hg.mozilla.org/mozilla-central/rev/110992838484
http://hg.mozilla.org/mozilla-central/rev/75ac238f8796

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

Backed out due to errors in caused by one of the changesets I pushed this with.

http://hg.mozilla.org/mozilla-central/rev/fb80c67c4df9
http://hg.mozilla.org/mozilla-central/rev/537464253ab3
http://hg.mozilla.org/mozilla-central/rev/f6a2f51db344
http://hg.mozilla.org/mozilla-central/rev/fe78fbd444b5
http://hg.mozilla.org/mozilla-central/rev/16eecca16e86
http://hg.mozilla.org/mozilla-central/rev/3b056c2faa0d
http://hg.mozilla.org/mozilla-central/rev/d21fb01615ad

Some of the errors:

Rev3 Fedora 12 mozilla-central opt test mochitests-5/5
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1287262011.1287262859.26658.gz

19554 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/content/tests/widgets/test_videocontrols.html | [SimpleTest/SimpleTest.js, window.onerror] An error occurred - [object Event] at undefined:undefined
Thread 0 (crashed)
0  libxul.so!nsMediaCacheStream::Close [nsMediaCache.cpp:f9f10c04dceb : 1891 + 0x6]
1  libxul.so!nsMediaChannelStream::Close [nsMediaStream.cpp:f9f10c04dceb : 498 + 0x8]
2  libxul.so!nsWaveDecoder::Stop [nsWaveDecoder.cpp:f9f10c04dceb : 1372 + 0x8]

19557 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/content/tests/widgets/test_videocontrols_audio_direction.html | Test timed out.
19560 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/content/tests/widgets/test_videocontrols_video_direction.html | Test timed out.

Rev3 Fedora 12 mozilla-central debug test reftest
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1287261600.1287263280.28360.gz
Lots of failures...

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

The failures were caused by bug 572579. Landed again without that...

http://hg.mozilla.org/mozilla-central/rev/dcc37d9634b5
http://hg.mozilla.org/mozilla-central/rev/de699f592c81
http://hg.mozilla.org/mozilla-central/rev/0efc5866bd8a