Pass pref_task_runner through HttpServerPropertiesManager's cxtor so that tests could inject a Test…

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 11 matching lines @@
 #include "base/files/scoped_file.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/scoped_vector.h"
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/statistics_recorder.h"
 #include "base/single_thread_task_runner.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "base/values.h"
 #include "components/cronet/android/cert/cert_verifier_cache_serializer.h"
 #include "components/cronet/android/cert/proto/cert_verification.pb.h"
 #include "components/cronet/histogram_manager.h"
 #include "components/cronet/url_request_context_config.h"
 #include "components/prefs/pref_change_registrar.h"
 #include "components/prefs/pref_filter.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
@@ skipping 544 matching lines @@
     PrefServiceFactory factory;
     factory.set_user_prefs(json_pref_store_);
     scoped_refptr<PrefRegistrySimple> registry(new PrefRegistrySimple());
     registry->RegisterDictionaryPref(kHttpServerProperties,
                                      new base::DictionaryValue());
     pref_service_ = factory.Create(registry.get());
 
     std::unique_ptr<net::HttpServerPropertiesManager>
         http_server_properties_manager(new net::HttpServerPropertiesManager(
             new PrefServiceAdapter(pref_service_.get()),
-            GetNetworkTaskRunner()));
+            base::ThreadTaskRunnerHandle::Get(), GetNetworkTaskRunner()));

[xunjieli, 2016/12/14 01:47:11]

Hmm.. This method is invoked on the network thread.
base::ThreadTaskRunnerHandle::Get() is essentially GetNetworkTaskRunner(). I
know this CL doesn't introduce any functional change, but my question is what
should we use for |pref_task_runner_|? Is it bad for it to be the same as
|network_task_runner_|?

[Zhongyi Shi, 2016/12/14 04:20:43]

Yeah, this CL shouldn't introduce functional change. If I read the code
correctly (you might also want to confirm), in src/chrome/browser, the
HttpServerPropertiesManager is created by ProfileImplIOData::Handle::Init where
it should be running on the BrowserThread::UI thread. I agreed that
base::ThreaTaskRunnerHanlde::Get() is actually getting a network task runner as
it's running on the network thread. Do you think this is a bug? 

[xunjieli, 2016/12/14 18:49:38]

I think the bug might be in HttpServerPropertiesManager. It's not clear to me
why HttpServerPropertiesManager needs to receive notifications on UI thread.
Maybe the PrefService has such limitation? I found one line on thread safety in
pref_value_store.h but not anywhere else. It will be good to find out why we
have this separation (pref_task_runner_ vs network_task_runner_).

I talked to Bence offline as well. He wasn't sure about reason behind this
setup.

In Cronet, this will basically mean that we are posting tasks from the network
thread to itself and back, which seems wasteful (but not dangerous).

However, in Chrome, if this class starts to post too many tasks to the
pref_task_runner_, we will be causing UI jank which seems dangerous and
possible. 

Since this CL introduces no functional change, lgtm.

[Zhongyi Shi, 2016/12/14 19:47:24]

If you look at the HttpServerPropertiesManager documentation, it interacts with
pref thread(on desktop, it's the UI thread) and network thread. I have also
updated the cl description explaining why there're two task_runners in this
class. 

However, note it's also required that "HttpServerPropertiesManager is
constructed by on the pref thread, to set up |pref_task_runner_| and the prefs
listeners". In cronet case, it's just all handed to networking thread and all
the data are accessible by the networking thread. I talked to Ryan offline, and
he felt it should be okay. 
We used timer to schedule posts every 60s so that we don't post too often.
Agreed that this CL shouldn't be blocked by the discussion here. It might
get your attention that in cronet, network_task_runner and pref_task_runner
are the same though. 

     http_server_properties_manager->InitializeOnNetworkThread();
     http_server_properties_manager_ = http_server_properties_manager.get();
     context_builder.SetHttpServerProperties(
         std::move(http_server_properties_manager));
   }
 
   // Explicitly disable the persister for Cronet to avoid persistence of dynamic
   // HPKP. This is a safety measure ensuring that nobody enables the persistence
   // of HPKP by specifying transport_security_persister_path in the future.
   context_builder.set_transport_security_persister_path(base::FilePath());
@@ skipping 486 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