1550177 - RTCPeerConnection fires "complete" but never "gathering" icegatheringstatechange on answerer side

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P2)

Description

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

Attachment: Screenshot

STRs:

1. Open https://jsfiddle.net/jib1/y6dxtks9/
2. Check the top ☐ Camera
3. Allow camera permission if necessary

Expected results:
.-iframe---
| ☑ Camera ☐ Noise ☑ Log [Restart]
| gathering
| onicecandidate: 268e41f8
| onicecandidate: 268e41f8
| onicecandidate: 268e41f8
| onicecandidate: 268e41f8
| onicecandidate: 268e41f8
| complete
| addIceCandidate: c724b6be
| addIceCandidate: c724b6be
| addIceCandidate: c724b6be
|
'---
☐ Camera ☐ Noise ☑ Log [Restart]
gathering
onicecandidate: c724b6be
onicecandidate: c724b6be
onicecandidate: c724b6be
complete
addIceCandidate: 268e41f8
addIceCandidate: 268e41f8
addIceCandidate: 268e41f8
addIceCandidate: 268e41f8
addIceCandidate: 268e41f8

Actual results:
.-iframe---
| ☑ Camera ☐ Noise ☑ Log [Restart]
| gathering
| onicecandidate: 268e41f8
| onicecandidate: 268e41f8
| onicecandidate: 268e41f8
| onicecandidate: 268e41f8
| onicecandidate: 268e41f8
| complete
| addIceCandidate: c724b6be
| addIceCandidate: c724b6be
| addIceCandidate: c724b6be
|
'---
☐ Camera ☐ Noise ☑ Log [Restart]
complete
onicecandidate: c724b6be
onicecandidate: c724b6be
onicecandidate: c724b6be
addIceCandidate: 268e41f8
addIceCandidate: 268e41f8
addIceCandidate: 268e41f8
addIceCandidate: 268e41f8
addIceCandidate: 268e41f8

Comment 1

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

FYI the order of events on the answerer side seem inconsistent. Sometimes I see this order:

addIceCandidate: 3e59ea5b
addIceCandidate: 3e59ea5b
addIceCandidate: 3e59ea5b
addIceCandidate: 3e59ea5b
addIceCandidate: 3e59ea5b
complete
onicecandidate: 8a229bd5
onicecandidate: 8a229bd5
onicecandidate: 8a229bd5

The "gathering" event is always missing however.

Comment 2

[Byron Campen [:bwc]]

Looking into this.

Comment 3

[Byron Campen [:bwc]]

Ok, it just looks like we're skipping the transition to "gathering" sometimes.

https://jsfiddle.net/L3bat8pc/1/

Comment 4

[Byron Campen [:bwc]]

This seems to be due to the extra stack unwind we're doing here:

https://searchfox.org/mozilla-central/rev/197210b8c139b64f642edaa3336d26b9c1761568/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp#2560

Comment 5

[Byron Campen [:bwc]]

Whew, there's a lot of cruft to clean up here.

Comment 6

[Byron Campen [:bwc]]

Attachment: Bug 1550177: (WIP) Remove an unneeded stack unwind, and decruft.

Comment 7

[Byron Campen [:bwc]]

Attachment: Bug 1550177 - Part 0: Test-case for bug. r?jib

Comment 8

[Byron Campen [:bwc]]

Attachment: Bug 1550177 - Part 1: Remove some duplicate webidl enums. r?mjf,smaug

Depends on D30990

Comment 9

[Byron Campen [:bwc]]

Attachment: Bug 1550177 - Part 2: Remove a stack-unwind, and simplify signal handling considerably. r?mjf

Depends on D30991

Comment 10

[Byron Campen [:bwc]]

Attachment: Bug 1550177 - Part 3: There is no need to dispatch these calls to STS, because MediaTransportHandler does that for us. r?mjf

Depends on D30992

Comment 11

[Byron Campen [:bwc]]

Attachment: Bug 1550177 - Part 4: Simplify ICE gathering state change callback stuff. r?jib

Depends on D30993

Comment 12

[Byron Campen [:bwc]]

Attachment: Bug 1550177 - Part 5: Remove one more bit of cruft. r?mjf

Depends on D30994

Comment 13

[Byron Campen [:bwc]]

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

Comment 14

[Byron Campen [:bwc]]

Try looks good.

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:bwc, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 16

[Byron Campen [:bwc]]

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

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1ff47be32265
Part 0: Test-case for bug. r=jib
https://hg.mozilla.org/integration/autoland/rev/22a2b3e65223
Part 1: Remove some duplicate webidl enums. r=mjf,smaug
https://hg.mozilla.org/integration/autoland/rev/4ce46b3be46e
Part 2: Remove a stack-unwind, and simplify signal handling considerably. r=mjf
https://hg.mozilla.org/integration/autoland/rev/01c9aca20892
Part 3: There is no need to dispatch these calls to STS, because MediaTransportHandler does that for us. r=mjf
https://hg.mozilla.org/integration/autoland/rev/2653b8243f4f
Part 4: Simplify ICE gathering state change callback stuff. r=jib
https://hg.mozilla.org/integration/autoland/rev/8f9779bb2d41
Part 5: Remove one more bit of cruft. r=mjf

Comment 18

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/1ff47be32265
https://hg.mozilla.org/mozilla-central/rev/22a2b3e65223
https://hg.mozilla.org/mozilla-central/rev/4ce46b3be46e
https://hg.mozilla.org/mozilla-central/rev/01c9aca20892
https://hg.mozilla.org/mozilla-central/rev/2653b8243f4f
https://hg.mozilla.org/mozilla-central/rev/8f9779bb2d41

Comment 19

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

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

Comment 20

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

Upstream PR merged