Closed
Bug 1173656
Opened 9 years ago
Closed 9 years ago
Disallow TrackID reuse in TrackUnionStream
Categories
(Core :: Audio/Video, defect)
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.
Assignee | ||
Comment 1•9 years ago
|
||
Bug 1173656 - Disallow TrackID reuse in TrackUnionStream. r=roc
Attachment #8621531 -
Flags: review?(roc)
Assignee | ||
Comment 2•9 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•9 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)
Comment 5•9 years ago
|
||
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•9 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
Comment 8•9 years ago
|
||
Backed out for the same OSX crashes/asserts you starred away on your Try push. https://treeherder.mozilla.org/logviewer.html#?job_id=10777924&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/758269f1463c
Assignee | ||
Comment 9•9 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)
Comment 10•9 years ago
|
||
(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•9 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
Comment 12•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a726ca79e03d
Keywords: checkin-needed
Comment 13•9 years ago
|
||
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.
Description
•