1281695 - mozGetUserMedia from zombie window ⇒ MOZ_CRASH [@ mozilla::MediaManager::PostTask] during shutdown

Status: RESOLVED WORKSFORME

(Core :: WebRTC: Audio/Video, defect, P4)

Description

[Jesse Ruderman]

Attachment: testcase

1. Load the testcase
2. Quit Firefox

Result: MOZ_CRASH [@ mozilla::MediaManager::PostTask] during shutdown

I'm testing on Mac and without e10s, in case that matters.

khuey added this MOZ_CRASH in
https://hg.mozilla.org/mozilla-central/rev/30dcf34cfb75#l7.336
(bug 1266595)

Is this a security hole (either before or after that patch)?

Comment 1

[Jesse Ruderman]

Attachment: Stack for "ASSERTION: Mainthread not available"

Sometimes I see this non-fatal assertion before the crash:

###!!! ASSERTION: Mainthread not available; running on current thread: 'false', file MediaManager.h, line 299

Comment 2

[Jesse Ruderman]

Attachment: Stack for MOZ_CRASH

Comment 3

[Jesse Ruderman]

Related testcases hit:

[GFX1]: Texture deallocated too late during shutdown
Assertion failure: false (An assert from the graphics logger), at Logging.h:519

Comment 4

[Andrew McCreight [:mccr8]]

(In reply to Jesse Ruderman from comment #0)
> Is this a security hole (either before or after that patch)?

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I don't think so but really someone who is familiar with this code should answer ...

Comment 6

[Andrew McCreight [:mccr8]]

Randell, how bad is this? The crash doesn't seem bad, but maybe an assertion would be...

Comment 7

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

The assertion is safe (though related to this crash).  Basically it says "Hey, you've held things that need threading in the CC graph past where we can use normal methods" aka DispatchToMainThread/etc.  We worked around a leak caused by this in MediaManager (and added the assertion as a poke to find a better fix); the irony is that almost by definition we're already running on MainThread.

In this case, mMediaThread is likely shut down/null already, so we we wouldn't be sending anything anyways, and the worst case was just a leak.  Before bug 1266595 it was just a return; kyle added the MOZ_CRASH() as part of some big change I don't think any media people reviewed.   Kyle - what was your reasoning? 

I don't see this as any sort of security hole at present, btw, and am good with opening this if jesse agrees.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Before bug 1266595 it was a DEBUG-only assertion in already_AddRefed's destructor.  I decided to make it more visible because the code doesn't explicitly leak, and the comment doesn't make it clear what the intended behavior is here.

Comment 9

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

Aha.  Last time I worked on it, it wasn't an already_AddRefed<> Runnable; you (kyle) landed that change in bug 1266595 as well.  Before that I think it just cleanly leaked.  The right solution might be to just cleanly leak again in this case (adopt the already-addrefed and forget it).  And if I understand the report I got from Nils at London (I wasn't able to attend); there will be a change in how final-CC works so we shouldn't get bitten by this once that change lands.

Comment 10

[Bulk Bug Changes for mreavy's org]

Mass change P3->P4 to align with new Mozilla triage process.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

No longer reproduces on trunk. I tried to bisect when exactly this got fixed, but ran into some issues getting it down to a short range for some reason. Appears to have been around early June, though.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/06a6bd2d785f
Add crashtest. r=me

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/06a6bd2d785f