Created attachment 621845 [details] testcase (crashes Firefox when loaded) ###!!! ASSERTION: Bad track ID!: 'Error', file MediaStreamGraph.h, line 466 Crash [@ mozilla::SourceMediaStream::HaveEnoughBuffered]
Created attachment 621846 [details] crash stack trace gdb only gave me a stack trace for the crash, not the assertion.
On Windows 7: bp-0bd8fd67-e77f-4364-871a-24f252120508.
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()).
Created attachment 621898 [details] [diff] [review] protect against media not having a track of the required type
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
https://tbpl.mozilla.org/?tree=Try&rev=c67b62e2dc78
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
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 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.
Created attachment 622268 [details] [diff] [review] actual fix
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.
Landed crash fix before checking rest of my bugmail. Per roc, leave in: https://hg.mozilla.org/integration/mozilla-inbound/rev/a251bada10b5
https://hg.mozilla.org/mozilla-central/rev/a251bada10b5
https://hg.mozilla.org/integration/mozilla-inbound/rev/714b1e450f69
https://hg.mozilla.org/mozilla-central/rev/714b1e450f69
or
