Closed
Bug 752784
Opened 13 years ago
Closed 13 years ago
mozCaptureStream on <audio> crashes
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: jruderman, Assigned: roc)
References
Details
(Keywords: crash, testcase, Whiteboard: [inbound])
Crash Data
Attachments
(4 files, 1 obsolete file)
290 bytes,
text/html
|
Details | |
1.54 KB,
text/plain
|
Details | |
2.14 KB,
patch
|
Details | Diff | Splinter Review | |
3.43 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
###!!! ASSERTION: Bad track ID!: 'Error', file MediaStreamGraph.h, line 466
Crash [@ mozilla::SourceMediaStream::HaveEnoughBuffered]
Reporter | ||
Comment 1•13 years ago
|
||
gdb only gave me a stack trace for the crash, not the assertion.
Comment 2•13 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
Comment 3•13 years ago
|
||
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•13 years ago
|
||
Comment 5•13 years ago
|
||
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 6•13 years ago
|
||
Assignee | ||
Comment 7•13 years ago
|
||
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•13 years ago
|
||
Updated patch - does not correct the fundamental problem, but should protect against crashing
Updated•13 years ago
|
Attachment #621898 -
Attachment is obsolete: true
Attachment #621898 -
Flags: review?(roc)
Updated•13 years ago
|
Attachment #621972 -
Flags: review?(roc)
Assignee | ||
Comment 9•13 years ago
|
||
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.
Assignee | ||
Comment 10•13 years ago
|
||
Assignee: nobody → roc
Attachment #622268 -
Flags: review?(cpearce)
Assignee | ||
Comment 11•13 years ago
|
||
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.
Updated•13 years ago
|
Attachment #622268 -
Flags: review?(cpearce) → review+
Comment 12•13 years ago
|
||
Landed crash fix before checking rest of my bugmail. Per roc, leave in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a251bada10b5
Whiteboard: [inbound]
Comment 13•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Assignee | ||
Comment 14•13 years ago
|
||
Comment 15•13 years ago
|
||
Updated•13 years ago
|
Attachment #621972 -
Flags: review?(roc)
You need to log in
before you can comment on or make changes to this bug.
Description
•