Closed
Bug 464007
Opened 16 years ago
Closed 16 years ago
crash [@ oggplay_buffer_set_last_data]
Categories
(Core :: Audio/Video, defect, P2)
Tracking
()
VERIFIED
FIXED
mozilla1.9.2a1
People
(Reporter: xtc4uall, Assigned: roc)
References
()
Details
(Keywords: crash, verified1.9.1)
Crash Data
Attachments
(3 files, 1 obsolete file)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
roc
:
review-
roc
:
superreview-
|
Details | Diff | Splinter Review |
(deleted),
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•16 years ago
|
||
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
OS: Windows XP → All
Updated•16 years ago
|
Flags: blocking1.9.1?
Comment 2•16 years ago
|
||
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?)
Assignee | ||
Updated•16 years ago
|
Flags: blocking1.9.1? → blocking1.9.1+
Comment 3•16 years ago
|
||
Here's a stack for this that I ran into on my mac mini while running video mochitests.
Comment 4•16 years ago
|
||
(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•16 years ago
|
||
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.
Assignee: nobody → chris
Status: NEW → ASSIGNED
Attachment #349032 -
Flags: superreview?(roc)
Attachment #349032 -
Flags: review?(chris.double)
Comment 6•16 years ago
|
||
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•16 years ago
|
||
(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•16 years ago
|
||
(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•16 years ago
|
||
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.
Assignee | ||
Updated•16 years ago
|
Attachment #349032 -
Flags: superreview?(roc)
Attachment #349032 -
Flags: superreview-
Attachment #349032 -
Flags: review?(chris.double)
Attachment #349032 -
Flags: review-
Assignee | ||
Updated•16 years ago
|
Priority: -- → P2
Assignee | ||
Comment 11•16 years ago
|
||
Is this still a problem. We need to figure out how to resolve this.
Comment 12•16 years ago
|
||
(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•16 years ago
|
||
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•16 years ago
|
||
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
Assignee | ||
Comment 15•16 years ago
|
||
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]".
Assignee | ||
Comment 16•16 years ago
|
||
Viktor, what do you think of this patch?
Assignee: chris → roc
Attachment #359246 -
Flags: review?
Assignee | ||
Updated•16 years ago
|
Attachment #359246 -
Flags: review? → review?(wiking)
Assignee | ||
Updated•16 years ago
|
Whiteboard: [needs review]
Comment 17•16 years ago
|
||
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.
Attachment #359246 -
Attachment is obsolete: true
Attachment #359434 -
Flags: superreview?(roc)
Attachment #359434 -
Flags: review?(roc)
Attachment #359246 -
Flags: review?(wiking)
Assignee | ||
Updated•16 years ago
|
Attachment #359434 -
Flags: superreview?(roc)
Attachment #359434 -
Flags: superreview+
Attachment #359434 -
Flags: review?(roc)
Attachment #359434 -
Flags: review+
Assignee | ||
Updated•16 years ago
|
Whiteboard: [needs review] → [needs landing]
Assignee | ||
Comment 18•16 years ago
|
||
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.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 landing]
Assignee | ||
Comment 19•16 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4d62592fcc67
Keywords: fixed1.9.1
Whiteboard: [needs 191 landing]
Comment 20•15 years ago
|
||
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
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
Comment 21•15 years ago
|
||
A crash with the same stack is still existent. I've filed it as bug 493936.
Target Milestone: --- → mozilla1.9.2a1
Updated•13 years ago
|
Crash Signature: [@ oggplay_buffer_set_last_data]
You need to log in
before you can comment on or make changes to this bug.
Description
•