Track pending ICE restarts independently for different media sections.

Track pending ICE restarts independently for different media sections.

RFC 5245 allows an ICE restart to occur on only one media section.
However, before this CL, if an endpoint attempted to do this, we would
change our local ICE ufrag/pwd in every media section.

Also did some refactoring, turning the transport options from
mediasesion.h into a map.

Committed: https://crrev.com/0ed85b2ee3079fb9597ec5e0eb5034a707c71aca
Cr-Commit-Position: refs/heads/master@{#11728}

[Taylor Brandstetter, 2016-02-05 20:54:41]

deadbeef@webrtc.org changed reviewers:
+ honghaiz@webrtc.org, pthatcher@webrtc.org

[Taylor Brandstetter, 2016-02-05 20:54:41]

This is a revised version of the "fix ICE restart issues" CL from a while back.

I realize the "audio/video/data" part is still ugly, but I plan to refactor this
soon, when I get a chance to work on the unified plan migration.

[honghaiz3, 2016-02-10 01:01:48]

Mostly looking good to me.

https://codereview.webrtc.org/1671173002/diff/20001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1671173002/diff/20001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:870: // generation
This and the next line can be merged.

https://codereview.webrtc.org/1671173002/diff/20001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsession.h (right):

https://codereview.webrtc.org/1671173002/diff/20001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.h:305: // bool IceRestartPending(const
std::string& mid) const;
Does this need a lot of work?
If not, maybe it can be done in this CL.

https://codereview.webrtc.org/1671173002/diff/20001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsessiondescriptionfactory.cc (left):

https://codereview.webrtc.org/1671173002/diff/20001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsessiondescriptionfactory.cc:464:
session_->ResetIceRestartLatch();
Is there a good reason to move this from here to SetLocalDescription?

https://codereview.webrtc.org/1671173002/diff/20001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsessiondescriptionfactory.cc (right):

https://codereview.webrtc.org/1671173002/diff/20001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsessiondescriptionfactory.cc:472: if
(session_->local_description()) {
Does it make sense to write a method for this part to remove the code
duplication? This and the above one (starting at line 402) are pretty much
identical.

[Taylor Brandstetter, 2016-02-10 21:18:33]

https://codereview.webrtc.org/1671173002/diff/20001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1671173002/diff/20001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:870: // generation
On 2016/02/10 01:01:48, honghaiz3 wrote:
> This and the next line can be merged.

Done.

https://codereview.webrtc.org/1671173002/diff/20001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsession.h (right):

https://codereview.webrtc.org/1671173002/diff/20001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.h:305: // bool IceRestartPending(const
std::string& mid) const;
On 2016/02/10 01:01:48, honghaiz3 wrote:
> Does this need a lot of work?
> If not, maybe it can be done in this CL. 

I couldn't do this before because there was an issue with mids not matching up
between successive offer/answer exchanges. But since that's fixed, I went ahead
and did the necessary refactoring.

https://codereview.webrtc.org/1671173002/diff/20001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsessiondescriptionfactory.cc (left):

https://codereview.webrtc.org/1671173002/diff/20001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsessiondescriptionfactory.cc:464:
session_->ResetIceRestartLatch();
On 2016/02/10 01:01:48, honghaiz3 wrote:
> Is there a good reason to move this from here to SetLocalDescription?

Yes. If the remote peer starts an ICE restart, createAnswer should continue to
generate new credentials until a new local description is set.

By resetting the latch here, if you called createAnswer twice, the second answer
would have the old credentials, which is wrong. That reminds me, I should have a
test for this.

https://codereview.webrtc.org/1671173002/diff/20001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsessiondescriptionfactory.cc (right):

https://codereview.webrtc.org/1671173002/diff/20001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsessiondescriptionfactory.cc:472: if
(session_->local_description()) {
On 2016/02/10 01:01:48, honghaiz3 wrote:
> Does it make sense to write a method for this part to remove the code
> duplication? This and the above one (starting at line 402) are pretty much
> identical. 

After refactoring, this section of code is smaller, so I think there's less
incentive to make a helper method.

[honghaiz3, 2016-02-12 18:21:04]

lgtm

https://codereview.webrtc.org/1671173002/diff/120001/talk/session/media/media...
File talk/session/media/mediasession.cc (right):

https://codereview.webrtc.org/1671173002/diff/120001/talk/session/media/media...
talk/session/media/mediasession.cc:743: auto it =
options.transport_options.find(content_name);
Perhaps this can be replaced with return options.transport_options[content_name]
and removing this function. 
The downside of [] operation is that it will insert a default value if it is not
found. So it really depends on whether you want to tell the difference between a
default value and "not found".

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession.cc:690: }
Did you change this part?
Either way, it would be better to leave it at the same location as before to
minimize the diff.

[Taylor Brandstetter, 2016-02-12 20:43:02]

https://codereview.webrtc.org/1671173002/diff/120001/talk/session/media/media...
File talk/session/media/mediasession.cc (right):

https://codereview.webrtc.org/1671173002/diff/120001/talk/session/media/media...
talk/session/media/mediasession.cc:743: auto it =
options.transport_options.find(content_name);
On 2016/02/12 18:21:04, honghaiz3 wrote:
> Perhaps this can be replaced with return
options.transport_options[content_name]
> and removing this function. 
> The downside of [] operation is that it will insert a default value if it is
not
> found. So it really depends on whether you want to tell the difference between
a
> default value and "not found".

I can't use the [] operator because the MediaSessionsOptions is const. That's
the main reason why this function is needed.

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession.cc:690: }
On 2016/02/12 18:21:04, honghaiz3 wrote:
> Did you change this part?
> Either way, it would be better to leave it at the same location as before to
> minimize the diff. 

No, I didn't change it. But I moved it in the header file to the "public"
section, so I moved it in the cc file too. I find it useful to keep things in
order.

[pthatcher1, 2016-02-17 06:46:59]

This is a clear improvement.  Sorry it took so long to review.

Mostly just style stuff, although there is one part I'm unsure about (clearing
the "pending restarts" if SetLocalDescription didn't actually change them).

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/peerconnection.h
File webrtc/api/peerconnection.h (right):

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/peerconnectio...
webrtc/api/peerconnection.h:33: // Assumes that
|session_options|->transport_options map entries exist.
Assumes which entries exist?

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession.cc:467: const SessionDescription* new_sd =
new_desc->description();
Using existing style, this would be "new_desc" and "old_desc".

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession.cc:779: pending_ice_restarts_.clear();
What if the call to SetLocalDescription didn't actually change the ufrag/pwd for
a content name that was triggered by the remote side (because the JS called
createOffer, then setRemote, then  setLocal, or some crazy thing).

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession...
File webrtc/api/webrtcsession_unittest.cc (right):

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession_unittest.cc:766: bool expect_equal) {
Can you wrap this in something with the name "Expect" in it, such as:

void ExpectEqualIceUfragAndPwd(...) {
  CompareUfragAndPwd(..., true);
}


void ExpectNotEqualIceUfragAndPwdl(...) {
  CompareUfragAndPwd(..., false);
}

That would just make the test cases more readable.

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession_unittest.cc:768: EXPECT_FALSE(expect_equal);
The test errors will be easier to read if you write it like this:

if (expect_equal) {
  EXPECT_EQ(desc1->contents().size(), desc2->contents());
} else {
  EXPECT_NE(desc1->contents().size(), desc2->contents());
}

Same below.

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession_unittest.cc:848: const std::string& modified_ice_pwd) {
Why not just call it SetIceUfragPwd and remove the comment?

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession_unittest.cc:848: const std::string& modified_ice_pwd) {
I think |ufrag| and |pwd| are probably sufficient (don't need modified_ice_
prefix).

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession_unittest.cc:3760: "aaaaaaaaaaaaaaaaaaaaaa");
This might be more readable as "ufrag_a1" and "pwd_a1" rather than "aaaa"
"aaaaaaa".  Same for video (_v1 and _v2).

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession_unittest.cc:3788:
SetLocalDescriptionWithoutError(updated_answer.release());
Should we also test if you change only the ufrag or only the pwd, but not both?

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession...
File webrtc/api/webrtcsessiondescriptionfactory.cc (left):

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession...
webrtc/api/webrtcsessiondescriptionfactory.cc:447:
session_->ResetIceRestartLatch();
Well, this was a terrible place to have this before.  You could only call
createAnswer once, and then the next time it wouldn't reset the ufrag/pwd?

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession...
File webrtc/api/webrtcsessiondescriptionfactory.cc (right):

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession...
webrtc/api/webrtcsessiondescriptionfactory.cc:387: for (const
cricket::ContentInfo& content :
Can you make a private helper method for this?

const std::vector<cricket::ContentInfo>& local_contents() {
  return session_->local_description()->description()->contents();
}

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession...
webrtc/api/webrtcsessiondescriptionfactory.cc:409:
session_->IceRestartPending(content.name);
I can't wait until we can stick all per-content-name/MID info in transceivers
instead of having so many methods that take a content name.

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession...
webrtc/api/webrtcsessiondescriptionfactory.cc:453: }
I don't like seeing this duplicated.  Can we make a
MaybeCopyCandidatesFromLocalDescription(JsepSessionDescription* desc) that has
this logic, and then use it in the two places?

[Taylor Brandstetter, 2016-02-17 21:43:51]

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/peerconnection.h
File webrtc/api/peerconnection.h (right):

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/peerconnectio...
webrtc/api/peerconnection.h:33: // Assumes that
|session_options|->transport_options map entries exist.
On 2016/02/17 06:46:58, pthatcher1 wrote:
> Assumes which entries exist?

The entries that the caller wants populated according to the |rtc_options|. I'll
make that more clear.

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession.cc:467: const SessionDescription* new_sd =
new_desc->description();
On 2016/02/17 06:46:58, pthatcher1 wrote:
> Using existing style, this would be "new_desc" and "old_desc".

But one is a SessionDescription and one is a SessionDescriptionInterface. (note
that this code is just being moved around)

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession.cc:779: pending_ice_restarts_.clear();
On 2016/02/17 06:46:58, pthatcher1 wrote:
> What if the call to SetLocalDescription didn't actually change the ufrag/pwd
for
> a content name that was triggered by the remote side (because the JS called
> createOffer, then setRemote, then  setLocal, or some crazy thing).

Well, for one, the issue you describe occurred before this CL too, so I don't
think we need to fix it here.

And second, changing the ICE ufrag/password is illegal anyway according to the
spec. So right now it should be considered a "use at your own risk" type of
munging. And once we actually do make it illegal, we won't have to worry about
this at all.

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession...
File webrtc/api/webrtcsession_unittest.cc (right):

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession_unittest.cc:766: bool expect_equal) {
On 2016/02/17 06:46:58, pthatcher1 wrote:
> Can you wrap this in something with the name "Expect" in it, such as:
> 
> void ExpectEqualIceUfragAndPwd(...) {
>   CompareUfragAndPwd(..., true);
> }
> 
> 
> void ExpectNotEqualIceUfragAndPwdl(...) {
>   CompareUfragAndPwd(..., false);
> }
> 
> That would just make the test cases more readable.

Again, just copy/pasted code. But I'll fix it...

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession_unittest.cc:768: EXPECT_FALSE(expect_equal);
On 2016/02/17 06:46:58, pthatcher1 wrote:
> The test errors will be easier to read if you write it like this:
> 
> if (expect_equal) {
>   EXPECT_EQ(desc1->contents().size(), desc2->contents());
> } else {
>   EXPECT_NE(desc1->contents().size(), desc2->contents());
> }
> 
> Same below.

I just made this function return a bool. I think it's cleaner that way.

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession_unittest.cc:848: const std::string& modified_ice_pwd) {
On 2016/02/17 06:46:58, pthatcher1 wrote:
> Why not just call it SetIceUfragPwd and remove the comment?

Done.

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession_unittest.cc:848: const std::string& modified_ice_pwd) {
On 2016/02/17 06:46:58, pthatcher1 wrote:
> I think |ufrag| and |pwd| are probably sufficient (don't need modified_ice_
> prefix).

Done.

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession_unittest.cc:3760: "aaaaaaaaaaaaaaaaaaaaaa");
On 2016/02/17 06:46:58, pthatcher1 wrote:
> This might be more readable as "ufrag_a1" and "pwd_a1" rather than "aaaa"
> "aaaaaaa".  Same for video (_v1 and _v2).

I think "a", "b" and "c" makes it more clear since you don't have to hunt for
the "v1" or "v2" number. And there are other tests in this file that use similar
nonsense values.

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession_unittest.cc:3788:
SetLocalDescriptionWithoutError(updated_answer.release());
On 2016/02/17 06:46:58, pthatcher1 wrote:
> Should we also test if you change only the ufrag or only the pwd, but not
both?

Maybe? What SHOULD we do when that happens? Is it even allowed? It seems like it
shouldn't be.

[pthatcher1, 2016-02-17 21:53:38]

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession.cc:779: pending_ice_restarts_.clear();
On 2016/02/17 21:43:50, Taylor Brandstetter wrote:
> On 2016/02/17 06:46:58, pthatcher1 wrote:
> > What if the call to SetLocalDescription didn't actually change the ufrag/pwd
> for
> > a content name that was triggered by the remote side (because the JS called
> > createOffer, then setRemote, then  setLocal, or some crazy thing).
> 
> Well, for one, the issue you describe occurred before this CL too, so I don't
> think we need to fix it here.
> 
> And second, changing the ICE ufrag/password is illegal anyway according to the
> spec. So right now it should be considered a "use at your own risk" type of
> munging. And once we actually do make it illegal, we won't have to worry about
> this at all.

But is it legal to do this?

var remoteOffer1 = ...;  
var localAnswer1 = pc.createAnswer(remoteOffer1);
var remoteOffer2 = ...;  // Changes ufrag/pwd
pc.setRemoteDesription(remoteOffer2);
pc.setLocalDescription(remoteAnswer1);
var localOffer1 = pc.createOffer();


If so, does localOffer1 have a changed ufrag/pwd or not?

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession...
File webrtc/api/webrtcsession_unittest.cc (right):

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession_unittest.cc:3788:
SetLocalDescriptionWithoutError(updated_answer.release());
On 2016/02/17 21:43:50, Taylor Brandstetter wrote:
> On 2016/02/17 06:46:58, pthatcher1 wrote:
> > Should we also test if you change only the ufrag or only the pwd, but not
> both?
> 
> Maybe? What SHOULD we do when that happens? Is it even allowed? It seems like
it
> shouldn't be.

I think we should treat changing either as restarting ICE.  What do you think? 
The other options are: ignore it and reject it, both of which seem worse.

[Taylor Brandstetter, 2016-02-18 00:33:21]

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession.cc:779: pending_ice_restarts_.clear();
On 2016/02/17 21:53:38, pthatcher1 wrote:
> On 2016/02/17 21:43:50, Taylor Brandstetter wrote:
> > On 2016/02/17 06:46:58, pthatcher1 wrote:
> > > What if the call to SetLocalDescription didn't actually change the
ufrag/pwd
> > for
> > > a content name that was triggered by the remote side (because the JS
called
> > > createOffer, then setRemote, then  setLocal, or some crazy thing).
> > 
> > Well, for one, the issue you describe occurred before this CL too, so I
don't
> > think we need to fix it here.
> > 
> > And second, changing the ICE ufrag/password is illegal anyway according to
the
> > spec. So right now it should be considered a "use at your own risk" type of
> > munging. And once we actually do make it illegal, we won't have to worry
about
> > this at all.
> 
> But is it legal to do this?
> 
> var remoteOffer1 = ...;  
> var localAnswer1 = pc.createAnswer(remoteOffer1);
> var remoteOffer2 = ...;  // Changes ufrag/pwd
> pc.setRemoteDesription(remoteOffer2);
> pc.setLocalDescription(remoteAnswer1);
> var localOffer1 = pc.createOffer();
> 
> 
> If so, does localOffer1 have a changed ufrag/pwd or not?

This is "legal", but very dangerous. Setting an old SDP blob is the same to us
as munging a new SDP blob to look like an old one. So, once we make munging the
ufrag/password illegal, setLocalDescription will throw an error there. It would
also throw an error if the second remote description added an m-line.

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession...
File webrtc/api/webrtcsession_unittest.cc (right):

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession_unittest.cc:3788:
SetLocalDescriptionWithoutError(updated_answer.release());
On 2016/02/17 21:53:38, pthatcher1 wrote:
> On 2016/02/17 21:43:50, Taylor Brandstetter wrote:
> > On 2016/02/17 06:46:58, pthatcher1 wrote:
> > > Should we also test if you change only the ufrag or only the pwd, but not
> > both?
> > 
> > Maybe? What SHOULD we do when that happens? Is it even allowed? It seems
like
> it
> > shouldn't be.
> 
> I think we should treat changing either as restarting ICE.  What do you think?

> The other options are: ignore it and reject it, both of which seem worse.

I was more asking what the spec says. The spec definitely says (paraphrased)
"you MUST change both to do an ICE restart", but it also says "If the offer
contained a change in the a=ice-ufrag or a=ice-pwd attributes compared to the
previous SDP from the peer, it indicates that ICE is restarting for this media
stream." Notice the "or". So I guess we should treat changing either as an ICE
restart, but always change both when we make our own descriptions.

I added a unit test and revised a comment.

[pthatcher1, 2016-02-19 02:33:44]

lgtm

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession.cc:779: pending_ice_restarts_.clear();
On 2016/02/18 00:33:21, Taylor Brandstetter wrote:
> On 2016/02/17 21:53:38, pthatcher1 wrote:
> > On 2016/02/17 21:43:50, Taylor Brandstetter wrote:
> > > On 2016/02/17 06:46:58, pthatcher1 wrote:
> > > > What if the call to SetLocalDescription didn't actually change the
> ufrag/pwd
> > > for
> > > > a content name that was triggered by the remote side (because the JS
> called
> > > > createOffer, then setRemote, then  setLocal, or some crazy thing).
> > > 
> > > Well, for one, the issue you describe occurred before this CL too, so I
> don't
> > > think we need to fix it here.
> > > 
> > > And second, changing the ICE ufrag/password is illegal anyway according to
> the
> > > spec. So right now it should be considered a "use at your own risk" type
of
> > > munging. And once we actually do make it illegal, we won't have to worry
> about
> > > this at all.
> > 
> > But is it legal to do this?
> > 
> > var remoteOffer1 = ...;  
> > var localAnswer1 = pc.createAnswer(remoteOffer1);
> > var remoteOffer2 = ...;  // Changes ufrag/pwd
> > pc.setRemoteDesription(remoteOffer2);
> > pc.setLocalDescription(remoteAnswer1);
> > var localOffer1 = pc.createOffer();
> > 
> > 
> > If so, does localOffer1 have a changed ufrag/pwd or not?
> 
> This is "legal", but very dangerous. Setting an old SDP blob is the same to us
> as munging a new SDP blob to look like an old one. So, once we make munging
the
> ufrag/password illegal, setLocalDescription will throw an error there. It
would
> also throw an error if the second remote description added an m-line.

It would still make me feel a lot better if we did something like:

if (ice_ufrag_or_pwd_changed) {
  pending_ice_restarts_.clear();
}


But I won't let it block submitting this CL.

[Taylor Brandstetter, 2016-02-19 21:59:26]

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession.cc:779: pending_ice_restarts_.clear();
On 2016/02/19 02:33:43, pthatcher1 wrote:
> On 2016/02/18 00:33:21, Taylor Brandstetter wrote:
> > On 2016/02/17 21:53:38, pthatcher1 wrote:
> > > On 2016/02/17 21:43:50, Taylor Brandstetter wrote:
> > > > On 2016/02/17 06:46:58, pthatcher1 wrote:
> > > > > What if the call to SetLocalDescription didn't actually change the
> > ufrag/pwd
> > > > for
> > > > > a content name that was triggered by the remote side (because the JS
> > called
> > > > > createOffer, then setRemote, then  setLocal, or some crazy thing).
> > > > 
> > > > Well, for one, the issue you describe occurred before this CL too, so I
> > don't
> > > > think we need to fix it here.
> > > > 
> > > > And second, changing the ICE ufrag/password is illegal anyway according
to
> > the
> > > > spec. So right now it should be considered a "use at your own risk" type
> of
> > > > munging. And once we actually do make it illegal, we won't have to worry
> > about
> > > > this at all.
> > > 
> > > But is it legal to do this?
> > > 
> > > var remoteOffer1 = ...;  
> > > var localAnswer1 = pc.createAnswer(remoteOffer1);
> > > var remoteOffer2 = ...;  // Changes ufrag/pwd
> > > pc.setRemoteDesription(remoteOffer2);
> > > pc.setLocalDescription(remoteAnswer1);
> > > var localOffer1 = pc.createOffer();
> > > 
> > > 
> > > If so, does localOffer1 have a changed ufrag/pwd or not?
> > 
> > This is "legal", but very dangerous. Setting an old SDP blob is the same to
us
> > as munging a new SDP blob to look like an old one. So, once we make munging
> the
> > ufrag/password illegal, setLocalDescription will throw an error there. It
> would
> > also throw an error if the second remote description added an m-line.
> 
> It would still make me feel a lot better if we did something like:
> 
> if (ice_ufrag_or_pwd_changed) {
>   pending_ice_restarts_.clear();
> }
> 
> 
> But I won't let it block submitting this CL.

Ok, let's say the spec DOES allow munging the ufrag/pwd. We still would want to
reset the "pending restart" flag here. The flag is only valid for a single
offer/answer exchange.  The next generated answer should only have a new
ufrag/pwd if the next offer does. Right?

The only question is: If you set a pranswer with the old ufrag/pwd, should the
next generated answer have old or new credentials? Since this is unspecified
behavior (since munging the ufrag/pwd is illegal), I think we're free to decide
what happens for the time being, and "old credentials" makes enough sense to me.

[Taylor Brandstetter, 2016-02-20 00:02:48]

Description was changed from

==========
Track pending ICE restarts independently for different media sections.

RFC 5245 allows an ICE restart to occur on only one media section.
However, before this CL, if an endpoint attempted to do this, we would
change our local ICE ufrag/pwd in every media section.
==========

to

==========
Track pending ICE restarts independently for different media sections.

Also did some refactoring, turning the transport options from
mediasesion.h into a map.

RFC 5245 allows an ICE restart to occur on only one media section.
However, before this CL, if an endpoint attempted to do this, we would
change our local ICE ufrag/pwd in every media section.
==========

[Taylor Brandstetter, 2016-02-20 00:03:12]

Description was changed from

==========
Track pending ICE restarts independently for different media sections.

Also did some refactoring, turning the transport options from
mediasesion.h into a map.

RFC 5245 allows an ICE restart to occur on only one media section.
However, before this CL, if an endpoint attempted to do this, we would
change our local ICE ufrag/pwd in every media section.
==========

to

==========
Track pending ICE restarts independently for different media sections.

RFC 5245 allows an ICE restart to occur on only one media section.
However, before this CL, if an endpoint attempted to do this, we would
change our local ICE ufrag/pwd in every media section.

Also did some refactoring, turning the transport options from
mediasesion.h into a map.
==========

[pthatcher1, 2016-02-20 05:57:25]

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/1671173002/diff/120001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession.cc:779: pending_ice_restarts_.clear();
On 2016/02/19 21:59:26, Taylor Brandstetter wrote:
> On 2016/02/19 02:33:43, pthatcher1 wrote:
> > On 2016/02/18 00:33:21, Taylor Brandstetter wrote:
> > > On 2016/02/17 21:53:38, pthatcher1 wrote:
> > > > On 2016/02/17 21:43:50, Taylor Brandstetter wrote:
> > > > > On 2016/02/17 06:46:58, pthatcher1 wrote:
> > > > > > What if the call to SetLocalDescription didn't actually change the
> > > ufrag/pwd
> > > > > for
> > > > > > a content name that was triggered by the remote side (because the JS
> > > called
> > > > > > createOffer, then setRemote, then  setLocal, or some crazy thing).
> > > > > 
> > > > > Well, for one, the issue you describe occurred before this CL too, so
I
> > > don't
> > > > > think we need to fix it here.
> > > > > 
> > > > > And second, changing the ICE ufrag/password is illegal anyway
according
> to
> > > the
> > > > > spec. So right now it should be considered a "use at your own risk"
type
> > of
> > > > > munging. And once we actually do make it illegal, we won't have to
worry
> > > about
> > > > > this at all.
> > > > 
> > > > But is it legal to do this?
> > > > 
> > > > var remoteOffer1 = ...;  
> > > > var localAnswer1 = pc.createAnswer(remoteOffer1);
> > > > var remoteOffer2 = ...;  // Changes ufrag/pwd
> > > > pc.setRemoteDesription(remoteOffer2);
> > > > pc.setLocalDescription(remoteAnswer1);
> > > > var localOffer1 = pc.createOffer();
> > > > 
> > > > 
> > > > If so, does localOffer1 have a changed ufrag/pwd or not?
> > > 
> > > This is "legal", but very dangerous. Setting an old SDP blob is the same
to
> us
> > > as munging a new SDP blob to look like an old one. So, once we make
munging
> > the
> > > ufrag/password illegal, setLocalDescription will throw an error there. It
> > would
> > > also throw an error if the second remote description added an m-line.
> > 
> > It would still make me feel a lot better if we did something like:
> > 
> > if (ice_ufrag_or_pwd_changed) {
> >   pending_ice_restarts_.clear();
> > }
> > 
> > 
> > But I won't let it block submitting this CL.
> 
> Ok, let's say the spec DOES allow munging the ufrag/pwd. We still would want
to
> reset the "pending restart" flag here. The flag is only valid for a single
> offer/answer exchange.  The next generated answer should only have a new
> ufrag/pwd if the next offer does. Right?
> 
> The only question is: If you set a pranswer with the old ufrag/pwd, should the
> next generated answer have old or new credentials? Since this is unspecified
> behavior (since munging the ufrag/pwd is illegal), I think we're free to
decide
> what happens for the time being, and "old credentials" makes enough sense to
me.


I was thinking of the next createOffer from the local side should generate a new
ufrag/pwd.  But now that I think about it more, I realize that would just
trigger a new ICE restart from the remote side again, which probably is not what
we want.  

Perhaps what we should do is say "if you manage to set unchanged local
ufrag/pwd, then you get a half-restart".  The more I think about it, maybe
that's a feature and not a bug.

So, I agree with you now: just clear the flag.

[Taylor Brandstetter, 2016-02-22 20:07:04]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2016-02-22 20:07:05]

The patchset sent to the CQ was uploaded after l-g-t-m from honghaiz@webrtc.org,
pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1671173002/#ps180001 (title:
"Merging with master.")

[commit-bot: I haz the power, 2016-02-22 20:07:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1671173002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1671173002/180001

[commit-bot: I haz the power, 2016-02-22 20:10:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-22 20:10:02]

Try jobs failed on following builders:
  ios32_sim_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/5317)
  ios64_sim_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/5315)
  ios_arm64_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/7560)
  ios_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/11468)

[Taylor Brandstetter, 2016-02-24 00:09:31]

The CQ bit was checked by deadbeef@webrtc.org

[commit-bot: I haz the power, 2016-02-24 00:09:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1671173002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1671173002/180001

[commit-bot: I haz the power, 2016-02-24 01:24:59]

Description was changed from

==========
Track pending ICE restarts independently for different media sections.

RFC 5245 allows an ICE restart to occur on only one media section.
However, before this CL, if an endpoint attempted to do this, we would
change our local ICE ufrag/pwd in every media section.

Also did some refactoring, turning the transport options from
mediasesion.h into a map.
==========

to

==========
Track pending ICE restarts independently for different media sections.

RFC 5245 allows an ICE restart to occur on only one media section.
However, before this CL, if an endpoint attempted to do this, we would
change our local ICE ufrag/pwd in every media section.

Also did some refactoring, turning the transport options from
mediasesion.h into a map.
==========

[commit-bot: I haz the power, 2016-02-24 01:25:00]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-02-24 01:25:01]

Description was changed from

==========
Track pending ICE restarts independently for different media sections.

RFC 5245 allows an ICE restart to occur on only one media section.
However, before this CL, if an endpoint attempted to do this, we would
change our local ICE ufrag/pwd in every media section.

Also did some refactoring, turning the transport options from
mediasesion.h into a map.
==========

to

==========
Track pending ICE restarts independently for different media sections.

RFC 5245 allows an ICE restart to occur on only one media section.
However, before this CL, if an endpoint attempted to do this, we would
change our local ICE ufrag/pwd in every media section.

Also did some refactoring, turning the transport options from
mediasesion.h into a map.

Committed: https://crrev.com/0ed85b2ee3079fb9597ec5e0eb5034a707c71aca
Cr-Commit-Position: refs/heads/master@{#11728}
==========

[commit-bot: I haz the power, 2016-02-24 01:25:03]

Patchset 10 (id:??) landed as
https://crrev.com/0ed85b2ee3079fb9597ec5e0eb5034a707c71aca
Cr-Commit-Position: refs/heads/master@{#11728}