Closed Bug 465832 Opened 16 years ago Closed 15 years ago

Crash [@ nsTimerImpl::Fire], apparently due to nsOggDecoder::Stop spinning event loop

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: jruderman, Assigned: cajbir)

References

Details

(Keywords: crash, fixed1.9.1, Whiteboard: [sg:critical?])

Crash Data

Attachments

(1 file, 2 obsolete files)

This recursion looks like bad news:

Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: KERN_INVALID_ADDRESS at 0x00000000c00585b1
Crashed Thread:  0

Thread 0 Crashed:
0   nsTimerImpl::Fire() + 1093 (nsTimerImpl.cpp:451)
1   nsTimerEvent::Run() + 192 (nsTimerImpl.cpp:514)
2   nsThread::ProcessNextEvent(int, int*) + 676 (nsThread.cpp:511)
3   NS_ProcessNextEvent_P(nsIThread*, int) + 130 (nsThreadUtils.cpp:227)
4   nsThread::Shutdown() + 397 (nsThread.cpp:464)
5   nsOggDecoder::Stop() + 270 (nsOggDecoder.cpp:1355)
6   nsOggDecoder::PlaybackEnded() + 24 (nsOggDecoder.cpp:1438)
7   nsRunnableMethod<nsOggDecoder>::Run() + 102 (nsThreadUtils.h:265)
8   nsThread::ProcessNextEvent(int, int*) + 676 (nsThread.cpp:511)
9   NS_ProcessNextEvent_P(nsIThread*, int) + 130 (nsThreadUtils.cpp:227)
10  nsThread::Shutdown() + 397 (nsThread.cpp:464)
11  nsOggDecoder::Stop() + 270 (nsOggDecoder.cpp:1355)
12  nsOggDecoder::PlaybackEnded() + 24 (nsOggDecoder.cpp:1438)
13  nsRunnableMethod<nsOggDecoder>::Run() + 102 (nsThreadUtils.h:265)
14  nsThread::ProcessNextEvent(int, int*) + 676 (nsThread.cpp:511)
15  NS_ProcessNextEvent_P(nsIThread*, int) + 130 (nsThreadUtils.cpp:227)
16  nsThread::Shutdown() + 397 (nsThread.cpp:464)
17  nsOggDecoder::Stop() + 270 (nsOggDecoder.cpp:1355)
18  nsOggDecoderShutdown::Run() + 34 (nsOggDecoder.cpp:1209)
19  nsThread::ProcessNextEvent(int, int*) + 676 (nsThread.cpp:511)
20  NS_ProcessPendingEvents_P(nsIThread*, unsigned int) + 146 (nsThreadUtils.cpp:180)
21  nsBaseAppShell::NativeEventCallback() + 181 (nsBaseAppShell.cpp:122)
22  nsAppShell::ProcessGeckoEvents(void*) + 518 (nsAppShell.mm:374)
23  CFRunLoopRunSpecific + 3141
24  CFRunLoopRunInMode + 88
25  RunCurrentEventLoopInMode + 283
26  ReceiveNextEventCommon + 374
27  BlockUntilNextEventMatchingListInMode + 106
28  _DPSNextEvent + 657
29  -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 128
30  -[NSApplication run] + 795
31  nsAppShell::Run() + 288 (nsAppShell.mm:692)
32  nsAppStartup::Run() + 148 (nsAppStartup.cpp:192)
33  XRE_main + 14008 (nsAppRunner.cpp:3264)
34  main + 709 (nsBrowserApp.cpp:156)
35  _start + 216
36  start + 41
Whiteboard: [sg:critical?]
Is there a test case, or any information on how it got there? Shutting down the browser? Closing a page, or navigating away from it?
Would you happen to know if the build where this bug occurred has this patch in it:

https://bugzilla.mozilla.org/show_bug.cgi?id=462878

That patch contains a guard to prevent recursion into ::Stop.
Assignee: nobody → chris.double
I don't have a test case or other information.  I think the build had that patch, but I could have screwed up.
Some of the event's that get posted to the event loop and processed after the decoder is shutdown can callback into JavaScript. So even if that guard from that bug stops recursive calls into stop I think I need to stop those events from firing if the decoder is being shutdown. I'm digging into it.
Attached patch Fixes for wav and ogg (obsolete) — Splinter Review
Adds guards in all event handling functions so they don't run if the being shutdown, which is when the event loop can spin. Adds a guard in the wav backend to prevent recursive calls into Stop().
Attachment #350084 - Flags: superreview?(roc)
Attachment #350084 - Flags: review?
Attachment #350084 - Flags: review? → review?(kinetik)
I think you need to protect ResourceLoaded and NetworkError too.  One minor nit: the code in nsWaveDecoder.cpp always uses braces, even for single-line conditionals.
Attached patch Address review comments (obsolete) — Splinter Review
Attachment #350084 - Attachment is obsolete: true
Attachment #350259 - Flags: superreview?(roc)
Attachment #350259 - Flags: review?(kinetik)
Attachment #350084 - Flags: superreview?(roc)
Attachment #350084 - Flags: review?(kinetik)
Attachment #350259 - Flags: review?(kinetik) → review+
Flags: blocking1.9.1?
Wouldn't it make sense to put mShuttingDown/mStopping in nsMediaDecoder and use them in nsOggDecoder instead of checking PLAY_STATE_SHUTDOWN? That would avoid the need for locking to check whether we're shutting down (since mShuttingDown is main-thread-only) --- simplifying the code a fair bit.
Attachment #350259 - Attachment is obsolete: true
Attachment #350888 - Flags: superreview?(roc)
Attachment #350888 - Flags: review?(kinetik)
Attachment #350259 - Flags: superreview?(roc)
Attachment #350888 - Flags: review?(kinetik) → review+
Attachment #350888 - Flags: superreview?(roc) → superreview+
Whiteboard: [sg:critical?] → [sg:critical?][needs landing]
Flags: blocking1.9.1? → blocking1.9.1+
Chris, do you want me to land this for you or do you want to land it yourself?
Pushed 04307da4e0e6
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [sg:critical?][needs landing] → [sg:critical?][needs 191 landing]
Sorry, that was changeset 1e81ca8ea5a8
Pushed 6dcbb4f48275 to 1.9.1
Keywords: fixed1.9.1
Whiteboard: [sg:critical?][needs 191 landing] → [sg:critical?]
Any chance to get it verified? It even looks like we don't have a valid testcase, or?
Flags: in-testsuite?
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.2a1
Right, this never had a testcase.  IMO, the proper way to create a regression test for this bug is to write a new static analysis script that will catch all bugs of this type.
Flags: in-testsuite? → in-testsuite-
Filed bug 477432 for creating such a static analysis script.
Flags: wanted1.9.0.x-
Flags: wanted1.8.1.x-
Group: core-security
Crash Signature: [@ nsTimerImpl::Fire]
You need to log in before you can comment on or make changes to this bug.