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

chrome/browser/chromeos/web_socket_proxy_controller.cc

Index: chrome/browser/chromeos/web_socket_proxy_controller.cc
diff --git a/chrome/browser/chromeos/web_socket_proxy_controller.cc b/chrome/browser/chromeos/web_socket_proxy_controller.cc
index 5436afa0e0622ece2671d31bdaa20d6fa056dc64..80a19919359a8905eb524df2106e0dc35fb178d8 100644
--- a/chrome/browser/chromeos/web_socket_proxy_controller.cc
+++ b/chrome/browser/chromeos/web_socket_proxy_controller.cc
@@ -24,6 +24,7 @@
 #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"
@@ -99,18 +100,34 @@ 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 {
@@ -121,7 +138,10 @@ class ProxyLifetime : public net::NetworkChangeNotifier::OnlineStateObserver {
   }
 
   // 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_;
@@ -183,6 +203,14 @@ bool WebSocketProxyController::IsInitiated() {
 }
 
 // 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";