Address review comments from @iefremov

browser/tor/tor_profile_service_impl.cc

@@ -46,19 +46,13 @@ void TorProfileServiceImpl::ReLaunchTor(const TorConfig& config) {
   tor_launcher_factory_->ReLaunchTorProcess(config);
 }
 
-void TorProfileServiceImpl::OnSetNewTorCircuitComplete(bool success) {
-  if (tor_circuit_callback_)
-    std::move(tor_circuit_callback_).Run(success);
-}
-
 void TorProfileServiceImpl::OnProxyLookupComplete(
     int32_t net_error,
     const base::Optional<net::ProxyInfo>& proxy_info) {
   bool success = proxy_info.has_value() && !proxy_info->is_direct();
-  if (tor_circuit_callback_) {
-    std::move(tor_circuit_callback_).Run(success);
-    binding_.Close();
-  }
+  DCHECK(tor_circuit_callback_);
+  std::move(tor_circuit_callback_).Run(success);
+  binding_.Close();
 }
 
 void TorProfileServiceImpl::SetNewTorCircuit(const GURL& request_url,

browser/tor/tor_profile_service_impl.h

@@ -57,8 +57,6 @@ class TorProfileServiceImpl : public TorProfileService,
       const base::Optional<net::ProxyInfo>& proxy_info) override;
 
  private:
-  void OnSetNewTorCircuitComplete(bool success);
-
   Profile* profile_;  // NOT OWNED
   TorLauncherFactory* tor_launcher_factory_;  // Singleton
   NewTorCircuitCallback tor_circuit_callback_;

chromium_src/net/proxy_resolution/proxy_resolution_service.cc

@@ -43,6 +43,12 @@ void SetTorCircuitIsolation(const ProxyConfig& config,
   DCHECK(IsTorProxy(config));
   std::string proxy_uri = config.proxy_rules().single_proxies.Get().ToURI();
 
+  // Adding username & password to global sock://127.0.0.1:[port] config without
+  // actually modifying it when resolving proxy for each url.
+  // username is derived from url and we keep password for 10 minutes, detail
+  // encapsulated in ProxyConfigServiceTor.
+  // TorProxyMap stores username/password mapping and it can only be manipulated
+  // by ProxyConfigServiceTor.
   ProxyConfigServiceTor tor_proxy_config_service(proxy_uri);
   ProxyConfigWithAnnotation fetched_config;
   tor_proxy_config_service.GetLatestProxyConfig(&fetched_config);

common/tor/tor_proxy_uri_helper.cc

@@ -10,19 +10,27 @@
 namespace tor {
 
 std::string GetTorProxyURI() {
+  const std::string TorProxyScheme("socks5://");
+  const std::string TorProxyAddress("127.0.0.1");
+  std::string port;
   switch (chrome::GetChannel()) {
     case version_info::Channel::STABLE:
-      return std::string("socks5://127.0.0.1:9350");
+      port = std::string("9350");
+      break;
     case version_info::Channel::BETA:
-      return std::string("socks5://127.0.0.1:9360");
+      port = std::string("9360");
+      break;
     case version_info::Channel::DEV:
-      return std::string("socks5://127.0.0.1:9370");
+      port = std::string("9370");
+      break;
     case version_info::Channel::CANARY:
-      return std::string("socks5://127.0.0.1:9380");
+      port = std::string("9380");
+      break;
     case version_info::Channel::UNKNOWN:
     default:
-      return std::string("socks5://127.0.0.1:9390");
+      port = std::string("9390");
   }
+  return std::string(TorProxyScheme + TorProxyAddress + ":" + port);
 }
 
 }  // namespace tor

net/proxy_resolution/proxy_config_service_tor_unittest.cc

@@ -123,7 +123,7 @@ TEST_F(ProxyConfigServiceTorTest, SetUsername) {
   EXPECT_EQ(host_port.port(), 5566);
   EXPECT_EQ(host_port.username(), isolation_key);
 
-  // persistent circuit isolation until timeout
+  // Test persistent circuit isolation until timeout.
   std::string password = host_port.password();
   EXPECT_FALSE(host_port.password().empty());
   proxy_config_service.SetUsername(isolation_key, GetTorProxyMap());
@@ -133,7 +133,7 @@ TEST_F(ProxyConfigServiceTorTest, SetUsername) {
   host_port = server.host_port_pair();
   EXPECT_EQ(host_port.password(), password);
 
-  // New identity
+  // Test new identity.
   GetTorProxyMap()->Erase(isolation_key);
   proxy_config_service.SetUsername(isolation_key, GetTorProxyMap());
   proxy_config_service.GetLatestProxyConfig(&config);

net/proxy_resolution/proxy_resolution_service_unittest.cc

@@ -23,33 +23,36 @@ namespace net {
 
 class ProxyResolutionServiceTest : public TestWithScopedTaskEnvironment {
  public:
-  ProxyResolutionServiceTest() {}
-  ~ProxyResolutionServiceTest() override {}
+  ProxyResolutionServiceTest() = default;
+  ~ProxyResolutionServiceTest() override = default;
+
+  void SetUp() override {
+    const std::string proxy_uri("socks5://127.0.0.1:5566");
+    service_ = std::make_unique<ProxyResolutionService>(
+        std::make_unique<ProxyConfigServiceTor>(proxy_uri),
+        std::make_unique<MockAsyncProxyResolverFactory>(false), nullptr);
+  }
+
+  ProxyResolutionService* GetProxyResolutionService() { return service_.get(); }
 
  private:
+  std::unique_ptr<ProxyResolutionService> service_;
   DISALLOW_COPY_AND_ASSIGN(ProxyResolutionServiceTest);
 };
 
 TEST_F(ProxyResolutionServiceTest, TorProxy) {
-  MockAsyncProxyResolverFactory* factory =
-      new MockAsyncProxyResolverFactory(false);
-  std::string proxy_uri("socks5://127.0.0.1:5566");
-  ProxyResolutionService service(
-      std::make_unique<ProxyConfigServiceTor>(proxy_uri),
-      base::WrapUnique(factory), nullptr);
-
-  GURL site_url("https://check.torproject.org/");
-  std::string isolation_key =
+  ProxyResolutionService* service = GetProxyResolutionService();
+  const GURL site_url("https://check.torproject.org/");
+  const std::string isolation_key =
       ProxyConfigServiceTor::CircuitIsolationKey(site_url);
 
   ProxyInfo info;
   TestCompletionCallback callback;
   BoundTestNetLog log;
   std::unique_ptr<ProxyResolutionService::Request> request;
-  int rv = service.ResolveProxy(site_url, std::string(), &info,
-                                callback.callback(), &request, log.bound());
+  int rv = service->ResolveProxy(site_url, std::string(), &info,
+                                 callback.callback(), &request, log.bound());
   EXPECT_THAT(rv, IsOk());
-  EXPECT_TRUE(factory->pending_requests().empty());
 
   EXPECT_TRUE(info.is_socks());
   ASSERT_FALSE(info.proxy_list().IsEmpty());
@@ -64,32 +67,26 @@ TEST_F(ProxyResolutionServiceTest, TorProxy) {
 }
 
 TEST_F(ProxyResolutionServiceTest, NewTorCircuit) {
-  MockAsyncProxyResolverFactory* factory =
-      new MockAsyncProxyResolverFactory(false);
-  std::string proxy_uri("socks5://127.0.0.1:5566");
-  ProxyResolutionService service(
-      std::make_unique<ProxyConfigServiceTor>(proxy_uri),
-      base::WrapUnique(factory), nullptr);
-
-  GURL site_url("https://check.torproject.org/");
-  std::string isolation_key =
+  ProxyResolutionService* service = GetProxyResolutionService();
+  const GURL site_url("https://check.torproject.org/");
+  const std::string isolation_key =
       ProxyConfigServiceTor::CircuitIsolationKey(site_url);
 
   ProxyInfo info;
   TestCompletionCallback callback;
   BoundTestNetLog log;
   std::unique_ptr<ProxyResolutionService::Request> request;
-  int rv = service.ResolveProxy(site_url, std::string(), &info,
-                                callback.callback(), &request, log.bound());
+  int rv = service->ResolveProxy(site_url, std::string(), &info,
+                                 callback.callback(), &request, log.bound());
 
   ProxyServer server = info.proxy_list().Get();
   HostPortPair host_port = server.host_port_pair();
   EXPECT_FALSE(host_port.password().empty());
   std::string password = host_port.password();
 
   GURL site_url_ref("https://check.torproject.org/#NewTorCircuit");
-  rv = service.ResolveProxy(site_url_ref, std::string(), &info,
-                            callback.callback(), &request, log.bound());
+  rv = service->ResolveProxy(site_url_ref, std::string(), &info,
+                             callback.callback(), &request, log.bound());
 
   server = info.proxy_list().Get();
   host_port = server.host_port_pair();