websocket-to-TCP proxy: observe value of listening port in right place.

chrome/browser/chromeos/web_socket_proxy_controller.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/web_socket_proxy_controller.h"
 
 #include <algorithm>
 
 #include <netinet/in.h>
 #include <sys/wait.h>
 #include <unistd.h>
 
 #include "base/command_line.h"
 #include "base/lazy_instance.h"
 #include "base/message_loop.h"
 #include "base/string_tokenizer.h"
 #include "base/threading/thread.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/web_socket_proxy.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/url_constants.h"
 #include "content/browser/browser_thread.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
+#include "content/public/browser/notification_service.h"
 #include "content/public/common/url_constants.h"
 #include "googleurl/src/gurl.h"
 #include "net/base/network_change_notifier.h"
 
 namespace {
 
 const char* kAllowedIds[] = {
     "haiffjcadagjlijoggckpgfnoeiflnem",
     "gnedhmakppccajfpfiihfcdlnpgomkcf",
     "fjcibdnjlbfnbfdjneajpipnlcppleek",
@@ skipping 55 matching lines @@
   std::vector<std::string> allowed_ids_;
   std::vector<std::string> allowed_origins_;
 };
 
 base::LazyInstance<OriginValidator> g_validator(base::LINKER_INITIALIZED);
 
 class ProxyTask : public Task {
   virtual void Run() OVERRIDE;
 };
 
-class ProxyLifetime : public net::NetworkChangeNotifier::OnlineStateObserver {
+class ProxyLifetime
+    : public net::NetworkChangeNotifier::OnlineStateObserver,
+      public content::NotificationObserver {
  public:
-  ProxyLifetime() : delay_ms_(1000), shutdown_requested_(false) {
+  ProxyLifetime() : delay_ms_(1000), port_(-1), shutdown_requested_(false) {
     BrowserThread::PostTask(
         BrowserThread::WEB_SOCKET_PROXY, FROM_HERE, new ProxyTask());
     net::NetworkChangeNotifier::AddOnlineStateObserver(this);
+    registrar_.Add(
+        this, chrome::NOTIFICATION_WEB_SOCKET_PROXY_STARTED,
+        content::NotificationService::AllSources());
   }
 
   virtual ~ProxyLifetime() {
     net::NetworkChangeNotifier::RemoveOnlineStateObserver(this);
   }
 
+  virtual void Observe(int type, const content::NotificationSource& source,
+                       const content::NotificationDetails& details) OVERRIDE {
+    base::AutoLock alk(lock_);
+    port_ = *content::Details<int>(details).ptr();
+  }
+
+  int GetPort() {
+    base::AutoLock alk(lock_);
+    return port_;

[miket_OOO, 2011/10/24 17:50:46]

I can't say that synchronizing an int getter/setter is unnecessary... but I'd
sure like to.

[Denis Lagno, 2011/10/24 19:47:58]

It should be infrequent operation, it is called only when authorized extension
makes an attempt to establish connection, so it's probably uncontended and its
price is negligible.

[miket_OOO, 2011/10/24 19:53:00]

It's fine, and the code is correct. I'm just thinking out loud about C++'s
nonexistent thread support...

+  }
+
  private:
   // net::NetworkChangeNotifier::OnlineStateObserver overrides.
   virtual void OnOnlineStateChanged(bool online) OVERRIDE {
     DCHECK(chromeos::WebSocketProxyController::IsInitiated());
     base::AutoLock alk(lock_);
     if (server_)
       server_->OnNetworkChange();
   }
 
   // Delay between next attempt to run proxy.
-  int delay_ms_;
+  int volatile delay_ms_;
+
+  // Proxy listens incoming websocket connections on this port.

[miket_OOO, 2011/10/24 17:50:46]

"listens for incoming"

[Denis Lagno, 2011/10/24 19:47:58]

Done.

+  int volatile port_;
 
   chromeos::WebSocketProxy* volatile server_;
   volatile bool shutdown_requested_;
   base::Lock lock_;
   content::NotificationRegistrar registrar_;
   friend class ProxyTask;
   friend class chromeos::WebSocketProxyController;
 };
 
 base::LazyInstance<ProxyLifetime> g_proxy_lifetime(base::LINKER_INITIALIZED);
@@ skipping 41 matching lines @@
   LOG(INFO) << "WebSocketProxyController initiation";
   g_proxy_lifetime.Get();
 }
 
 // static
 bool WebSocketProxyController::IsInitiated() {
   return !(g_proxy_lifetime == NULL);
 }
 
 // static
+int WebSocketProxyController::GetPort() {
+  if (IsInitiated())
+    return g_proxy_lifetime.Get().GetPort();
+  else
+    return -1;

[miket_OOO, 2011/10/24 17:50:46]

Is this case a run-time error or a logic error? Can you DCHECK that GetPort() is
called only when IsInitiated()?

[Denis Lagno, 2011/10/24 19:47:58]

I've changed the code.

Generally we do not want to start proxy (Initiate) if just random unauthorized
extension makes an attempt to call this API.

+}
+
+// static
 void WebSocketProxyController::Shutdown() {
   if (IsInitiated()) {
     LOG(INFO) << "WebSocketProxyController shutdown";
     base::AutoLock alk(g_proxy_lifetime.Get().lock_);
     g_proxy_lifetime.Get().shutdown_requested_ = true;
     if (g_proxy_lifetime.Get().server_)
       g_proxy_lifetime.Get().server_->Shutdown();
   }
 }
 
 // static
 bool WebSocketProxyController::CheckCredentials(
     const std::string& extension_id,
     const std::string& hostname,
     unsigned short port,
     ConnectionFlags flags) {
   return g_validator.Get().CheckCredentials(
       extension_id, hostname, port, flags);
 }
 
 }  // namespace chromeos