464007 - crash [@ oggplay_buffer_set_last_data]

Status: VERIFIED FIXED

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

Description

[(mostly gone) XtC4UaLL [:xtc4uall]]

this has no real reliable STRs:

- open site (or data:text/html,<video src="http://piercedotzler.com/asa/Munch-Autumn.ogg" controls></video>)
- press play
- let the video buffer its way
- open other site in new tab
- close tab with the video whilst other site is still loading
=> crash with Minefield/3.1b2pre ID:20081109032233 on XP

Signature	oggplay_buffer_set_last_data
UUID	765a57b3-aef9-11dd-869e-001cc45a2c28
Time	2008-11-09 23:29:55-08
Uptime	3587
Product	Firefox
Version	3.1b2pre
Build ID	20081109032233
OS	Windows NT
OS Version	5.1.2600 Service Pack 3
CPU	x86
CPU Info	GenuineIntel family 15 model 2 stepping 9
Crash Reason	EXCEPTION_ACCESS_VIOLATION
Crash Address	0xc

Frame 	Module 	Signature 	Source
0 	xul.dll 	oggplay_buffer_set_last_data 	media/liboggplay/src/liboggplay/oggplay_buffer.c:133
1 	xul.dll 	oggplay_step_decoding 	media/liboggplay/src/liboggplay/oggplay.c:528
2 	xul.dll 	nsOggDecodeStateMachine::DecodeFrame() 	content/media/video/src/nsOggDecoder.cpp:475
3 	xul.dll 	nsOggDecodeStateMachine::Run() 	content/media/video/src/nsOggDecoder.cpp:877
4 	xul.dll 	nsThread::ProcessNextEvent(int,int*) 	xpcom/threads/nsThread.cpp:510
5 	xul.dll 	nsThread::ThreadFunc(void*) 	xpcom/threads/nsThread.cpp:254
6 	nspr4.dll 	_PR_NativeRunThread 	nsprpub/pr/src/threads/combined/pruthr.c:436
7 	nspr4.dll 	pr_root 	nsprpub/pr/src/md/windows/w95thred.c:122
8 	mozcrt19.dll 	_callthreadstartex 	obj-firefox/memory/jemalloc/src/threadex.c:348
9 	mozcrt19.dll 	_threadstartex 	obj-firefox/memory/jemalloc/src/threadex.c:326
10 	kernel32.dll 	kernel32.dll@0xb712

bp-765a57b3-aef9-11dd-869e-001cc45a2c28
bp-551a43ca-af24-11dd-9d5a-001a4bd43e5c

Comment 1

[(mostly gone) XtC4UaLL [:xtc4uall]]

bp-789524d7-af0e-11dd-99fb-001cc4e2bf68 is on linux (found via crash-stats.mozilla.com, with comment "Tried to play some Ogg/Theora demos and Minefield crashed") => ALL

Comment 2

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

I just reproduced this on Vista, it looks like while we enter DecodeFrame() from nsOggDecodeStateMachine::Run(), we shutdown, which I presume is freeing something Ogg then goes on to try to use (the buffer list?)

Comment 3

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

Attachment: Crash stack dump

Here's a stack for this that I ran into on my mac mini while running video mochitests.

Comment 4

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

(In reply to comment #3)
> Created an attachment (id=348699) [details]
> Crash stack dump
> 
> Here's a stack for this that I ran into on my mac mini while running video
> mochitests.

Note I had patches for bug 462878 and bug 451958 applied, so line numbers won't match up exactly.

Comment 5

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

Attachment: Patch v1 - Synchronize oggplay_prepare_for_close() on decode thread

I think this will fix the crash. The problem is that while we're decoding, we switch to shutdown state, which calls oggplay_prepare_for_close(), which releases buffers, which causes the decode to use a released buffer and causes the crash. This patch moves the call to oggplay_prepare_for_close() to be synchronized on the decode thread, once we've transitioned to shutdown state. That way we can never be decoding after oggplay_prepare_for_close() is called.

I have seen this intermittently while running mochitests, so I think this crash *may* be what's caused the tinderbox to crash on mac when bug 451958 was checked in. I can't be sure though, as I don't know how to get call stacks for tinderbox crashes.

Comment 6

[cajbir (:cajbir)]

oggplay_prepare_for_close does not release any buffers. It must be called on the non-decode thread. Why? Because what oggplay_prepare_for_close does is reset a semaphore that tells any thread blocked on oggplay_step_decoding to unblock and exit. So the intent of calling it on the main thread is to immediately cause the decode thread to unblock if it is blocked on that call so it can process the shutdown change state.

I don't see how this can cause the crash you're seeing. Can you go into more detail why you think this is the case?

Comment 7

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

(In reply to comment #6)
> oggplay_prepare_for_close does not release any buffers.

Ok, maybe not directly, but something is releasing buffers, which is causing oggplay_buffer_set_last_data() to crash at oggplay_buffer.c:133...

> I don't see how this can cause the crash you're seeing. Can you go into more
> detail why you think this is the case?

Well we are still using buffers etc after the decode thread has transitioned to shutdown state, so I naturally suspect that we've begun shutting down somehow and released buffers etc prematurely. Perhaps the fact that the player's shutdown flag is set causes it to not follow a code path which results in the buffer being null?

I also didn't observe this crash once I applied the patch, and I have been thrashing mochitests over the past few days. Though honestly, it's an intermittent crash, and can't be reliably reproduced.

Comment 8

[cajbir (:cajbir)]

(In reply to comment #7)
> Perhaps the fact that the player's
> shutdown flag is set causes it to not follow a code path which results in the
> buffer being null?

If this is the case, and the bug is in liboggplay, then it should be fixed there.

> I also didn't observe this crash once I applied the patch, and I have been
> thrashing mochitests over the past few days. Though honestly, it's an
> intermittent crash, and can't be reliably reproduced.

Calling oggplay_prepare_for_close makes no sense on the decode thread. You might as well just remove it, since the only intent of it is to release the semaphore that oggplay_step_decoding waits on. If oggplay_step_decoding is blocked waiting on this semaphore, then the decode thread is not running and ogplay_prepare_for_close cannnot be called.

If you remove it then oggplay_step_decoding can block forever if its buffer gets full.

Comment 9

[cajbir (:cajbir)]

Also note the comment in nsOggDecoder.h:

>If the decode thread is blocked due to internal decode buffers being
>full, it is unblocked during the shutdown process by calling
>oggplay_prepare_for_close.
>
>In practice the OggPlay internal buffer should never fill as we retrieve and
>process the frame immediately on decoding.

It may be that removing the call to oggplay_prepare_for_close is enough to workaround the bug if it is a bug in liboggplay, given that we take steps to ensure the internal buffer is never full. You'll need to fix this comment for your changes.

Comment 11

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

Is this still a problem. We need to figure out how to resolve this.

Comment 12

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

(In reply to comment #11)
> Is this still a problem. We need to figure out how to resolve this.

Yes, I hit this crash a few days ago. I still don't have a reliable way to reproduce though.

Comment 13

[Axel Hecht [:Pike]]

I hit this while looking at asa's airmozilla test right now, http://crash-stats.mozilla.com/report/index/cc7a3d70-1a9f-4f50-91b7-a00ad2090109?p=1

Happened after hours of ogging, so this is far from reproducable, stack trace looks slightly different though.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Got this while running mochitest on 64bit-linux
#0  oggplay_buffer_set_last_data (me=0x5c8eb60, buffer=0x59ef9e0)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/media/liboggplay/src/liboggplay/oggplay_buffer.c:133
#1  0x00002aaab0bc80d2 in oggplay_step_decoding (me=0x5c8eb60)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/media/liboggplay/src/liboggplay/oggplay.c:528
#2  0x00002aaab0bc0ccd in nsOggDecodeStateMachine::Run (this=0x2aaabc40d4c0)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/content/media/video/src/nsOggDecoder.cpp:920
#3  0x00002aaaab27e285 in nsThread::ProcessNextEvent (this=0x3fb8390, mayWait=1, result=0x4640908c)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/xpcom/threads/nsThread.cpp:510
#4  0x00002aaaab23b663 in NS_ProcessNextEvent_P (thread=0x5c8eb60, mayWait=1) at nsThreadUtils.cpp:230
#5  0x00002aaaab27e7a7 in nsThread::ThreadFunc (arg=<value optimized out>)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/xpcom/threads/nsThread.cpp:254
#6  0x00002aaaab91e295 in _pt_root (arg=<value optimized out>)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/nsprpub/pr/src/pthreads/ptthread.c:221
#7  0x00000032d90062f7 in start_thread () from /lib64/libpthread.so.0
#8  0x00000032d70d0fbd in clone () from /lib64/libc.so.6


(gdb) p p
$1 = (OggPlayCallbackInfo *) 0x0

Comment 15

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

Looks to me like the problem is in the end-of-file path in oggplay.c. It calls me->callback:
http://mxr.mozilla.org/mozilla-central/source/media/liboggplay/src/liboggplay/oggplay.c#520
but it never checks me->shutdown on this path. It just goes ahead and calls oggplay_buffer_set_last_data. However, when me->shutdown is set in oggplay_buffer_callback we exit early:
http://mxr.mozilla.org/mozilla-central/source/media/liboggplay/src/liboggplay/oggplay_buffer.c#158
This leaves the buffer object in a very different state to the state it's in when oggplay_buffer_callback does not exit early. In particular, oggplay_buffer_callback would normally initialize  buffer->buffer_list[buffer->last_filled], but when me->shutdown is set, it doesn't. I strongly suspect this leads to the crash in oggplay_buffer_set_last_data --- the pointer reference at line 133 that we crash on is exactly "buffer->buffer_list[buffer->last_filled]".

Comment 16

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

Attachment: fix

Viktor, what do you think of this patch?

Comment 17

[cajbir (:cajbir)]

Attachment: Packaged up as a patch to liboggplay

This is Robert's fix, with the addition of it as a patch to be applied whenever we update the liboggplay version. We can't currently update to liboggplay tip as it has changes that break code.

Comment 18

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

Pushed http://hg.mozilla.org/mozilla-central/rev/3509c34d4ffb to trunk.

I'm marking this fixed although we can't be 100% sure it will fix the crash, since the crash is not easily reproducible. However, I did see the crash once today, and when I checked me->shutdown it was 1, and with this patch we can't reach oggplay_buffer_set_last_data with me->shutdown set to 1, so I'm confident.

Comment 19

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

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4d62592fcc67

Comment 20

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090507 Minefield/3.6a1pre 
and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b5pre) Gecko/20090507 Shiretoko/3.5b5pre

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

A crash with the same stack is still existent. I've filed it as bug 493936.