[Cronet] Write effective experimental options to NetLog

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 dda9ce4852405a045032c2c610282049a4ab170b..35f2ddff8da0c5015f09fac8797bac466d675862 100644
--- a/components/cronet/android/cronet_url_request_context_adapter.cc
+++ b/components/cronet/android/cronet_url_request_context_adapter.cc
@@ -58,7 +58,6 @@
 #include "net/http/http_server_properties_manager.h"
 #include "net/log/file_net_log_observer.h"
 #include "net/log/net_log_util.h"
-#include "net/log/write_to_file_net_log_observer.h"
 #include "net/nqe/external_estimate_provider.h"
 #include "net/nqe/network_qualities_prefs_manager.h"
 #include "net/proxy/proxy_config_service_android.h"
@@ -506,7 +505,7 @@ CronetURLRequestContextAdapter::~CronetURLRequestContextAdapter() {
   }
 
   // Stop NetLog observer if there is one.
-  StopNetLogHelper();
+  StopNetLogOnNetworkThread();
 }
 
 void CronetURLRequestContextAdapter::InitRequestContextOnMainThread(
@@ -610,6 +609,7 @@ void CronetURLRequestContextAdapter::InitializeOnNetworkThread(
   DCHECK(GetNetworkTaskRunner()->BelongsToCurrentThread());
   DCHECK(!is_context_initialized_);
   DCHECK(proxy_config_service_);
+
   // TODO(mmenke):  Add method to have the builder enable SPDY.
   net::URLRequestContextBuilder context_builder;
 
@@ -648,6 +648,9 @@ void CronetURLRequestContextAdapter::InitializeOnNetworkThread(
                                             g_net_log.Get().net_log(),
                                             GetFileThread()->task_runner());
 
+  effective_experimental_options_ =
+      std::move(config->effective_experimental_options);
+
   // Set up pref file if storage path is specified.
   if (!config->storage_path.empty()) {
     base::FilePath storage_path(config->storage_path);
@@ -874,34 +877,17 @@ CronetURLRequestContextAdapter::GetNetworkTaskRunner() const {
   return network_thread_->task_runner();
 }
 
-bool CronetURLRequestContextAdapter::StartNetLogToFile(
+void CronetURLRequestContextAdapter::StartNetLogToFile(
     JNIEnv* env,
     const JavaParamRef<jobject>& jcaller,
     const JavaParamRef<jstring>& jfile_name,
     jboolean jlog_all) {
-  base::AutoLock lock(write_to_file_observer_lock_);
-  // Do nothing if already logging to a file.
-  if (write_to_file_observer_)
-    return true;
-  std::string file_name =
-      base::android::ConvertJavaStringToUTF8(env, jfile_name);
-  base::FilePath file_path(file_name);
-  base::ScopedFILE file(base::OpenFile(file_path, "w"));
-  if (!file) {
-    LOG(ERROR) << "Failed to open NetLog file for writing.";
-    return false;
-  }
-
-  write_to_file_observer_.reset(new net::WriteToFileNetLogObserver());
-  if (jlog_all == JNI_TRUE) {
-    write_to_file_observer_->set_capture_mode(
-        net::NetLogCaptureMode::IncludeSocketBytes());
-  }
-  write_to_file_observer_->StartObserving(
-      g_net_log.Get().net_log(), std::move(file),
-      /*constants=*/nullptr, /*url_request_context=*/nullptr);
-
-  return true;
+  PostTaskToNetworkThread(
+      FROM_HERE,
+      base::Bind(&CronetURLRequestContextAdapter::StartNetLogOnNetworkThread,
+                 base::Unretained(this),
+                 base::android::ConvertJavaStringToUTF8(env, jfile_name),
+                 jlog_all == JNI_TRUE));
 }
 
 void CronetURLRequestContextAdapter::StartNetLogToDisk(
@@ -922,7 +908,11 @@ void CronetURLRequestContextAdapter::StartNetLogToDisk(
 void CronetURLRequestContextAdapter::StopNetLog(
     JNIEnv* env,
     const JavaParamRef<jobject>& jcaller) {
-  StopNetLogHelper();
+  DCHECK(!GetNetworkTaskRunner()->BelongsToCurrentThread());
+  PostTaskToNetworkThread(
+      FROM_HERE,
+      base::Bind(&CronetURLRequestContextAdapter::StopNetLogOnNetworkThread,
+                 base::Unretained(this)));
 }
 
 void CronetURLRequestContextAdapter::GetCertVerifierData(
@@ -1009,6 +999,32 @@ void CronetURLRequestContextAdapter::OnThroughputObservation(
       (timestamp - base::TimeTicks::UnixEpoch()).InMilliseconds(), source);
 }
 
+void CronetURLRequestContextAdapter::StartNetLogOnNetworkThread(
+    const std::string& file_name,
+    bool include_socket_bytes) {
+  DCHECK(GetNetworkTaskRunner()->BelongsToCurrentThread());
+
+  // Do nothing if already logging to a file.
+  if (net_log_file_observer_)
+    return;
+  base::FilePath file_path(file_name);
+  base::ScopedFILE file(base::OpenFile(file_path, "w"));

[kapishnikov, 2017/03/14 14:37:09]

The same file can be opened for writing twice before the |file| variable goes
out of scope: once here, and once in
net::FileNetLogObserver::CreateUnbounded(...). I guess it is okay, just to
double check.

[xunjieli, 2017/03/14 19:03:06]

Yes, it's okay. But on second thought, let's move this back to the original
method and keep the method signature to return false upon open failure. Done.

+  if (!file) {
+    LOG(ERROR) << "Failed to open NetLog file for writing.";
+    return;
+  }
+
+  net_log_file_observer_ = net::FileNetLogObserver::CreateUnbounded(
+      GetFileThread()->task_runner(), file_path, /*constants=*/nullptr);
+  CreateNetLogEntriesForActiveObjects({context_.get()},
+                                      net_log_file_observer_.get());
+  net::NetLogCaptureMode capture_mode =
+      include_socket_bytes ? net::NetLogCaptureMode::IncludeSocketBytes()
+                           : net::NetLogCaptureMode::Default();
+  net_log_file_observer_->StartObserving(g_net_log.Get().net_log(),
+                                         capture_mode);
+}
+
 void CronetURLRequestContextAdapter::StartNetLogToBoundedFileOnNetworkThread(
     const std::string& dir_path,
     bool include_socket_bytes,
@@ -1016,34 +1032,37 @@ void CronetURLRequestContextAdapter::StartNetLogToBoundedFileOnNetworkThread(
   DCHECK(GetNetworkTaskRunner()->BelongsToCurrentThread());
 
   // Do nothing if already logging to a directory.
-  if (bounded_file_observer_)
+  if (net_log_file_observer_)
     return;
 
   // Filepath for NetLog files must exist and be writable.
   base::FilePath file_path(dir_path);
   DCHECK(base::PathIsWritable(file_path));
 
-  bounded_file_observer_ = net::FileNetLogObserver::CreateBounded(
+  net_log_file_observer_ = net::FileNetLogObserver::CreateBounded(
       GetFileThread()->task_runner(), file_path, size, kNumNetLogEventFiles,
       /*constants=*/nullptr);
 
   CreateNetLogEntriesForActiveObjects({context_.get()},
-                                      bounded_file_observer_.get());
+                                      net_log_file_observer_.get());
 
   net::NetLogCaptureMode capture_mode =
       include_socket_bytes ? net::NetLogCaptureMode::IncludeSocketBytes()
                            : net::NetLogCaptureMode::Default();
-  bounded_file_observer_->StartObserving(g_net_log.Get().net_log(),
+  net_log_file_observer_->StartObserving(g_net_log.Get().net_log(),
                                          capture_mode);
 }
 
-void CronetURLRequestContextAdapter::StopBoundedFileNetLogOnNetworkThread() {
+void CronetURLRequestContextAdapter::StopNetLogOnNetworkThread() {
   DCHECK(GetNetworkTaskRunner()->BelongsToCurrentThread());
-  bounded_file_observer_->StopObserving(
-      net::GetNetInfo(context_.get(), net::NET_INFO_ALL_SOURCES),
+
+  if (!net_log_file_observer_)
+    return;
+  net_log_file_observer_->StopObserving(
+      GetNetLogInfo(),
       base::Bind(&CronetURLRequestContextAdapter::StopNetLogCompleted,
                  base::Unretained(this)));
-  bounded_file_observer_.reset();
+  net_log_file_observer_.reset();
 }
 
 void CronetURLRequestContextAdapter::StopNetLogCompleted() {
@@ -1051,18 +1070,15 @@ void CronetURLRequestContextAdapter::StopNetLogCompleted() {
       base::android::AttachCurrentThread(), jcronet_url_request_context_.obj());
 }
 
-void CronetURLRequestContextAdapter::StopNetLogHelper() {
-  base::AutoLock lock(write_to_file_observer_lock_);
-  DCHECK(!(write_to_file_observer_ && bounded_file_observer_));
-  if (write_to_file_observer_) {
-    write_to_file_observer_->StopObserving(/*url_request_context=*/nullptr);
-    write_to_file_observer_.reset();
-  } else if (bounded_file_observer_) {
-    PostTaskToNetworkThread(FROM_HERE,
-                            base::Bind(&CronetURLRequestContextAdapter::
-                                           StopBoundedFileNetLogOnNetworkThread,
-                                       base::Unretained(this)));
+std::unique_ptr<base::DictionaryValue>
+CronetURLRequestContextAdapter::GetNetLogInfo() const {
+  std::unique_ptr<base::DictionaryValue> net_info =
+      net::GetNetInfo(context_.get(), net::NET_INFO_ALL_SOURCES);
+  if (effective_experimental_options_) {
+    net_info->Set("cronetExperimentalParams",
+                  effective_experimental_options_->CreateDeepCopy());
   }
+  return net_info;
 }
 
 // Create a URLRequestContextConfig from the given parameters.