935723 - PeerConnectionImpl misinterprets the IceGatheringCompleted callback in trickle ICE cases

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Byron Campen [:bwc]]

When PeerConnectionImpl::IceGatheringCompleted is called, it is treated as a signal that ICE checks are are now starting, causing mIceState to be set to Waiting. In the trickle ICE case, this is incorrect.

Since other code (notably unit-tests) are using the same signal that causes this callback to be hit, we cannot simply rename it to match what PeerConnectionImpl wants (unless, of course, this is what the unit tests also want). We either need to add a new signal to be called when ICE checks are starting (eg; IceChecksStarting), or a new signal like IceStateChanged (and have PeerConnectionImpl just use this).

Comment 1

[Byron Campen [:bwc]]

It looks like the assumption that gathering completes before checking begins is baked in all the way from NrIceCtx to PeerConnection.js. This will be a largeish patch.

Comment 2

[Byron Campen [:bwc]]

Attachment: (WIP) Decouple ICE state with ICE gathering state. Builds, but that's about all I can say at this point.

First cut. Builds. Totally untested. More to come tomorrow.

Comment 3

[Byron Campen [:bwc]]

Hey, it might work. It could happen.

https://tbpl.mozilla.org/?tree=Try&rev=fa7ed0d65b0a

Comment 4

[Byron Campen [:bwc]]

Attachment: (WIP) Decouple ICE state with ICE gathering state. Seems to be passing tests.

Get signalling_unittest to work. Seems to be passing tests now.

Comment 5

[Eric Rescorla (:ekr)]

Comment on attachment 828768 [details] [diff] [review]
(WIP) Decouple ICE state with ICE gathering state. Seems to be passing tests.

Review of attachment 828768 [details] [diff] [review]:
-----------------------------------------------------------------

I just reviewed the C++ code, which looks basically right.

I did not review the JS or the unit tests.

Please make the requested changes and request r?ekr for the C++ and
r?abr for the JS.

::: media/mtransport/nricectx.h
@@ +167,3 @@
>                 ICE_CTX_CHECKING,
>                 ICE_CTX_OPEN,
>                 ICE_CTX_FAILED

Let's rename state here to "check state" or something consistently.

So throughout the NrIceCtx code we will have "check state" and "gather state" OK?

And lets lose "GatheringState".

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +1554,5 @@
>  
> +static mozilla::dom::PCImplIceState
> +toDomIceState(NrIceCtx::State state) {
> +  switch(state) {
> +    case NrIceCtx::ICE_CTX_INIT:

Should we try to harmonize the names up and down the stack?

@@ +1564,5 @@
> +      return PCImplIceState::IceConnected;
> +    case NrIceCtx::ICE_CTX_FAILED:
> +      return PCImplIceState::IceFailed;
> +  }
> +  MOZ_CRASH();

Good idea.

@@ +1578,5 @@
> +      return PCImplIceGatheringState::Gathering;
> +    case NrIceCtx::ICE_CTX_GATHER_COMPLETE:
> +      return PCImplIceGatheringState::Complete;
> +  }
> +  MOZ_CRASH();

Good idea.

@@ +1667,5 @@
> +    case PCImplIceGatheringState::Gathering:
> +      STAMP_TIMECARD(mTimeCard, "Ice gathering state: gathering");
> +      break;
> +    case PCImplIceGatheringState::Complete:
> +      STAMP_TIMECARD(mTimeCard, "Ice state: checking");

This looks wrong.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
@@ +379,5 @@
>    mozilla::dom::PCImplIceState IceState()
>    {
>      mozilla::dom::PCImplIceState state;
>      IceState(&state);
>      return state;

Shouldn't this return NS_OK?

@@ +387,5 @@
> +  mozilla::dom::PCImplIceGatheringState IceGatheringState()
> +  {
> +    mozilla::dom::PCImplIceGatheringState state;
> +    IceGatheringState(&state);
> +    return state;

Shouldn't this return NS_OK.

Comment 6

[Byron Campen [:bwc]]

(In reply to Eric Rescorla (:ekr) from comment #5)
> Comment on attachment 828768 [details] [diff] [review]
> (WIP) Decouple ICE state with ICE gathering state. Seems to be passing tests.
> 
> Review of attachment 828768 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: media/mtransport/nricectx.h
> @@ +167,3 @@
> >                 ICE_CTX_CHECKING,
> >                 ICE_CTX_OPEN,
> >                 ICE_CTX_FAILED
> 
> Let's rename state here to "check state" or something consistently.
> 
> So throughout the NrIceCtx code we will have "check state" and "gather
> state" OK?
>

The terminology used on the w3c side seems to be "ice connection state". (http://dev.w3.org/2011/webrtc/editor/webrtc.html#rtciceconnectionstate-enum) Probably makes sense to make this consistent.
 
> And lets lose "GatheringState".

This is the spelling that w3c is using; I think it makes sense to be consistent. http://dev.w3.org/2011/webrtc/editor/webrtc.html#rtcicegatheringstate-enum
 
> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
> @@ +1554,5 @@
> >  
> > +static mozilla::dom::PCImplIceState
> > +toDomIceState(NrIceCtx::State state) {
> > +  switch(state) {
> > +    case NrIceCtx::ICE_CTX_INIT:
> 
> Should we try to harmonize the names up and down the stack?
> 

I really hate blanket search/replace of names, but it might make sense to align this to the w3c spec.

> @@ +1667,5 @@
> > +    case PCImplIceGatheringState::Gathering:
> > +      STAMP_TIMECARD(mTimeCard, "Ice gathering state: gathering");
> > +      break;
> > +    case PCImplIceGatheringState::Complete:
> > +      STAMP_TIMECARD(mTimeCard, "Ice state: checking");
> 
> This looks wrong.
>

Yep. Is wrong, will fix.
 
> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
> @@ +379,5 @@
> >    mozilla::dom::PCImplIceState IceState()
> >    {
> >      mozilla::dom::PCImplIceState state;
> >      IceState(&state);
> >      return state;
> 
> Shouldn't this return NS_OK?
> 
> @@ +387,5 @@
> > +  mozilla::dom::PCImplIceGatheringState IceGatheringState()
> > +  {
> > +    mozilla::dom::PCImplIceGatheringState state;
> > +    IceGatheringState(&state);
> > +    return state;
> 
> Shouldn't this return NS_OK.

Pretty sure these are working as intended.

Comment 7

[Byron Campen [:bwc]]

Attachment: (WIP) Decouple ICE state with ICE gathering state. Seems to be passing tests.

Incorporate some feedback. May need some more work.

Comment 8

[Byron Campen [:bwc]]

Attachment: Part 1. Decouple ICE state with ICE gathering state. Seems to be passing tests.

Update patch description.

Comment 9

[Byron Campen [:bwc]]

Attachment: Part 2 (optional). This is what is required to move from IceState -> IceConnectionState for consistency with the w3c spec.

If we want to use IceConnectionState in place of IceState for consistency, this is a patch that does it.

Comment 10

[Byron Campen [:bwc]]

Comment on attachment 831061 [details] [diff] [review]
Part 1. Decouple ICE state with ICE gathering state. Seems to be passing tests.

Review of attachment 831061 [details] [diff] [review]:
-----------------------------------------------------------------

c++ for ekr, js and webidl for abr.

Comment 11

[Byron Campen [:bwc]]

Comment on attachment 831064 [details] [diff] [review]
Part 2 (optional). This is what is required to move from IceState -> IceConnectionState for consistency with the w3c spec.

Review of attachment 831064 [details] [diff] [review]:
-----------------------------------------------------------------

This is what it takes to harmonize the NrIceCtx::State -> NrIceCtx::ConnectionState change all the way to JS. Not too terrible, but still touches a good handful of files.

Comment 12

[Byron Campen [:bwc]]

https://tbpl.mozilla.org/?tree=Try&rev=d841fb2c58e8

Comment 13

[Eric Rescorla (:ekr)]

Comment on attachment 831061 [details] [diff] [review]
Part 1. Decouple ICE state with ICE gathering state. Seems to be passing tests.

Review of attachment 831061 [details] [diff] [review]:
-----------------------------------------------------------------

This seems basically sound but the changes from changing the signal signature are pervasive enough
I want to see that fixed and then rereview.

::: media/mtransport/nricectx.h
@@ +201,5 @@
> +
> +  // Current state
> +  GatheringState gatheringState() const {
> +    return gathering_state_;
> +  }

convention here is underscore, not camelcase.

connection_state()
gathering_state()

Also, please make either both of these one-liners or neither.

@@ +240,5 @@
>  
>    // Signals to indicate events. API users can (and should)
>    // register for these.
> +  sigslot::signal1<NrIceCtx *> SignalGatheringStateChange;
> +  sigslot::signal1<NrIceCtx *> SignalConnectionStateChange;

Please make these take the state as arguments, so you don't need to call the state getter.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
@@ +386,5 @@
> +  NS_IMETHODIMP IceGatheringState(mozilla::dom::PCImplIceGatheringState* aState);
> +  mozilla::dom::PCImplIceGatheringState IceGatheringState()
> +  {
> +    mozilla::dom::PCImplIceGatheringState state;
> +    IceGatheringState(&state);

Should we be error checking this call and the call above?

Comment 14

[Eric Rescorla (:ekr)]

Please fold patch 2 into patch 1 and ask for rereview of both.

Comment 15

[Byron Campen [:bwc]]

(In reply to Eric Rescorla (:ekr) from comment #13)
> Comment on attachment 831061 [details] [diff] [review]
> Part 1. Decouple ICE state with ICE gathering state. Seems to be passing
> tests.
> 
> Review of attachment 831061 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
> @@ +386,5 @@
> > +  NS_IMETHODIMP IceGatheringState(mozilla::dom::PCImplIceGatheringState* aState);
> > +  mozilla::dom::PCImplIceGatheringState IceGatheringState()
> > +  {
> > +    mozilla::dom::PCImplIceGatheringState state;
> > +    IceGatheringState(&state);
> 
> Should we be error checking this call and the call above?

Neither of these can return anything other than NS_OK presently; if they encounter some sort of problem, they just assert. This is probably fine.

Comment 16

[Byron Campen [:bwc]]

Attachment: Part 1. Decouple ICE state with ICE gathering state. Seems to be passing tests.* * *

Incorporate feedback, and fold naming_consistency.patch

Comment 17

[Byron Campen [:bwc]]

Attachment: Part 1. Decouple ICE state with ICE gathering state. Seems to be passing tests.

Trim extraneous commit message from fold.

Comment 18

[Eric Rescorla (:ekr)]

Comment on attachment 831838 [details] [diff] [review]
Part 1. Decouple ICE state with ICE gathering state. Seems to be passing tests.

Review of attachment 831838 [details] [diff] [review]:
-----------------------------------------------------------------

Adam please review the JS

Comment 19

[Adam Roach [:abr]]

Comment on attachment 831838 [details] [diff] [review]
Part 1. Decouple ICE state with ICE gathering state. Seems to be passing tests.

Review of attachment 831838 [details] [diff] [review]:
-----------------------------------------------------------------

The javascript and webidl parts look largely correct to me. I have some pretty substantial nits, though.

I'm setting to r- for the CLOBBER issue, simply because it needs to be fixed or the tree will burn.

I didn't review the C++ part of this patch, although I referred to it for context a few times.

For my money, I'd say the second patch is highly advisable. It's a lot easier to figure out what's going on here if the terms match those in the spec. Since we're updating from the old terminology to the new with this fix, let's untangle the whole mess.

PeerConnectionImpl.h has a *ton* of ws-only changes that will cause unnecessary collisions. If you really have a burning desire to make formatting-only changes, please extract them into a separate patch.

Some formatting nits:
PeerConnectionImpl.cpp:708: Line is 100 characters long; please wrap to 80
PeerConnectionImpl.cpp:709: Line is 102 characters long; please wrap to 80
PeerConnectionImpl.cpp:1587: Line is 84 characters long; please wrap to 80
PeerConnectionImpl.h:387: Line is 83 characters long; please wrap to 80
PeerConnectionImpl.h:396: Line is 81 characters long; please wrap to 80
PeerConnectionImpl.h:504: Line is 85 characters long; please wrap to 80
PeerConnectionMedia.h:337: Line is 84 characters long; please wrap to 80
PeerConnectionMedia.h:338: Line is 86 characters long; please wrap to 80
signaling_unittests.cpp:717: Line is 87 characters long; please wrap to 80
signaling_unittests.cpp:723: Line is 82 characters long; please wrap to 80
signaling_unittests.cpp:724: Line is 87 characters long; please wrap to 80

::: dom/media/PeerConnection.js
@@ +981,5 @@
>    //
>    //   closed        The ICE Agent has shut down and is no longer responding to
>    //                 STUN requests.
>  
>    handleIceStateChanges: function(iceState) {

handleIceStateChange (non-plural), please, to match the other methods (we used to change two states; now we change one).

@@ +1007,5 @@
> +        this._dompc.changeIceConnectionState(iceState);
> +        if (VALID_STATES[iceState].success !== undefined) {
> +          histogram.add(VALID_STATES[iceState].success);
> +        }
> +    }

The table-driven approach made a lot of sense when we had one state variable that we had to tease apart into two. Now that we have a one-to-one mapping, it's just a bit baroque.

I would strongly suggest reworking this along the lines of:


> if (iceState === 'failed') {
>   histogram.add(false);
> }
> if (this._dompc.iceConnectionState === 'checking' &&
>     (iceState === 'completed' || iceState === 'connected')) {
>   histogram.add(true);
> }
> this._dompc.changeIceConnectionState(iceState);

@@ +1031,5 @@
> +    // once we've left.
> +    const VALID_STATES = {
> +      gathering: true,
> +      complete: true
> +    };

As above, this seems like overkill. I don't think we need a triple-check on the state value. It's just one more annoying little thing to touch if the list of states ever changes again.

::: dom/webidl/PeerConnectionImplEnums.webidl
@@ +42,3 @@
>  };
> +
> +// Deliberately identical to the values specified in webrtc

Since Bug 928195 is still open, I believe that adding these enums is going to require you to update the CLOBBER file.

::: dom/webidl/PeerConnectionObserverEnums.webidl
@@ +9,5 @@
>  enum PCObserverStateType {
>      "None",
>      "ReadyState",
>      "IceState",
> +    "IceGatheringState",

Same comment as above: edit CLOBBER.

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +376,5 @@
>    PCImplSignalingState gotsignaling;
>  
>    std::cout << name << ": ";
>  
>    switch (state_type)

This switch statement doesn't appear to handle the new PCObserverStateType::IceGatheringState.

It is probably also worth adding a "default: assert(0)" case to make sure we don't run into this issue in the future.

Comment 20

[Adam Roach [:abr]]

Comment on attachment 831838 [details] [diff] [review]
Part 1. Decouple ICE state with ICE gathering state. Seems to be passing tests.

Review of attachment 831838 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/PeerConnection.js
@@ +981,5 @@
>    //
>    //   closed        The ICE Agent has shut down and is no longer responding to
>    //                 STUN requests.
>  
>    handleIceStateChanges: function(iceState) {

Sorry, that should be handleIceConectionStateChange. Also, please s/iceState/connectionState/

Comment 21

[Byron Campen [:bwc]]

Attachment: Part 1. Decouple ICE state with ICE gathering state. Seems to be passing tests.

Incorporate feedback from abr.

Comment 22

[Byron Campen [:bwc]]

Attachment: Part 1. Decouple ICE state with ICE gathering state.

Update description.

Comment 23

[Byron Campen [:bwc]]

Attachment: Part 2: Adding some vertical ws for readability.

Breaking these changes into their own patch.

Comment 24

[Byron Campen [:bwc]]

Comment on attachment 831064 [details] [diff] [review]
Part 2 (optional). This is what is required to move from IceState -> IceConnectionState for consistency with the w3c spec.

Has been folded into part 1.

Comment 25

[Byron Campen [:bwc]]

Comment on attachment 832488 [details] [diff] [review]
Part 2: Adding some vertical ws for readability.

Review of attachment 832488 [details] [diff] [review]:
-----------------------------------------------------------------

r+, trivial

Comment 26

[Byron Campen [:bwc]]

Comment on attachment 832483 [details] [diff] [review]
Part 1. Decouple ICE state with ICE gathering state.

Review of attachment 832483 [details] [diff] [review]:
-----------------------------------------------------------------

Re-requesting review.

Comment 27

[Adam Roach [:abr]]

Comment on attachment 832483 [details] [diff] [review]
Part 1. Decouple ICE state with ICE gathering state.

Review of attachment 832483 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks -- this version looks good to me.

Comment 28

[Byron Campen [:bwc]]

Attachment: Part 1. Decouple ICE state with ICE gathering state. Seems to be passing tests. (old patch unbitrotted for interdiff)

Unbitrot this old patch, so we can get an interdiff.

Comment 29

[Byron Campen [:bwc]]

Attachment: Part 1. Decouple ICE state with ICE gathering state.

Incorporate some feedback from ekr.

Comment 30

[Byron Campen [:bwc]]

Comment on attachment 8334812 [details] [diff] [review]
Part 1. Decouple ICE state with ICE gathering state.

Review of attachment 8334812 [details] [diff] [review]:
-----------------------------------------------------------------

Carry forward feedback from abr.

Comment 31

[Eric Rescorla (:ekr)]

Comment on attachment 8334812 [details] [diff] [review]
Part 1. Decouple ICE state with ICE gathering state.

Review of attachment 8334812 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm with nits.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +1404,5 @@
>  PeerConnectionImpl::CheckApiState(bool assert_ice_ready) const
>  {
>    PC_AUTO_ENTER_API_CALL_NO_CHECK();
>    MOZ_ASSERT(mTrickle || !assert_ice_ready ||
> +             (mIceConnectionState != PCImplIceConnectionState::New));

This should be checking for the gathering state != gathering.

@@ +1606,5 @@
> +static mozilla::dom::PCImplIceConnectionState
> +toDomIceConnectionState(NrIceCtx::ConnectionState state) {
> +  switch(state) {
> +    case NrIceCtx::ICE_CTX_INIT:
> +      // TODO([e-mail]): Is this waiting, or new?

Waiting no longer seems to exist.

@@ +1622,5 @@
> +static mozilla::dom::PCImplIceGatheringState
> +toDomIceGatheringState(NrIceCtx::GatheringState state) {
> +  switch (state) {
> +    case NrIceCtx::ICE_CTX_GATHER_INIT:
> +      // TODO([e-mail]): Is this waiting, or new?

Waiting no longer seems to exist.

Comment 32

[Byron Campen [:bwc]]

Attachment: Part 1. Decouple ICE state with ICE gathering state. Seems to be passing tests.

Unbitrot.

Comment 33

[Byron Campen [:bwc]]

(In reply to Eric Rescorla (:ekr) from comment #31)
> Comment on attachment 8334812 [details] [diff] [review]
> Part 1. Decouple ICE state with ICE gathering state.
> 
> Review of attachment 8334812 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> lgtm with nits.
> 
> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
> @@ +1404,5 @@
> >  PeerConnectionImpl::CheckApiState(bool assert_ice_ready) const
> >  {
> >    PC_AUTO_ENTER_API_CALL_NO_CHECK();
> >    MOZ_ASSERT(mTrickle || !assert_ice_ready ||
> > +             (mIceConnectionState != PCImplIceConnectionState::New));
> 
> This should be checking for the gathering state != gathering.

We should be checking gathering state == complete, right?

Comment 34

[Eric Rescorla (:ekr)]

Sure. This is a(In reply to Byron Campen [:bwc] from comment #33)
> (In reply to Eric Rescorla (:ekr) from comment #31)
> > Comment on attachment 8334812 [details] [diff] [review]
> > Part 1. Decouple ICE state with ICE gathering state.
> > 
> > Review of attachment 8334812 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > lgtm with nits.
> > 
> > ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
> > @@ +1404,5 @@
> > >  PeerConnectionImpl::CheckApiState(bool assert_ice_ready) const
> > >  {
> > >    PC_AUTO_ENTER_API_CALL_NO_CHECK();
> > >    MOZ_ASSERT(mTrickle || !assert_ice_ready ||
> > > +             (mIceConnectionState != PCImplIceConnectionState::New));
> > 
> > This should be checking for the gathering state != gathering.
> 
> We should be checking gathering state == complete, right?

Sure. This is going to be a check we never use b/c trickle defaults on, but yeah.

Comment 35

[Byron Campen [:bwc]]

Attachment: Part 1. Decouple ICE state with ICE gathering state. Seems to be passing tests.

Fix nits

Comment 36

[Byron Campen [:bwc]]

Comment on attachment 8338556 [details] [diff] [review]
Part 1. Decouple ICE state with ICE gathering state. Seems to be passing tests.

Review of attachment 8338556 [details] [diff] [review]:
-----------------------------------------------------------------

Carry forward r+ from ekr and abr, requesting checkin.

Comment 37

[Byron Campen [:bwc]]

Comment on attachment 8338556 [details] [diff] [review]
Part 1. Decouple ICE state with ICE gathering state. Seems to be passing tests.

Review of attachment 8338556 [details] [diff] [review]:
-----------------------------------------------------------------

Carry forward r+ from ekr and abr, requesting checkin.

Comment 38

[Byron Campen [:bwc]]

Attachment: Part 1. Decouple ICE state with ICE gathering state.

Pre-unbitrot against 906990 part 10.

Comment 39

[Byron Campen [:bwc]]

Attachment: Part 2: Adding some vertical ws for readability.

Pre-unbitrot against 906990 part 10.

Comment 40

[Byron Campen [:bwc]]

Comment on attachment 8338681 [details] [diff] [review]
Part 1. Decouple ICE state with ICE gathering state.

Review of attachment 8338681 [details] [diff] [review]:
-----------------------------------------------------------------

Carry forward r+ from ekr and abr, request checkin. Requires 906990 part 10 to be applied first.

Comment 41

[Byron Campen [:bwc]]

Comment on attachment 8338682 [details] [diff] [review]
Part 2: Adding some vertical ws for readability.

Review of attachment 8338682 [details] [diff] [review]:
-----------------------------------------------------------------

Carry forward r+, request checkin.

Comment 42

[Randell Jesup [:jesup] (needinfo me)]

parts 1 & 2 (folded): https://hg.mozilla.org/integration/mozilla-inbound/rev/df21e1d3eaa2

Comment 43

[Carsten Book [:Tomcat]]

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