Closed Bug 1290049 Opened 8 years ago Closed 8 years ago

Intermittent test_peerConnection_restartIceLocalAndRemoteRollback.html | iceconnectionstate event 'connected' matches expected state 'checking' - got "connected", expected "checking"

Categories

(Core :: WebRTC: Networking, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox49 --- unaffected
firefox50 --- fixed
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: mjf)

Details

(Keywords: intermittent-failure)

Attachments

(1 file)

Rank: 35
Priority: -- → P3
Looks to me like our tests are to strict in expecting connected->checking->connected and don't allow connected->connected.

Michael, Byron: what do you guys think about making the tests less strict on this one?
Component: WebRTC → WebRTC: Networking
Flags: needinfo?(mfroman)
Flags: needinfo?(docfaraday)
Nils: Are you thinking that we're missing the transition through checking?
Flags: needinfo?(mfroman)
I am not sure how we are ending up missing a transition to checking, it really should happen every time.
Flags: needinfo?(docfaraday)
I captured this in rr on Friday in the midst of looking at another bug.
Assignee: nobody → mfroman
Attachment #8800334 - Flags: review?(docfaraday)
Comment on attachment 8800334 [details]
Bug 1290049 - only process ice connection state change if new state differs from old state.

https://reviewboard.mozilla.org/r/85254/#review83856

Hmm. So I think I see the thing you're trying to prevent here (we do an ICE restart, and then rollback before the new ctx makes it to checking, which causes us to fire a duplicate state change for whatever state the original ctx was in). And I think this change will prevent that. But I think it will also eat state changes; suppose that the original ctx changes state before we rollback.

We may need to put the filtering closer to JS-land.
Attachment #8800334 - Flags: review?(docfaraday) → review-
Comment on attachment 8800334 [details]
Bug 1290049 - only process ice connection state change if new state differs from old state.

https://reviewboard.mozilla.org/r/85254/#review84582
Attachment #8800334 - Flags: review?(docfaraday) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/65e0762afc23
only process ice connection state change if new state differs from old state. r=bwc
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/65e0762afc23
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Please request Aurora/Beta approval on this when you get a chance.
Flags: needinfo?(mfroman)
Comment on attachment 8800334 [details]
Bug 1290049 - only process ice connection state change if new state differs from old state.

Approval Request Comment
[Feature/regressing bug #]: Bug 906986 - Support ICE restart
[User impact if declined]: When rolling back an ICE restart, if the next ICE context has not had time to progress from the init state, after rollback a second connection status with state 'connected' is sent.  This second 'connected' state causes orange in the tree.
[Describe test coverage new/current, TreeHerder]: (current) test_peerConnection_restartIceLocalAndRemoteRollback.html
[Risks and why]: Very little risk here.  Before ICE restart was implemented we were already filtering connection state updates when there was no change, and this patch ensures that rolling back an ICE restart matches that behavior.
[String/UUID change made/needed]: none
Flags: needinfo?(mfroman)
Attachment #8800334 - Flags: approval-mozilla-beta?
Attachment #8800334 - Flags: approval-mozilla-aurora?
Comment on attachment 8800334 [details]
Bug 1290049 - only process ice connection state change if new state differs from old state.

Fixes an intermittent, patch seems low risk, Aurora51+, Beta50+
Attachment #8800334 - Flags: approval-mozilla-beta?
Attachment #8800334 - Flags: approval-mozilla-beta+
Attachment #8800334 - Flags: approval-mozilla-aurora?
Attachment #8800334 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.