900341 - IsSafeToRunScript() error from nsEventDispatcher, running event loop while shutting down AudioContext from nsGlobalWindow

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Karl Tomlinson (:karlt)]

980 INFO TEST-END | /tests/content/media/webaudio/test/test_mediaDecoding.html | finished in 21910ms
++DOMWINDOW == 20 (0x2804cf8) [serial = 132] [outer = 0x30a2d98]
###!!! ASSERTION: This is unsafe! Fix the caller!: 'Error', file /home/karl/moz/dev/content/events/src/nsEventDispatcher.cpp, line 506
nsEventDispatcher::Dispatch(nsISupports*, nsPresContext*, nsEvent*, nsIDOMEvent*, nsEventStatus*, nsDispatchingCallback*, nsCOMArray<mozilla::dom::EventTarget>*) (/home/karl/moz/dev/content/events/src/nsEventDispatcher.cpp:507)
nsEventDispatcher::DispatchDOMEvent(nsISupports*, nsEvent*, nsIDOMEvent*, nsPresContext*, nsEventStatus*) (/home/karl/moz/dev/content/events/src/nsEventDispatcher.cpp:705)
nsGlobalWindow::DispatchEvent(nsIDOMEvent*, bool*) (/home/karl/moz/dev/dom/base/nsGlobalWindow.cpp:8026)
nsGlobalWindow::DispatchEvent(nsIDOMEvent*, bool*) (/home/karl/moz/dev/dom/base/nsGlobalWindow.cpp:8009 (discriminator 3))
nsContentUtils::DispatchEvent(nsIDocument*, nsISupports*, nsAString_internal const&, bool, bool, bool, bool*) (/home/karl/moz/dev/content/base/src/nsContentUtils.cpp:3300 (discriminator 3))
nsContentUtils::DispatchTrustedEvent(nsIDocument*, nsISupports*, nsAString_internal const&, bool, bool, bool*) (/home/karl/moz/dev/content/base/src/nsContentUtils.cpp:3272)
nsFocusManager::WindowLowered(nsIDOMWindow*) (/home/karl/moz/dev/dom/base/nsFocusManager.cpp:768)
nsWebShellWindow::WindowDeactivated() (/home/karl/moz/dev/xpfe/appshell/src/nsWebShellWindow.cpp:421)
nsWindow::DispatchDeactivateEvent() (/home/karl/moz/dev/widget/gtk2/nsWindow.cpp:442)
nsWindow::OnContainerFocusOutEvent(_GdkEventFocus*) (/home/karl/moz/dev/widget/gtk2/nsWindow.cpp:2852)
focus_out_event_cb (/home/karl/moz/dev/widget/gtk2/nsWindow.cpp:5248)
_gtk_marshal_BOOLEAN__BOXED (/var/tmp/portage/x11-libs/gtk+-2.24.17/work/gtk+-2.24.17/gtk/gtkmarshalers.c:90 (discriminator 3))
closure_invoke_notifiers (/var/tmp/portage/dev-libs/glib-2.32.4-r1/work/glib-2.32.4/gobject/gclosure.c:280)
accumulate (/var/tmp/portage/dev-libs/glib-2.32.4-r1/work/glib-2.32.4/gobject/gsignal.c:3052)
g_signal_emit_valist (/var/tmp/portage/dev-libs/glib-2.32.4-r1/work/glib-2.32.4/gobject/gsignal.c:3306)
g_signal_emit (/var/tmp/portage/dev-libs/glib-2.32.4-r1/work/glib-2.32.4/gobject/gsignal.c:3358)
gtk_widget_event_internal (/var/tmp/portage/x11-libs/gtk+-2.24.17/work/gtk+-2.24.17/gtk/gtkwidget.c:5010)
IA__gtk_widget_send_focus_change (/var/tmp/portage/x11-libs/gtk+-2.24.17/work/gtk+-2.24.17/gtk/gtkwidget.c:11445)
do_focus_change (/var/tmp/portage/x11-libs/gtk+-2.24.17/work/gtk+-2.24.17/gtk/gtkwindow.c:5306)
_gtk_window_set_has_toplevel_focus (/var/tmp/portage/x11-libs/gtk+-2.24.17/work/gtk+-2.24.17/gtk/gtkwindow.c:8476)
gtk_window_focus_out_event (/var/tmp/portage/x11-libs/gtk+-2.24.17/work/gtk+-2.24.17/gtk/gtkwindow.c:5337)
_gtk_marshal_BOOLEAN__BOXED (/var/tmp/portage/x11-libs/gtk+-2.24.17/work/gtk+-2.24.17/gtk/gtkmarshalers.c:90 (discriminator 3))
closure_invoke_notifiers (/var/tmp/portage/dev-libs/glib-2.32.4-r1/work/glib-2.32.4/gobject/gclosure.c:280)
accumulate (/var/tmp/portage/dev-libs/glib-2.32.4-r1/work/glib-2.32.4/gobject/gsignal.c:3052)
g_signal_emit_valist (/var/tmp/portage/dev-libs/glib-2.32.4-r1/work/glib-2.32.4/gobject/gsignal.c:3306)
g_signal_emit (/var/tmp/portage/dev-libs/glib-2.32.4-r1/work/glib-2.32.4/gobject/gsignal.c:3358)
gtk_widget_event_internal (/var/tmp/portage/x11-libs/gtk+-2.24.17/work/gtk+-2.24.17/gtk/gtkwidget.c:5010)
IA__gtk_main_do_event (/var/tmp/portage/x11-libs/gtk+-2.24.17/work/gtk+-2.24.17/gtk/gtkmain.c:1639)
gdk_event_dispatch (/var/tmp/portage/x11-libs/gtk+-2.24.17/work/gtk+-2.24.17/gdk/x11/gdkevents-x11.c:2405)
g_main_dispatch (/var/tmp/portage/dev-libs/glib-2.32.4-r1/work/glib-2.32.4/glib/gmain.c:2542)
g_main_context_iterate (/var/tmp/portage/dev-libs/glib-2.32.4-r1/work/glib-2.32.4/glib/gmain.c:3146)
g_main_context_iteration (/var/tmp/portage/dev-libs/glib-2.32.4-r1/work/glib-2.32.4/glib/gmain.c:3208)
nsAppShell::ProcessNextNativeEvent(bool) (/home/karl/moz/dev/widget/gtk2/nsAppShell.cpp:135)
nsBaseAppShell::DoProcessNextNativeEvent(bool, unsigned int) (/home/karl/moz/dev/widget/xpwidgets/nsBaseAppShell.cpp:139)
nsBaseAppShell::OnProcessNextEvent(nsIThreadInternal*, bool, unsigned int) (/home/karl/moz/dev/widget/xpwidgets/nsBaseAppShell.cpp:280 (discriminator 5))
nsThread::ProcessNextEvent(bool, bool*) (/home/karl/moz/dev/xpcom/threads/nsThread.cpp:597)
NS_ProcessNextEvent(nsIThread*, bool) (/mnt/sda11/karl/obj/xpcom/build/nsThreadUtils.cpp:238)
nsThread::Shutdown() (/home/karl/moz/dev/xpcom/threads/nsThread.cpp:462 (discriminator 1))
nsThreadPool::Shutdown() (/home/karl/moz/dev/xpcom/threads/nsThreadPool.cpp:276 (discriminator 2))
mozilla::MediaBufferDecoder::Shutdown() (/home/karl/moz/dev/content/media/webaudio/MediaBufferDecoder.cpp:812)
mozilla::dom::AudioContext::Shutdown() (/home/karl/moz/dev/content/media/webaudio/AudioContext.cpp:485)
nsGlobalWindow::FreeInnerObjects() (/home/karl/moz/dev/dom/base/nsGlobalWindow.cpp:1531 (discriminator 2))
nsGlobalWindow::SetNewDocument(nsIDocument*, nsISupports*, bool) (/home/karl/moz/dev/dom/base/nsGlobalWindow.cpp:2397)
nsDocumentViewer::InitInternal(nsIWidget*, nsISupports*, nsIntRect const&, bool, bool, bool) (/home/karl/moz/dev/layout/base/nsDocumentViewer.cpp:938)
nsDocumentViewer::Init(nsIWidget*, nsIntRect const&) (/home/karl/moz/dev/layout/base/nsDocumentViewer.cpp:684)
nsDocShell::SetupNewViewer(nsIContentViewer*) (/home/karl/moz/dev/docshell/base/nsDocShell.cpp:8294)
nsDocShell::Embed(nsIContentViewer*, char const*, nsISupports*) (/home/karl/moz/dev/docshell/base/nsDocShell.cpp:6313)
nsDocShell::CreateContentViewer(char const*, nsIRequest*, nsIStreamListener**) (/home/karl/moz/dev/docshell/base/nsDocShell.cpp:8081)
nsDSURIContentListener::DoContent(char const*, bool, nsIRequest*, nsIStreamListener**, bool*) (/home/karl/moz/dev/docshell/base/nsDSURIContentListener.cpp:123)
nsDocumentOpenInfo::TryContentListener(nsIURIContentListener*, nsIChannel*) (/home/karl/moz/dev/uriloader/base/nsURILoader.cpp:681)
nsDocumentOpenInfo::DispatchContent(nsIRequest*, nsISupports*) (/home/karl/moz/dev/uriloader/base/nsURILoader.cpp:382 (discriminator 1))
nsDocumentOpenInfo::OnStartRequest(nsIRequest*, nsISupports*) (/home/karl/moz/dev/uriloader/base/nsURILoader.cpp:258)
mozilla::net::nsHttpChannel::CallOnStartRequest() (/home/karl/moz/dev/netwerk/protocol/http/nsHttpChannel.cpp:1003)
mozilla::net::nsHttpChannel::ContinueOnStartRequest3(tag_nsresult) (/home/karl/moz/dev/netwerk/protocol/http/nsHttpChannel.cpp:5093)
mozilla::net::nsHttpChannel::ContinueOnStartRequest2(tag_nsresult) (/home/karl/moz/dev/netwerk/protocol/http/nsHttpChannel.cpp:5084)
mozilla::net::nsHttpChannel::OnStartRequest(nsIRequest*, nsISupports*) (/home/karl/moz/dev/netwerk/protocol/http/nsHttpChannel.cpp:5056)
nsInputStreamPump::OnStateStart() (/home/karl/moz/dev/netwerk/base/src/nsInputStreamPump.cpp:459)
nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) (/home/karl/moz/dev/netwerk/base/src/nsInputStreamPump.cpp:388)
nsInputStreamReadyEvent::Run() (/home/karl/moz/dev/xpcom/io/nsStreamUtils.cpp:83)
nsThread::ProcessNextEvent(bool, bool*) (/home/karl/moz/dev/xpcom/threads/nsThread.cpp:622)
NS_ProcessNextEvent(nsIThread*, bool) (/mnt/sda11/karl/obj/xpcom/build/nsThreadUtils.cpp:238)
mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (/home/karl/moz/dev/ipc/glue/MessagePump.cpp:81)
MessageLoop::RunInternal() (/home/karl/moz/dev/ipc/chromium/src/base/message_loop.cc:221)
MessageLoop::RunHandler() (/home/karl/moz/dev/ipc/chromium/src/base/message_loop.cc:214)
MessageLoop::Run() (/home/karl/moz/dev/ipc/chromium/src/base/message_loop.cc:187)
nsBaseAppShell::Run() (/home/karl/moz/dev/widget/xpwidgets/nsBaseAppShell.cpp:165)
nsAppStartup::Run() (/home/karl/moz/dev/toolkit/components/startup/nsAppStartup.cpp:269)
XREMain::XRE_mainRun() (/home/karl/moz/dev/toolkit/xre/nsAppRunner.cpp:3864)
XREMain::XRE_main(int, char**, nsXREAppData const*) (/home/karl/moz/dev/toolkit/xre/nsAppRunner.cpp:3932)

Comment 1

[Karl Tomlinson (:karlt)]

Picking sec-high, guessing this could lead to crashes but mitigating circumstances might make it hard to exploit.

Comment 2

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

Huh, MediaBufferDecoder::Shutdown spins event loop?

Comment 3

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

http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/nsThread.cpp?rev=b3533aba6520#460
Threads are tricky.

Most probably sec-critical.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

nsThread and nsThreadPool and LazyIdleThread (and probably others) all spin the event loop when you call Shutdown on them. Additionally, nsThreadPool and LazyIdleThread call Shutdown from their destructors (though, those are no-ops when Shutdown has already been called). nsThread doesn't and that has caused some leaks in the past...

In general it's safest to post a NS_NewRunnableMethod to the main thread to call Shutdown on nsThread/nsThreadPool/LazyIdleThread.

Comment 5

[(no longer active)]

We call MediaBufferDecoder::Shutdown on the main thread already.  Am I missing something?

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #5)
> We call MediaBufferDecoder::Shutdown on the main thread already.

The issue isn't which thread it runs on, just that it spins the event loop. You want to do that with nothing on the stack, usually.

Comment 7

[Karl Tomlinson (:karlt)]

(In reply to ben turner [:bent] from comment #4)
> nsThreadPool and LazyIdleThread (and probably others) all spin
> the event loop when you call Shutdown on them. 

Filed bug 900711 on nsThreadPool.
(LazyIdleThread is not directly scriptable.)

Comment 8

[(no longer active)]

Attachment: Patch (v1)

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 784670 [details] [diff] [review]
Patch (v1)

Review of attachment 784670 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/webaudio/AudioContext.cpp
@@ +481,5 @@
>  
>  void
> +AudioContext::ShutdownDecoder()
> +{
> +  mDecoder.Shutdown();

Is there some higher logic that prevents mDecoder from being used again between the call to Shutdown() and whenever the runnable runs ShutdownDecoder()?

It's probably fine, but I'm always nervous when previously existing code gets switched from sync to async...

@@ +495,5 @@
> +  // a chance to run.
> +  nsCOMPtr<nsIRunnable> threadShutdownEvent =
> +    NS_NewRunnableMethod(this, &AudioContext::ShutdownDecoder);
> +  if (threadShutdownEvent) {
> +    NS_DispatchToMainThread(threadShutdownEvent, NS_DISPATCH_NORMAL);

If you know that you're on the main thread already (I suspect you do) then NS_DispatchToCurrentThread is better.

Comment 10

[(no longer active)]

(In reply to ben turner [:bent] from comment #9)
> Comment on attachment 784670 [details] [diff] [review]
> Patch (v1)
> 
> Review of attachment 784670 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/webaudio/AudioContext.cpp
> @@ +481,5 @@
> >  
> >  void
> > +AudioContext::ShutdownDecoder()
> > +{
> > +  mDecoder.Shutdown();
> 
> Is there some higher logic that prevents mDecoder from being used again
> between the call to Shutdown() and whenever the runnable runs
> ShutdownDecoder()?
> 
> It's probably fine, but I'm always nervous when previously existing code
> gets switched from sync to async...

Yeah, the context gets shut down when the page is being torn down <http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#1510> so we should not be running any scripts after that.

> @@ +495,5 @@
> > +  // a chance to run.
> > +  nsCOMPtr<nsIRunnable> threadShutdownEvent =
> > +    NS_NewRunnableMethod(this, &AudioContext::ShutdownDecoder);
> > +  if (threadShutdownEvent) {
> > +    NS_DispatchToMainThread(threadShutdownEvent, NS_DISPATCH_NORMAL);
> 
> If you know that you're on the main thread already (I suspect you do) then
> NS_DispatchToCurrentThread is better.

OK, I'll make that change before landing.

Comment 11

[(no longer active)]

Attachment: Patch (v2)

[Security approval request comment]
How easily could an exploit be constructed based on the patch?  Not very easily.  We don't have an exploit yet.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No

Which older supported branches are affected by this flaw? Aurora, since Web Audio is disabled elsewhere.  But I don't think we need to uplift this, given the fact that the next uplift happens on Monday.

If not all supported branches, which bug introduced the flaw? N/A

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? See above.

How likely is this patch to cause regressions; how much testing does it need? Not likely.

Comment 12

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 784983 [details] [diff] [review]
Patch (v2)

Sec-approval+ for trunk assuming it passes Ben's review.

I approved it for Aurora as well. If it only affects Aurora and up, we should land this there as soon as we land and go green on trunk.

Comment 13

[(no longer active)]

Like I said, there is little point in landing this on Aurora, but ok.

Comment 14

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/434d827b2d09

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/434d827b2d09

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cf6c28aac228