448636 - Crash [@ theora_decode_packetin] with ogg video

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, which crashes current trunk build on load.
I guess the ogg video file is invalid, but Mozilla should not crash on it.

http://crash-stats.mozilla.com/report/index/f27859f5-5e69-11dd-9801-001cc4e2bf68?p=1
0  	xul.dll  	theora_decode_packetin  	 modules/libtheora/lib/dec/decapiwrapper.c:170
1 	xul.dll 	oggplay_callback_theora 	modules/liboggplay/src/liboggplay/oggplay_callback.c:133
2 	xul.dll 	oggplay_callback_predetected 	modules/liboggplay/src/liboggplay/oggplay_callback.c:552
3 	xul.dll 	oggz_read_deliver_packet 	modules/liboggz/src/liboggz/oggz_read.c:326
4 	xul.dll 	oggz_dlist_deliter 	modules/liboggz/src/liboggz/oggz_dlist.c:149
5 	xul.dll 	oggz_read_sync 	modules/liboggz/src/liboggz/oggz_read.c:474
6 	xul.dll 	oggz_read 	modules/liboggz/src/liboggz/oggz_read.c:629
7 	xul.dll 	oggplay_initialise 	modules/liboggplay/src/liboggplay/oggplay.c:103
8 	xul.dll 	oggplay_open_with_reader 	modules/liboggplay/src/liboggplay/oggplay.c:137
9 	xul.dll 	nsOggDecoder::LoadOggHeaders 	content/media/video/src/nsOggDecoder.cpp:611
10 	xul.dll 	nsRunnableMethod<nsScriptLoader>::Run 	obj-firefox/dist/include/xpcom/nsThreadUtils.h:261
11 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:510
12 	xul.dll 	nsThread::ThreadFunc 	xpcom/threads/nsThread.cpp:254
13 		@0x9afffab

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[ivo]

Yes, the 3.1alpha2 build crashes indeed. And I had to kill -9 the process <defunct> to restart the browser.

Comment 3

[cajbir (:cajbir)]

This appears to be a problem with liboggplay since it's own player crashes with this file, whereas libtheora handles it fine (reporting an error). I'm working with annodex people to see what the problem is.

Comment 4

[cajbir (:cajbir)]

Ticket raised with Annodex: http://trac.annodex.net/ticket/409

Comment 5

[Ralph Giles (:rillian)]

My hypothesis is that liboggplay (or liboggz) is ignoring the corruption when libogg reports it. The decoder doesn't get initialized, and then segfaults when it tries to decode without being set up by the header packet.

Therefore there are two bugs here: liboggplay needs to detect the corruption, and libtheora needs to do more checking in th_decode_packetin().

Comment 6

[Ralph Giles (:rillian)]

The segfault can be moved to liboggplay by applying r15144 from http://svn.xiph.org/trunk/theora

https://trac.xiph.org/changeset/15144

Comment 7

[cajbir (:cajbir)]

Attachment: Apply libtheora changeset r15144 to fix corruption issues

This applies the changeset r15144 from libtheora. Fix provided by Ralph Giles as per comment 6. 

A change is also required for liboggplay. Patch for that following shortly.

Comment 8

[cajbir (:cajbir)]

Attachment: Apply liboggplay changset r3673 to fix corruption issues

patch for liboggplay. I applied the fix to liboggplay svn and imported the changes into the mozilla tree.

http://trac.annodex.net/changeset/3673

This patch combined with the theora patch in attachment 331921 [details] [diff] [review] fixes the issue.

Comment 9

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

I don't want to review Ogg patches. You should feel free to apply Ogg patches from upstream without review. If you write your own Ogg patches, ideally you'd get them reviewed by upstream maintainers.

I'll leave the decision about whether to cherrypick patches or do a full update to you.

Comment 10

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

Pushed 249c616e4355 and 4f50285c3a41.

We need a testcase here. It would be nice if Martijn's testcase could be reduced in size. It would also be nice if we could test that the right errors are raised in the element and that fallback is triggered correctly.

Comment 11

[Ralph Giles (:rillian)]

Attachment: truncated version of the testcase file which still shows the problem

I've trimmed the file so it's a little easier to ship around.

Comment 12

[Yannis]

verified1.9.1 using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b5pre) Gecko/20090428 Shiretoko/3.5b5pre on x86 Windows XP SP3

Comment 13

[Marcia Knous [:marcia]]

Adding the verified1.9.1 keyword since this is fine based on Comment12 and my testing on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b5pre) Gecko/20090430 Shiretoko/3.5b5pre.

Comment 14

[cmtalbert]

I can't get this video to play anywhere - not in current trunk or shiretoko.  It doesn't crash but it also doesn't play.

Furthermore, I can no longer reproduce the crash when testing with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1a2pre) Gecko/2008072903 Minefield/3.1a2pre

So we aren't crashing which is good, but we also aren't playing, which is bad?  Should I file a new bug or reopen this one?

Comment 15

[cajbir (:cajbir)]

The file is corrupt - it's not a valid file - that's why we don't play it. mplayer also doesn't play it for example.

Comment 16

[cmtalbert]

(In reply to comment #15)
> The file is corrupt - it's not a valid file - that's why we don't play it.
> mplayer also doesn't play it for example.

Ok, thanks, I didn't realize we gated on corrupt files. So a simple crash test should be sufficient to clear the in-testsuite flag, just a simple "does-the-page-load-without-crashing crashtest.  We'll have it coming right up.

Comment 17

[cmtalbert]

(In reply to comment #16)
> (In reply to comment #15)
> > The file is corrupt - it's not a valid file - that's why we don't play it.
> > mplayer also doesn't play it for example.
> 
> Ok, thanks, I didn't realize we gated on corrupt files. So a simple crash test
> should be sufficient to clear the in-testsuite flag, just a simple
> "does-the-page-load-without-crashing crashtest.  We'll have it coming right up.

Chris, Sorry, but I'm confused.  We were going to make a quick reftest for this, but on very recent builds, this renders as a "loading" video.  It has the gray video box and the throbber as though the video is buffering. Is that expected?  If we're not going to play it, then we probably shouldn't show anything the way we did until recently (it just rendered to blank but the controls were still active on mouseover (ideally those should also be disabled)).

What's the expected behavior here?

Comment 18

[cajbir (:cajbir)]

Attachment: Testcase added to media testing framework

Comment 19

[cajbir (:cajbir)]

Test in attachment 392384 [details] [diff] [review] pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/0099558dc16f