Closed Bug 900341 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Web Audio, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25
Tracking Status
firefox23 --- unaffected
firefox24 + fixed
firefox25 + fixed
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: karlt, Assigned: ehsan)

References

Details

(Keywords: sec-critical, Whiteboard: [blocking-webaudio+])

Attachments

(2 files)

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)
Picking sec-high, guessing this could lead to crashes but mitigating circumstances might make it hard to exploit.
Keywords: sec-high
Huh, MediaBufferDecoder::Shutdown spins event loop?
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.
We call MediaBufferDecoder::Shutdown on the main thread already.  Am I missing something?
(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.
(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.)
Attached patch Patch (v1)Splinter Review
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #784670 - Flags: review?(roc)
Attachment #784670 - Flags: review?(bent.mozilla)
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.
(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.
Attached patch Patch (v2)Splinter Review
[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.
Attachment #784983 - Flags: sec-approval?
Attachment #784983 - Flags: review?(bent.mozilla)
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.
Attachment #784983 - Flags: sec-approval?
Attachment #784983 - Flags: sec-approval+
Attachment #784983 - Flags: approval-mozilla-aurora+
Attachment #784983 - Flags: review?(bent.mozilla) → review+
Like I said, there is little point in landing this on Aurora, but ok.
https://hg.mozilla.org/integration/mozilla-inbound/rev/434d827b2d09
Whiteboard: [blocking-webaudio+][checkin-needed-aurora]
https://hg.mozilla.org/mozilla-central/rev/434d827b2d09
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
https://hg.mozilla.org/releases/mozilla-aurora/rev/cf6c28aac228
Whiteboard: [blocking-webaudio+][checkin-needed-aurora] → [blocking-webaudio+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.