Closed Bug 935723 Opened 7 years ago Closed 6 years ago

PeerConnectionImpl misinterprets the IceGatheringCompleted callback in trickle ICE cases

Categories

(Core :: WebRTC: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: bwc, Assigned: bwc)

References

Details

Attachments

(2 files, 14 obsolete files)

43.65 KB, patch
bwc
: review+
jesup
: checkin+
Details | Diff | Splinter Review
3.59 KB, patch
bwc
: review+
jesup
: checkin+
Details | Diff | Splinter Review
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).
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.
First cut. Builds. Totally untested. More to come tomorrow.
Hey, it might work. It could happen.

https://tbpl.mozilla.org/?tree=Try&rev=fa7ed0d65b0a
Get signalling_unittest to work. Seems to be passing tests now.
Attachment #828377 - Attachment is obsolete: true
Attachment #828768 - Flags: feedback?(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.
Attachment #828768 - Flags: feedback?(ekr) → feedback+
(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.
Incorporate some feedback. May need some more work.
Attachment #828768 - Attachment is obsolete: true
Update patch description.
Attachment #830984 - Attachment is obsolete: true
If we want to use IceConnectionState in place of IceState for consistency, this is a patch that does it.
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.
Attachment #831061 - Flags: review?(ekr)
Attachment #831061 - Flags: review?(adam)
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.
Attachment #831064 - Flags: feedback?(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?
Attachment #831061 - Flags: review?(ekr) → review-
Please fold patch 2 into patch 1 and ask for rereview of both.
Attachment #831064 - Flags: feedback?(ekr)
(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.
Incorporate feedback, and fold naming_consistency.patch
Attachment #831061 - Attachment is obsolete: true
Attachment #831061 - Flags: review?(adam)
Trim extraneous commit message from fold.
Attachment #831837 - Attachment is obsolete: true
Attachment #831838 - Flags: review?(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
Attachment #831838 - Flags: review?(adam)
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.
Attachment #831838 - Flags: review?(adam) → review-
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/
Incorporate feedback from abr.
Attachment #831838 - Attachment is obsolete: true
Attachment #831838 - Flags: review?(ekr)
Update description.
Attachment #832482 - Attachment is obsolete: true
Breaking these changes into their own patch.
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.
Attachment #831064 - Attachment is obsolete: true
Comment on attachment 832488 [details] [diff] [review]
Part 2: Adding some vertical ws for readability.

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

r+, trivial
Attachment #832488 - Flags: review+
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.
Attachment #832483 - Flags: review?(ekr)
Attachment #832483 - Flags: review?(adam)
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.
Attachment #832483 - Flags: review?(adam) → review+
Unbitrot this old patch, so we can get an interdiff.
Attachment #8334691 - Attachment description: Part 1. Decouple ICE state with ICE gathering state. Seems to be passing tests. → Part 1. Decouple ICE state with ICE gathering state. Seems to be passing tests. (old patch unbitrotted for interdiff)
Attachment #8334691 - Attachment is obsolete: true
Incorporate some feedback from ekr.
Attachment #832483 - Attachment is obsolete: true
Attachment #832483 - Flags: review?(ekr)
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.
Attachment #8334812 - Flags: review+
Attachment #8334812 - Flags: review?(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(bcampen@mozilla.com): 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(bcampen@mozilla.com): Is this waiting, or new?

Waiting no longer seems to exist.
Attachment #8334812 - Flags: review?(ekr) → review+
Attachment #8334812 - Attachment is obsolete: true
(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 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.
Attachment #8338547 - Attachment is obsolete: true
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.
Attachment #8338556 - Flags: review+
Attachment #8338556 - Flags: checkin?(adam)
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.
Attachment #8338556 - Flags: checkin?(adam) → checkin?(rjesup)
Pre-unbitrot against 906990 part 10.
Attachment #8338556 - Attachment is obsolete: true
Attachment #8338556 - Flags: checkin?(rjesup)
Pre-unbitrot against 906990 part 10.
Attachment #832488 - Attachment is obsolete: true
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.
Attachment #8338681 - Flags: review+
Attachment #8338681 - Flags: checkin?(rjesup)
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.
Attachment #8338682 - Flags: review+
Attachment #8338682 - Flags: checkin?(rjesup)
Attachment #8338681 - Flags: checkin?(rjesup) → checkin+
Attachment #8338682 - Flags: checkin?(rjesup) → checkin+
https://hg.mozilla.org/mozilla-central/rev/df21e1d3eaa2
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Depends on: 943898
You need to log in before you can comment on or make changes to this bug.