Offer created with offerToReceive false does not reflect the transceiver state

RESOLVED FIXED in Firefox 59

Status

()

defect
P2
normal
Rank:
11
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: bwc, Assigned: bwc)

Tracking

59 Branch
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59+ fixed, firefox60 fixed)

Details

Attachments

(1 attachment)

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

https://jsfiddle.net/jib1/1h8mr8fw/
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=
Attachment #8947565 - Flags: review?(jib) → review+
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 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.
Rank: 11
Priority: -- → P2
Ok, since I've changed the approach on this patch, it needs a re-review. Try looks good.
Flags: needinfo?(jib)
Comment on attachment 8947565 [details]
Bug 1435013: Sync transceivers before creating offers/answers.

https://reviewboard.mozilla.org/r/217268/#review224056
Lgtm.
Flags: needinfo?(jib)
Needinfo self to request uplift once this looks good on m-c.
Flags: needinfo?(docfaraday)
Pushed by bcampen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c20f2aab0fa3
Sync transceivers before creating offers/answers. r=jib
Tracking to make sure to follow up here and in bug 1425818, for 59 beta.
https://hg.mozilla.org/mozilla-central/rev/c20f2aab0fa3
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
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.
Flags: needinfo?(docfaraday)
Attachment #8947565 - Flags: approval-mozilla-beta?
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.
Attachment #8947565 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.