945334 - MediaManager's JS Objects released on non-main threads

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Sotaro Ikeda [:sotaro]]

+++ This bug was initially created as a clone of Bug #929029 +++

From, Bug 929029 Comment 57, some MediaManager's JS Objects seems to be released on non main thread.

Comment 1

[Sotaro Ikeda [:sotaro]]

Copied from Bug 929029.

(In reply to Ryan VanderMeulen [:RyanVM UTC-5][Back 2-Dec] from comment #55)
> Backed out for crashes/hangs in test_dataChannel_basicAudio.html on all
> platforms. Please run this through Try before requesting checkin again.
> https://hg.mozilla.org/integration/b2g-inbound/rev/d99d3514fd6a
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=31322007&tree=B2g-Inbound
> https://tbpl.mozilla.org/php/getParsedLog.php?id=31321907&tree=B2g-Inbound
> https://tbpl.mozilla.org/php/getParsedLog.php?id=31322194&tree=B2g-Inbound

Comment 2

[Sotaro Ikeda [:sotaro]]

From the following log, JS object seems to be released on media thread. MediaOperationRunnable::Run() rund on media thread.

---------------------------------------------------------

09:05:02     INFO -  Assertion: Unknown assertion type 0x00000000
09:05:02     INFO -  Thread 25 (crashed)
09:05:02     INFO -   0  xul.dll!nsXPCWrappedJS::Release() [XPCWrappedJS.cpp:bc50783a4cf7 : 175 + 0x0]
09:05:02     INFO -      eip = 0x7333aca2   esp = 0x0a9ef6a4   ebp = 0x0a9ef6a4   ebx = 0x1008dbe0
09:05:02     INFO -      esi = 0x09fadee0   edi = 0x0a191514   eax = 0x00000000   ecx = 0xfeefd000
09:05:02     INFO -      edx = 0x09fadee4   efl = 0x00000246
09:05:02     INFO -      Found by: given as instruction pointer in context
09:05:02     INFO -   1  xul.dll!nsXPTCStubBase::Release() [xptcall.cpp:bc50783a4cf7 : 36 + 0xb]
09:05:02     INFO -      eip = 0x72d68e23   esp = 0x0a9ef6ac   ebp = 0x0a9ef6b0
09:05:02     INFO -      Found by: call frame info
09:05:02     INFO -   2  xul.dll!mozilla::GetUserMediaNotificationEvent::~GetUserMediaNotificationEvent() [MediaManager.h:bc50783a4cf7 : 223 + 0xc]
09:05:02     INFO -      eip = 0x733e4375   esp = 0x0a9ef6b8   ebp = 0x0a9ef6c8
09:05:02     INFO -      Found by: call frame info
09:05:02     INFO -   3  xul.dll!mozilla::GetUserMediaNotificationEvent::`scalar deleting destructor'(unsigned int) + 0xa
09:05:02     INFO -      eip = 0x733e43af   esp = 0x0a9ef6c4   ebp = 0x0a9ef6c8
09:05:02     INFO -      Found by: call frame info
09:05:02     INFO -   4  xul.dll!nsTransportEventSinkProxy::Release() [EventTokenBucket.cpp:bc50783a4cf7 : 46 + 0x26]
09:05:02     INFO -      eip = 0x72d9a2a1   esp = 0x0a9ef6d0   ebp = 0x0a9ef6d4
09:05:02     INFO -      Found by: call frame info
09:05:02     INFO -   5  xul.dll!mozilla::MediaOperationRunnable::Run() [MediaManager.h:bc50783a4cf7 : 373 + 0x9]
09:05:02     INFO -      eip = 0x733e7bd7   esp = 0x0a9ef6dc   ebp = 0x0a9ef6ec
09:05:02     INFO -      Found by: call frame info
09:05:02     INFO -   6  xul.dll!nsThread::ProcessNextEvent(bool,bool *) [nsThread.cpp:bc50783a4cf7 : 612 + 0x5]
09:05:02     INFO -      eip = 0x72d64698   esp = 0x0a9ef6f4   ebp = 0x0a9ef728
09:05:02     INFO -      Found by: call frame info
09:05:02     INFO -   7  xul.dll!NS_ProcessNextEvent(nsIThread *,bool) [nsThreadUtils.cpp:bc50783a4cf7 : 263 + 0xc]
09:05:02     INFO -      eip = 0x72d2ea74   esp = 0x0a9ef730   ebp = 0x0a9ef73c
09:05:02     INFO -      Found by: call frame info
09:05:02     INFO -   8  xul.dll!nsThread::ThreadFunc(void *) [nsThread.cpp:bc50783a4cf7 : 246 + 0x7]
09:05:02     INFO -      eip = 0x72d6617b   esp = 0x0a9ef744   ebp = 0x0a9ef758
09:05:02     INFO -      Found by: call frame info

Comment 3

[Sotaro Ikeda [:sotaro]]

To fix the problem, it might be better to use nsMainThreadPtrHandle and nsMainThreadPtrHolder like Bug 827503 and Bug 929029.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

This appears to be a B2G (or at least E10S-only) issue given the release from nsTransportEventSinkProxy::Release()

Comment 5

[Jason Smith [:jsmith]]

Blocks a blocker.

Comment 6

[Preeti Raghunath(:Preeti)]

Blocking a blocker and hence koi+ on the same.

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Caused by the landing of bug 866514, which added a JS object to the items released by the NotificationEvent object (the TracksAvailableCallback object).  This was landed in 23 and uplifted to 22.

Before bug 926746, the only things we'd seen that might have been this bug were occasional did-not-exit bugs.  That bug landing made this very frequent and caused it to be backed out.  Thanks to Steven for digging in to find the root cause.

Marking as a sec bug because I can't guarantee that all the failure paths are safe if this happens (which isn't easy without that new landing).  Because of this, we'll want to uplift it as far as we can.  There's no indication this is exploitable, so I would not at this moment recommend any form of spill for it; we might consider it riding along with any other releases is they occur (particularly for 26 and 24ESR).

Patch will be attached; it's simply making sure no reference is kept to the event even temporarily after NS_DispatchToMainThread().  I'm also going to run a Try with the bug 926746 patches.

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Fix runnable pointer holding

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Fix runnable pointer holding

simpler patch; avoids leaking

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8342591 [details] [diff] [review]
Fix runnable pointer holding

Try build is running

Comment 11

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

Comment on attachment 8342591 [details] [diff] [review]
Fix runnable pointer holding

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

::: dom/media/MediaManager.h
@@ +374,1 @@
>            NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);

So if NS_DispatchToMainThread fails, we just leak. I guess that's OK.

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11)
> Comment on attachment 8342591 [details] [diff] [review]
> Fix runnable pointer holding
> 
> Review of attachment 8342591 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaManager.h
> @@ +374,1 @@
> >            NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);
> 
> So if NS_DispatchToMainThread fails, we just leak. I guess that's OK.

Yeah, it has to be ok, since we can't free it on this thread anyways.  (And if it fails, all sorts of other things are in massive trouble).  Thanks

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8342591 [details] [diff] [review]
Fix runnable pointer holding

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Not easily (likely very hard) - it would be very hard to force the timing, and our only indication is it fails to exit when this is hit.

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

Pretty much they point out the wrong-thread release.  I could pull the comments which would make it considerably more obtuse, but knowing it was a sec bug likely someone could eyeball it and figure out the issue (and also from the refs in the open bug this was spun off from)

Which older supported branches are affected by this flaw?

All back to FF22.  This includes 24ESR. We'll want to uplift to Aurora, and if the opportunity presents itself I'd update it on 26, 25 and 24ESR.  Right now I wouldn't recommend a spill for this.

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

Caused by bug 886514's landing (in 23, uplifted to 22).

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

The patch will trivially apply to all branches.

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

Virtually no chances of regressions.

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

https://tbpl.mozilla.org/?tree=Try&rev=c412ee30f24c

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

This is with Steven's patches and without this fix: (ugly)
https://tbpl.mozilla.org/?tree=Try&rev=1065025135af

Comment 16

[Daniel Veditz [:dveditz]]

Calling this 'sec-moderate' because we don't currently know a way to trigger this reliably in an exploitable way, but higher than 'low' because we know this kind of bug can be exploited should someone clever figure out how to catch it.

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 8342591 [details] [diff] [review]
Fix runnable pointer holding

[Triage Comment]
approval for aurora and sec-approval

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

Backed out the wallpaper patch from bug 926746, and landed this.  Will land on Aurora once we're cleanly on m-c
https://hg.mozilla.org/integration/b2g-inbound/rev/c4b843dfa959
https://hg.mozilla.org/integration/b2g-inbound/rev/d212ce6a9a18

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d212ce6a9a18

I'll uplift this in the morning if nobody beats me to it.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/de98352c72c6
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/5e634bd9d29f

Is this wontfix for esr24?

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

> Is this wontfix for esr24?

No, I think this will be opportunistically landed if there are convenient respins of ESR24 or 26, but no spills for those (or rather at least considered if there are spins caused by other bugs).  Given ESR24's lifetime, that's fairly likely for ESR24; 26 is at least 50/50 given history.  (From above and discussion with dveditz).

Comment 22

[Daniel Veditz [:dveditz]]

Randell: do we want to land this on ESR24 for the release that corresponds to Fx27 (24.3)?

Comment 23

[Randell Jesup [:jesup] (needinfo me)]

Per my previous comments, if we're spinning ESR24 I think this is something we should probably take as a safety measure.

Comment 24

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8342591 [details] [diff] [review]
Fix runnable pointer holding

Let's get this into the ESR going today then.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr24/rev/d9fb5f6ddb35