Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1248063002:
Fix the generation mismatch assertion error. (Closed)
|
Created:
5 years, 5 months ago by honghaiz3 Modified:
5 years, 4 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, qiang.lu, niklas.enbom, yujie_mao (webrtc), Andrew MacDonald, juberti1 Base URL:
https://chromium.googlesource.com/external/webrtc@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFix the generation mismatch assertion error.
BUG=4860
Committed: https://crrev.com/503726c3498201822079c5abe9e528498846c9f2
Cr-Commit-Position: refs/heads/master@{#9667}
Patch Set 1 #
Total comments: 6
Patch Set 2 : #
Total comments: 4
Patch Set 3 : #
Total comments: 4
Patch Set 4 : #
Messages
Total messages: 20 (7 generated)
Patchset #1 (id:1) has been deleted
honghaiz@webrtc.org changed reviewers: + pthatcher@webrtc.org
The change in webrtcsession.cc may not be necessary but it will help reduce unnecessary candidates which are dropped later.
Can you add a unit test for the part where we don't copy candidates during an ICE restart? https://codereview.webrtc.org/1248063002/diff/20001/talk/app/webrtc/webrtcses... File talk/app/webrtc/webrtcsession.cc (right): https://codereview.webrtc.org/1248063002/diff/20001/talk/app/webrtc/webrtcses... talk/app/webrtc/webrtcsession.cc:463: ice_restart_ = true; Instead of setting ice_restart_ here, can you move it to the place where CheckForRemoteIceRestart is called? Something like: if (CheckForIceRestart(...) { ice_restart_ = true; } https://codereview.webrtc.org/1248063002/diff/20001/talk/app/webrtc/webrtcses... talk/app/webrtc/webrtcsession.cc:846: // We retain all received candidates unless ICE is restarted; Can you expand the comment to explain in more detail why we don't keep candidates during an ICE restart? https://codereview.webrtc.org/1248063002/diff/20001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1248063002/diff/20001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:693: Candidate new_remote_candidate(remote_candidate); Can you move the generation check down here and remove GetRemoteCandidateGeneration? It's duplicate with the check you are doing.
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Added tests and addressed comments. PTAL. https://codereview.webrtc.org/1248063002/diff/20001/talk/app/webrtc/webrtcses... File talk/app/webrtc/webrtcsession.cc (right): https://codereview.webrtc.org/1248063002/diff/20001/talk/app/webrtc/webrtcses... talk/app/webrtc/webrtcsession.cc:463: ice_restart_ = true; On 2015/07/29 21:09:42, pthatcher1 wrote: > Instead of setting ice_restart_ here, can you move it to the place where > CheckForRemoteIceRestart is called? > > Something like: > > if (CheckForIceRestart(...) { > ice_restart_ = true; > } It was called outside the class and we cannot set the member variable in this class from that location. https://codereview.webrtc.org/1248063002/diff/20001/talk/app/webrtc/webrtcses... talk/app/webrtc/webrtcsession.cc:846: // We retain all received candidates unless ICE is restarted; On 2015/07/29 21:09:42, pthatcher1 wrote: > Can you expand the comment to explain in more detail why we don't keep > candidates during an ICE restart? Done. https://codereview.webrtc.org/1248063002/diff/20001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1248063002/diff/20001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:693: Candidate new_remote_candidate(remote_candidate); On 2015/07/29 21:09:42, pthatcher1 wrote: > Can you move the generation check down here and remove > GetRemoteCandidateGeneration? It's duplicate with the check you are doing. If we move it down here, it means that even if it is a candidate with old generation, we will still do the SortConnection() (after an early return in this method), which may not be needed. Well, I guess we can use the return value of CreateConnections to determine if we should do sort but the logic will be somewhat different from the current one. Plus I only filtered the candidates with a smaller generation. If something bigger came in, it is still an ASSERT error. Plus GetRemoteCandidateGeneration has a different value depending on whether it is G-ICE, although maybe no one is using G-ICE?
lgtm
juberti@webrtc.org changed reviewers: + juberti@webrtc.org
https://codereview.webrtc.org/1248063002/diff/80001/talk/app/webrtc/webrtcses... File talk/app/webrtc/webrtcsession.cc (right): https://codereview.webrtc.org/1248063002/diff/80001/talk/app/webrtc/webrtcses... talk/app/webrtc/webrtcsession.cc:847: // When ICE is restarted, all existing candidates belong to an old generation What if the new SDP contains candidates? https://codereview.webrtc.org/1248063002/diff/80001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1248063002/diff/80001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:669: uint32 generation = candidate.generation(); Add a comment to indicate when this would happen (unclear to me)
https://codereview.webrtc.org/1248063002/diff/80001/talk/app/webrtc/webrtcses... File talk/app/webrtc/webrtcsession.cc (right): https://codereview.webrtc.org/1248063002/diff/80001/talk/app/webrtc/webrtcses... talk/app/webrtc/webrtcsession.cc:847: // When ICE is restarted, all existing candidates belong to an old generation On 2015/07/30 20:23:13, juberti1 wrote: > What if the new SDP contains candidates? The candidates in the new SDP will be kept regardless of the conditions. Slightly revised the comments. https://codereview.webrtc.org/1248063002/diff/80001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1248063002/diff/80001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:669: uint32 generation = candidate.generation(); On 2015/07/30 20:23:13, juberti1 wrote: > Add a comment to indicate when this would happen (unclear to me) Added comments. This happened pretty rarely after the changes in webrtcsession.cc although it still happens.
lgtm https://codereview.webrtc.org/1248063002/diff/100001/talk/app/webrtc/webrtcse... File talk/app/webrtc/webrtcsession_unittest.cc (right): https://codereview.webrtc.org/1248063002/diff/100001/talk/app/webrtc/webrtcse... talk/app/webrtc/webrtcsession_unittest.cc:2617: // The third offer has a different pwd and different address. this should really be a changed ufrag. Changing the pwd should never happen without a changed ufrag. https://codereview.webrtc.org/1248063002/diff/100001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1248063002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:670: // Network does not guarantee the order of the candidate delivery. If a does not -> may not
Thanks! https://codereview.webrtc.org/1248063002/diff/100001/talk/app/webrtc/webrtcse... File talk/app/webrtc/webrtcsession_unittest.cc (right): https://codereview.webrtc.org/1248063002/diff/100001/talk/app/webrtc/webrtcse... talk/app/webrtc/webrtcsession_unittest.cc:2617: // The third offer has a different pwd and different address. On 2015/07/31 05:55:21, juberti1 wrote: > this should really be a changed ufrag. Changing the pwd should never happen > without a changed ufrag. Done. https://codereview.webrtc.org/1248063002/diff/100001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1248063002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:670: // Network does not guarantee the order of the candidate delivery. If a On 2015/07/31 05:55:21, juberti1 wrote: > does not -> may not Done.
lgtm
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/1248063002/#ps120001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1248063002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1248063002/120001
Message was sent while issue was closed.
Committed patchset #4 (id:120001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/503726c3498201822079c5abe9e528498846c9f2 Cr-Commit-Position: refs/heads/master@{#9667} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
