Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2053043003:
Update ICE role on all ports, not just ones used for new connections. (Closed)
|
Created:
4 years, 6 months ago by Taylor Brandstetter Modified:
4 years, 6 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionUpdate ICE role on all ports, not just ones used for new connections.
Previously, if the ICE role changed, SetIceRole was only called on
the ports from the most recent ICE generation. However, STUN pings
may still be sent and received by older generation ports, so they
should receive an updated role as well.
This was previously triggering an ASSERT, because a P2PTransportChannel
expects the ICE role of each of its ports to match its own role.
Committed: https://crrev.com/370544594e18deb7f560f961295c8cf3f0a679f1
Committed: https://crrev.com/dfc4244d9efb1f2f340a5c88fa55f3a03e1e372e
Cr-Original-Commit-Position: refs/heads/master@{#13226}
Cr-Commit-Position: refs/heads/master@{#13247}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Renaming variables, adding tests. #
Total comments: 1
Patch Set 3 : Adding comment Peter requested. #Patch Set 4 : Fixing issue with call to `erase`. Adding test for port destruction. #
Messages
Total messages: 28 (9 generated)
deadbeef@webrtc.org changed reviewers: + honghaiz@webrtc.org, pthatcher@webrtc.org
PTAL Honghai. This fixes the ASSERT that Vlad found. Also, I realize that "active_ports_"/"inactive_ports_" may not be the best names; feel free to suggest replacements.
On 2016/06/10 19:25:43, Taylor Brandstetter wrote: > PTAL Honghai. This fixes the ASSERT that Vlad found. > > Also, I realize that "active_ports_"/"inactive_ports_" may not be the best > names; feel free to suggest replacements. Should we still replace the assert for ice role in the function OnNominated? The chance of that happens is getting smaller with this CL and the one which does not change ICE role with ICE restart, but it may still happen if ICE role changed in other circumstances, right? Does this need some tests?
On 2016/06/11 01:21:17, honghaiz3 wrote: > On 2016/06/10 19:25:43, Taylor Brandstetter wrote: > > PTAL Honghai. This fixes the ASSERT that Vlad found. > > > > Also, I realize that "active_ports_"/"inactive_ports_" may not be the best > > names; feel free to suggest replacements. > > Should we still replace the assert for ice role in the function OnNominated? > The chance of that happens is getting smaller with this CL and the one which > does not change ICE role with ICE restart, > but it may still happen if ICE role changed in other circumstances, right? > > Does this need some tests? I don't think the assert should be changed. SignalNominated is only fired if the port thinks it's "controlled". So the assert will only occur if the Port thinks it's "controlled" but the P2PTransportChannel thinks it's "controlling", which should never happen. A test is still needed, I agree. Do you have any suggestions on how to implement the test? Maybe something like: 1. Create 2 channels and connect them, with channel 1 having a higher tie-breaker. 2. Do an ICE restart but only on channel 1 (simulating the ICE restart signal being delayed). Also change the ICE role. 3. Expect all packets from channel 1 to have the same role. (How do I verify this expectation, though?)
On 2016/06/13 17:41:51, Taylor Brandstetter wrote: > On 2016/06/11 01:21:17, honghaiz3 wrote: > > On 2016/06/10 19:25:43, Taylor Brandstetter wrote: > > > PTAL Honghai. This fixes the ASSERT that Vlad found. > > > > > > Also, I realize that "active_ports_"/"inactive_ports_" may not be the best > > > names; feel free to suggest replacements. > > > > Should we still replace the assert for ice role in the function OnNominated? > > The chance of that happens is getting smaller with this CL and the one which > > does not change ICE role with ICE restart, > > but it may still happen if ICE role changed in other circumstances, right? > > > > Does this need some tests? > > I don't think the assert should be changed. SignalNominated is only fired if the > port thinks it's "controlled". So the assert will only occur if the Port thinks > it's "controlled" but the P2PTransportChannel thinks it's "controlling", which > should never happen. > > A test is still needed, I agree. Do you have any suggestions on how to implement > the test? Maybe something like: > > 1. Create 2 channels and connect them, with channel 1 having a higher > tie-breaker. > 2. Do an ICE restart but only on channel 1 (simulating the ICE restart signal > being delayed). Also change the ICE role. > 3. Expect all packets from channel 1 to have the same role. (How do I verify > this expectation, though?) Agree on the assert part. In step 3, could you just verify that the ice roles of all ports have been updated?
On 2016/06/13 17:57:37, honghaiz3 wrote: > On 2016/06/13 17:41:51, Taylor Brandstetter wrote: > > On 2016/06/11 01:21:17, honghaiz3 wrote: > > > On 2016/06/10 19:25:43, Taylor Brandstetter wrote: > > > > PTAL Honghai. This fixes the ASSERT that Vlad found. > > > > > > > > Also, I realize that "active_ports_"/"inactive_ports_" may not be the best > > > > names; feel free to suggest replacements. > > > > > > Should we still replace the assert for ice role in the function OnNominated? > > > The chance of that happens is getting smaller with this CL and the one which > > > does not change ICE role with ICE restart, > > > but it may still happen if ICE role changed in other circumstances, right? > > > > > > Does this need some tests? > > > > I don't think the assert should be changed. SignalNominated is only fired if > the > > port thinks it's "controlled". So the assert will only occur if the Port > thinks > > it's "controlled" but the P2PTransportChannel thinks it's "controlling", which > > should never happen. > > > > A test is still needed, I agree. Do you have any suggestions on how to > implement > > the test? Maybe something like: > > > > 1. Create 2 channels and connect them, with channel 1 having a higher > > tie-breaker. > > 2. Do an ICE restart but only on channel 1 (simulating the ICE restart signal > > being delayed). Also change the ICE role. > > 3. Expect all packets from channel 1 to have the same role. (How do I verify > > this expectation, though?) > Agree on the assert part. > > In step 3, could you just verify that the ice roles of all ports have been > updated? I suppose we could, if we have a reference to the PortAllocator and can hold onto pointers to the ports it creates.
It does need tests. For tests, why not create a P2pTransportChannel, fire SignalNetworkInactive on one of the ports, then P2pTransportChannel::SetIceRole and make sure that port has the ice role changed? https://codereview.webrtc.org/2053043003/diff/1/webrtc/p2p/base/p2ptransportc... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/2053043003/diff/1/webrtc/p2p/base/p2ptransportc... webrtc/p2p/base/p2ptransportchannel.h:306: std::vector<PortInterface*> inactive_ports_; For names, how about ports_ and removed_ports_waiting_to_be_destroyed_, with a note that we keep the removed ports around so that we can change the role?
Tests added. Both for the ICE restart case and the continual gathering network inactive case. https://codereview.webrtc.org/2053043003/diff/1/webrtc/p2p/base/p2ptransportc... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/2053043003/diff/1/webrtc/p2p/base/p2ptransportc... webrtc/p2p/base/p2ptransportchannel.h:306: std::vector<PortInterface*> inactive_ports_; On 2016/06/15 22:13:20, pthatcher1 wrote: > For names, how about ports_ and removed_ports_waiting_to_be_destroyed_, with a > note that we keep the removed ports around so that we can change the role? I think removed_ports_ is sufficiently unambiguous. I'll include the rest of the information in a comment.
lgtm https://codereview.webrtc.org/2053043003/diff/20001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2053043003/diff/20001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:313: port->SetIceRole(ice_role); Maybe add a comment of why we set the ICE role of removed ports.
lgtm
The CQ bit was checked by deadbeef@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org, honghaiz@webrtc.org Link to the patchset: https://codereview.webrtc.org/2053043003/#ps40001 (title: "Adding comment Peter requested.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2053043003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Update ICE role on all ports, not just ones used for new connections. Previously, if the ICE role changed, SetIceRole was only called on the ports from the most recent ICE generation. However, STUN pings may still be sent and received by older generation ports, so they should receive an updated role as well. This was previously triggering an ASSERT, because a P2PTransportChannel expects the ICE role of each of its ports to match its own role. ========== to ========== Update ICE role on all ports, not just ones used for new connections. Previously, if the ICE role changed, SetIceRole was only called on the ports from the most recent ICE generation. However, STUN pings may still be sent and received by older generation ports, so they should receive an updated role as well. This was previously triggering an ASSERT, because a P2PTransportChannel expects the ICE role of each of its ports to match its own role. Committed: https://crrev.com/370544594e18deb7f560f961295c8cf3f0a679f1 Cr-Commit-Position: refs/heads/master@{#13226} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/370544594e18deb7f560f961295c8cf3f0a679f1 Cr-Commit-Position: refs/heads/master@{#13226}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.webrtc.org/2078423004/ by phoglund@webrtc.org. The reason for reverting is: Speculative revert: breaks video quality tests on Win and Mac (???): https://build.chromium.org/p/chromium.webrtc.fyi/builders/Mac%20Tester/builds....
Message was sent while issue was closed.
Description was changed from ========== Update ICE role on all ports, not just ones used for new connections. Previously, if the ICE role changed, SetIceRole was only called on the ports from the most recent ICE generation. However, STUN pings may still be sent and received by older generation ports, so they should receive an updated role as well. This was previously triggering an ASSERT, because a P2PTransportChannel expects the ICE role of each of its ports to match its own role. Committed: https://crrev.com/370544594e18deb7f560f961295c8cf3f0a679f1 Cr-Commit-Position: refs/heads/master@{#13226} ========== to ========== Update ICE role on all ports, not just ones used for new connections. Previously, if the ICE role changed, SetIceRole was only called on the ports from the most recent ICE generation. However, STUN pings may still be sent and received by older generation ports, so they should receive an updated role as well. This was previously triggering an ASSERT, because a P2PTransportChannel expects the ICE role of each of its ports to match its own role. Committed: https://crrev.com/370544594e18deb7f560f961295c8cf3f0a679f1 Cr-Commit-Position: refs/heads/master@{#13226} ==========
Please review again. There was a crash in OnPortDestroyed, which wasn't caught because somehow this method had *no* unit test coverage. We were simply lucky enough that this happened to be triggered by the performance tests.
lgtm
The CQ bit was checked by deadbeef@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from honghaiz@webrtc.org Link to the patchset: https://codereview.webrtc.org/2053043003/#ps60001 (title: "Fixing issue with call to `erase`. Adding test for port destruction.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2053043003/60001
lgtm
Message was sent while issue was closed.
Description was changed from ========== Update ICE role on all ports, not just ones used for new connections. Previously, if the ICE role changed, SetIceRole was only called on the ports from the most recent ICE generation. However, STUN pings may still be sent and received by older generation ports, so they should receive an updated role as well. This was previously triggering an ASSERT, because a P2PTransportChannel expects the ICE role of each of its ports to match its own role. Committed: https://crrev.com/370544594e18deb7f560f961295c8cf3f0a679f1 Cr-Commit-Position: refs/heads/master@{#13226} ========== to ========== Update ICE role on all ports, not just ones used for new connections. Previously, if the ICE role changed, SetIceRole was only called on the ports from the most recent ICE generation. However, STUN pings may still be sent and received by older generation ports, so they should receive an updated role as well. This was previously triggering an ASSERT, because a P2PTransportChannel expects the ICE role of each of its ports to match its own role. Committed: https://crrev.com/370544594e18deb7f560f961295c8cf3f0a679f1 Cr-Commit-Position: refs/heads/master@{#13226} ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Update ICE role on all ports, not just ones used for new connections. Previously, if the ICE role changed, SetIceRole was only called on the ports from the most recent ICE generation. However, STUN pings may still be sent and received by older generation ports, so they should receive an updated role as well. This was previously triggering an ASSERT, because a P2PTransportChannel expects the ICE role of each of its ports to match its own role. Committed: https://crrev.com/370544594e18deb7f560f961295c8cf3f0a679f1 Cr-Commit-Position: refs/heads/master@{#13226} ========== to ========== Update ICE role on all ports, not just ones used for new connections. Previously, if the ICE role changed, SetIceRole was only called on the ports from the most recent ICE generation. However, STUN pings may still be sent and received by older generation ports, so they should receive an updated role as well. This was previously triggering an ASSERT, because a P2PTransportChannel expects the ICE role of each of its ports to match its own role. Committed: https://crrev.com/370544594e18deb7f560f961295c8cf3f0a679f1 Committed: https://crrev.com/dfc4244d9efb1f2f340a5c88fa55f3a03e1e372e Cr-Original-Commit-Position: refs/heads/master@{#13226} Cr-Commit-Position: refs/heads/master@{#13247} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/dfc4244d9efb1f2f340a5c88fa55f3a03e1e372e Cr-Commit-Position: refs/heads/master@{#13247} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
