Closed Bug 448636 Opened 12 years ago Closed 12 years ago

Crash [@ theora_decode_packetin] with ogg video

Categories

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

x86
Windows XP
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla1.9.1a2

People

(Reporter: martijn.martijn, Assigned: cajbir)

References

(Blocks 1 open bug)

Details

(Keywords: crash, testcase, verified1.9.1)

Crash Data

Attachments

(5 files, 1 obsolete file)

Attached file testcase (obsolete) —
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
Attached file testcase
Attachment #331834 - Attachment is obsolete: true
Component: DOM: HTML → DOM: Core & HTML
Yes, the 3.1alpha2 build crashes indeed. And I had to kill -9 the process <defunct> to restart the browser.
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.
Status: NEW → ASSIGNED
Ticket raised with Annodex: http://trac.annodex.net/ticket/409
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().
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.
Assignee: nobody → chris.double
Attachment #331921 - Flags: superreview?(roc)
Attachment #331921 - Flags: review?(roc)
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.
Attachment #331922 - Flags: superreview?(roc)
Attachment #331922 - Flags: review?(roc)
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.
Keywords: checkin-needed
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.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
I've trimmed the file so it's a little easier to ship around.
Keywords: checkin-needed
Target Milestone: --- → mozilla1.9.1a2
Component: DOM: Core & HTML → Video/Audio
QA Contact: general → video.audio
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
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.
Keywords: verified1.9.1
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?
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.
(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.
(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?
Test in attachment 392384 [details] [diff] [review] pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/0099558dc16f
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ theora_decode_packetin]
You need to log in before you can comment on or make changes to this bug.