[tor-bugs] #25688 [Obfuscation/Snowflake]: proxy-go is still deadlocking occasionally

[Tor Bug Tracker & Wiki]

#25688: proxy-go is still deadlocking occasionally
--------------------------------------------+------------------------------
 Reporter:  dcf                             |          Owner:  cohosh
     Type:  defect                          |         Status:
                                            |  needs_revision
 Priority:  Low                             |      Milestone:
Component:  Obfuscation/Snowflake           |        Version:
 Severity:  Normal                          |     Resolution:
 Keywords:  network-team-roadmap-2019-Q1Q2  |  Actual Points:
Parent ID:                                  |         Points:  3
 Reviewer:                                  |        Sponsor:
--------------------------------------------+------------------------------

Comment (by dcf):

 Replying to [comment:11 cohosh]:
 > This is likely the cause of the problem. OnICEComplete is fired when the
 ICEGatheringState is  complete:

 Great analysis!

 > {{{
 >         case _, ok := <-answerChan:
 >                 if !ok {
 >                         pc.Destroy()
 >                         return nil, fmt.Errorf("Failed gathering ICE
 candidates.")
 >                 }
 >         }
 >         return pc, nil
 > }}}

 This code is weird... At first I saw the `answerChan <- struct{}{}` and I
 thought, this channel is only used to send a single signal; maybe this
 would be better as `close(answerChan)` as in
 [https://blog.golang.org/pipelines#TOC_6. this example]. But then I don't
 understand why it's checking `ok` from the channel read--that seems to
 indicate that it's trying to distinguish a send from a channel close...
 But the code only uses one of those two possibilities.

 Replying to [comment:15 arlolra]:
 > > This error appears to occur *after* OnIceComplete() is fired, meaning
 that in the proxy-go code:
 >
 > Ah, so it is a bug in go-webrtc to assume `iceComplete`, when only
 `iceGathering` has completed,
 > https://github.com/keroserene/go-
 webrtc/blob/master/peerconnection.go#L518-L522

 Yes, I think you're right.

 > > It's also worth pointing out that apparently OnICECompletel is being
 deprecated (​peerconnection.go#L518) we are better off not relying on it
 anyway.
 >
 > If that's the case, we should remove it there too.

 Agreed.

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