Closed Bug 826576 Opened 7 years ago Closed 7 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: jesup, Assigned: roc)

References

Details

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

Attachments

(4 files, 3 obsolete files)

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.
FYI, opt build
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
Attached file GDB log
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.
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).
Note: this may be a different bug, but I suspect both symptoms have the same root cause
Attached file WIP patch for datachannels (obsolete) —
Severity: major → critical
Depends on: 822956
Priority: -- → P1
Whiteboard: [getUserMedia], [blocking-gum-] → [getUserMedia], [blocking-gum+]
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().
Perhaps we should fire NotifyDestroyed/NotifyRemoved instead
Attachment #698406 - Flags: review?(tterribe)
Attachment #698406 - Flags: review?(roc)
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)
Depends on: 827007
Blocks: 825594
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-
Attachment #698407 - Attachment is obsolete: true
Attachment #698407 - Flags: review?(roc)
Attachment #698435 - Flags: review?(tterribe)
Attachment #698435 - Flags: review?(roc)
(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+
Attachment #698434 - Attachment is obsolete: true
Attachment #698271 - Attachment is obsolete: true
Attachment #698406 - Flags: review?(roc) → review+
Attachment #698435 - Flags: review?(roc) → review+
Attachment #698406 - Flags: review?(tterribe)
Whiteboard: [getUserMedia], [blocking-gum+] → [getUserMedia], [blocking-gum+], [qa-]
You need to log in before you can comment on or make changes to this bug.