Closed
Bug 465832
Opened 16 years ago
Closed 16 years ago
Crash [@ nsTimerImpl::Fire], apparently due to nsOggDecoder::Stop spinning event loop
Categories
(Core :: Audio/Video, defect, P2)
Core
Audio/Video
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)
11.92 KB,
patch
|
kinetik
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Updated•16 years ago
|
Whiteboard: [sg:critical?]
Assignee | ||
Comment 1•16 years ago
|
||
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?
Assignee | ||
Comment 2•16 years ago
|
||
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 | ||
Updated•16 years ago
|
Assignee: nobody → chris.double
Reporter | ||
Comment 3•16 years ago
|
||
I don't have a test case or other information. I think the build had that patch, but I could have screwed up.
Assignee | ||
Comment 4•16 years ago
|
||
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.
Assignee | ||
Comment 5•16 years ago
|
||
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?
Assignee | ||
Updated•16 years ago
|
Attachment #350084 -
Flags: review? → review?(kinetik)
Comment 6•16 years ago
|
||
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.
Assignee | ||
Comment 7•16 years ago
|
||
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)
Updated•16 years ago
|
Attachment #350259 -
Flags: review?(kinetik) → review+
Reporter | ||
Updated•16 years ago
|
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.
Assignee | ||
Comment 9•16 years ago
|
||
Attachment #350259 -
Attachment is obsolete: true
Attachment #350888 -
Flags: superreview?(roc)
Attachment #350888 -
Flags: review?(kinetik)
Attachment #350259 -
Flags: superreview?(roc)
Updated•16 years ago
|
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+
Priority: -- → P2
Chris, do you want me to land this for you or do you want to land it yourself?
Pushed 04307da4e0e6
Status: NEW → RESOLVED
Closed: 16 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?]
Comment 14•15 years ago
|
||
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
Reporter | ||
Comment 15•15 years ago
|
||
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-
Reporter | ||
Comment 16•15 years ago
|
||
Filed bug 477432 for creating such a static analysis script.
Updated•15 years ago
|
Flags: wanted1.9.0.x-
Flags: wanted1.8.1.x-
Updated•15 years ago
|
Group: core-security
Updated•13 years ago
|
Crash Signature: [@ nsTimerImpl::Fire]
You need to log in
before you can comment on or make changes to this bug.
Description
•