1141230 - Some mochitest checks don't handle removed tracks properly

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect, P1)

Description

[Byron Campen [:bwc]]

When removing a track, the track is not actually removed from the DOMMediaStream; PC merely stops using it. Unfortunately, there is checking in pc.js that uses the set of all tracks on all local streams to verify SDP and such, leading to false failures when the DOMMediaStream still has a track in use (only one of multiple tracks was removed).

Comment 1

[Byron Campen [:bwc]]

So, to fix this "right" we probably want to augment expectedLocalTrackTypesById (and friends) to carry information about stream ids as well, and then use them to check things like the list of RtpSender/Receivers, msid attributes in SDP, the number of expected ICE streams, etc:

https://dxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/pc.js#878

The trouble is, in removeTrack() we need to modify this, but don't know the stream id (and currently have no way of knowing without tagging RtpSenders with an extra property to hold the stream id):

https://dxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/pc.js#896

If we assume that a track is never added to more than one stream, then we're ok (we just remove the only entry with that track id), but I'm not sure when we think we'll start supporting this. Alternately, we could add a |stream| member for the WebIDL of RtpSender, and populate it, but I'm not sure how palatable that would be (as a bonus, this would let us avoid modifying |senderReplaceTrack| to take a stream id). Or, we could just add a property to the RtpSenders from pc.js and call it a day.

jib, how do these options sound to you?

Comment 2

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

OK so I have a better understanding of remote streams now thanks to Justin [1].

In short, remote streams don't track real local streams at all, rather they track the stream arguments of pc.addTrack(), and that's it.

In other words, users can totally re-plumb their tracks and streams locally without this affecting the remote side one bit. To alter the remote picture, one would have to explicitly call pc.removeTrack(track) followed by pc.addTrack(track, newstream1, newstream2 etc.) and wait for renegotiation.

So we need to track the arguments given to pc.addTrack and use those for msids. Tests are obviously affected as well.

We need to figure out how this affects legacy getLocalStreams(), but it's probably OK to have that only work for people who call addStream rather than addTrack. 

[1] https://github.com/w3c/webrtc-pc/pull/195#issuecomment-78777881

Comment 3

[Byron Campen [:bwc]]

Attachment: MozReview Request: bz://1141230/bwc

/r/5439 - Bug 1141230: Stop using getLocalStreams() to drive checking of various things in the mochitest suite.

Pull down this commit:

hg pull review -r 2e2d817ef53e5598c1247c4bd0bf859b201dcbca

Comment 4

[Byron Campen [:bwc]]

Comment on attachment 8578114 [details]
MozReview Request: bz://1141230/bwc

/r/5439 - Bug 1141230: Stop using getLocalStreams() to drive checking of various things in the mochitest suite.

Pull down this commit:

hg pull review -r b5171e233999bdbfdd92e38fc59d9b9deb5ae947

Comment 5

[Byron Campen [:bwc]]

Comment on attachment 8578114 [details]
MozReview Request: bz://1141230/bwc

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4ebb142bd2b

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8578114 [details]
MozReview Request: bz://1141230/bwc

https://reviewboard.mozilla.org/r/5437/#review4463

lgtm! Just style nits.

::: dom/media/tests/mochitest/pc.js
(Diff revision 2)
> -    if (Object.keys(this.expectedRemoteTrackTypesById).length === 0) {
> +    if (Object.keys(this.expectedAndNotObservedRemoteTrackInfoById).length === 0) {

Object.keys().length will never be wrong type, so prefer: if(!Object.keys(...).length)

::: dom/media/tests/mochitest/templates.js
(Diff revision 2)
> -var checkAllTrackStats = pc =>
> -    Promise.all([0, 1, 2, 3].map(i => checkTrackStats(pc, i & 1, i & 2)));
> +  var promises = [].concat(
> +    pc._pc.getSenders().map(sender => checkTrackStats(pc, sender, true)),
> +    pc._pc.getReceivers().map(receiver => checkTrackStats(pc, receiver, false)));
> +  return Promise.all(promises);

Might as well put Promise.all at the top here.

::: dom/media/tests/mochitest/templates.js
(Diff revision 2)
> -    Object.keys(test.pcLocal.observedRemoteTrackTypesById).forEach(id => {
> -      delete test.pcLocal.expectedRemoteTrackTypesById[id];
> +    Object.keys(test.pcLocal.observedRemoteTrackInfoById).forEach(id => {
> +      delete test.pcLocal.expectedAndNotObservedRemoteTrackInfoById[id];

I like s/id/key/ since it's not Object.ids()

::: dom/media/tests/mochitest/templates.js
(Diff revision 2)
>      if (test.steeplechase) {
>        return test.getSignalingMessage("remote_expected_tracks").then(
>            message => {
> -            test.pcLocal.expectedRemoteTrackTypesById = message.expected_tracks;
> +            test.pcLocal.expectedRemoteTrackInfoById = message.expected_tracks;
>            });
>      } else {

Don't need else after return.

::: dom/media/tests/mochitest/templates.js
(Diff revision 2)
> +    test.pcLocal.expectedAndNotObservedRemoteTrackInfoById =
> +      JSON.parse(JSON.stringify(test.pcLocal.expectedRemoteTrackInfoById));
> +
>      // Remove what we've already observed
> -    Object.keys(test.pcLocal.observedRemoteTrackTypesById).forEach(id => {
> -      delete test.pcLocal.expectedRemoteTrackTypesById[id];
> +    Object.keys(test.pcLocal.observedRemoteTrackInfoById).forEach(id => {
> +      delete test.pcLocal.expectedAndNotObservedRemoteTrackInfoById[id];
>      });

pc.expectedAndNotObservedRemoteTrackInfoById seems like a cache. Why not make it a function:

pc.getExpectedAndNotObservedRemoteTrackInfoById() ?

::: dom/media/tests/mochitest/test_peerConnection_capturedVideo.html
(Diff revision 2)
>            stream.getTracks().forEach(tr => {
>              test.pcLocal._pc.addTrack(tr, stream);
> -            test.pcLocal.expectedLocalTrackTypesById[tr.id] = tr.kind;
> +            test.pcLocal.expectedLocalTrackInfoById[tr.id] = {
> +                type: tr.kind,

maybe s/tr/track/ ?

::: dom/media/tests/mochitest/pc.js
(Diff revision 2)
>    checkLocalMediaTracks : function() {
> -    var observedLocalTrackTypesById = {};
> -    // We do not want to empty out this.expectedLocalTrackTypesById, so make a
> +    var observedLocalTrackInfoById = {};
> +    // We do not want to empty out this.expectedLocalTrackInfoById, so make a
>      // copy.
> -    var expectedLocalTrackTypesById =
> -      JSON.parse(JSON.stringify((this.expectedLocalTrackTypesById)));
> +    var expectedLocalTrackInfoById =
> +      JSON.parse(JSON.stringify((this.expectedLocalTrackInfoById)));
>      info(this + " Checking local tracks " +
> -         JSON.stringify(expectedLocalTrackTypesById));
> -    this._pc.getLocalStreams().forEach(stream => {
> -      stream.getTracks().forEach(track => {
> -        this.checkTrackIsExpected(track,
> -                                  expectedLocalTrackTypesById,
> +         JSON.stringify(expectedLocalTrackInfoById));
> +    this._pc.getSenders().forEach(sender => {
> +      this.checkTrackIsExpected(sender.track,
> +                                expectedLocalTrackInfoById,
> +                                observedLocalTrackInfoById);
> -                                  observedLocalTrackTypesById);
> -      });
>      });
>  
> -    Object.keys(expectedLocalTrackTypesById).forEach(id => {
> +    Object.keys(expectedLocalTrackInfoById).forEach(id => {
>        ok(false, this + " local id " + id + " was observed");
>      });
>    },

This function seems like it could be shorter. How about:

    checkLocalMediaTracks: function() {
      // Don't empty out this.expectedLocalTrackInfoById, so make a copy
      var copy = JSON.parse(JSON.stringify(this.expectedLocalTrackInfoById));
      info(this + " Checking local tracks " + JSON.stringify(copy));
      this._pc.getSenders().forEach(sender =>
        this.checkTrackIsExpected(sender.track, copy, {}));
      Object.keys(copy).forEach(id =>
        ok(false, this + " localid " + id + " was observed"));
    }

::: dom/media/tests/mochitest/templates.js
(Diff revision 2)
> -function checkTrackStats(pc, audio, outbound) {
> +function checkTrackStats(pc, rtpSenderOrReceiver, outbound) {

Maybe senderOrReceiver? (I don't see rtp prefix anywhere else).

::: dom/media/tests/mochitest/templates.js
(Diff revision 2)
> -    }), msg + "2");
> +    }), msg + " - did not find stats with wrong direction");
>      ok(!pc.hasStat(stats, {
>        mediaType: audio ? "video" : "audio"
> -    }), msg + "3");
> +    }), msg + " - did not find stats with wrong media type");

Negation here means double-negative when they actually fail, which is hard to read. How about:

" - stats have correct direction"

" - stats have correct media type"

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

https://reviewboard.mozilla.org/r/5437/#review4469

I alredy r+ed this patch because I see it as an improvement, but it does not address comment 2.

::: dom/media/tests/mochitest/pc.js
(Diff revision 2)
> -  senderReplaceTrack : function(index, withTrack) {
> +  senderReplaceTrack : function(index, withTrack, withStreamId) {
>      var sender = this._pc.getSenders()[index];
> -    delete this.expectedLocalTrackTypesById[sender.track.id];
> -    this.expectedLocalTrackTypesById[withTrack.id] = withTrack.kind;
> +    delete this.expectedLocalTrackInfoById[sender.track.id];
> +    this.expectedLocalTrackInfoById[withTrack.id] = {
> +        type: withTrack.kind,
> +        streamId: withStreamId
> +      };

In the spec, pc.replaceTrack(withTrack) doesn't have any stream args because it does not alter the set of streams emitted to the other side.

Once we fix msids to come from the set of stream args given to addTrack, then this test is wrong, because it replaces the expected track and later compares it against msid.

But this requires a code-change as well as a test change, so we should probably open a new bug on that.

::: dom/media/tests/mochitest/pc.js
(Diff revision 2)
> -    checkSdpForMsids(this.localDescription, this._pc.getLocalStreams(),
> +    checkSdpForMsids(this.localDescription, this.expectedLocalTrackInfoById,
>                       "local");

Msid's do not map to local tracks, but to the arguments of pc.addtrack().

Comment 8

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

s/do not/should not/

Comment 9

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/5437/#review4509

> Negation here means double-negative when they actually fail, which is hard to read. How about:
> 
> " - stats have correct direction"
> 
> " - stats have correct media type"

Unfortunately, that's less accurate. These checks are attempting to verify that there aren't any rtp stats that we did not ask for. Maybe " - did not find extra stats ..." would be more clear. It would be nice if we could just validate that there was exactly one set of stats.

> I like s/id/key/ since it's not Object.ids()

"Id" here refers to the track id.

> pc.expectedAndNotObservedRemoteTrackInfoById seems like a cache. Why not make it a function:
> 
> pc.getExpectedAndNotObservedRemoteTrackInfoById() ?

That should be doable.

> Maybe senderOrReceiver? (I don't see rtp prefix anywhere else).

The prefix helps make it clear (to me at least) that we're talking about RtpSender/RtpReceiver.

> This function seems like it could be shorter. How about:
> 
>     checkLocalMediaTracks: function() {
>       // Don't empty out this.expectedLocalTrackInfoById, so make a copy
>       var copy = JSON.parse(JSON.stringify(this.expectedLocalTrackInfoById));
>       info(this + " Checking local tracks " + JSON.stringify(copy));
>       this._pc.getSenders().forEach(sender =>
>         this.checkTrackIsExpected(sender.track, copy, {}));
>       Object.keys(copy).forEach(id =>
>         ok(false, this + " localid " + id + " was observed"));
>     }

I can probably safely shorten some names. I'd prefer to not pass a literal '{}', since it is hard to tell what it means. I suppose I could make that param optional, which would keep things clean in this function.

> maybe s/tr/track/ ?

I guess I could fix that while I'm in here.

> Object.keys().length will never be wrong type, so prefer: if(!Object.keys(...).length)

Why not.

Comment 10

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/5437/#review4511

> In the spec, pc.replaceTrack(withTrack) doesn't have any stream args because it does not alter the set of streams emitted to the other side.
> 
> Once we fix msids to come from the set of stream args given to addTrack, then this test is wrong, because it replaces the expected track and later compares it against msid.
> 
> But this requires a code-change as well as a test change, so we should probably open a new bug on that.

Well, replaceTrack also doesn't allow intra-stream replacement, so the stream identifier won't be changing anyway right (once we've finished implementing)?

> Msid's do not map to local tracks, but to the arguments of pc.addtrack().

The args of addTrack() are what we record in expectedLocalTrackInfoById.

Comment 11

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/5437/#review4515

> I can probably safely shorten some names. I'd prefer to not pass a literal '{}', since it is hard to tell what it means. I suppose I could make that param optional, which would keep things clean in this function.

Ok, passing '{}' here is actually incorrect; that object is filled (and checked) as the tracks are checked.

Comment 12

[Byron Campen [:bwc]]

Comment on attachment 8578114 [details]
MozReview Request: bz://1141230/bwc

/r/5439 - Bug 1141230: Stop using getLocalStreams() to drive checking of various things in the mochitest suite. r=jib

Pull down this commit:

hg pull review -r 5a8790c2d7e8d2336f9ae68ad76ba4253f596093

Comment 13

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/5437/#review4519

> That should be doable.

Actually, I can remove this entirely, and it will be less code.

Comment 14

[Byron Campen [:bwc]]

Comment on attachment 8578114 [details]
MozReview Request: bz://1141230/bwc

Incorporated feedback, rebased, carry forward r=jib

https://treeherder.mozilla.org/#/jobs?repo=try&revision=82dafbcda55a

Comment 15

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

https://reviewboard.mozilla.org/r/5437/#review4539

::: dom/media/tests/mochitest/pc.js
(Diff revisions 2 - 3)
> +  allExpectedTracksAreObserved: function(expected, observed) {
> +    Object.keys(expected).forEach(trackId => {
> +      if (!observed[trackId]) {
> +        return false;
> +      }
> +    });
> +
> +    return true;
> +  },

.forEach() takes a function and ignores its return value, so this will never return false! Use:

Object.keys(expected).every(id => observed[id]);

Comment 16

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

https://reviewboard.mozilla.org/r/5437/#review4537

> Well, replaceTrack also doesn't allow intra-stream replacement, so the stream identifier won't be changing anyway right (once we've finished implementing)?

Not sure what you mean by "intra-stream replacement". In my mind, if I do:

1. var sender = pc.addTrack(trackA, streamA);
2. sender.replaceTrack(trackB);

then a=msid:{streamA} {trackB}, even though trackB remains in streamB locally, until I do

3. pc.removeTrack(sender);
4. sender = pc.addTrack(trackB, streamB);

Which means, in order to not fail, the code above would then need to be:

    var stream = this.expectedLocalTrackInfoById[sender.track.id].streamId;
    delete this.expectedLocalTrackInfoById[sender.track.id];
    this.expectedLocalTrackTypesById[withTrack.id] = withTrack.kind;
    this.expectedLocalTrackInfoById[withTrack.id] = {
      type: withTrack.kind,
      streamId: stream
    };

Comment 17

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/5437/#review4541

> Not sure what you mean by "intra-stream replacement". In my mind, if I do:
> 
> 1. var sender = pc.addTrack(trackA, streamA);
> 2. sender.replaceTrack(trackB);
> 
> then a=msid:{streamA} {trackB}, even though trackB remains in streamB locally, until I do
> 
> 3. pc.removeTrack(sender);
> 4. sender = pc.addTrack(trackB, streamB);
> 
> Which means, in order to not fail, the code above would then need to be:
> 
>     var stream = this.expectedLocalTrackInfoById[sender.track.id].streamId;
>     delete this.expectedLocalTrackInfoById[sender.track.id];
>     this.expectedLocalTrackTypesById[withTrack.id] = withTrack.kind;
>     this.expectedLocalTrackInfoById[withTrack.id] = {
>       type: withTrack.kind,
>       streamId: stream
>     };

By "doesn't allow intra-stream replacement", I mean that replaceTrack would fail in your example, unless we're changing our proposal for replaceTrack. If we do intend to allow this kind of replacement in the spec, then yeah we'll need to update this when we update PeerConnectionImpl.

Comment 18

[Byron Campen [:bwc]]

Comment on attachment 8578114 [details]
MozReview Request: bz://1141230/bwc

/r/5439 - Bug 1141230: Stop using getLocalStreams() to drive checking of various things in the mochitest suite. r=jib

Pull down this commit:

hg pull review -r 4c5faa0c8fbaf9f2531ee4a0fcee99c077d2e662

Comment 19

[Byron Campen [:bwc]]

Comment on attachment 8578114 [details]
MozReview Request: bz://1141230/bwc

More feedback, carry forward r=jib

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4e0cb0c205b

Comment 20

[Byron Campen [:bwc]]

Maybe someday I'll get to resubmit that try push and have it run on working infra...

Comment 21

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c38b4579acb5

Comment 22

[Byron Campen [:bwc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1534390c5c6e

Comment 23

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1534390c5c6e

Comment 25

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1141230: Stop using getLocalStreams() to drive checking of various things in the mochitest suite. r=jib