[tor-bugs] #17568 [Tor Browser]: Clean up tor-control-port.js in Torbutton

[Tor Bug Tracker & Wiki]

#17568: Clean up tor-control-port.js in Torbutton
-------------------------------------------------+-------------------------
 Reporter:  gk                                   |          Owner:  tbb-
     Type:  task                                 |  team
 Priority:  Medium                               |         Status:
Component:  Tor Browser                          |  needs_review
 Severity:  Normal                               |      Milestone:
 Keywords:  tbb-torbutton,                       |        Version:
  TorBrowserTeam201512R                          |     Resolution:
Parent ID:                                       |  Actual Points:
  Sponsor:                                       |         Points:
-------------------------------------------------+-------------------------

Comment (by arthuredelstein):

 Replying to [comment:18 gk]:
 > Seems I forgot to mention that we don't need `"circ" :
 info.circuitStatusParser` either at the moment as it seems. :(

 Commented out for now.

 > One additional thing:
 > {{{
 > +  let dataText = message.match(/^650[ \+-]\S+?\s(.*)/m)[1];
 > +  return controlSocket.addNotificationCallback(new RegExp("^650[ \+-]"
 + type),
 > }}}
 > I think taking care of the `+`-case is fine but I wonder whether we need
 to do that given that we only watch for `STREAM` events (currently). If we
 want to cover the generic case here and not want to bother with the RegEx
 again in case we add more events to watch for later, then I wonder whether
 the RegEx is correct at all given e.g. `650+" Severity CRLF Data 650 SP
 "OK" CRLF` (you are checking only for one `\s` before trying to capture
 but you get `CRLF`).

 Good point. I have dropped the "650+" and "650-" cases for now, as we
 aren't using them.

 > Then there is still the `+`-case from '(the "?" modifier added to "+" or
 "*")' in comment:1, no? Could you make an argument for why we (still) need
 this?

 > And for the one after the * in
 string.match(/^250\+.+?=([\s\S]*?)^\.$/m), as well?

 In general, I think it is safer to use lazy rather than greedy
 quantifiers. At least it avoids errors where we accidentally match a
 second (or later) instance of something when we intend to match on the
 first instance.

 A couple of blog posts make this argument:
 * http://blog.stevenlevithan.com/archives/greedy-lazy-performance
 * https://blog.mariusschulz.com/2014/06/03/why-using-in-regular-
 expressions-is-almost-never-what-you-actually-want

 Here's the revised patch, for review:
 https://github.com/arthuredelstein/torbutton/commit/17568+3

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