Disallow TrackID reuse in TrackUnionStream

RESOLVED FIXED in Firefox 41

Status

()

Core
Audio/Video
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: pehrsons, Assigned: pehrsons)

Tracking

41 Branch
mozilla41
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8621531 [details]
MozReview Request: Bug 1173656 - Disallow TrackID reuse in TrackUnionStream. r=roc

Bug 1173656 - Disallow TrackID reuse in TrackUnionStream. r=roc
Attachment #8621531 - Flags: review?(roc)
(Assignee)

Comment 2

3 years ago
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+
(Assignee)

Comment 4

3 years ago
(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)
(Assignee)

Comment 6

3 years ago
(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
(Assignee)

Comment 9

3 years ago
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)
(Assignee)

Comment 11

3 years ago
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
Last Resolved: 3 years ago
status-firefox41: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.