Closed Bug 752784 Opened 13 years ago Closed 13 years ago

mozCaptureStream on <audio> crashes

Categories

(Core :: Audio/Video, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: jruderman, Assigned: roc)

References

Details

(Keywords: crash, testcase, Whiteboard: [inbound])

Crash Data

Attachments

(4 files, 1 obsolete file)

###!!! ASSERTION: Bad track ID!: 'Error', file MediaStreamGraph.h, line 466 Crash [@ mozilla::SourceMediaStream::HaveEnoughBuffered]
Attached file crash stack trace
gdb only gave me a stack trace for the crash, not the assertion.
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()).
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)
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
Updated patch - does not correct the fundamental problem, but should protect against crashing
Attachment #621898 - Attachment is obsolete: true
Attachment #621898 - 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.
Attached patch actual fixSplinter Review
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]
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Depends on: 757707
No longer depends on: 757707
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: