752784 - mozCaptureStream on <audio> crashes

Status: RESOLVED FIXED

Description

[Jesse Ruderman]

Created attachment 621845 [details]
testcase (crashes Firefox when loaded)

###!!! ASSERTION: Bad track ID!: 'Error', file MediaStreamGraph.h, line 466

Crash [@ mozilla::SourceMediaStream::HaveEnoughBuffered]

Comment 1

[Jesse Ruderman]

Created attachment 621846 [details]
crash stack trace

gdb only gave me a stack trace for the crash, not the assertion.

Comment 2

[Scoobidiver (away)]

On Windows 7: bp-0bd8fd67-e77f-4364-871a-24f252120508.

Comment 3

[Randell Jesup [:jesup]]

There's an inherent assumption that the track of the needed type will exist, and they may not (FindDataForTrack() can return NULL).

In general all the uses of TRACK_AUDIO and TRACK_VIDEO should be checked; HaveEnoughBuffered() and all other users of FindDataForTrack() need to null-check the result.  (And it may not want to have NS_ERROR()).

Comment 4

[Randell Jesup [:jesup]]

Created attachment 621898 [details] [diff] [review]
protect against media not having a track of the required type

Comment 5

[Randell Jesup [:jesup]]

Comment on attachment 621898 [details] [diff] [review]
protect against media not having a track of the required type

Quick patch - mostly just avoids crashing.  Probably could use some tests as well (missing audio and missing video both).  May be sufficient, though

Comment 6

[Randell Jesup [:jesup]]

https://tbpl.mozilla.org/?tree=Try&rev=c67b62e2dc78

Comment 7

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

Comment on attachment 621898 [details] [diff] [review]
protect against media not having a track of the required type

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

This patch is OK but the real fix needs to be in nsBuiltinDecoderStateMachine.cpp.

::: content/media/MediaStreamGraph.cpp
@@ +1832,5 @@
> +  TrackData *track = FindDataForTrack(aID);
> +  if (track) {
> +    return track->mHaveEnough;
> +  } else {
> +    return true; // XXX is this correct?

No, this is just a coding error, so NS_ERROR it.

@@ +1834,5 @@
> +    return track->mHaveEnough;
> +  } else {
> +    return true; // XXX is this correct?
> +  }
> +  return track;

This return is dead code, which is good because it doesn't make sense.

@@ +1843,5 @@
>      nsIThread* aSignalThread, nsIRunnable* aSignalRunnable)
>  {
>    MutexAutoLock lock(mMutex);
>    TrackData* data = FindDataForTrack(aID);
> +  if (data && data->mHaveEnough) {

NS_ERROR if !data here.

@@ +1859,5 @@
> +    TrackData *track = FindDataForTrack(aID);
> +    if (track) {
> +      track->mCommands |= TRACK_END;
> +    } else {
> +      NS_ERROR("End of non-existant tracl");

typo

Comment 8

[Randell Jesup [:jesup]]

Created attachment 621972 [details] [diff] [review]
protect against media not having a track of the required type

Updated patch - does not correct the fundamental problem, but should protect against crashing

Comment 9

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

Comment on attachment 621972 [details] [diff] [review]
protect against media not having a track of the required type

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

r+ with that

::: content/media/MediaStreamGraph.cpp
@@ +1833,5 @@
> +  if (track) {
> +    return track->mHaveEnough;
> +  }
> +  NS_ERROR("No track in HaveEnoughBuffered!");
> +  return false;

return true here I think. Otherwise we may try adding an observer or doing extra work that makes no sense.

@@ +1843,5 @@
>  {
>    MutexAutoLock lock(mMutex);
>    TrackData* data = FindDataForTrack(aID);
> +  if (!data)
> +    NS_ERROR("No track in DispatchWhenNotEnoughBuffered");

return here. And put {} around the if body.

Comment 10

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

Created attachment 622268 [details] [diff] [review]
actual fix

Comment 11

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

Now that I have the actual fix, which is simple, I think we should limit the first patch to just adding assertions and let the browser crash instead of adding return paths to bail out.

Comment 12

[Randell Jesup [:jesup]]

Landed crash fix before checking rest of my bugmail.  Per roc, leave in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a251bada10b5

Comment 13

[Ed Morley (Away 27th-2nd) [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a251bada10b5

Comment 14

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/714b1e450f69

Comment 15

[Ed Morley (Away 27th-2nd) [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/714b1e450f69