[Cronet] Fix race with NetLogWithNetworkChangeEvents registration with NCN

components/cronet/android/cronet_url_request_context_adapter.cc

 // Copyright 2014 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 "components/cronet/android/cronet_url_request_context_adapter.h"
 
 #include <limits.h>
 #include <stddef.h>
 #include <stdint.h>
 
@@ skipping 46 matching lines @@
 
 #if defined(DATA_REDUCTION_PROXY_SUPPORT)
 #include "components/cronet/android/cronet_data_reduction_proxy.h"
 #endif
 
 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);
 };
 
 // Use a global NetLog instance. See crbug.com/486120.
 static base::LazyInstance<NetLogWithNetworkChangeEvents>::Leaky g_net_log =
     LAZY_INSTANCE_INITIALIZER;
 
 const char kHttpServerProperties[] = "net.http_server_properties";
 // Current version of disk storage.
@@ skipping 319 matching lines @@
   jcaller_ref.Reset(env, jcaller);
   proxy_config_service_ = net::ProxyService::CreateSystemProxyConfigService(
       GetNetworkTaskRunner(), nullptr /* Ignored on Android */);
   net::ProxyConfigServiceAndroid* android_proxy_config_service =
       static_cast<net::ProxyConfigServiceAndroid*>(proxy_config_service_.get());
   // If a PAC URL is present, ignore it and use the address and port of
   // Android system's local HTTP proxy server. See: crbug.com/432539.
   // 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,
                  base::Unretained(this), base::Passed(&context_config_),
                  jcaller_ref));
 }
 
 void CronetURLRequestContextAdapter::
     EnableNetworkQualityEstimatorOnNetworkThread(bool use_local_host_requests,
                                                  bool use_smaller_responses) {
@@ skipping 478 matching lines @@
     JNIEnv* env,
     const JavaParamRef<jclass>& jcaller) {
   base::StatisticsRecorder::Initialize();
   std::vector<uint8_t> data;
   if (!HistogramManager::GetInstance()->GetDeltas(&data))
     return ScopedJavaLocalRef<jbyteArray>();
   return base::android::ToJavaByteArray(env, &data[0], data.size());
 }
 
 }  // namespace cronet