952145 - JSEP support for state rollbacks

Status: RESOLVED FIXED

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

Description

[Randell Jesup [:jesup] (needinfo me)]

Implement support for state rollbacks in JSEP

Adam has a proposal for this in the standards bodies (link?)

Comment 1

[Byron Campen [:bwc]]

Let me see how hard this will be (doesn't seem hard).

Comment 2

[Byron Campen [:bwc]]

Attachment: MozReview Request: bz://952145/bwc

/r/5775 - Bug 952145: (WIP) Rollback support

Pull down this commit:

hg pull review -r 050b0f75cbf33450c744b2b3edfc4a99733b6641

Comment 3

[Byron Campen [:bwc]]

Need to write some tests.

Comment 4

[Byron Campen [:bwc]]

The tricky bit here is probably going to be the rollback of remote offers, since that requires doing stuff like firing onremovetrack, possibly getting ourselves tangled in bug 1122306.

Comment 5

[Byron Campen [:bwc]]

Comment on attachment 8580400 [details]
MozReview Request: bz://952145/bwc

/r/5775 - Bug 952145: (WIP) Rollback support

Pull down this commit:

hg pull review -r cfa1bd558d4c5bfe7007d287f12f9571e9a5d792

Comment 6

[Byron Campen [:bwc]]

Comment on attachment 8580400 [details]
MozReview Request: bz://952145/bwc

/r/5775 - Bug 952145: (WIP) Rollback support

Pull down this commit:

hg pull -r af00f2e011cd8ef10a17268a8c3aed45c3d66695 https://reviewboard-hg.mozilla.org/gecko/

Comment 7

[Byron Campen [:bwc]]

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

Comment 8

[Byron Campen [:bwc]]

Comment on attachment 8580400 [details]
MozReview Request: bz://952145/bwc

/r/5775 - Bug 952145: (WIP) Rollback support

Pull down this commit:

hg pull -r c60af694689ef1203d6f7c0bc84bf290ee861ed5 https://reviewboard-hg.mozilla.org/gecko/

Comment 9

[Byron Campen [:bwc]]

Comment on attachment 8580400 [details]
MozReview Request: bz://952145/bwc

/r/5775 - Bug 952145: (WIP) Rollback support

Pull down this commit:

hg pull -r 494afec0d8e9f969f45fe0604296e3ebfac07059 https://reviewboard-hg.mozilla.org/gecko/

Comment 10

[Byron Campen [:bwc]]

Comment on attachment 8580400 [details]
MozReview Request: bz://952145/bwc

/r/5775 - Bug 952145: (WIP) Rollback support

Pull down this commit:

hg pull -r 7c32b7f8d758ddcf02c15c99c7d500ab6d386839 https://reviewboard-hg.mozilla.org/gecko/

Comment 12

[Byron Campen [:bwc]]

Comment on attachment 8580400 [details]
MozReview Request: bz://952145/bwc

/r/5775 - Bug 952145: Rollback support

Pull down this commit:

hg pull -r 534a75246af61de7546812f6c8d770cf9b77e083 https://reviewboard-hg.mozilla.org/gecko/

Comment 13

[Martin Thomson [:mt:]]

Comment on attachment 8580400 [details]
MozReview Request: bz://952145/bwc

https://reviewboard.mozilla.org/r/5773/#review7687

I'm not happy with the test coverage here, but it looks generally OK.

I'd like to see tests of rollback when there is active communication (i.e., rollback a renegotiation offer).

When you rollback a remote offer, shouldn't there be onremovetrack events being fired?  A test to verify that would be good.

::: dom/media/tests/mochitest/test_peerConnection_remoteRollback.html:23
(Diff revision 7)
> +              new mozRTCSessionDescription({ type: "rollback", sdp: ""}),

What happens if you omit sdp from the dictionary?

Comment 14

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/5773/#review7689

So, as it stands we aren't firing OnAddTrack/OnAddStream until negotiation completes (known bug, will require some work to fix), so rollback isn't going to have any effects there. Once that is fixed, though...

Rolling back a renegotiation sounds like a good use of time.

> What happens if you omit sdp from the dictionary?

Not sure, is that commonly done? The w3c spec doesn't spell this out, but the JSEP draft says that you're supposed to pass an SDP with an "empty contents".

Comment 15

[Martin Thomson [:mt:]]

https://reviewboard.mozilla.org/r/5773/#review7691

> Not sure, is that commonly done? The w3c spec doesn't spell this out, but the JSEP draft says that you're supposed to pass an SDP with an "empty contents".

I only ask, because having to include { sdp: "" } seems like a waste of typing.  It would be nice if we allowed it to be absent, since we don't actually use the value.  See also the last email I sent to the ML.

Comment 16

[Byron Campen [:bwc]]

Comment on attachment 8580400 [details]
MozReview Request: bz://952145/bwc

/r/5775 - Bug 952145: Rollback support

Pull down this commit:

hg pull -r 9d873bb4637a6407d0e56ec9581150db38335653 https://reviewboard-hg.mozilla.org/gecko/

Comment 17

[Martin Thomson [:mt:]]

Comment on attachment 8580400 [details]
MozReview Request: bz://952145/bwc

https://reviewboard.mozilla.org/r/5773/#review7763

I don't know how easy it would be to add a check to see that media isn't interrupted or affected by the renegotiation.  Do the current renegotiation tests check that media is flowing after all is done?

::: dom/media/tests/mochitest/test_peerConnection_localReofferRollback.html:27
(Diff revision 8)
> +            test.pcRemote.endOfTrickleIce.then(() => {
> +              send_message({"type": "end_of_trickle_ice"});
> +            });

This doesn't enter the chain, please add a comment explaining why you need to have this message setup here (and not in PC_REMOTE_WAIT_FOR_END_OF_TRICKLE_WITH_MORE_UNDERSCORES_FOR_LUCK

::: dom/media/PeerConnection.js:700
(Diff revisions 7 - 8)
> +          if (!desc.sdp) {
> +            desc.sdp = "";
> +          }

Maybe we should add a default in the WebIDL instead.

Comment 18

[Martin Thomson [:mt:]]

Comment on attachment 8580400 [details]
MozReview Request: bz://952145/bwc

https://reviewboard.mozilla.org/r/5773/#review7765

Ship It!

Comment 19

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/5773/#review7769

::: dom/media/tests/mochitest/test_peerConnection_localReofferRollback.html:27
(Diff revision 8)
> +            test.pcRemote.endOfTrickleIce.then(() => {
> +              send_message({"type": "end_of_trickle_ice"});
> +            });

This just comes verbatim from a step in templates.js.

Comment 20

[Byron Campen [:bwc]]

Comment on attachment 8580400 [details]
MozReview Request: bz://952145/bwc

/r/5775 - Bug 952145: Rollback support

Pull down this commit:

hg pull -r 085a14216551a67b4beb82fe272d90d72c702916 https://reviewboard-hg.mozilla.org/gecko/

Comment 21

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/5773/#review7767

They check the media flow in the same way the normal tests do, although I don't think I've verified that sabotaging renegotiation causes these checks to fail. I've filed an investigation bug; https://bugzilla.mozilla.org/show_bug.cgi?id=1166832

> Maybe we should add a default in the WebIDL instead.

I'll do that.

Comment 22

[Byron Campen [:bwc]]

Comment on attachment 8580400 [details]
MozReview Request: bz://952145/bwc

/r/5775 - Bug 952145: Rollback support r=mt

Pull down this commit:

hg pull -r 915e60b8fd77a80eb57d0aeaa84424898ec812f2 https://reviewboard-hg.mozilla.org/gecko/

Comment 23

[Byron Campen [:bwc]]

Comment on attachment 8580400 [details]
MozReview Request: bz://952145/bwc

Need review for a webidl change.

Comment 24

[Olli Pettay [:smaug]]

Comment on attachment 8580400 [details]
MozReview Request: bz://952145/bwc

I was told "rollback" will be added to the spec.

Not about this bug, but why isn't RTCSessionDescriptionInit exactly the same in
Gecko and in the spec? The spec doesn't have default values.

Comment 25

[Byron Campen [:bwc]]

Never mind, this rotted too.

Comment 26

[Byron Campen [:bwc]]

Comment on attachment 8580400 [details]
MozReview Request: bz://952145/bwc

/r/5775 - Bug 952145: Rollback support r=mt, r=smaug

Pull down this commit:

hg pull -r bd1965a3d8929468e3604506c409a5d218470727 https://reviewboard-hg.mozilla.org/gecko/

Comment 27

[Byron Campen [:bwc]]

Check back on try for unrot.

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/61511c9486a9

Comment 29

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/61511c9486a9

Comment 31

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 952145: Rollback support r=mt, r=smaug