1629565 - null candidate fired ahead of SLD(answer) on SRD w/rollback or double SRD (regression)

Status: RESOLVED FIXED

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

Description

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

Doing SRD twice on the answerer (which is allowed), causes ICE gathering to jump the gate, firing answer candidates ahead of answer.

This is against spec.

STRs:

1. Open https://jsfiddle.net/jib1/gjcms01p/ and share camera

Expected results:

pc2.SRD DONE
pc2.SRD TWICE DONE
stable
pc2.SLD DONE
pc2.onicecandidate in stable
pc2.onicecandidate in stable

Actual results:

pc2.SRD DONE
pc2.SRD TWICE DONE
pc2.onicecandidate in have-remote-offer BOOOH!
pc1.addIce: InvalidStateError: Cannot add ICE candidate when there is no remote SDP
stable
pc2.SLD DONE
pc2.onicecandidate in stable

Regression range:

45:19.27 INFO: Narrowed inbound regression window from [442f57db, f829ca2e] (4 builds) to [fde54097, f829ca2e] (2 builds) (~1 steps left)
45:19.27 INFO: No more inbound revisions, bisection finished.
45:19.27 INFO: Last good revision: fde540977b4e52dd21031777aa35906ff81d6e63
45:19.27 INFO: First bad revision: f829ca2e1ad6d637eb01f5cbe50876f22fe53c37
45:19.27 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=fde540977b4e52dd21031777aa35906ff81d6e63&tochange=f829ca2e1ad6d637eb01f5cbe50876f22fe53c37

Comment 1

[Byron Campen [:bwc]]

Yeah, this is just another race opened up by bug 1591199.

Comment 2

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

Seems like a logic problem, not a race. Note the

console.log("pc2.SRD DONE");
await wait(1000);

in the fiddle, which is not needed to reproduce btw, but shows it isn't timing related. I.e. we can wait any number of seconds after the first SRD and NO candidates fire prematurely.

It specifically takes a second SRD to trigger ICE, at which point it's 100% reproducible (no SLD needed). No timing involved. The other end will have no answer to apply these candidates to, likely causing the call to fail.

I think investigating why this happens should be a P2 in case there are other ways to trigger this logic error.

In fact, it appears to be happening on SRD implicit rollback: https://jsfiddle.net/jib1/gjcms01p/28/ which may miss an important candidate, causing calls to fail to connect.

Comment 3

[Byron Campen [:bwc]]

This test case widens the window of opportunity for the race, even without any waiting. The problem is that the handling for state changes for the sLD(answer) has been delayed by bug 1591199 enough that the candidates can arrive before those state changes are carried out, provided the ICE stack is given enough head start.

Comment 4

[Jens Stutte [:jstutte]]

Hi Nico, is there currently someone working on this? Thank you?

Comment 5

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

(In reply to Byron Campen [:bwc] from comment #3)

This test case widens the window of opportunity for the race, even without any waiting. The problem is that the handling for state changes for the sLD(answer) has been delayed by bug 1591199 enough that the candidates can arrive before those state changes are carried out, provided the ICE stack is given enough head start.

I don't understand this explanation, because this reproduces even without calling sLD(answer) https://jsfiddle.net/jib1/gjcms01p/39/

I also verified this is not fixed by the patches in bug 1622384.

Comment 6

[Byron Campen [:bwc]]

Ok, that's weird. I'll look into it.

Comment 7

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

Ah, it's the null candidate fired from handleIceGatheringStateChange - https://jsfiddle.net/jib1/gjcms01p/45/

Comment 8

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

iceGatheringState goes to "complete" for some reason on second SRD(offer) - https://jsfiddle.net/jib1/gjcms01p/48/

Comment 9

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

I've verified it's the null candidate that's being fired on single SRD() with implicit rollback as well. https://jsfiddle.net/jib1/gjcms01p/50/

Comment 10

[Byron Campen [:bwc]]

Ok, this is due to the implicit rollback itself; rolling back a sLD shuts down gathering, which means we emit an end-of-candidates. Looking at the spec now to determine what it says should happen on rollback.

Comment 11

[Byron Campen [:bwc]]

Yeah, webrtc-pc says that whenever gathering transitions to complete for all ice transports, we fire this null candidate. I wonder if this should apply when we're rolling back to a state where there are no ICE transports at all? The ICE gathering state should be "new" in that case, probably?

Comment 12

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

Ah, unlike comment 8 which said pc2.onicecandidate in have-remote-offer null BOOOH!,
comment 9 actually says pc2.onicecandidate null in have-local-offer BOOOH! which is quite normal. I'll remove the BOOOH! there.

But one problem remains with the comment 9 fiddle updated with onicegatheringstatechange: It looks like we're firing "complete" without ever having fired "gathering", which the spec indicates we should have done "When the ICE Agent indicates that it began gathering" which sans a state diagram clearly has to have happened before "When the ICE Agent is finished gathering". That seems wrong (in Firefox).

Yeah, webrtc-pc says that whenever gathering transitions to complete for all ice transports, we fire this null candidate. I wonder if this should apply when we're rolling back to a state where there are no ICE transports at all? The ICE gathering state should be "new" in that case, probably?

I think it's a stretch to say ice gathering is "complete" when it got aborted by its transport being rolled back and vaporized.

After all, the spec says: "When the ICE Agent is finished gathering a generation of candidates for an RTCIceTransport, and those candidates have been surfaced to the application". I'd argue it wasn't allowed to finish in this case.

ICE seems supportive of this interpretation: "ICE processing across all media streams also has a state associated with it. This state is equal to Running while ICE processing is under way. The state is Completed when ICE processing is complete and Failed if it failed without success."

But regardless of interpretation, I see no support anywhere for the behavior in comment 8 (without any SLD on pc2) and earlier, because the phrase "and those candidates have been surfaced to the application" above is a reference to [[EarlyCandidates]], which means we must not ever fire "complete" before SLD(answer).

Comment 13

[Byron Campen [:bwc]]

Yeah, I think in this case we should be going back to "new". As for going directly from "new" to "complete", there are corner-cases like failure to start gathering that look under-specified (although that isn't what is happening here).

As for comment 8, I think maybe we're triggering the ICE agent rollback stuff with sRD somehow? Looking into it.

Comment 14

[Byron Campen [:bwc]]

Ok, flaw is here for comment 8:

https://searchfox.org/mozilla-central/rev/d2cec90777d573585f8477d5170892e5dcdfb0ab/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp#2170-2171

Somehow, this is true in the double sRD(offer) case.

Comment 15

[Byron Campen [:bwc]]

Attachment: Bug 1629565: Test-case for bug.

Comment 16

[Byron Campen [:bwc]]

Attachment: Bug 1629565: Fix bug where repeated sRD(offer) could start gathering.

Depends on D72234

Comment 17

[Byron Campen [:bwc]]

Attachment: Bug 1629565: Test that we do not transition to "gathering" on a simple reoffer.

Depends on D72235

Comment 18

[Byron Campen [:bwc]]

Attachment: Bug 1629565: Do not transition back to "gathering" if no gathering is needed.

Depends on D72236

Comment 19

[Byron Campen [:bwc]]

Attachment: Bug 1629565: Test that we do not transition to "gathering" on rollback if we were already done gathering.

Depends on D72238

Comment 20

[Byron Campen [:bwc]]

Attachment: Bug 1629565: Fix bug where rollback of local offer would stomp the pre-existing transport with a new one with the same ufrag/pwd.

Depends on D72239

Comment 21

[Byron Campen [:bwc]]

Attachment: Bug 1629565: Test-cases for transitioning to "new" gathering state.

Depends on D72240

Comment 22

[Byron Campen [:bwc]]

Attachment: Bug 1629565: Transition gathering state to "new" if a negotiation leaves us with no transports.

Depends on D72241

Comment 23

[Byron Campen [:bwc]]

Attachment: Bug 1629565: Some logging that was helpful.

Depends on D72242

Comment 24

[Byron Campen [:bwc]]

Attachment: Bug 1629565: Mochitest fixes.

Depends on D72243

Comment 25

[Byron Campen [:bwc]]

Attachment: Bug 1629565: Another test we should probably have. Passes.

Depends on D72244

Comment 26

[Byron Campen [:bwc]]

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

Gtests here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c6d537e955d7ccc7a7f696956e92b30229d78b4

Comment 27

[Byron Campen [:bwc]]

Try looks good.

Comment 28

[Byron Campen [:bwc]]

More thorough try push here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0311cb13bc110167d3b76f33926734a3038d7b8f

Comment 29

[Byron Campen [:bwc]]

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

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e2f411ffa079
Test-case for bug. r=jib
https://hg.mozilla.org/integration/autoland/rev/5e5e7b27d296
Fix bug where repeated sRD(offer) could start gathering. r=mjf
https://hg.mozilla.org/integration/autoland/rev/673ef75f8fe6
Test that we do not transition to "gathering" on a simple reoffer. r=jib
https://hg.mozilla.org/integration/autoland/rev/bee9068e5a39
Do not transition back to "gathering" if no gathering is needed. r=mjf
https://hg.mozilla.org/integration/autoland/rev/41ac898a5d97
Test that we do not transition to "gathering" on rollback if we were already done gathering. r=jib
https://hg.mozilla.org/integration/autoland/rev/3a5601c68d76
Fix bug where rollback of local offer would stomp the pre-existing transport with a new one with the same ufrag/pwd. r=mjf
https://hg.mozilla.org/integration/autoland/rev/40c9fc4fa292
Test-cases for transitioning to "new" gathering state. r=jib
https://hg.mozilla.org/integration/autoland/rev/065ddaaefe60
Transition gathering state to "new" if a negotiation leaves us with no transports. r=mjf
https://hg.mozilla.org/integration/autoland/rev/09af49103607
Some logging that was helpful. r=mjf
https://hg.mozilla.org/integration/autoland/rev/53ba1301dfdd
Mochitest fixes. r=jib
https://hg.mozilla.org/integration/autoland/rev/7917d7e0a471
Another test we should probably have. Passes. r=jib

Comment 31

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/23274 for changes under testing/web-platform/tests

Comment 32

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 33

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/e2f411ffa079
https://hg.mozilla.org/mozilla-central/rev/5e5e7b27d296
https://hg.mozilla.org/mozilla-central/rev/673ef75f8fe6
https://hg.mozilla.org/mozilla-central/rev/bee9068e5a39
https://hg.mozilla.org/mozilla-central/rev/41ac898a5d97
https://hg.mozilla.org/mozilla-central/rev/3a5601c68d76
https://hg.mozilla.org/mozilla-central/rev/40c9fc4fa292
https://hg.mozilla.org/mozilla-central/rev/065ddaaefe60
https://hg.mozilla.org/mozilla-central/rev/09af49103607
https://hg.mozilla.org/mozilla-central/rev/53ba1301dfdd
https://hg.mozilla.org/mozilla-central/rev/7917d7e0a471

Comment 34

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot