[Cronet] Fix race with NetLogWithNetworkChangeEvents registration with NCN

components/cronet/android/cronet_url_request_context_adapter.cc

Index: components/cronet/android/cronet_url_request_context_adapter.cc
diff --git a/components/cronet/android/cronet_url_request_context_adapter.cc b/components/cronet/android/cronet_url_request_context_adapter.cc
index 1141e8b2cc0a90b18a593ecb41f478b52168e1a0..d9cb905ed9a181c1c1268479e68e616f8fe45cae 100644
--- a/components/cronet/android/cronet_url_request_context_adapter.cc
+++ b/components/cronet/android/cronet_url_request_context_adapter.cc
@@ -64,16 +64,26 @@ namespace {
 // This class wraps a NetLog that also contains network change events.
 class NetLogWithNetworkChangeEvents {
  public:
-  NetLogWithNetworkChangeEvents() : net_log_(), net_change_logger_(&net_log_) {}
+  NetLogWithNetworkChangeEvents() {}
 
   net::NetLog* net_log() { return &net_log_; }
+  // This function registers with the NetworkChangeNotifier and so must be
+  // called *after* the NetworkChangeNotifier is created. Should only be
+  // called on the UI thread as it is not thread-safe and the UI thread is

[xunjieli, 2016/06/24 19:13:03]

I think I understand it slightly better now. Thanks for the explanations!
Could you add a comment here to mention that it is not thread-safe to access
|net_change_logger_| because we might have multiple CronetEngine and hence
multiple network threads?

[pauljensen, 2016/06/27 11:49:43]

Done.

+  // the thread the NetworkChangeNotifier is created on.
+  void EnsureInitializedOnMainThread() {
+    DCHECK(base::MessageLoopForUI::IsCurrent());
+    if (net_change_logger_)
+      return;
+    net_change_logger_.reset(new net::LoggingNetworkChangeObserver(&net_log_));
+  }
 
  private:
   net::NetLog net_log_;
   // LoggingNetworkChangeObserver logs network change events to a NetLog.
   // This class bundles one LoggingNetworkChangeObserver with one NetLog,
   // so network change event are logged just once in the NetLog.
-  net::LoggingNetworkChangeObserver net_change_logger_;
+  std::unique_ptr<net::LoggingNetworkChangeObserver> net_change_logger_;
 
   DISALLOW_COPY_AND_ASSIGN(NetLogWithNetworkChangeEvents);
 };
@@ -413,6 +423,7 @@ void CronetURLRequestContextAdapter::InitRequestContextOnMainThread(
   // TODO(csharrison) Architect the wrapper better so we don't need to cast for
   // android ProxyConfigServices.
   android_proxy_config_service->set_exclude_pac_url(true);
+  g_net_log.Get().EnsureInitializedOnMainThread();

[xunjieli, 2016/06/24 13:49:53]

Why do we initialize the observer on the main thread? 
It seems that doing it in InitializeOnNetworkThread() also achieves the same
purpose. It is might better since the observer callbacks will be called on the
network thread so we do fewer things on the UI thread?

[pauljensen, 2016/06/24 14:09:43]

A few reasons:
1. so as to initialize it ASAP so we don't miss any events
2. so I don't have to make
NetLogWithNetworkChangeEvents::EnsureInitializedOnMainThread thread-safe because
it's called on only one thread as it stands now.  This better matches Chrome's
threading model which discourages thread-safe code:
http://dev.chromium.org/developers/design-documents/threading
3. network threads can come and go, but the NetLog lives forever, so I need the
logger to receive callbacks on a thread that lives forever, otherwise we'll
crash when the network thread goes away and we try and post to it from the NCN
4. when communicating with the NCN I prefer to use the UI thread as this is what
thread the NCN runs on, so I can possibly avoid some locking and races by
calling the NCN on the UI thread.

[xunjieli, 2016/06/24 16:20:11]

AddObserver is already thread safe. There isn't additional thing that is needed
to ensure thread safety.
That is fair, although we could remove observer when the network thread is torn
down.
io_thread.cc initializes the logging observer on the browser i/o thread instead
of on the UI thread. Maybe worth a comment here and/or in
logging_network_change_observer.h on which thread the observer should be created
on. 

When calling into NetLog, we will also try to grab a lock which is often held by
the network thread
(https://cs.chromium.org/chromium/src/net/log/net_log.cc?rcl=0&l=410). Acquiring
lock on the UI thread isn't a whole lot better than posting a task to the
network thread to add the event.

[pauljensen, 2016/06/24 19:03:36]

No, take a look at EnsureInitializedOnMainThread(), access to net_change_logger_
is not atomic.  Writing thread-safe functions is tricky and fraught with peril;
this is why the Chromium threading policy recommends against thread-safe code.
It's possible, but it would require the addition of thread-safe reference
counting which would be fairly ugly.
Are there restrictions about which thread the observer can be created on?  AFAIK
it can be created on any thread.
Posting tasks and taking tasks off the queue requires locking anyhow.  The
operations performed by the LoggingNetworkChangeObserver are so tiny and they
happen very very rarely (once every few minutes at best) so I don't think it's
worth trying to optimize given I don't see an easy way to do so without
involving thread-safe coding given the multi-threaded nature of the NetLog and
NCN.

   GetNetworkTaskRunner()->PostTask(
       FROM_HERE,
       base::Bind(&CronetURLRequestContextAdapter::InitializeOnNetworkThread,