[Cronet] Set up HttpServerPropertiesManager

net/url_request/url_request_context_builder.cc

Index: net/url_request/url_request_context_builder.cc
diff --git a/net/url_request/url_request_context_builder.cc b/net/url_request/url_request_context_builder.cc
index 771bc3f25f28e91c929b3e60c4baae85ce8ca54b..3598cce1354f98b4306e77ba2bfbaa14f7c3ba2b 100644
--- a/net/url_request/url_request_context_builder.cc
+++ b/net/url_request/url_request_context_builder.cc
@@ -26,6 +26,7 @@
 #include "net/http/http_network_layer.h"
 #include "net/http/http_network_session.h"
 #include "net/http/http_server_properties_impl.h"
+#include "net/http/http_server_properties_manager.h"
 #include "net/http/transport_security_persister.h"
 #include "net/http/transport_security_state.h"
 #include "net/ssl/channel_id_service.h"
@@ -150,8 +151,23 @@ class BasicURLRequestContext : public URLRequestContext {
     transport_security_persister = transport_security_persister.Pass();
   }
 
+  // Sets a HttpServerPropertiesManager. Note that the manager must be
+  // initialized prior to this call.
+  void set_http_server_properties_manager(
+      scoped_ptr<HttpServerPropertiesManager> http_server_properties_manager) {
+    http_server_properties_manager_ = http_server_properties_manager.Pass();
+    set_http_server_properties(http_server_properties_manager_->GetWeakPtr());
+  }
+
  protected:
-  ~BasicURLRequestContext() override { AssertNoURLRequests(); }
+  ~BasicURLRequestContext() override {
+    AssertNoURLRequests();
+    // Shut down the |http_server_properties_manager_| if there is one, since
+    // this is the only place in BasicURLRequestContext to do so.
+    // Note that the shutdown is performed on the network thread.
+    if (http_server_properties_manager_)
+      http_server_properties_manager_->ShutdownOnPrefThread();

[mmenke, 2015/07/07 20:28:12]

Isn't the pref thread actually the file thread?  If so, we're calling this on
the wrong thread.

Solution seems to be to have Cronet keep around a pointer to the manager, and
take care of that before shutting down the context.

Think the order would have to be:

* Post task to do that on the pref (file) thread
* Wait for that task to complete
* Post task to shut down and destroy the context
* Stop the network thread (Which implicitly waits for the previous event to
complete)
* Stop the file thread.

[xunjieli, 2015/07/08 20:07:22]

Why does the pref thread need to be the file thread? It can also be the network
thread, right? The pref thread just needs to do scheduling and listen for pref
updates. It doesn't do disk i/o. Sorry, I tried reading the code, but I don't
really understand the threading requirement. Could you explain a bit more on why
the pref thread is actually the file thread?

[mmenke, 2015/07/08 20:21:48]

I'm not sure.  The pref code doesn't say much about threading.  I had assumed it
was the thread we pass into PrefStore's constructor, I may be wrong.

Regardless, I'd rather not make URLRequestContextBuilder dependent on
HttpServerProperties rather than HttpServerPropertiesManager.  Disallowing use
of HttpServerPropertiesImpl in the interface seems a bit weird, and unnecessary.

[xunjieli, 2015/07/09 15:51:02]

Done. I made the builder take in a weak ptr of the parent class, and manage the
shutdown on cronet side. PTAL.

+  }
 
  private:
   // The thread should be torn down last.
@@ -160,6 +176,7 @@ class BasicURLRequestContext : public URLRequestContext {
 
   URLRequestContextStorage storage_;
   scoped_ptr<TransportSecurityPersister> transport_security_persister_;
+  scoped_ptr<HttpServerPropertiesManager> http_server_properties_manager_;
 
   DISALLOW_COPY_AND_ASSIGN(BasicURLRequestContext);
 };
@@ -243,6 +260,11 @@ void URLRequestContextBuilder::SetFileTaskRunner(
   file_task_runner_ = task_runner;
 }
 
+void URLRequestContextBuilder::SetHttpServerPropertiesManager(
+    scoped_ptr<HttpServerPropertiesManager> http_server_properties_manager) {
+  http_server_properties_manager_ = http_server_properties_manager.Pass();
+}
+
 URLRequestContext* URLRequestContextBuilder::Build() {
   BasicURLRequestContext* context =
       new BasicURLRequestContext(file_task_runner_);
@@ -326,8 +348,14 @@ URLRequestContext* URLRequestContextBuilder::Build() {
                                            false)));
   }
 
-  storage->set_http_server_properties(
-      scoped_ptr<HttpServerProperties>(new HttpServerPropertiesImpl()));
+  if (http_server_properties_manager_) {
+    context->set_http_server_properties_manager(
+        http_server_properties_manager_.Pass());
+  } else {
+    storage->set_http_server_properties(
+        scoped_ptr<HttpServerProperties>(new HttpServerPropertiesImpl()));
+  }
+
   storage->set_cert_verifier(CertVerifier::CreateDefault());
 
   if (throttling_enabled_)