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

[Tor Bug Tracker & Wiki]

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

 * status:  merge_ready => closed
 * resolution:   => fixed


Comment:

 Replying to [comment:7 dcf]:
 > I think the go-webrtc change is fine as well.

 Merged as https://github.com/keroserene/go-
 webrtc/commit/68a6fb1b4353657c205bd7f6eba67630a5702bf2

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

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

[Tor Bug Tracker & Wiki]

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

 * status:  needs_review => merge_ready


Comment:

 I think the go-webrtc change is fine as well.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:
-+--

Old description:

> This is a follow up from #25688
>
> https://github.com/keroserene/snowflake/compare/ice
>
> https://github.com/keroserene/go-
> webrtc/commit/f0f4f0a1a37c432619370035c0596b8094d6cbe8

New description:

 This is a follow up from #25688

 https://github.com/keroserene/snowflake/compare/ice

 https://github.com/keroserene/go-webrtc/pull/106

--

Comment (by arlolra):

 Replying to [comment:5 cohosh]:
 > Otherwise the patches look good to me.

 Thanks, I merged the snowflake changes in,
 https://gitweb.torproject.org/pluggable-
 transports/snowflake.git/commit/?id=d7676d2b9e45c957bf39ea2d5662996aeab5b4de
 https://gitweb.torproject.org/pluggable-
 transports/snowflake.git/commit/?id=5380aaca8c1c789fa4f692e93dfdc4e59d4992af

 For go-webrtc, I wasn't sure if you all looked at that yet and since it's
 a backwards incompatible change, I'll leave some more time.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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 cohosh):

 Also noting that with the current implementation, `pc.CreateAnswer` won't
 block indefinitely. It will time out and return `nil` after 3 seconds:
 [https://github.com/keroserene/go-
 webrtc/blob/a1272c08ab1d5ca154c6794ddc5f73d2e576fe1b/peerconnection.cc#L355
 peerconnection.cc#L355].

 It might be better practice to include our own timeout and not rely on the
 underlying implementation, at the very least we should document that this
 method times out since right now the documentation only mentions that it
 is blocking: https://godoc.org/github.com/keroserene/go-
 webrtc#PeerConnection.CreateAnswer

 I left the [https://github.com/cohosh/snowflake/blob/master/proxy-
 go/snowflake.go#L317 comment] to note that we might want to change this
 later.

 Otherwise the patches look good to me.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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 dcf):

 I'm trying to understand the change around `pc.CreateAnswer`:
 https://github.com/keroserene/snowflake/compare/ice#diff-
 e32e5d12043e17d487f1252ee61dfd5fL167

 Was it always unnecessary to run `pc.CreateAnswer()` in a goroutine? Is
 the removal of `errChan` and `answerChan` a separate cleanup, or is it
 necessitated by the removal of `OnIceComplete`?

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:
-+--
Changes (by arlolra):

 * status:  new => needs_review


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

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

[Tor Bug Tracker & Wiki]

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

 * cc: arlolra, dcf (added)


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

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

[Tor Bug Tracker & Wiki]

#30511: Remove OnIceComplete
-+
 Reporter:  arlolra  |  Owner:  (none)
 Type:  defect   | Status:  new
 Priority:  Low  |  Milestone:
Component:  Circumvention/Snowflake  |Version:
 Severity:  Normal   |   Keywords:
Actual Points:   |  Parent ID:
   Points:   |   Reviewer:
  Sponsor:   |
-+
 This is a follow up from #25688

 https://github.com/keroserene/snowflake/compare/ice

 https://github.com/keroserene/go-
 webrtc/commit/f0f4f0a1a37c432619370035c0596b8094d6cbe8

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs