Closed Bug 1173656 Opened 9 years ago Closed 9 years ago

Disallow TrackID reuse in TrackUnionStream

Categories

(Core :: Audio/Video, defect)

41 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: pehrsons, Assigned: pehrsons)

References

Details

Attachments

(1 file)

TrackUnionStream may reuse a TrackID if the previous track with that ID has ended (due to it ending in the source, or the input port being removed). This has caused issues like bug 1172397 and causes unexpected behavior when reattaching sources often like in bug 1172394.

If we disallow reuse we'll get rid of this non-determinism.
Bug 1173656 - Disallow TrackID reuse in TrackUnionStream. r=roc
Attachment #8621531 - Flags: review?(roc)
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd2b742c397f
Assignee: nobody → pehrsons
Status: NEW → ASSIGNED
Comment on attachment 8621531 [details]
MozReview Request: Bug 1173656 - Disallow TrackID reuse in TrackUnionStream. r=roc

https://reviewboard.mozilla.org/r/10985/#review9689

Ship It!
Attachment #8621531 - Flags: review?(roc) → review+
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #2)
> Try push:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd2b742c397f

Paul, have you seen this shutdown crash before? I'm guessing it's related to my `--tag msg` and would pass on a full try, but still a problem.
Flags: needinfo?(padenot)
This is deleting an audio output stream from the main thread, because the MSG is being shut down, and the MSG ControlMessage runs synchronously.

It is likely to race with the normal deletion (and in fact this might simply be a UAF). Normaly we would use a separate thread to avoid blocking either the main thread or the system thread that runs the graph.

I filed bug 1174647 to fix this, as we might run into it if mochitest re-chunking occurs (or in the wild, of course).
Flags: needinfo?(padenot)
(In reply to Paul Adenot (:padenot) from comment #5)
> I filed bug 1174647 to fix this, as we might run into it if mochitest
> re-chunking occurs (or in the wild, of course).

Great, thanks!

Try push with full chunks: https://treeherder.mozilla.org/#/jobs?repo=try&revision=159729a81738

There were surprisingly many oranges at first but after some retriggers I dare say they are old issues. I have no idea how this patch could have introduced any of them in any case.
Keywords: checkin-needed
Thanks Ryan.

Bobby, I see that you've been in MediaTaskQueue.cpp recently. Could you take a look at this assertion failure?

I don't see how this patch could be related but there are MediaRecorder and ImageCapture tests (using MediaStreams and so could affect timing) prior to the failure.
Flags: needinfo?(bobbyholley)
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #9)
> Thanks Ryan.
> 
> Bobby, I see that you've been in MediaTaskQueue.cpp recently. Could you take
> a look at this assertion failure?
> 
> I don't see how this patch could be related but there are MediaRecorder and
> ImageCapture tests (using MediaStreams and so could affect timing) prior to
> the failure.

This is the same crash that I was backed out for in bug 1163223 comment 34. Jean-Yves was also hitting it locally, so I think the problem is pretty independent of any of our patches.

I just pushed bug 1174939 to inbound, which should fix it.
Depends on: 1174939
Flags: needinfo?(bobbyholley)
Thanks Bobby! I verified it here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=58a35a7b695f

So unless bug 1174939 doesn't get backed out, please land.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a726ca79e03d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: