Last Comment Bug 752784 - mozCaptureStream on <audio> crashes
: mozCaptureStream on <audio> crashes
Status: RESOLVED FIXED
[inbound]
: crash, testcase
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla15
Assigned To: Robert O'Callahan (:roc) (email my personal email if necessary)
:
Mentors:
Depends on:
Blocks: 326633 664918
  Show dependency treegraph
 
Reported: 2012-05-07 19:58 PDT by Jesse Ruderman
Modified: 2012-12-19 21:21 PST (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (crashes Firefox when loaded) (290 bytes, text/html)
2012-05-07 19:58 PDT, Jesse Ruderman
no flags Details
crash stack trace (1.54 KB, text/plain)
2012-05-07 19:59 PDT, Jesse Ruderman
no flags Details
protect against media not having a track of the required type (2.08 KB, patch)
2012-05-08 00:14 PDT, Randell Jesup [:jesup]
no flags Details | Diff | Splinter Review
protect against media not having a track of the required type (2.14 KB, patch)
2012-05-08 07:51 PDT, Randell Jesup [:jesup]
no flags Details | Diff | Splinter Review
actual fix (3.43 KB, patch)
2012-05-08 19:46 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
cpearce: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2012-05-07 19:58:39 PDT
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 2012-05-07 19:59:50 PDT
Created attachment 621846 [details]
crash stack trace

gdb only gave me a stack trace for the crash, not the assertion.
Comment 2 Scoobidiver (away) 2012-05-07 21:47:10 PDT
On Windows 7: bp-0bd8fd67-e77f-4364-871a-24f252120508.
Comment 3 Randell Jesup [:jesup] 2012-05-07 23:16:27 PDT
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] 2012-05-08 00:14:07 PDT
Created attachment 621898 [details] [diff] [review]
protect against media not having a track of the required type
Comment 5 Randell Jesup [:jesup] 2012-05-08 00:16:37 PDT
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] 2012-05-08 00:58:28 PDT
https://tbpl.mozilla.org/?tree=Try&rev=c67b62e2dc78
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-08 02:10:22 PDT
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] 2012-05-08 07:51:20 PDT
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) 2012-05-08 14:35:48 PDT
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) 2012-05-08 19:46:53 PDT
Created attachment 622268 [details] [diff] [review]
actual fix
Comment 11 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-08 19:52:33 PDT
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] 2012-05-08 23:23:08 PDT
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 [:emorley] 2012-05-09 07:44:44 PDT
https://hg.mozilla.org/mozilla-central/rev/a251bada10b5
Comment 14 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-09 20:31:27 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/714b1e450f69
Comment 15 Ed Morley [:emorley] 2012-05-10 08:07:44 PDT
https://hg.mozilla.org/mozilla-central/rev/714b1e450f69

Note You need to log in before you can comment on or make changes to this bug.