1214506 - crash in mozilla::AudioChunk::SliceTo(__int64, __int64)

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Mantaroh Yoshinaga[:mantaroh]]

This bug was filed from the Socorro interface and is 
report bp-e3c64afb-843a-4170-a9e5-59a152151014.
=============================================================
Reproduce step:
 1) access the testpage[1].
 2) select ogv file.

Actually Result:
 Crash (non e10s)

Comment 1

[Mantaroh Yoshinaga[:mantaroh]]

Sorry, I have filed an unfinished bug.

Reproduce Step:
 1) access the testpage[1]
 2) select ogv file.

Actually Result:
 - Crashed the firefox.(none e10s)
 - Crashed the tab.(e10s)

Expected Result:
 we can play the video without crash.

[1] http://mantaroh.github.io/crash-audio-chunk/

Comment 2

[Mantaroh Yoshinaga[:mantaroh]]

Hi cpearce,

I tried to play ogg video using blob and mozCaptureStream.
However current nightly was crashed.
(It was success using video's src attribute.)

Some ogg video was success.

I have a question about ogg file format.
Does support ogg video using blob in current Firefox?

Additional Info:
In fact current B2G Simulator for TV is using similar implementation,
And this simulator have similar crash bug.(bug 1214115)

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #2)
> Does support ogg video using blob in current Firefox?

Should do. Looks like this is a crash in MSG code, not in Ogg specific code.

Comment 4

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: bug1214506_20151016_ASSERTION_in_TrackUnionStream.log

Thanks Chris.

I tested nightly with debug symbols. It seems MediaStreamGraphImpl::UpdateCurrentTimeForStreams() accessed the released stream object.[1] I don't know why we are accessing a released object.

I also tried testing with a debug build. The assertion at TrackUnionStream:CopyTrackData[2] failed. I have attached a console log.

I'm not familiar with MediaStreamGraph. Can you suggest someone who can help?

[1] https://dxr.mozilla.org/mozilla-central/rev/e193b4da0a8c1025aa76a403c64663ff1cd41709/dom/media/MediaStreamGraph.cpp#245
[2] https://dxr.mozilla.org/mozilla-central/rev/e193b4da0a8c1025aa76a403c64663ff1cd41709/dom/media/TrackUnionStream.cpp#270

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Roc or JW know more about the MSG and may be able to help.

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

Haven't touch MSG for a while. Paul should know better.

Comment 7

[Paul Adenot (:padenot)]

Andreas, this is asserting like crazy in tracks code, care to have a look? This is easy to repro here. You open the URL, save the two files locally, and load them using the button, alternating between the two (sometimes it works, and I find sometimes shift reloading helps repro-ing).

Comment 8

[Andreas Pehrson [:pehrsons]]

I'm a bit swamped with MediaStream stuff at the moment, but I'll put it on my list of things to do while waiting for reviews.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm looking into it.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We get into DecodedStream::SendVideo with mInfo.mAudio.mTrackId == mInfo.mVideo.mTrackId == -1.

This actually comes from OggReader. In OggReader::ReadMetadata mInfo ends up with those two mTrackIds both being -1. They definitely shouldn't have the same track ID, and I guess they shouldn't be TRACK_INVALID.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah, so OggReader simply doesn't assign track IDs. So duplicate TRACK_INVALID track IDs get put into the MediaStream and things go downhill from there.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: MozReview Request: Bug 1214506. Ensure OggReader sets proper IDs for its tracks. r=jya

Bug 1214506. Ensure OggReader sets proper IDs for its tracks. r=jya

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: MozReview Request: Bug 1214506. Eliminate default values for track IDs. r=jya

Bug 1214506. Eliminate default values for track IDs. r=jya

Comment 14

[Jean-Yves Avenard [:jya]]

Comment on attachment 8681038 [details]
MozReview Request: Bug 1214506. Ensure OggReader sets proper IDs for its tracks. r=jya

https://reviewboard.mozilla.org/r/23721/#review21245

::: dom/media/ogg/OggReader.cpp:126
(Diff revision 1)
> -              aEnable);
> +              aEnable, (TrackID)aTrackType);

This is a bit ugly IMHO. What does casting a TrackType into a TrackID means? And a C-cast at that :)

If we want to set the track id for audio to 1 and video track id to 2 ; It would be better to have this set explicitly.
eg.
aTrackType == TrackType::kAudioTrack ? ...

  enum TrackType {
    kUndefinedTrack,
    kAudioTrack,
    kVideoTrack,
    kTextTrack
  };

the value of kAudioTrack and kVideoTrack should be valueless.
If anyone add a constant before kAudioTrack, the OggReader will then be broken.

I think it would be safer to have a pure virtual TrackInfo::Init that doesn't take a TrackType.
It seems wrong to me to be able to run Init on an existing VideoTrack and be able to change its type.
This would cause havoc in the MediaCode as we typically pass a const TrackInfo& and rely on the TrackType value to cast this into either a VideoInfo& or an AudioInfo&.
We would then likely perform an out of bound read

Comment 15

[Jean-Yves Avenard [:jya]]

Comment on attachment 8681039 [details]
MozReview Request: Bug 1214506. Eliminate default values for track IDs. r=jya

https://reviewboard.mozilla.org/r/23723/#review21249

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Jean-Yves Avenard [:jya] from comment #14)
> ::: dom/media/ogg/OggReader.cpp:126
> (Diff revision 1)
> > -              aEnable);
> > +              aEnable, (TrackID)aTrackType);
> 
> This is a bit ugly IMHO. What does casting a TrackType into a TrackID means?
> And a C-cast at that :)

It doesn't matter as long as it gives us unique values.

> If we want to set the track id for audio to 1 and video track id to 2 ; It
> would be better to have this set explicitly.
> eg.
> aTrackType == TrackType::kAudioTrack ? ...
> 
>   enum TrackType {
>     kUndefinedTrack,
>     kAudioTrack,
>     kVideoTrack,
>     kTextTrack
>   };
> 
> the value of kAudioTrack and kVideoTrack should be valueless.

I don't know what this means.

> If anyone add a constant before kAudioTrack, the OggReader will then be
> broken.

I don't know what this means either.

> I think it would be safer to have a pure virtual TrackInfo::Init that
> doesn't take a TrackType.
> It seems wrong to me to be able to run Init on an existing VideoTrack and be
> able to change its type.

I can just remove the aTrackType parameter from Init.

Comment 17

[Jean-Yves Avenard [:jya]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16)
> > This is a bit ugly IMHO. What does casting a TrackType into a TrackID means?
> > And a C-cast at that :)
> 
> It doesn't matter as long as it gives us unique values.

I thought you wanted them to be of a specific value (here: audio=1 and video=2) which made sense as it's the default values given by AudioInfo and VideoInfo constructor respectively.

> > If anyone add a constant before kAudioTrack, the OggReader will then be
> > broken.
> 
> I don't know what this means either.

If it's the value 1 and 2 that we need; adding an enum in that declaration would change it to 2 and 3 etc.

> 
> > I think it would be safer to have a pure virtual TrackInfo::Init that
> > doesn't take a TrackType.
> > It seems wrong to me to be able to run Init on an existing VideoTrack and be
> > able to change its type.
> 
> I can just remove the aTrackType parameter from Init.

that would work. It appears this is only used by OggReader however. so maybe it doesn't matter.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8681038 [details]
MozReview Request: Bug 1214506. Ensure OggReader sets proper IDs for its tracks. r=jya

Bug 1214506. Ensure OggReader sets proper IDs for its tracks. r=jya

Comment 19

[Jean-Yves Avenard [:jya]]

Comment on attachment 8681038 [details]
MozReview Request: Bug 1214506. Ensure OggReader sets proper IDs for its tracks. r=jya

https://reviewboard.mozilla.org/r/23721/#review21257

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/485a23bbf84793dd05268420c61642f3856de5a3
Bug 1214506. Ensure OggReader sets proper IDs for its tracks. r=jya

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/485a23bbf847

Comment 22

[Paul Adenot (:padenot)]

(clearing NI)

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/485a23bbf847

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8681038 [details]
MozReview Request: Bug 1214506. Ensure OggReader sets proper IDs for its tracks. r=jya

Approval Request Comment
[Feature/regressing bug #]: none
[User impact if declined]: possible crashes mixing Ogg files with new stream APIs
[Describe test coverage new/current, TreeHerder]: automated tests catch this (with asserts added in this patch)
[Risks and why]: very low risk, simple change.
[String/UUID change made/needed]: none.

Comment 25

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8681038 [details]
MozReview Request: Bug 1214506. Ensure OggReader sets proper IDs for its tracks. r=jya

This has been on Nightly for over a week, seems safe to uplift to Aurora44.

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d7ab2f17e377

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/d7ab2f17e377