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

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P1)

Description

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

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.

Comment 1

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

FYI, opt build

Comment 2

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

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

gGraph is null when we call RemoveListener

gdb logs attached

Comment 3

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

Attachment: GDB log

Comment 4

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

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)

Comment 5

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

Not crashing for me me in a Windows debug build. I'll try opt.

Comment 6

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

(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.

Comment 7

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

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.

Comment 8

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

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).

Comment 9

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

Attachment: nspr/gdb logs of already-destroyed

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

Comment 10

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

Attachment: WIP patch for datachannels

Comment 11

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

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().

Comment 12

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

Attachment: Stop LocalMediaStreams before Destroy

Perhaps we should fire NotifyDestroyed/NotifyRemoved instead

Comment 13

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

Attachment: Manage the lifetimes of GUMCMSL objects

Comment 14

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

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.

Comment 15

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

adding dao - these fixes with the last patch in bug 827007 solve many of the UI state issues.

Comment 16

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

As in bug 827007, don't we just want nsDOMUserMediaStream to remove GetUserMediaCallbackMediaStreamListener before it does mSourceStream->Destroy()?

Comment 17

[Timothy B. Terriberry (:derf)]

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.

Comment 18

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

Attachment: interdiffs for changes to add an inactive Listener and activate it later

Comment 19

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

Attachment: Manage the lifetimes of GUMCMSL objects (with inactive Listeners)

Comment 20

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

(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 21

[Timothy B. Terriberry (:derf)]

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.

Comment 22

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/de7d61c4b11b
https://hg.mozilla.org/integration/mozilla-inbound/rev/177d8769bb85

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/177d8769bb85
https://hg.mozilla.org/mozilla-central/rev/de7d61c4b11b