Closed Bug 1423842 Opened 6 years ago Closed 6 years ago

onaddstream changed behaviour with transceivers

Categories

(Core :: WebRTC: Signaling, enhancement, P2)

Product:
Core
Component:
WebRTC: Signaling
Version:
59 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: fippo, Assigned: bwc)

Details

Attachments

(2 files)

Bug 1423842 - Part 1: Test-case: onaddstream should fire after tracks are added, but before SRD resolves.
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1423842 - Part 2: Fire onaddstream after track additions, but before SRD resolves.
59 bytes, text/x-review-board-request
jib
: review+
Details 
Firefox 59 broke some test assertions in adapter:
https://github.com/webrtc/adapter/blob/6cabb6ea8ba9a9a6fa9c5a6fad2f60bc250ef54f/test/e2e/connection.js#L141

https://jsfiddle.net/m0ypzqab/3/ shows the behaviour a bit more clear.
onaddstream is triggered with a stream that has no tracks, then ontrack adds the tracks.

This means that it is no longer possible to determine whether the incoming stream is audio or audio-video in onaddstream.
Rank: 15
Priority: -- → P2
Assignee: nobody → docfaraday
Comment on attachment 8937520 [details]
Bug 1423842 - Part 2: Fire onaddstream after track additions, but before SRD resolves.

https://reviewboard.mozilla.org/r/208210/#review214028

Queueing fires the addtrack event after SRD resolves (I checked), which we don't want.

I compared using https://jsfiddle.net/jib1/8rkLyq4x/

FF57 (desired legacy behavior):

SRD start...
ontrack
ontrack
onaddstream
...SRD success

With this patch:

SRD start...
ontrack
ontrack
...SRD success
onaddstream

::: dom/media/PeerConnection.js:1330
(Diff revision 1)
> +      // Queue it so it comes in after the track additions have happened
> +      this._queueTaskWithClosedCheck(() => {

Instead of queueing, we should pass in an empty array as a `streamEvents` argument, attach any event to that array, and rely on the caller [1] firing the event right after the track event.

Also note that care should be taken when dispatching multiple events synchronously (they're synchronous callbacks), to avoid malicious JS messing with state in ways that could mess us up (cache all needed state once, for-loop on copies of  arrays, etc.).

[1] https://searchfox.org/mozilla-central/rev/b1e0ae2573e88391a7ed92407568be77994c9317/dom/media/PeerConnection.js#1820
Attachment #8937520 - Flags: review?(jib) → review-
Comment on attachment 8937519 [details]
Bug 1423842 - Part 1: Test-case: onaddstream should fire after tracks are added, but before SRD resolves.

https://reviewboard.mozilla.org/r/208208/#review214030

Thanks! It would be good to test the order of the events and the SRD success callback, but I'd take a follow-up bug on that to get this landed before break.
Attachment #8937519 - Flags: review?(jib) → review+
Comment on attachment 8937520 [details]
Bug 1423842 - Part 2: Fire onaddstream after track additions, but before SRD resolves.

https://reviewboard.mozilla.org/r/208210/#review214028

> Instead of queueing, we should pass in an empty array as a `streamEvents` argument, attach any event to that array, and rely on the caller [1] firing the event right after the track event.
> 
> Also note that care should be taken when dispatching multiple events synchronously (they're synchronous callbacks), to avoid malicious JS messing with state in ways that could mess us up (cache all needed state once, for-loop on copies of  arrays, etc.).
> 
> [1] https://searchfox.org/mozilla-central/rev/b1e0ae2573e88391a7ed92407568be77994c9317/dom/media/PeerConnection.js#1820

I believe this should be fine now.
Comment on attachment 8937520 [details]
Bug 1423842 - Part 2: Fire onaddstream after track additions, but before SRD resolves.

https://reviewboard.mozilla.org/r/208210/#review214504

::: dom/media/PeerConnection.js:369
(Diff revisions 1 - 2)
> +    // Used to fire onaddstream, remove when we don't do that anymore.
> +    this._newStreams = [];

I expected not to need this, and instead have this array passed in to onTrack and OnSetRemoteDescriptionSuccess, since this is only needed for a narrow window of time on a single task.

Is this to avoid touching the PeerConnectionImpl.webidl? I guess this is fine, provided there's never any way for the coder to fail inbetween these two steps [1] (if it did, this would leave tracks on the array and they'd be fired later on by some unrelated event.

[1] https://searchfox.org/mozilla-central/rev/ff462a7f6b4dd3c3fdc53c9bc0b328f42a5b3d2b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp#1936,1938

::: dom/media/PeerConnection.js:1327
(Diff revisions 1 - 2)
>          this.dispatchEvent(new this._win.Event("negotiationneeded"));
>        }
>      });
>    }
>  
> -  _getOrCreateStream(id) {
> +  _fireAddStreamEvents() {

Maybe _fireLegacyAddStreamEvents() ?

::: dom/media/PeerConnection.js:1328
(Diff revisions 1 - 2)
>        }
>      });
>    }
>  
> -  _getOrCreateStream(id) {
> -    if (!this._receiveStreams.has(id)) {
> +  _fireAddStreamEvents() {
> +    this._newStreams.forEach(stream => {

Prefer

    for (let stream of this._newStreams) {

Better for maintenance (if someone tries to add an early return in the loop or something).

::: dom/media/PeerConnection.js:1329
(Diff revisions 1 - 2)
>  
> -  _getOrCreateStream(id) {
> -    if (!this._receiveStreams.has(id)) {
> +  _fireAddStreamEvents() {
> +    this._newStreams.forEach(stream => {
> -      let stream = new this._win.MediaStream();
> -      stream.assignId(id);
>        // Legacy event, remove eventually

Please link to Bug 1241291.
Attachment #8937520 - Flags: review?(jib) → review+
Comment on attachment 8937520 [details]
Bug 1423842 - Part 2: Fire onaddstream after track additions, but before SRD resolves.

https://reviewboard.mozilla.org/r/208210/#review214504

> I expected not to need this, and instead have this array passed in to onTrack and OnSetRemoteDescriptionSuccess, since this is only needed for a narrow window of time on a single task.
> 
> Is this to avoid touching the PeerConnectionImpl.webidl? I guess this is fine, provided there's never any way for the coder to fail inbetween these two steps [1] (if it did, this would leave tracks on the array and they'd be fired later on by some unrelated event.
> 
> [1] https://searchfox.org/mozilla-central/rev/ff462a7f6b4dd3c3fdc53c9bc0b328f42a5b3d2b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp#1936,1938

Yes, this is to avoid the need to pass this stuff all the way back to PeerConnectionImpl. I can add a try/catch around the dispatchEvent to guard against content throwing us out of the loop.
Pushed by bcampen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/17a32b697729
Part 1: Test-case: onaddstream should fire after tracks are added, but before SRD resolves. r=jib
https://hg.mozilla.org/integration/autoland/rev/24eff1df1d3b
Part 2: Fire onaddstream after track additions, but before SRD resolves. r=jib
https://hg.mozilla.org/mozilla-central/rev/17a32b697729
https://hg.mozilla.org/mozilla-central/rev/24eff1df1d3b
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox59: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: