Closed Bug 1437832 Opened 6 years ago Closed 6 years ago

RTCPeerconnection.removeTrack throws exception

Categories

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

59 Branch
defect

Tracking

()

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

People

(Reporter: felix.ilbring, Assigned: bwc)

References

Details

(Keywords: regression)

Attachments

(4 files)

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
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.