[tor-bugs] #30511 [Circumvention/Snowflake]: Remove OnIceComplete

[Tor Bug Tracker & Wiki]

#30511: Remove OnIceComplete
-------------------------------------+------------------------------
 Reporter:  arlolra                  |          Owner:  (none)
     Type:  defect                   |         Status:  needs_review
 Priority:  Low                      |      Milestone:
Component:  Circumvention/Snowflake  |        Version:
 Severity:  Normal                   |     Resolution:
 Keywords:                           |  Actual Points:
Parent ID:                           |         Points:
 Reviewer:                           |        Sponsor:
-------------------------------------+------------------------------

Comment (by arlolra):

 Bear in mind that this is just a port of an already merged patch (see the
 inline comments there),
 https://github.com/keroserene/snowflake/commit/c28c8ca489633aae2d9b9dbea0e781ca5e44cc66

 `pc.CreateAnswer()` is a blocking operation, which is probably why it was
 put in a goroutine.  However, `makePeerConnectionFromOffer` falsely
 assumed that if `OnIceComplete` was called, it was ready to move on to
 sending the answer.  That setup a race between getting the local
 description in `sendAnswer` and setting it in the goroutine, which was
 never great.  Not to mention that there was plenty of time for ice to fail
 after the gathering stage completed, which resulted in the deadlock.

 Since `makePeerConnectionFromOffer` always blocked waiting on the
 channels, it isn't a significant change to remove the goroutine since, as
 the comment in the commit states,

   // ... we need
   // SetLocalDescription(answer) to be called before sendAnswer

 It might be worth looking at whether `runSession` deserves to be called
 asynchronously, but that seems orthogonal to the change here.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/30511#comment:4>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online