Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1549633004:
Fill the remote pwd in ice candidates when an ICE credential is received. (Closed)
|
Created:
5 years ago by honghaiz3 Modified:
4 years, 11 months ago Reviewers:
pthatcher1 CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFill the remote pwd in the ice candidates when an ICE credential is received.
Also when a STUN ping arrives from an unknown address, try to find the pwd and generation from the remote ICE parameters.
BUG=
Committed: https://crrev.com/112fe43b9f3f04592f1ccff6b4f23fcd2890bf30
Cr-Commit-Position: refs/heads/master@{#11144}
Patch Set 1 : #
Total comments: 15
Patch Set 2 : Address comments #Patch Set 3 : Fix a type conversion issue. #
Messages
Total messages: 20 (12 generated)
Description was changed from ========== Fill remote the pwd in ice candidates when an ICE credential is received. A little other tideup BUG= ========== to ========== Fill remote the pwd in ice candidates when an ICE credential is received. Also when a STUN ping arrives from unknown address, try to find the pwd and generation from the remote ICE parameters. BUG= ==========
Description was changed from ========== Fill remote the pwd in ice candidates when an ICE credential is received. Also when a STUN ping arrives from unknown address, try to find the pwd and generation from the remote ICE parameters. BUG= ========== to ========== Fill remote the pwd in ice candidates when an ICE credential is received. Also when a STUN ping arrives from an unknown address, try to find the pwd and generation from the remote ICE parameters. BUG= ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Fill remote the pwd in ice candidates when an ICE credential is received. Also when a STUN ping arrives from an unknown address, try to find the pwd and generation from the remote ICE parameters. BUG= ========== to ========== Fill the remote pwd in the ice candidates when an ICE credential is received. Also when a STUN ping arrives from an unknown address, try to find the pwd and generation from the remote ICE parameters. BUG= ==========
honghaiz@webrtc.org changed reviewers: + pthatcher@webrtc.org
Patchset #1 (id:40001) has been deleted
https://codereview.webrtc.org/1549633004/diff/60001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1549633004/diff/60001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:359: if (candidate.username() == ice_ufrag && candidate.password().empty()) { Can you put a TODO in candidate.h to change username() to either usernameFragment() or ufrag()? And, better yet, make a CL to do so? https://codereview.webrtc.org/1549633004/diff/60001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:541: remote_password = ice_param->pwd; If the remote_username is empty (not signalled), do we want to continue to assume it means the latest ufrag/pwd? If we don't, this is a behavior changes which might break things. https://codereview.webrtc.org/1549633004/diff/60001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:645: *generation = static_cast<uint32_t>(params.size()); I find it a bit confusing that we fill in the generation even when the generation isn't found. That's a little too magic/implicit. Can we instead of the caller explicitly use remote_ice_parameters_.size() when a generation isn't found? https://codereview.webrtc.org/1549633004/diff/60001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:814: // If the candidate contains ufrag, use that to find the generation. contains ufrag => has a ufrag use that => use it https://codereview.webrtc.org/1549633004/diff/60001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/1549633004/diff/60001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.h:255: // Returns the IceParameters that match the ufrag, also fill the generation Might read better as: "Returns the remote IceParameters and generation that match |ufrag| if found, and returns nullptr if not found." Actually, for consistency, can you make it have both return values be out pointers, and have the return value be a bool? bool FindRemoteIceParametersByUfrag( const std::string& ufrag, IceParameters* params, uint32_t* generation); https://codereview.webrtc.org/1549633004/diff/60001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/1549633004/diff/60001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel_unittest.cc:1908: candidate.username() == kIceUfrag[2]); I think you want EXPECT_TRUE, not ASSERT_TRUE
PTAL. Thanks! https://codereview.webrtc.org/1549633004/diff/60001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1549633004/diff/60001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:359: if (candidate.username() == ice_ufrag && candidate.password().empty()) { On 2015/12/28 21:33:59, pthatcher1 wrote: > Can you put a TODO in candidate.h to change username() to either > usernameFragment() or ufrag()? And, better yet, make a CL to do so? Added TODO... although I think there is no confusion there. A username/password is perhaps more readable than ufrag/pwd for general people? https://codereview.webrtc.org/1549633004/diff/60001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:541: remote_password = ice_param->pwd; On 2015/12/28 21:33:59, pthatcher1 wrote: > If the remote_username is empty (not signalled), do we want to continue to > assume it means the latest ufrag/pwd? If we don't, this is a behavior changes > which might break things. If the remote_username is not signalled, remote ufrag and pwd will be empty in both cases, right. The behavior should not matter. Plus, in a stun request, the ufrag should never be empty, right? https://codereview.webrtc.org/1549633004/diff/60001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:645: *generation = static_cast<uint32_t>(params.size()); On 2015/12/28 21:33:59, pthatcher1 wrote: > I find it a bit confusing that we fill in the generation even when the > generation isn't found. That's a little too magic/implicit. Can we instead of > the caller explicitly use remote_ice_parameters_.size() when a generation isn't > found? Done. https://codereview.webrtc.org/1549633004/diff/60001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:814: // If the candidate contains ufrag, use that to find the generation. On 2015/12/28 21:33:59, pthatcher1 wrote: > contains ufrag => has a ufrag > use that => use it Done. https://codereview.webrtc.org/1549633004/diff/60001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/1549633004/diff/60001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.h:255: // Returns the IceParameters that match the ufrag, also fill the generation On 2015/12/28 21:33:59, pthatcher1 wrote: > Might read better as: "Returns the remote IceParameters and generation that > match |ufrag| if found, and returns nullptr if not found." > > Actually, for consistency, can you make it have both return values be out > pointers, and have the return value be a bool? > > bool FindRemoteIceParametersByUfrag( > const std::string& ufrag, > IceParameters* params, > uint32_t* generation); > If we use IceParameters* as the argument, we will have to copy IceParameters unnecessarily. If we use IceParameters** as the argument, the double pointer looks more confusing to me (although it is pretty subjective). It will be something like this. bool P2PTransportChannel::FindRemoteIce( const std::string& ufrag, IceParameters** ice_params, uint32_t* generation) { const auto& params = remote_ice_parameters_; auto it = std::find_if( params.rbegin(), params.rend(), [ufrag](const IceParameters& param) { return param.ufrag == ufrag; }); if (it == params.rend()) { // Not found. return false; } *ice_params = &(*it); *generation = params.rend() - it - 1; return true; } Plus, it will more cumbersome to call this. You have to define two variables before calling this. Let me know if you really want this. https://codereview.webrtc.org/1549633004/diff/60001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/1549633004/diff/60001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel_unittest.cc:1908: candidate.username() == kIceUfrag[2]); On 2015/12/28 21:33:59, pthatcher1 wrote: > I think you want EXPECT_TRUE, not ASSERT_TRUE Done.
lgtm https://codereview.webrtc.org/1549633004/diff/60001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1549633004/diff/60001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:359: if (candidate.username() == ice_ufrag && candidate.password().empty()) { On 2015/12/30 18:59:33, honghaiz3 wrote: > On 2015/12/28 21:33:59, pthatcher1 wrote: > > Can you put a TODO in candidate.h to change username() to either > > usernameFragment() or ufrag()? And, better yet, make a CL to do so? > Added TODO... although I think there is no confusion there. A username/password > is perhaps more readable than ufrag/pwd for general people? username() is more readable, but it's also wrong. usernameFragment() is correct. I'd be happy with usernameFragment/password. But on the other hand, we use "ufrag" and "pwd" in all of the other code, and we should probably be consistent. Your TODO is sufficient for this CL. https://codereview.webrtc.org/1549633004/diff/60001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:541: remote_password = ice_param->pwd; On 2015/12/30 18:59:33, honghaiz3 wrote: > On 2015/12/28 21:33:59, pthatcher1 wrote: > > If the remote_username is empty (not signalled), do we want to continue to > > assume it means the latest ufrag/pwd? If we don't, this is a behavior changes > > which might break things. > If the remote_username is not signalled, remote ufrag and pwd will be empty in > both cases, right. The behavior should not matter. > Plus, in a stun request, the ufrag should never be empty, right? Sorry, when I said "not signalled", I meant "not signalled on the candidate". But looking at this more closely, I see that my comment was wrong. This is covering the case where we know the remote ufrag from a STUN ping and are filling in the password. So, your code looks good here. Forget my previous comment. https://codereview.webrtc.org/1549633004/diff/60001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/1549633004/diff/60001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.h:255: // Returns the IceParameters that match the ufrag, also fill the generation On 2015/12/30 18:59:33, honghaiz3 wrote: > On 2015/12/28 21:33:59, pthatcher1 wrote: > > Might read better as: "Returns the remote IceParameters and generation that > > match |ufrag| if found, and returns nullptr if not found." > > > > Actually, for consistency, can you make it have both return values be out > > pointers, and have the return value be a bool? > > > > bool FindRemoteIceParametersByUfrag( > > const std::string& ufrag, > > IceParameters* params, > > uint32_t* generation); > > > If we use IceParameters* as the argument, we will have to copy IceParameters > unnecessarily. If we use IceParameters** as the argument, the double pointer > looks more confusing to me (although it is pretty subjective). It will be > something like this. > bool P2PTransportChannel::FindRemoteIce( > const std::string& ufrag, > IceParameters** ice_params, > uint32_t* generation) { > const auto& params = remote_ice_parameters_; > auto it = std::find_if( > params.rbegin(), params.rend(), > [ufrag](const IceParameters& param) { return param.ufrag == ufrag; }); > if (it == params.rend()) { > // Not found. > return false; > } > *ice_params = &(*it); > *generation = params.rend() - it - 1; > return true; > } > Plus, it will more cumbersome to call this. You have to define two variables > before calling this. > Let me know if you really want this. Good point about the double pointer. I don't think it's a big issue, though. We do similar things other places in the code base and it's fine. I also don't think putting two variables on the stack is a big deal, especially since we need one for the generation anyway. So, I prefer what you have written there with the double pointer. But I don't feel that strongly about it. I'll let you decide.
The CQ bit was checked by honghaiz@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1549633004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1549633004/80001
The CQ bit was checked by honghaiz@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/1549633004/#ps100001 (title: "Fix a type conversion issue.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1549633004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1549633004/100001
Message was sent while issue was closed.
Description was changed from ========== Fill the remote pwd in the ice candidates when an ICE credential is received. Also when a STUN ping arrives from an unknown address, try to find the pwd and generation from the remote ICE parameters. BUG= ========== to ========== Fill the remote pwd in the ice candidates when an ICE credential is received. Also when a STUN ping arrives from an unknown address, try to find the pwd and generation from the remote ICE parameters. BUG= ==========
Message was sent while issue was closed.
Committed patchset #3 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Fill the remote pwd in the ice candidates when an ICE credential is received. Also when a STUN ping arrives from an unknown address, try to find the pwd and generation from the remote ICE parameters. BUG= ========== to ========== Fill the remote pwd in the ice candidates when an ICE credential is received. Also when a STUN ping arrives from an unknown address, try to find the pwd and generation from the remote ICE parameters. BUG= Committed: https://crrev.com/112fe43b9f3f04592f1ccff6b4f23fcd2890bf30 Cr-Commit-Position: refs/heads/master@{#11144} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/112fe43b9f3f04592f1ccff6b4f23fcd2890bf30 Cr-Commit-Position: refs/heads/master@{#11144} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
