Closed
Bug 826576
Opened 12 years ago
Closed 12 years ago
Crash due to null gGraph/GraphImpl() in MediaStream::RemoveListener
Categories
(Core :: Audio/Video, defect, P1)
Core
Audio/Video
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.
Reporter | ||
Comment 2•12 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•12 years ago
|
||
Reporter | ||
Comment 4•12 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)
Assignee | ||
Comment 6•12 years ago
|
||
(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.
Assignee | ||
Comment 7•12 years ago
|
||
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•12 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•12 years ago
|
||
Note: this may be a different bug, but I suspect both symptoms have the same root cause
Reporter | ||
Comment 10•12 years ago
|
||
Reporter | ||
Updated•12 years ago
|
Reporter | ||
Updated•12 years ago
|
Whiteboard: [getUserMedia], [blocking-gum-] → [getUserMedia], [blocking-gum+]
Reporter | ||
Comment 11•12 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 13•12 years ago
|
||
Reporter | ||
Updated•12 years ago
|
Attachment #698406 -
Flags: review?(tterribe)
Attachment #698406 -
Flags: review?(roc)
Reporter | ||
Comment 14•12 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 | ||
Comment 15•12 years ago
|
||
adding dao - these fixes with the last patch in bug 827007 solve many of the UI state issues.
Assignee | ||
Comment 16•12 years ago
|
||
As in bug 827007, don't we just want nsDOMUserMediaStream to remove GetUserMediaCallbackMediaStreamListener before it does mSourceStream->Destroy()?
Comment 17•12 years ago
|
||
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•12 years ago
|
||
Reporter | ||
Comment 19•12 years ago
|
||
Reporter | ||
Updated•12 years ago
|
Attachment #698407 -
Attachment is obsolete: true
Attachment #698407 -
Flags: review?(roc)
Reporter | ||
Updated•12 years ago
|
Attachment #698435 -
Flags: review?(tterribe)
Attachment #698435 -
Flags: review?(roc)
Reporter | ||
Comment 20•12 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 21•12 years ago
|
||
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•12 years ago
|
Attachment #698434 -
Attachment is obsolete: true
Reporter | ||
Updated•12 years ago
|
Attachment #698271 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #698406 -
Flags: review?(roc) → review+
Assignee | ||
Updated•12 years ago
|
Attachment #698435 -
Flags: review?(roc) → review+
Reporter | ||
Comment 22•12 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•12 years ago
|
Attachment #698406 -
Flags: review?(tterribe)
Comment 23•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/177d8769bb85 https://hg.mozilla.org/mozilla-central/rev/de7d61c4b11b
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 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.
Description
•