The default bug view has changed. See this FAQ.

mozCaptureStream on <audio> crashes

RESOLVED FIXED in mozilla15

Status

()

Core
Audio/Video
--
critical
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Jesse Ruderman, Assigned: roc)

Tracking

(Blocks: 1 bug, {crash, testcase})

Trunk
mozilla15
crash, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound], crash signature)

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

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

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

Crash [@ mozilla::SourceMediaStream::HaveEnoughBuffered]
(Reporter)

Comment 1

5 years ago
Created attachment 621846 [details]
crash stack trace

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

Comment 2

5 years ago
On Windows 7: bp-0bd8fd67-e77f-4364-871a-24f252120508.
Crash Signature: [@ mozilla::SourceMediaStream::HaveEnoughBuffered] [@ mozilla::SourceMediaStream::HaveEnoughBuffered(int)]
OS: Mac OS X → All
Hardware: x86_64 → All
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
Attachment #621898 - Flags: review?(roc)
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

Updated

5 years ago
Attachment #621898 - Attachment is obsolete: true
Attachment #621898 - Flags: review?(roc)

Updated

5 years ago
Attachment #621972 - Flags: review?(roc)
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
Assignee: nobody → roc
Attachment #622268 - Flags: review?(cpearce)
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.
Attachment #622268 - Flags: review?(cpearce) → review+
Landed crash fix before checking rest of my bugmail.  Per roc, leave in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a251bada10b5
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/a251bada10b5
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
https://hg.mozilla.org/integration/mozilla-inbound/rev/714b1e450f69
https://hg.mozilla.org/mozilla-central/rev/714b1e450f69
Depends on: 757707
No longer depends on: 757707

Updated

4 years ago
Attachment #621972 - Flags: review?(roc)
You need to log in before you can comment on or make changes to this bug.