929977 - ICE consent freshness (RFC 7675) not implemented

Status: RESOLVED FIXED

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

Description

[Max Jonas Werner [:Makkes]]

Attachment: webrtc-dc-close.html

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:24.0) Gecko/20100101 Firefox/24.0 (Beta/Release)
Build ID: 20130911164256

Steps to reproduce:


http://tools.ietf.org/html/draft-ietf-rtcweb-stun-consent-freshness-00 mandates assertions about connection timeouts that aren't implemented ATM.

To reproduce follow these steps:

1. Open webrtc-dc-close.html in two browser instances
2. Click on "Create Offer" in instance A
3. Paste the resulting offer into the upper textarea of instance B
4. Click "Accept offer" in instance B
5. Copy the resulting answer into the upper textarea of instance A
6. instance B should now show "on data channel"
7. kill -9 PID_OF_INSTANCE_A

This will result in instance B displaying "DC closed after X ms". In my tests X was a value between 704,000 and 711,000 but this may vary I guess.

What should happen is that the above message shall be displayed after at least 15s as mentioned in the spec.

Quote from jesup: "Though I'll also note that real-world applications have no wish to 
wait even 15 seconds to determine that a channel is dead....   Typically 
3-5 seconds is about it, if that."

Comment 1

[Nils Ohlmeier [:drno]]

For clarification: this bug is specifically about killing the browser with SIGKILL so it won't be able to shutdown cleanly. Only then you get these big timeout values, correct?

Because when I simply close the browser or the tab I get more something like 7-9s.

Comment 2

[Martin Thomson [:mt:]]

I think that if we call pc.close(), then the corresponding ICE consent needs to cease immediately (based on the usual definition of "immediate", allowing for scheduling delays and so forth).  And I expect that it should.  Killing the browser is one way to achieve the same effect, but it should be unnecessary.

Comment 3

[Max Jonas Werner [:Makkes]]

(In reply to Nils Ohlmeier [:drno] from comment #1)
> For clarification: this bug is specifically about killing the browser with
> SIGKILL so it won't be able to shutdown cleanly. Only then you get these big
> timeout values, correct?
> 
> Because when I simply close the browser or the tab I get more something like
> 7-9s.

Yes, I used SIGKILL to simulate reachability problems (browser crash, network failure, whatever). I was not talking about a graceful shutdown of the connection.

Comment 4

[Nils Ohlmeier [:drno]]

The meanwhile finished spec this needs to follow is: http://tools.ietf.org/html/rfc7675

Comment 5

[Nils Ohlmeier [:drno]]

Attachment: MozReview Request: Bug 929977: Add support for RFC 7675 ICE consent freshness. r=mt,bwc

Review commit: https://reviewboard.mozilla.org/r/32909/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32909/

Comment 6

[Martin Thomson [:mt:]]

https://reviewboard.mozilla.org/r/32909/#review29737

It's a start.  Not much chop until you have some tests.

Prize if you can run all the tests in less than 30 seconds.

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1173
(Diff revision 1)
> +    gettimeofday(&now, 0);
> +    r_timeval_diff(&now, &comp->consent_last_seen, &elapsed);
> +    if (elapsed.tv_sec >= 30) {
> +      // TODO consent expired => switch ICE connection state
> +    }

Is your plan to do this check here?  Because the best place for this check is probably when you send every packet.  The problem with this timer is that it is going to be up to almost 6 seconds late, which is too long.

If you don't want to check on every packet because you want to avoid calling gettimeofday every time, then you will probably need to maintain a separate 30s timer that is reset every time you receive a check.  If that ever pops, you have lost consent.  That's a less good solution though.

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1200
(Diff revision 1)
> +    if (r=nr_stun_client_ctx_create("consent", comp->active->local->osock,
> +                                    &comp->active->remote->addr, 5000,
> +                                    &comp->consent_ctx))

The 5000 here is a timeout, right?  The problem with using this is that it cancels the transaction after 5s.  What we really want is to cancel it after the normal timeout.

But that means we could have multiple transactions in flight.  I don't know if that's possible, but it would be the best outcome.  Otherwise, if you have to cancel the in-progress request before starting another, you end up risking spurious consent loss if the RTT is high (admittedly, 4s is high enough that maybe that's not a problem).

Comment 7

[Nils Ohlmeier [:drno]]

https://reviewboard.mozilla.org/r/32909/#review29737

> Is your plan to do this check here?  Because the best place for this check is probably when you send every packet.  The problem with this timer is that it is going to be up to almost 6 seconds late, which is too long.
> 
> If you don't want to check on every packet because you want to avoid calling gettimeofday every time, then you will probably need to maintain a separate 30s timer that is reset every time you receive a check.  If that ever pops, you have lost consent.  That's a less good solution though.

I'm not sure I get what you mean by "when you send every packet".
This function (the name is bad) is the one which gets called by the timer for sending each of the refresh requests. Are you suggesting to a counter based approached, e.g. 30s/5s = 6 refresh requests (or 5 and the last one is just the timeout)?
Yes I tried to implement it with a single timer which sends the refresh requests and checks if we haven't seen a response for 30s. But after thinking about the problem a little longer, I don't see how that is possible thanks to the 20% variation demand from the RFC. Because 6 times +/-1s would mean my final timer would be between 24s and 36s.

So I think I'll go the little heavier route of having a refresh timer (every 5s +/-1s), plus a second over all timeout timer (30s).

> The 5000 here is a timeout, right?  The problem with using this is that it cancels the transaction after 5s.  What we really want is to cancel it after the normal timeout.
> 
> But that means we could have multiple transactions in flight.  I don't know if that's possible, but it would be the best outcome.  Otherwise, if you have to cancel the in-progress request before starting another, you end up risking spurious consent loss if the RTT is high (admittedly, 4s is high enough that maybe that's not a problem).

Yeah I have to check how nICEr will behave on that.

I guess one can argue that if I miss a single late reply follow by one in time reply it does not matter. But if the other side responds multiple times more then 4s late the connection is screwed any way (in case of an A/V call the user has probably given up on that call way before already).

Comment 8

[Nils Ohlmeier [:drno]]

Martin one things which is not clear to me from RFC 7675: if I receive a consent refresh request and respond to it (before my own consent timer pops), do I still have to send a consent refresh request?
As we are using request and responses here the keep alive is by-directional already. So sending another one 1-2s later looks a little bit like a waste of resources. Although "detecting" the remote's consent refresh request in nICEr is not as trivial as matching the response to your own request.

Comment 9

[Martin Thomson [:mt:]]

You can only be certain that consent is fresh if you have received a response to one of your connectivity checks.  An off-path attacker can spoof a valid check packet, but they can't spoof receiving one.

Comment 10

[Nils Ohlmeier [:drno]]

Comment on attachment 8713944 [details]
MozReview Request: Bug 929977: Add support for RFC 7675 ICE consent freshness. r=mt,bwc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32909/diff/1-2/

Comment 11

[Martin Thomson [:mt:]]

https://reviewboard.mozilla.org/r/32909/#review31507

I'm not sure why I keep getting review requests for this.  Is this supposed to be done?

::: media/mtransport/test/ice_unittest.cpp:2790
(Diff revision 2)
> +  PR_Sleep(25000);

This isn't good.  5 second sleeps are poor, but this means that the test takes 30s to complete.  Is there any way you can accelerate the timers?  Through the environment, or something.  If nicer runs the consent loop at 50ms interfaces with a 300ms timeout, that might be a shade too fast for slow machines, but you could probably get it closer to that.

Comment 12

[Nils Ohlmeier [:drno]]

Comment on attachment 8713944 [details]
MozReview Request: Bug 929977: Add support for RFC 7675 ICE consent freshness. r=mt,bwc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32909/diff/2-3/

Comment 13

[Nils Ohlmeier [:drno]]

https://reviewboard.mozilla.org/r/32909/#review31507

I don't know either. I never nominated you, nor does mozreview show you as reviewer. I guess mozreview informs you because you volunteered on the initial patch?!
The WIP in the patch indicates that I'm still working on it. I'm just submitting whenever I have something working which solved one of the remaining issues.

> This isn't good.  5 second sleeps are poor, but this means that the test takes 30s to complete.  Is there any way you can accelerate the timers?  Through the environment, or something.  If nicer runs the consent loop at 50ms interfaces with a 300ms timeout, that might be a shade too fast for slow machines, but you could probably get it closer to that.

Yes. The timers are now hopefully the last remaining open issue.

Comment 14

[Nils Ohlmeier [:drno]]

Comment on attachment 8713944 [details]
MozReview Request: Bug 929977: Add support for RFC 7675 ICE consent freshness. r=mt,bwc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32909/diff/3-4/

Comment 15

[Nils Ohlmeier [:drno]]

Comment on attachment 8713944 [details]
MozReview Request: Bug 929977: Add support for RFC 7675 ICE consent freshness. r=mt,bwc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32909/diff/4-5/

Comment 16

[Nils Ohlmeier [:drno]]

https://reviewboard.mozilla.org/r/32909/#review31507

> Yes. The timers are now hopefully the last remaining open issue.

19 seconds for all tests combined now. What's the prize? :-)

Comment 17

[Martin Thomson [:mt:]]

Comment on attachment 8713944 [details]
MozReview Request: Bug 929977: Add support for RFC 7675 ICE consent freshness. r=mt,bwc

https://reviewboard.mozilla.org/r/32909/#review32395

::: media/mtransport/nricectx.h:232
(Diff revision 5)
> +  // Divide some timers to faster testing. Used only for testing.
> +  void internal_SetTimerAccelarator(int divider);

That's a fairly crude way of doing it, but I guess that's OK.

The alternative is to store a pointer to the "set timer" function and have the tests provide a new implementation.

::: media/mtransport/nricemediastream.h:164
(Diff revision 5)
> +  // Get the current ICE consent send and recv status plus the timeval
> +  // of the last consent update.
> +  nsresult GetConsentStatus(int component, int *send_recv, struct timeval *ts);

Send is the only thing we care about for consent.  Obviously, if you can't send, you should probably not bother receiving, but consent is just for sending.

::: media/mtransport/nricemediastream.cpp:521
(Diff revision 5)
> +nsresult NrIceMediaStream::GetConsentStatus(int component_id, int *send_recv, struct timeval *ts) {

Can you change send_recv to a `bool`.  I know that nICEr is aggressively c-ish, but no need to inflict that on the rest of the world.

::: media/mtransport/test/ice_unittest.cpp:2829
(Diff revision 5)
> +TEST_F(IceConnectTest, TestConsentIntermittent) {

This test is invalid.

::: media/mtransport/test/ice_unittest.cpp:2854
(Diff revision 5)
> +  p1_->SetTimerDivider(10);
> +  p2_->SetTimerDivider(10);

Can you move these to a SetUp function?  Seems like something you might want to have on all tests.

::: media/mtransport/test_nr_socket.cpp:307
(Diff revision 5)
> +    if(nat_->delay_stun_resp_ms_ &&

ws after if

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1170
(Diff revision 5)
> +    if ((comp->can_send_recv == state) && (state == 0)) {
> +      // still dead => nothing to do
> +      return;
> +    }

I think that you can make this a lot simpler.  Consent, once lost, is permanently lost.  
```
   After consent is lost, the same ICE credentials MUST NOT be used on
   the affected 5-tuple again.  That means that a new session, or an ICE
   restart, is needed to obtain consent to send on the affected
   candidate pair.
```

Therefore, this check is simply:
```
if (!comp->can_send_recv) {
  return;
}
```

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1193
(Diff revision 5)
> +      // TODO flip ICE connection state to failed

What specifically do you imagine doing here?  Because this seems like a pretty big omission.

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1251
(Diff revision 5)
> +        if (nr_transport_addr_is_reliable_transport(&comp->consent_ctx->my_addr)) {
> +          nr_ice_component_set_consent_state(comp, 0);
> +        }

Just to confirm that I understand this.  This is only for end-to-end tcp, right?  It doesn't include TURN-TCP.

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1256
(Diff revision 5)
> +      /*
> +      if (nr_ice_ctx_remember_id(comp->ctx, comp->consent_ctx->request))
> +        r_log(LOG_ICE,LOG_ERR,"ICE(%s)/STREAM(%s)/COMP(%d): Remember ID failed",
> +              comp->ctx->label, comp->stream->label, comp->component_id);
> +        */

?

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1264
(Diff revision 5)
> +    tval = MAX(NR_ICE_CONSENT_TIMER_DEFAULT - trange, RFC_7675_MIN_CONSENT_TIMER);

This is currently a noop, since both values should be the same.

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1265
(Diff revision 5)
> +    if (!nr_crypto_random_bytes(buf, 2))

2 -> sizeof(trand)

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1286
(Diff revision 5)
> +    if (r=nr_stun_client_ctx_create("consent", comp->active->local->osock,
> +                                    &comp->active->remote->addr, 375,
> +                                    &comp->consent_ctx))

What does the 375 mean?

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1297
(Diff revision 5)
> +    comp->consent_ctx->params.ice_binding_request.priority =
> +      comp->active->priority;
> +    comp->consent_ctx->params.ice_binding_request.control =
> +      comp->active->remote->stream->pctx->controlling?
> +      NR_ICE_CONTROLLING:NR_ICE_CONTROLLED;

We shouldn't need to send priority and controlled/controlling for consent checks.  Or is that too hard to do here?

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1309
(Diff revision 5)
> +    nr_ice_component_consent_timer_cb(0, 0, comp);
> +
> +    nr_ice_component_set_consent_state(comp, 1);

Maybe reorder these two?

Comment 18

[Nils Ohlmeier [:drno]]

https://reviewboard.mozilla.org/r/32909/#review32395

> This test is invalid.

I don't understand why it should be invalid. It emulates that consent requests are not answered for some period of time, but are answered before the 30s timeout. Specifically it checks if this behavior correctly still updates the time stamp for last confirmed consent while not changing the can_send flag.

> What specifically do you imagine doing here?  Because this seems like a pretty big omission.

It's mostly because the RFC doesn't say what to do in this case. And as I pointed out on the list the PeerConnection spec is not 100% clear either. So I was planing on doing this in a follow up bug.

> Just to confirm that I understand this.  This is only for end-to-end tcp, right?  It doesn't include TURN-TCP.

Well this is kind of a short cut (instead of waiting for the timeout to happen):
The nr_ice_component_refresh_consent() fails with an error on reliable transports if the reliable connection is not alive any more and it tries to write a message to it. If the TCP connection to your TURN server is broken I think it is fair to revoke the consent as well, or? I also assume that on a real broken connection some other write operation would fail, before the 5s consent detect it.

> ?

Sorry that was a left over from trying to see if I can get nICEr to remember previously timed out STUN transactions. Unfortunately this only changes the log message when receiving a reply to late.

> What does the 375 mean?

Added a comment.
375ms (RTO) * 16 = 6000ms (the maximum transaction timeout value for 5000ms + 20%).

> We shouldn't need to send priority and controlled/controlling for consent checks.  Or is that too hard to do here?

The ICE specific requests require priority and control. I switched it to plain old STUN short term auth requests instead.

> Maybe reorder these two?

They are on purpose in this order, because nr_ice_component_set_consent_state() sets the time stamp when called. And nr_ice_component_consent_timer_cb() has an extra check which prevents sending a consent request for when we just paired a component, as I don't think it makes sense to send a check on a freshly paired component as we just successfully exchanged binding requests.

Comment 19

[Martin Thomson [:mt:]]

https://reviewboard.mozilla.org/r/32909/#review32395

> I don't understand why it should be invalid. It emulates that consent requests are not answered for some period of time, but are answered before the 30s timeout. Specifically it checks if this behavior correctly still updates the time stamp for last confirmed consent while not changing the can_send flag.

My bad.  I misread the timing.  I thought that you had consent going on and off.

> It's mostly because the RFC doesn't say what to do in this case. And as I pointed out on the list the PeerConnection spec is not 100% clear either. So I was planing on doing this in a follow up bug.

OK, leave a bug number there and we can move on.

Doing anything other than fail the pair is going to be hard.

> Well this is kind of a short cut (instead of waiting for the timeout to happen):
> The nr_ice_component_refresh_consent() fails with an error on reliable transports if the reliable connection is not alive any more and it tries to write a message to it. If the TCP connection to your TURN server is broken I think it is fair to revoke the consent as well, or? I also assume that on a real broken connection some other write operation would fail, before the 5s consent detect it.

Definitely worth explaining.  A broken TCP connection to the TURN server *could* be repaired, but I'm guessing that we can't realistically do that.

> Added a comment.
> 375ms (RTO) * 16 = 6000ms (the maximum transaction timeout value for 5000ms + 20%).

Constants, comments, etc...

> They are on purpose in this order, because nr_ice_component_set_consent_state() sets the time stamp when called. And nr_ice_component_consent_timer_cb() has an extra check which prevents sending a consent request for when we just paired a component, as I don't think it makes sense to send a check on a freshly paired component as we just successfully exchanged binding requests.

Is that what the tv_sec check is about?  I think that I would rather have this function call a separate nr_ice_component_schedule_consent() method in that case.

Comment 20

[Nils Ohlmeier [:drno]]

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

Comment 21

[Nils Ohlmeier [:drno]]

https://reviewboard.mozilla.org/r/32909/#review32395

> OK, leave a bug number there and we can move on.
> 
> Doing anything other than fail the pair is going to be hard.

Turned out that switching to 'failed' was a lot easier then I expected. Fixed.

> Definitely worth explaining.  A broken TCP connection to the TURN server *could* be repaired, but I'm guessing that we can't realistically do that.

Connection repairing or re-establishing is somewhere on our wish list... but not with high priority ;-)

Comment 22

[Nils Ohlmeier [:drno]]

Comment on attachment 8713944 [details]
MozReview Request: Bug 929977: Add support for RFC 7675 ICE consent freshness. r=mt,bwc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32909/diff/5-6/

Comment 23

[Martin Thomson [:mt:]]

Comment on attachment 8713944 [details]
MozReview Request: Bug 929977: Add support for RFC 7675 ICE consent freshness. r=mt,bwc

https://reviewboard.mozilla.org/r/32909/#review32975

::: media/mtransport/test/ice_unittest.cpp:1087
(Diff revision 6)
> +  void SendFailure(int stream, int component, const unsigned char *data,
> +                   int len) {

Does it really matter what you send in this case, can you drop the last two arguments?

::: media/mtransport/test/ice_unittest.cpp:1138
(Diff revision 6)
> +  void AssertConsentRefresh_s(size_t stream, int component_id, bool status, bool updated) {

You always run this with stream = 0, component_id = 1, why not hard code that?

status and updated are hard to parse when reading tests.  Using an enum might help with that, maybe three states:
 - stale = has consent, but no change to the time
 - expired = no consent
 - fresh = has consent and time has updated since the last call to this function

I don't think that you care about time in the case that you have no consent.

::: media/mtransport/third_party/nICEr/src/ice/ice_component.h:77
(Diff revision 6)
> +  // TODO should/could we roll this into the comp state?
> +  int can_send;

Where are we at with this?

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1151
(Diff revision 6)
> +/* TODO do we want user prefs for these? */

I think that the answer for now is no.

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1168
(Diff revision 6)
> +static void nr_ice_component_set_consent_state(nr_ice_component *comp, int state)

This really looks like it should be two functions.  The only common part only needs to be set when state == 0.

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1199
(Diff revision 6)
> +      if (nr_ice_media_stream_component_failed(comp->stream, comp))

This is probably worth a comment: consent failure is elevated to component failure.  That's not something that the spec requires, but it's just hygiene on our part.

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1200
(Diff revision 6)
> +        r_log(LOG_ICE,LOG_ERR,"ICE(%s)/STREAM(%s)/COMP(%d): failed to mark media stream failed",

s/media stream/component

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1313
(Diff revision 6)
> +    /* We use 375ms as the initial RTO, because 16 * 375 = 6000ms, which is the
> +     * maximum timeout value for 5000ms + 20%. */

RTO shouldn't be used when the number of transmissions is 1, so set this to MAX_INT or something large.

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1333
(Diff revision 6)
> +    nr_ice_component_consent_schedule_conset_timer(comp);

typo in name

Comment 24

[Byron Campen [:bwc]]

Comment on attachment 8713944 [details]
MozReview Request: Bug 929977: Add support for RFC 7675 ICE consent freshness. r=mt,bwc

https://reviewboard.mozilla.org/r/32909/#review32989

I'm not done reviewing yet, will get the rest tomorrow.

::: media/mtransport/nricemediastream.cpp:528
(Diff revision 6)
> +    MOZ_MTLOG(ML_ERROR, "Failed to find peer stream for " << name_ <<
> +              component_id);

Let's put a ':' between the name and component id like other places.

::: media/mtransport/nricemediastream.cpp:537
(Diff revision 6)
> +    MOZ_MTLOG(ML_ERROR, "Failed to get consent status for " << name_ <<
> +              component_id);

Same here.

::: media/mtransport/nricemediastream.cpp:541
(Diff revision 6)
> +  *can_send = send == 1;

Most places I see this as !!send

::: media/mtransport/test/ice_unittest.cpp:1146
(Diff revision 6)
> +    ASSERT_TRUE(can_send == status);

ASSERT_EQ

::: media/mtransport/test/ice_unittest.cpp:1667
(Diff revision 6)
> -  void SendReceive() {
> +  void SendReceive(size_t count = 1) {

Maybe a better way of doing this is verifying that p1_->sent() and p2_->received() both increase by 1? That would remove the need for manually keeping track in the test cases.

::: media/mtransport/test_nr_socket.cpp:314
(Diff revision 6)
> +      NR_ASYNC_TIMER_SET(nat_->delay_stun_resp_ms_,
> +                         process_delayed_cb,
> +                         new DeferredPacket(this, msg, len, flags, to,
> +                                            internal_socket_),
> +                         &timer_handle_);

What if we try to send another STUN response before this timer goes off? Also, we only seem to run this logic when transmitting to something behind the same NAT.

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1151
(Diff revision 6)
> +/* TODO do we want user prefs for these? */
> +#define NR_ICE_CONSENT_TIMER_DEFAULT 5000
> +#define NR_ICE_CONSENT_TIMEOUT_DEFAULT 30000

Seems like if we had variables for these on the ctx, we could go without the timer divider business...

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1154
(Diff revision 6)
> +/* 375ms * 16 = 6000ms = 5000ms + 20% */
> +#define NR_ICE_CONSENT_MAX_RTO 375

This seems fragile; I'm worried that we might change how we calculate the final timeout inside nr_stun_client_ctx_create. Why don't we set comp->consent_ctx->maximum_transmits_timeout_ms directly like we do comp->consent_ctx->maximum_transmits? That would be a little easier to understand.

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1215
(Diff revision 6)
> +        if (comp->consent_ctx->error_code == 403) {

What about other errors? I guess we just ignore them and hope that the next request fares better?

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1253
(Diff revision 6)
> +                                         nr_ice_component_refresh_consent_cb,
> +                                         comp)) {

Nit: fix ws

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1260
(Diff revision 6)
> +      if (nr_transport_addr_is_reliable_transport(&comp->consent_ctx->my_addr)) {

Honestly, there's a bunch more that can go wrong other than transport stuff here. I would probably mark this failed if r != R_WOULDBLOCK.

Side note: it seems that nr_stun_client_send_request fails completely if R_WOULDBLOCK is returned by nr_socket_sendto. That's probably bad.

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1269
(Diff revision 6)
> +void nr_ice_component_consent_schedule_conset_timer(nr_ice_component *comp)

%s/conset/consent/g

Comment 25

[Nils Ohlmeier [:drno]]

https://reviewboard.mozilla.org/r/32909/#review32975

> Where are we at with this?

For the case of expired consent an additional final comp state feels like a natural fit.
But the case of fresh consent is or should be independent of comp state. Thus adding another comp state, would make the consent code more complex and add the risk of the existing code not handling an additional new comp state properly.
=> Removed comment.

> RTO shouldn't be used when the number of transmissions is 1, so set this to MAX_INT or something large.

The problem is that the RTO gets calculated at creation of the stun client context. But the current API doesn't allow me to set the amount of transmissions. It gets automatically set based on the type of transport.
And Consent is the only STUN/ICE spec which requires this new send once over any transport, don't resend but wait super long for answer request-response model.
Setting RTO to MAX_INT is just going to cause an overflow :-) The idea here was that we theoretically still catch timeouts if they happen. Although in reality the exact stun client timeout value would have to be set precisely every time after calculating the random delay for the next consent refresh.

Not sure what is the best solution here.

Comment 26

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/32909/#review33131

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1191
(Diff revision 6)
> +      if (comp->consent_timeout) {
> +        NR_async_timer_cancel(comp->consent_timeout);
> +        comp->consent_timeout = 0;
> +      }

Common code with the other branch.

Comment 27

[Nils Ohlmeier [:drno]]

https://reviewboard.mozilla.org/r/32909/#review32989

> What about other errors? I guess we just ignore them and hope that the next request fares better?

RFC 7675 explicitly lists only 403 to revoke consent.
I guess we would need to divide errors into "recoverable" vs "non-recoverable". Which is doable, but I'm not sure if it worth the effort to build a list here.

Comment 29

[Nils Ohlmeier [:drno]]

https://reviewboard.mozilla.org/r/32909/#review32989

> What if we try to send another STUN response before this timer goes off? Also, we only seem to run this logic when transmitting to something behind the same NAT.

Sure it only solves my needs for the Consent tests right now. Are you okay with deferring improvements on this into a follow up bug?

Comment 30

[Nils Ohlmeier [:drno]]

Comment on attachment 8713944 [details]
MozReview Request: Bug 929977: Add support for RFC 7675 ICE consent freshness. r=mt,bwc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32909/diff/6-7/

Comment 31

[Byron Campen [:bwc]]

Comment on attachment 8713944 [details]
MozReview Request: Bug 929977: Add support for RFC 7675 ICE consent freshness. r=mt,bwc

https://reviewboard.mozilla.org/r/32909/#review34707

Just nits.

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1322
(Diff revisions 6 - 7)
> +    /* The timeout of the trnasaction is the maximum time until we send the

s/trnas/trans/

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1324
(Diff revisions 6 - 7)
> +     * TODO: set this every time we calcuclate the new random timeout. */

s/calcuc/calcu/

Comment 32

[Byron Campen [:bwc]]

Comment on attachment 8713944 [details]
MozReview Request: Bug 929977: Add support for RFC 7675 ICE consent freshness. r=mt,bwc

https://reviewboard.mozilla.org/r/32909/#review34709

::: media/mtransport/test_nr_socket.h:172
(Diff revision 7)
> +    uint32_t delay_stun_resp_ms_;

Don't forget to add a comment about the limitations of this.

Comment 33

[Nils Ohlmeier [:drno]]

Comment on attachment 8713944 [details]
MozReview Request: Bug 929977: Add support for RFC 7675 ICE consent freshness. r=mt,bwc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32909/diff/7-8/

Comment 34

[Byron Campen [:bwc]]

Comment on attachment 8713944 [details]
MozReview Request: Bug 929977: Add support for RFC 7675 ICE consent freshness. r=mt,bwc

https://reviewboard.mozilla.org/r/32909/#review34715

Comment 35

[Nils Ohlmeier [:drno]]

Looks like the try runs on Android have problems with consent checks. I need to take a closer look...

Comment 36

[Nils Ohlmeier [:drno]]

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

Comment 37

[Nils Ohlmeier [:drno]]

First try run with all video related mochitests disabled confirms that it looks like the Android emulator is to overloaded with the WebRTC video tests to handle the STUN consent requests within 5s.

Comment 38

[Nils Ohlmeier [:drno]]

So second try run with all video mochitests enabled, but resolution and sampling rates reduced also turns the WebRTC green, which used to be permafail after adding the ICE Consent feature.
So I think a clear indicator that the Android emulators are overloaded. Which probably results in all kind of strange intermittent problems.

Comment 39

[Nils Ohlmeier [:drno]]

Status update: landing bug 1254905 (soon) should eliminate the mid call test failure I got on try. But this is/was then followed by ICE connection establishing failures on try on about 30-40% of my try runs. Landing this probably have to wait until we know how to handle these ICE connection failures.

Comment 40

[Nils Ohlmeier [:drno]]

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

Comment 41

[Nils Ohlmeier [:drno]]

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

Comment 42

[Nils Ohlmeier [:drno]]

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

Comment 43

[Nils Ohlmeier [:drno]]

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

Comment 44

[Nils Ohlmeier [:drno]]

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

Comment 45

[Nils Ohlmeier [:drno]]

Attachment: MozReview Request: Bug 929977: disable orange Android tests. r=bwc

Review commit: https://reviewboard.mozilla.org/r/45109/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45109/

Comment 46

[Nils Ohlmeier [:drno]]

Comment on attachment 8713944 [details]
MozReview Request: Bug 929977: Add support for RFC 7675 ICE consent freshness. r=mt,bwc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32909/diff/8-9/

Comment 47

[Byron Campen [:bwc]]

Comment on attachment 8739208 [details]
MozReview Request: Bug 929977: disable orange Android tests. r=bwc

https://reviewboard.mozilla.org/r/45109/#review41621

r+, with regrets

Comment 48

[Nils Ohlmeier [:drno]]

Comment on attachment 8713944 [details]
MozReview Request: Bug 929977: Add support for RFC 7675 ICE consent freshness. r=mt,bwc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32909/diff/9-10/

Comment 49

[Nils Ohlmeier [:drno]]

Comment on attachment 8739208 [details]
MozReview Request: Bug 929977: disable orange Android tests. r=bwc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45109/diff/1-2/

Comment 50

[Nils Ohlmeier [:drno]]

Comment on attachment 8713944 [details]
MozReview Request: Bug 929977: Add support for RFC 7675 ICE consent freshness. r=mt,bwc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32909/diff/10-11/

Comment 51

[Nils Ohlmeier [:drno]]

Comment on attachment 8739208 [details]
MozReview Request: Bug 929977: disable orange Android tests. r=bwc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45109/diff/2-3/

Comment 52

[Nils Ohlmeier [:drno]]

Comment on attachment 8713944 [details]
MozReview Request: Bug 929977: Add support for RFC 7675 ICE consent freshness. r=mt,bwc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32909/diff/11-12/

Comment 53

[Nils Ohlmeier [:drno]]

Comment on attachment 8739208 [details]
MozReview Request: Bug 929977: disable orange Android tests. r=bwc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45109/diff/3-4/

Comment 54

[Nils Ohlmeier [:drno]]

Comment on attachment 8713944 [details]
MozReview Request: Bug 929977: Add support for RFC 7675 ICE consent freshness. r=mt,bwc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32909/diff/12-13/

Comment 55

[Nils Ohlmeier [:drno]]

Comment on attachment 8739208 [details]
MozReview Request: Bug 929977: disable orange Android tests. r=bwc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45109/diff/4-5/

Comment 56

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cdfbcb602366
https://hg.mozilla.org/integration/mozilla-inbound/rev/76ee908baab7

Comment 57

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cdfbcb602366
https://hg.mozilla.org/mozilla-central/rev/76ee908baab7