1435013 - Offer created with offerToReceive false does not reflect the transceiver state

Status: RESOLVED FIXED

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

Description

[Byron Campen [:bwc]]

We're missing a sync here. Fiddle to repro:

https://jsfiddle.net/jib1/1h8mr8fw/

Comment 1

[Byron Campen [:bwc]]

Attachment: Bug 1435013: Sync transceivers before creating offers/answers.

Review commit: https://reviewboard.mozilla.org/r/217268/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/217268/

Comment 2

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

Comment on attachment 8947565 [details]
Bug 1435013: Sync transceivers before creating offers/answers.

https://reviewboard.mozilla.org/r/217268/#review223114

Great, but how come the tests in bug 1425618 didn't catch this? Do we need to update them so they do?

::: dom/media/PeerConnection.js:842
(Diff revision 1)
>          if (transceiver.direction == "sendrecv") {
>            transceiver.setDirectionInternal("sendonly");
> +          transceiver.sync();
>          } else if (transceiver.direction == "recvonly") {
>            transceiver.setDirectionInternal("inactive");
> +          transceiver.sync();

I count 6 calls to setDirectionInternal() in this file [1] and now two of them also call .sync() but the 4 others still do not. How come? I'm worried about maintenance here.

Should maybe setDirectionInternal() itself call it? 

[1] https://searchfox.org/mozilla-central/search?q=setDirectionInternal(%22&case=false&regexp=false&path=

Comment 3

[Byron Campen [:bwc]]

Comment on attachment 8947565 [details]
Bug 1435013: Sync transceivers before creating offers/answers.

https://reviewboard.mozilla.org/r/217268/#review223114

The tests from bug 1425618 examine the transceivers only, not the SDP. The JS-side transceivers were in the correct state, but they had not synced.

> I count 6 calls to setDirectionInternal() in this file [1] and now two of them also call .sync() but the 4 others still do not. How come? I'm worried about maintenance here.
> 
> Should maybe setDirectionInternal() itself call it? 
> 
> [1] https://searchfox.org/mozilla-central/search?q=setDirectionInternal(%22&case=false&regexp=false&path=

I was wondering that myself. Everywhere else we've used this function has a sync at the end, so it has not been a problem before.

Comment 4

[Byron Campen [:bwc]]

Comment on attachment 8947565 [details]
Bug 1435013: Sync transceivers before creating offers/answers.

https://reviewboard.mozilla.org/r/217268/#review223114

> I was wondering that myself. Everywhere else we've used this function has a sync at the end, so it has not been a problem before.

I think what I'll do is call sync on the transceivers before hitting the createOffer API.

Comment 5

[Byron Campen [:bwc]]

Comment on attachment 8947565 [details]
Bug 1435013: Sync transceivers before creating offers/answers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217268/diff/1-2/

Comment 6

[Byron Campen [:bwc]]

Ok, since I've changed the approach on this patch, it needs a re-review. Try looks good.

Comment 7

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

Comment on attachment 8947565 [details]
Bug 1435013: Sync transceivers before creating offers/answers.

https://reviewboard.mozilla.org/r/217268/#review224056

Comment 8

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

Lgtm.

Comment 9

[Byron Campen [:bwc]]

Needinfo self to request uplift once this looks good on m-c.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c20f2aab0fa3
Sync transceivers before creating offers/answers. r=jib

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking to make sure to follow up here and in bug 1425818, for 59 beta.

Comment 12

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/c20f2aab0fa3

Comment 13

[Byron Campen [:bwc]]

Comment on attachment 8947565 [details]
Bug 1435013: Sync transceivers before creating offers/answers.

Approval Request Comment
[Feature/Bug causing the regression]:

   Bug 1290948.

[User impact if declined]:

   Minor web-compat problems on sites using webrtc.

[Is this code covered by automated tests?]:

   There's a blind spot in our tests here still.

[Has the fix been verified in Nightly?]:

   Yes.

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

[Why is the change risky/not risky?]:

   JS change only, of the belt-and-suspenders variety.

[String changes made/needed]:

   None.

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8947565 [details]
Bug 1435013: Sync transceivers before creating offers/answers.

Fix for regression from 59, let's uplift for 59 beta 10.

Comment 15

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3c16503565e6