Closed Bug 1437832 Opened 7 years ago Closed 7 years ago

RTCPeerconnection.removeTrack throws exception

Categories

(Core :: WebRTC: Networking, defect, P2)

Product:
Core
Component:
WebRTC: Networking
Version:
59 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- unaffected
firefox59 - fixed
firefox60 + fixed

People

(Reporter: felix.ilbring, Assigned: bwc)

References

Details

(Keywords: regression)

Attachments

(4 files)

Bug 1437832 - Part 1: Test-case for addTransceiver, then addTrack, then removeTrack. r+jib
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1437832 - Part 2: Use the same C++ logic for replaceTrack, removeTrack, and addTrack (when on a pre-existing transceiver). r+jib
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1437832 - Part 3: Remove unused function from PeerConnectionImpl.webidl
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1437832 - Part 4: Remove unused function from PeerConnectionImpl
59 bytes, text/x-review-board-request
jib
: review+
lizzard
: approval-mozilla-beta+
Details
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:59.0) Gecko/20100101 Firefox/59.0 Build ID: 20180205211730 Steps to reproduce: I created a fiddle to easily reproduce the problem. The first flow shows a working example The second flow throws the exception. https://jsfiddle.net/j4168zv8/1/ * send an offer without any streams first * add a stream to the local peer connection, do the offer - answer * remove track from local peer connection Actual results: * remote peer connection does not display the stream, although it receives data (see console for track added event. about:webrtc shows increasing bytes send/received) * removing the upstream from local peer connection throws an exception Expected results: * you can send as many offers with/without stream as you like * removing streams from a peer connection does not throw
Component: Untriaged → WebRTC: Networking
Product: Firefox → Core
To clarify the use case of this: In a setup with an MCU we set up the rx media streams before sharing local media streams towards the MCU. As such creating offers without initially offering to send media is a requirement for GoToMeeting to support Firefox and this is a breaking change in 59 compared to 58 and previous. E.g. it used to work and is still working in the other browsers we target.
I think I see what is going on here.
Assignee: nobody → docfaraday
Blocks: 1290948
status-firefox59: --- → affected
status-firefox60: --- → affected
tracking-firefox59: --- → ?
tracking-firefox60: --- → ?
Keywords: regression
Comment on attachment 8950716 [details] Bug 1437832 - Part 1: Test-case for addTransceiver, then addTrack, then removeTrack. r+jib https://reviewboard.mozilla.org/r/219962/#review225782 ::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:797 (Diff revision 1) > + let offer = await pc.createOffer(); > + await pc.setLocalDescription(offer); Nit: offer is unused. Why not await pc.setLocalDescription(await pc.createOffer());
Attachment #8950716 - Flags: review?(jib) → review+
Comment on attachment 8950719 [details] Bug 1437832 - Part 4: Remove unused function from PeerConnectionImpl https://reviewboard.mozilla.org/r/219968/#review225796
Attachment #8950719 - Flags: review?(jib) → review+
Comment on attachment 8950718 [details] Bug 1437832 - Part 3: Remove unused function from PeerConnectionImpl.webidl https://reviewboard.mozilla.org/r/219966/#review225798
Attachment #8950718 - Flags: review+
Comment on attachment 8950718 [details] Bug 1437832 - Part 3: Remove unused function from PeerConnectionImpl.webidl https://reviewboard.mozilla.org/r/219966/#review225800
Attachment #8950718 - Flags: review?(bugs) → review+
Comment on attachment 8950718 [details] Bug 1437832 - Part 3: Remove unused function from PeerConnectionImpl.webidl https://reviewboard.mozilla.org/r/219966/#review225802 I think we need to update MDN https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection/removeTrack to reflect this change.
@sheppy: just for your awareness see comment #11
Flags: needinfo?(eshepherd)
Never mind. I thought removing it from the webidl and C++ meant we are no longer supporting removeTrack() at all. But I now I see it only got moved to JS.
Flags: needinfo?(eshepherd)
Comment on attachment 8950717 [details] Bug 1437832 - Part 2: Use the same C++ logic for replaceTrack, removeTrack, and addTrack (when on a pre-existing transceiver). r+jib https://reviewboard.mozilla.org/r/219964/#review225846 Lgtm with nits. ::: dom/media/PeerConnection.js:2039 (Diff revision 1) > this.track = track; > + this._pc._replaceTrack(this._transceiverImpl, track); If something goes wrong in _replaceTrack (e.g. the pc is closed), then this sets sender.track before throwing an error, which seems incorrect. To avoid maintenance mistakes, maybe set the track after? I also notice _replaceTrack [1] calls this._checkClosed(); ...which seems redundant since AFAICT all code-paths that lead here have called it already. Remove? Also, maybe rename _replaceTrack to _replaceTrackNoRenegotiation or something to avoid confusion with the API implementation method? [1] https://searchfox.org/mozilla-central/rev/d03ad8843e3bf2e856126bc53b0475c595e5183b/dom/media/PeerConnection.js#1379-1381
Attachment #8950717 - Flags: review?(jib) → review+
Status: UNCONFIRMED → NEW
Rank: 15
Ever confirmed: true
Priority: -- → P2
Pushed by bcampen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ec7939040da7 Part 1: Test-case for addTransceiver, then addTrack, then removeTrack. r+jib r=jib https://hg.mozilla.org/integration/autoland/rev/57b2226d05e8 Part 2: Use the same C++ logic for replaceTrack, removeTrack, and addTrack (when on a pre-existing transceiver). r+jib r=jib https://hg.mozilla.org/integration/autoland/rev/2040187280ad Part 3: Remove unused function from PeerConnectionImpl.webidl r=jib,smaug https://hg.mozilla.org/integration/autoland/rev/a20b068b82f7 Part 4: Remove unused function from PeerConnectionImpl r=jib
Request beta uplift once this looks good on m-c.
Flags: needinfo?(docfaraday)
Comment on attachment 8950719 [details] Bug 1437832 - Part 4: Remove unused function from PeerConnectionImpl Approval Request Comment [Feature/Bug causing the regression]: Bug 1290948 [User impact if declined]: webrtc services that use removeTrack might not work correctly [Is this code covered by automated tests?]: Yes, there's a test for this now. [Has the fix been verified in Nightly?]: It seems ok. [Needs manual test from QE? If yes, steps to reproduce]: No. [List of other uplifts needed for the feature/fix]: None. [Is the change risky?]: Not particularly. [Why is the change risky/not risky?]: We've consolidated some code here, and there is always the risk that something got lost in the shuffle, but the patch is pretty simple so probably not. [String changes made/needed]: None.
Flags: needinfo?(docfaraday)
Attachment #8950719 - Flags: approval-mozilla-beta?
Comment on attachment 8950719 [details] Bug 1437832 - Part 4: Remove unused function from PeerConnectionImpl Fix for a minor regression in 59, let's uplift for beta 11. I don't see a new test here - is the coverage in bug 1437832?
Flags: needinfo?(docfaraday)
Attachment #8950719 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Yeah, in part 1.
Flags: needinfo?(docfaraday)
Marking as qe- since it has automated coverage.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: