Crash due to null gGraph/GraphImpl() in MediaStream::RemoveListener

RESOLVED FIXED in mozilla20

Status

()

Core
Audio/Video
P1
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jesup, Assigned: roc)

Tracking

({crash})

unspecified
mozilla20
crash
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [getUserMedia], [blocking-gum+], [qa-])

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
If you run the testcase from Bug 826538, and disable the EnumerateRead line in MediaManager::GetActiveMediaCaptureWindows() in MediaManager.cpp (which lets it cycle even faster, by not telling the UI about the windows), we crash in RemoveListener with a null GraphImpl()/gGraph. 

Note: I had the patches from bug 822956 applied as they hadn't landed yet.
(Reporter)

Comment 1

5 years ago
FYI, opt build
(Reporter)

Comment 2

5 years ago
Hit this or a similar failure shutting down after a simple video GUM test.  

gGraph is null when we call RemoveListener

gdb logs attached
Keywords: crash
(Reporter)

Comment 3

5 years ago
Created attachment 698217 [details]
GDB log
(Reporter)

Comment 4

5 years ago
I have a possibly-related assertion when doing OnNavigation in gUM - (reload) 

When the gumMediaStreamListener calls RemoveListener on the SourceMediaStream, it throws an assertion that it was already destroyed.

I never saw these before landing the TrackUnion/SourceMediaStream changes to gum today.  (The lifetime of the MediaStream had been controlled by the nsDOMMediaStream, but we now don't have one.  See bug 822956)
Not crashing for me me in a Windows debug build. I'll try opt.
(In reply to Randell Jesup [:jesup] from comment #3)
> Created attachment 698217 [details]
> GDB log

It would be a little easier to read if it contained the commands as well as the output :-).

This is weird. GetUserMediaCallbackMediaStreamListener holds a strong ref to the nsDOMMediaStream, which strongly references its MediaStream. So the MediaStream should not go away, but it clearly has.
I'm not getting this in a Windows opt build, either.

You're on Mac or Linux I assume? It's a bit late for a Mac build for me, I can try that on Monday.
(Reporter)

Comment 8

5 years ago
Linux.  Here's a log of OnNavigation "stream already destroyed", with datachannel:5,mediastreamgraph:5.  Nothing really interesting.  GDB backtrace and dump of the stream included.  (mDestroyed is 1, of course).

This one is reproducible with moderate pain:

Use the current WIP patch to clean up some refcount issues with DataChannels (I'll attach).
Load the local data channel test from the webrtc-landing page
Hit start
Hit reload - timing may matter, I try to hit it after "HIP HIP HOORAY", right around when I think the main output from the test will show up (trying to have it reload during the setup).

Repeat many times.  (With that logging, my NSPR logfile was 81MB before I hit it this time).
(Reporter)

Comment 9

5 years ago
Created attachment 698270 [details]
nspr/gdb logs of already-destroyed

Note: this may be a different bug, but I suspect both symptoms have the same root cause
(Reporter)

Comment 10

5 years ago
Created attachment 698271 [details]
WIP patch for datachannels
(Reporter)

Updated

5 years ago
Severity: major → critical
Depends on: 822956
Priority: -- → P1
(Reporter)

Updated

5 years ago
Whiteboard: [getUserMedia], [blocking-gum-] → [getUserMedia], [blocking-gum+]
(Reporter)

Comment 11

5 years ago
Ok, I think I know more about what's going on.

MediaManager creates DOMMediaStreams and passes them back to the JS code.  DOMMediaStreams wrap TrackUnion/SourceMediaStreams.

After creating the stream, it creates a GetUserMediaCallbackStreamListener (phew!) (GUMCSL for short).  This is attached to the MediaStream via AddListener(), and is the destination for callbacks like NotifyPull, NotifyFinished, etc.  It also wraps access to invalidate a stream, and is kept on a per-windowID list which is stored in a hashtable.

GUMCSLs do hold a ref to the MediaStream, but not the DOMMediaStream.

Right now, the GUMCSL only removes itself as a Listener of the MediaStream on navigation or shutdown (or Revocation, but that isn't hooked up in the UI yet).  This is a problem, since the MediaStream's lifetime is controlled from the DOM object, and may be Destroyed at any time.  Once it's destroyed, even trying to remove the Listener will cause an assertion, and the MediaStreamGraph may shut down and null out gGraph, causing a crash on RemoveListener.

GUMCSL does listen for NotifyFinished(), in order to shut down the input device on stop().  We could RemoveListener() from NotifyFinished(), if we guarantee that we Finish streams before Destroying them (and that it won't somehow get on the wrong side of the Destroy in the command queue).  Alternatively, we could add a NotifyDestroyed() which also tells you your listener has been removed (which might be nice, since the object is unusable from then anyways, and you won't get any more notifications), or perhaps NotifyRemoved().
(Reporter)

Comment 12

5 years ago
Created attachment 698406 [details] [diff] [review]
Stop LocalMediaStreams before Destroy

Perhaps we should fire NotifyDestroyed/NotifyRemoved instead
(Reporter)

Comment 13

5 years ago
Created attachment 698407 [details] [diff] [review]
Manage the lifetimes of GUMCMSL objects
(Reporter)

Updated

5 years ago
Attachment #698406 - Flags: review?(tterribe)
Attachment #698406 - Flags: review?(roc)
(Reporter)

Comment 14

5 years ago
Comment on attachment 698407 [details] [diff] [review]
Manage the lifetimes of GUMCMSL objects

Apply on top of bug 827007's patch.

Tested - this removes windows from the list immediately on Stop/etc, and in limited testing (so far), doesn't have any shutdown problems (which the plain patch in bug 827007 has).

requesting r= from both, again: I'd only commit with this (after more testing) on r=derf if I can't find roc before the uplift cutoff.  We'll need to go with the fixup 827007 patch plus the two here, or with the full backout in bug 827007.
Attachment #698407 - Flags: review?(tterribe)
Attachment #698407 - Flags: review?(roc)
(Reporter)

Updated

5 years ago
Depends on: 827007
(Reporter)

Updated

5 years ago
Blocks: 825594
(Reporter)

Comment 15

5 years ago
adding dao - these fixes with the last patch in bug 827007 solve many of the UI state issues.
As in bug 827007, don't we just want nsDOMUserMediaStream to remove GetUserMediaCallbackMediaStreamListener before it does mSourceStream->Destroy()?
Comment on attachment 698407 [details] [diff] [review]
Manage the lifetimes of GUMCMSL objects

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

::: dom/media/MediaManager.cpp
@@ +499,5 @@
>        // be invalidated from the main-thread (see OnNavigation)
>        nsCOMPtr<nsIDOMGetUserMediaErrorCallback> error(mError);
>        error->OnError(NS_LITERAL_STRING("PERMISSION_DENIED"));
> +
> +      // If this was the only active MediaStream, remove the window from the list

So, this is broken for at least two reasons.

a) There's no equivalent logic in the else clause, and
b) This can't possibly work with the StreamFinished logic that removes the window from the hashtable as soon as listeners->Length() drops to 0.

If you want to have the window in the hashtable while a request is pending, but not have the listener in the list, we need to use something other than listeners->Length() to decide when the window should be removed.
Attachment #698407 - Flags: review?(tterribe) → review-
(Reporter)

Comment 18

5 years ago
Created attachment 698434 [details] [diff] [review]
interdiffs for changes to add an inactive Listener and activate it later
(Reporter)

Comment 19

5 years ago
Created attachment 698435 [details] [diff] [review]
Manage the lifetimes of GUMCMSL objects (with inactive Listeners)
(Reporter)

Updated

5 years ago
Attachment #698407 - Attachment is obsolete: true
Attachment #698407 - Flags: review?(roc)
(Reporter)

Updated

5 years ago
Attachment #698435 - Flags: review?(tterribe)
Attachment #698435 - Flags: review?(roc)
(Reporter)

Comment 20

5 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16)
> As in bug 827007, don't we just want nsDOMUserMediaStream to remove
> GetUserMediaCallbackMediaStreamListener before it does
> mSourceStream->Destroy()?

Right, that's the alternative I suggested - notify the listener "you've been removed" (or "the stream has been destroyed and you were removed", take your pick - if it's "you were removed", then a normal RemoveListener should fire it as well - which may not be a bad thing; ekr was asking "how do I know when something is removed" before.  A listener might need to keep state ("active remove I requested"), but only if it matters to the listener).

As I mentioned in comment 12, I think I like that alternative.  If I hadn't been so sleepy I'd have coded it up last night.
Comment on attachment 698435 [details] [diff] [review]
Manage the lifetimes of GUMCMSL objects (with inactive Listeners)

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

::: dom/media/MediaManager.cpp
@@ +191,5 @@
>    already_AddRefed<nsIDOMGetUserMediaErrorCallback> mError;
>    nsTArray<nsCOMPtr<nsIMediaDevice> > mDevices;
>  };
>  
> +// Handle removing GetUserMediaCallbackMediaStreamListener from correct thread

Please say what the "correct thread" is (if not actively asserting it).

@@ +902,5 @@
> +  // Ensure there's a thread for gum to proxy to off main thread
> +  nsIThread *mediaThread = MediaManager::GetThread();
> +
> +  // Create a disabled listener to act as a placeholder
> +  GetUserMediaCallbackMediaStreamListener* listener = 

Trailing whitespace.
Attachment #698435 - Flags: review?(tterribe) → review+
(Reporter)

Updated

5 years ago
Attachment #698434 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Attachment #698271 - Attachment is obsolete: true
Attachment #698406 - Flags: review?(roc) → review+
Attachment #698435 - Flags: review?(roc) → review+
(Reporter)

Comment 22

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/de7d61c4b11b
https://hg.mozilla.org/integration/mozilla-inbound/rev/177d8769bb85
Target Milestone: --- → mozilla20
(Reporter)

Updated

5 years ago
Attachment #698406 - Flags: review?(tterribe)
https://hg.mozilla.org/mozilla-central/rev/177d8769bb85
https://hg.mozilla.org/mozilla-central/rev/de7d61c4b11b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Whiteboard: [getUserMedia], [blocking-gum+] → [getUserMedia], [blocking-gum+], [qa-]
You need to log in before you can comment on or make changes to this bug.