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

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

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

Comment 1

[cajbir (:cajbir)]

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?

Comment 2

[cajbir (:cajbir)]

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.

Comment 3

[Jesse Ruderman]

I don't have a test case or other information.  I think the build had that patch, but I could have screwed up.

Comment 4

[cajbir (:cajbir)]

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.

Comment 5

[cajbir (:cajbir)]

Attachment: Fixes for wav and ogg

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().

Comment 6

[Matthew Gregan [: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.

Comment 7

[cajbir (:cajbir)]

Attachment: Address review comments

Comment 8

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

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.

Comment 9

[cajbir (:cajbir)]

Attachment: Address review comments

Comment 10

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

Chris, do you want me to land this for you or do you want to land it yourself?

Comment 11

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

Pushed 04307da4e0e6

Comment 12

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

Sorry, that was changeset 1e81ca8ea5a8

Comment 13

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

Pushed 6dcbb4f48275 to 1.9.1

Comment 14

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

Any chance to get it verified? It even looks like we don't have a valid testcase, or?

Comment 15

[Jesse Ruderman]

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.

Comment 16

[Jesse Ruderman]

Filed bug 477432 for creating such a static analysis script.