Move initialization of ChromeURLRequestContexts to the IO thread.

Before, these URLRequestContexts were lazily created from the UI thread. Unfortunately that model made it easy for consumers on the UI thread to poke at stuff which was being used from the IO thread, and introduce races.

So instead of providing a URLRequestContext*, the Profile now vends a URLRequestContextGetter*. 

The consequence of this is:

* Consumers on the UI thread can no longer get access to a URLRequestContext.
* Consumers on the IO thread need to call URLRequestContextGetter::GetURLRequestContext() to get at the context. This uses the same style lazy-creation of URLRequestContexts, albeit from the IO thread.

OK, so now the smelly part:

There were a couple of consumers of URLRequestContext on the UI thread that can't easily be moved to the IO thread -- these are the consumers of the cookie store. Before they could happily mess with the cookie store from the UI thread, and this was fine since CookieStore is threadsafe. However under the new model, they have no way to get at the URLRequestContext from the UI thread, hence can't get a pointer to the cookie store.

To support that use-cases, I bastardized the API some by adding a URLRequestContextGetter::GetCookieStore() method that lets UI thread consumers get a pointer to the cookie store, since we know this particular cross-thread usage is safe.

BUG=http://crbug.com/22294

Review URL: http://codereview.chromium.org/258008

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@29880 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 118 insertions, 53 deletions

diff --git a/chrome/browser/sync/glue/http_bridge.cc b/chrome/browser/sync/glue/http_bridge.cc
index dfda3b5..3448755 100644
--- a/chrome/browser/sync/glue/http_bridge.cc
+++ b/chrome/browser/sync/glue/http_bridge.cc
@@ -21,24 +21,46 @@
 
 namespace browser_sync {
 
+HttpBridge::RequestContextGetter::RequestContextGetter(
+    URLRequestContextGetter* baseline_context_getter)
+    : baseline_context_getter_(baseline_context_getter) {
+}
+
+URLRequestContext* HttpBridge::RequestContextGetter::GetURLRequestContext() {
+  // Lazily create the context.
+  if (!context_) {
+    URLRequestContext* baseline_context =
+        baseline_context_getter_->GetURLRequestContext();
+    context_ = new RequestContext(baseline_context);
+    baseline_context_getter_ = NULL;
+  }
+
+  // Apply the user agent which was set earlier.
+  if (is_user_agent_set())
+    context_->set_user_agent(user_agent_);
+
+  return context_;
+}
+
 HttpBridgeFactory::HttpBridgeFactory(
-    URLRequestContext* baseline_context) {
-  DCHECK(baseline_context != NULL);
-  request_context_ = new HttpBridge::RequestContext(baseline_context);
-  request_context_->AddRef();
+    URLRequestContextGetter* baseline_context_getter) {
+  DCHECK(baseline_context_getter != NULL);
+  request_context_getter_ =
+      new HttpBridge::RequestContextGetter(baseline_context_getter);
+  request_context_getter_->AddRef();
 }
 
 HttpBridgeFactory::~HttpBridgeFactory() {
-  if (request_context_) {
-    // Clean up request context on IO thread.
+  if (request_context_getter_) {
+    // Clean up request context getter on IO thread.
     ChromeThread::GetMessageLoop(ChromeThread::IO)->ReleaseSoon(FROM_HERE,
-        request_context_);
-    request_context_ = NULL;
+        request_context_getter_);
+    request_context_getter_ = NULL;
   }
 }
 
 sync_api::HttpPostProviderInterface* HttpBridgeFactory::Create() {
-  HttpBridge* http = new HttpBridge(request_context_,
+  HttpBridge* http = new HttpBridge(request_context_getter_,
       ChromeThread::GetMessageLoop(ChromeThread::IO));
   http->AddRef();
   return http;
@@ -87,9 +109,9 @@ HttpBridge::RequestContext::~RequestContext() {
   delete http_transaction_factory_;
 }
 
-HttpBridge::HttpBridge(HttpBridge::RequestContext* context,
+HttpBridge::HttpBridge(HttpBridge::RequestContextGetter* context_getter,
                        MessageLoop* io_loop)
-    : context_for_request_(context),
+    : context_getter_for_request_(context_getter),
       url_poster_(NULL),
       created_on_loop_(MessageLoop::current()),
       io_loop_(io_loop),
@@ -98,17 +120,17 @@ HttpBridge::HttpBridge(HttpBridge::RequestContext* context,
       http_response_code_(-1),
       http_post_completed_(false, false),
       use_io_loop_for_testing_(false) {
-  context_for_request_->AddRef();
+  context_getter_for_request_->AddRef();
 }
 
 HttpBridge::~HttpBridge() {
-  io_loop_->ReleaseSoon(FROM_HERE, context_for_request_);
+  io_loop_->ReleaseSoon(FROM_HERE, context_getter_for_request_);
 }
 
 void HttpBridge::SetUserAgent(const char* user_agent) {
   DCHECK_EQ(MessageLoop::current(), created_on_loop_);
   DCHECK(!request_completed_);
-  context_for_request_->set_user_agent(user_agent);
+  context_getter_for_request_->set_user_agent(user_agent);
 }
 
 void HttpBridge::SetExtraRequestHeaders(const char * headers) {
@@ -153,7 +175,6 @@ bool HttpBridge::MakeSynchronousPost(int* os_error_code, int* response_code) {
   DCHECK(!request_completed_);
   DCHECK(url_for_request_.is_valid()) << "Invalid URL for request";
   DCHECK(!content_type_.empty()) << "Payload not set";
-  DCHECK(context_for_request_->is_user_agent_set()) << "User agent not set";
 
   io_loop_->PostTask(FROM_HERE, NewRunnableMethod(this,
                                 &HttpBridge::CallMakeAsynchronousPost));
@@ -172,7 +193,7 @@ void HttpBridge::MakeAsynchronousPost() {
   DCHECK(!request_completed_);
 
   url_poster_ = new URLFetcher(url_for_request_, URLFetcher::POST, this);
-  url_poster_->set_request_context(context_for_request_);
+  url_poster_->set_request_context(context_getter_for_request_);
   url_poster_->set_upload_data(content_type_, request_content_);
   url_poster_->set_extra_request_headers(extra_headers_);
 
@@ -194,10 +215,6 @@ const char* HttpBridge::GetResponseContent() const {
   return response_content_.data();
 }
 
-URLRequestContext* HttpBridge::GetRequestContext() const {
-  return context_for_request_;
-}
-
 void HttpBridge::OnURLFetchComplete(const URLFetcher *source, const GURL &url,
                                     const URLRequestStatus &status,
                                     int response_code,
diff --git a/chrome/browser/sync/glue/http_bridge.h b/chrome/browser/sync/glue/http_bridge.h
index ab491a0..7e1d967 100644
--- a/chrome/browser/sync/glue/http_bridge.h
+++ b/chrome/browser/sync/glue/http_bridge.h
@@ -12,6 +12,7 @@
 #include "base/ref_counted.h"
 #include "base/waitable_event.h"
 #include "chrome/browser/net/url_fetcher.h"
+#include "chrome/browser/net/url_request_context_getter.h"
 #include "chrome/browser/sync/engine/syncapi.h"
 #include "googleurl/src/gurl.h"
 #include "net/url_request/url_request_context.h"
@@ -53,7 +54,6 @@ class HttpBridge : public base::RefCountedThreadSafe<HttpBridge>,
     // Set the user agent for requests using this context. The default is
     // the browser's UA string.
     void set_user_agent(const std::string& ua) { user_agent_ = ua; }
-    bool is_user_agent_set() const { return !user_agent_.empty(); }
 
     virtual const std::string& GetUserAgent(const GURL& url) const {
       // If the user agent is set explicitly return that, otherwise call the
@@ -73,7 +73,31 @@ class HttpBridge : public base::RefCountedThreadSafe<HttpBridge>,
     DISALLOW_COPY_AND_ASSIGN(RequestContext);
   };
 
-  HttpBridge(RequestContext* context, MessageLoop* io_loop);
+  // Lazy-getter for RequestContext objects.
+  class RequestContextGetter : public URLRequestContextGetter {
+   public:
+    explicit RequestContextGetter(
+        URLRequestContextGetter* baseline_context_getter);
+
+    void set_user_agent(const std::string& ua) { user_agent_ = ua; }
+    bool is_user_agent_set() const { return !user_agent_.empty(); }
+
+    // URLRequestContextGetter implementation.
+    virtual URLRequestContext* GetURLRequestContext();
+
+   private:
+    // User agent to apply to the URLRequestContext.
+    std::string user_agent_;
+
+    scoped_refptr<URLRequestContextGetter> baseline_context_getter_;
+
+    // Lazily initialized by GetURLRequestContext().
+    scoped_refptr<RequestContext> context_;
+
+    DISALLOW_COPY_AND_ASSIGN(RequestContextGetter);
+  };
+
+  HttpBridge(RequestContextGetter* context, MessageLoop* io_loop);
   virtual ~HttpBridge();
 
   // sync_api::HttpPostProvider implementation.
@@ -98,7 +122,11 @@ class HttpBridge : public base::RefCountedThreadSafe<HttpBridge>,
                                   const ResponseCookies& cookies,
                                   const std::string& data);
 
-  URLRequestContext* GetRequestContext() const;
+#if defined(UNIT_TEST)
+  URLRequestContextGetter* GetRequestContextGetter() const {
+    return context_getter_for_request_;
+  }
+#endif
 
  protected:
   // Protected virtual so the unit test can override to shunt network requests.
@@ -112,9 +140,9 @@ class HttpBridge : public base::RefCountedThreadSafe<HttpBridge>,
   // still have a function to statically pass to PostTask.
   void CallMakeAsynchronousPost() { MakeAsynchronousPost(); }
 
-  // A customized URLRequestContext for bridged requests. See RequestContext
-  // definition for details.
-  RequestContext* context_for_request_;
+  // Gets a customized URLRequestContext for bridged requests. See
+  // RequestContext definition for details.
+  RequestContextGetter* context_getter_for_request_;
 
   // Our hook into the network layer is a URLFetcher. USED ONLY ON THE IO LOOP,
   // so we can block created_on_loop_ while the fetch is in progress.
@@ -165,16 +193,16 @@ class HttpBridge : public base::RefCountedThreadSafe<HttpBridge>,
 class HttpBridgeFactory
   : public sync_api::HttpPostProviderFactory {
  public:
-  explicit HttpBridgeFactory(URLRequestContext* baseline_context);
+  explicit HttpBridgeFactory(URLRequestContextGetter* baseline_context_getter);
   virtual ~HttpBridgeFactory();
   virtual sync_api::HttpPostProviderInterface* Create();
   virtual void Destroy(sync_api::HttpPostProviderInterface* http);
  private:
   // This request context is built on top of the baseline context and shares
   // common components.
-  HttpBridge::RequestContext* GetRequestContext();
+  HttpBridge::RequestContextGetter* GetRequestContextGetter();
   // We must Release() this from the IO thread.
-  HttpBridge::RequestContext* request_context_;
+  HttpBridge::RequestContextGetter* request_context_getter_;
   DISALLOW_COPY_AND_ASSIGN(HttpBridgeFactory);
 };
 
diff --git a/chrome/browser/sync/glue/http_bridge_unittest.cc b/chrome/browser/sync/glue/http_bridge_unittest.cc
index 32e0bc8..da3eb67 100644
--- a/chrome/browser/sync/glue/http_bridge_unittest.cc
+++ b/chrome/browser/sync/glue/http_bridge_unittest.cc
@@ -16,10 +16,22 @@ namespace {
 const wchar_t kDocRoot[] = L"chrome/test/data";
 }
 
+// Lazy getter for TestURLRequestContext instances.
+class TestURLRequestContextGetter : public URLRequestContextGetter {
+ public:
+  virtual URLRequestContext* GetURLRequestContext() {
+    if (!context_)
+      context_ = new TestURLRequestContext;
+    return context_;
+  }
+ private:
+  scoped_refptr<URLRequestContext> context_;
+};
+
 class HttpBridgeTest : public testing::Test {
  public:
   HttpBridgeTest()
-      : fake_default_request_context_(NULL),
+      : fake_default_request_context_getter_(NULL),
         io_thread_("HttpBridgeTest IO thread") {
   }
 
@@ -30,19 +42,21 @@ class HttpBridgeTest : public testing::Test {
   }
 
   virtual void TearDown() {
-    io_thread_loop()->ReleaseSoon(FROM_HERE, fake_default_request_context_);
+    io_thread_loop()->ReleaseSoon(FROM_HERE,
+        fake_default_request_context_getter_);
     io_thread_.Stop();
-    fake_default_request_context_ = NULL;
+    fake_default_request_context_getter_ = NULL;
   }
 
   HttpBridge* BuildBridge() {
-    if (!fake_default_request_context_) {
-      fake_default_request_context_ = new TestURLRequestContext();
-      fake_default_request_context_->AddRef();
+    if (!fake_default_request_context_getter_) {
+      fake_default_request_context_getter_ = new TestURLRequestContextGetter();
+      fake_default_request_context_getter_->AddRef();
     }
     HttpBridge* bridge = new HttpBridge(
-        new HttpBridge::RequestContext(fake_default_request_context_),
-        io_thread_.message_loop());
+        new HttpBridge::RequestContextGetter(
+            fake_default_request_context_getter_),
+        io_thread_loop());
     bridge->use_io_loop_for_testing_ = true;
     return bridge;
   }
@@ -50,14 +64,14 @@ class HttpBridgeTest : public testing::Test {
   MessageLoop* io_thread_loop() { return io_thread_.message_loop(); }
 
   // Note this is lazy created, so don't call this before your bridge.
-  TestURLRequestContext* GetTestRequestContext() {
-    return fake_default_request_context_;
+  TestURLRequestContextGetter* GetTestRequestContextGetter() {
+    return fake_default_request_context_getter_;
   }
 
  private:
   // A make-believe "default" request context, as would be returned by
   // Profile::GetDefaultRequestContext().  Created lazily by BuildBridge.
-  TestURLRequestContext* fake_default_request_context_;
+  TestURLRequestContextGetter* fake_default_request_context_getter_;
 
   // Separate thread for IO used by the HttpBridge.
   base::Thread io_thread_;
@@ -67,9 +81,10 @@ class HttpBridgeTest : public testing::Test {
 // back with dummy response info.
 class ShuntedHttpBridge : public HttpBridge {
  public:
-  ShuntedHttpBridge(URLRequestContext* baseline_context,
+  ShuntedHttpBridge(URLRequestContextGetter* baseline_context_getter,
                     MessageLoop* io_loop, HttpBridgeTest* test)
-      : HttpBridge(new HttpBridge::RequestContext(baseline_context),
+      : HttpBridge(new HttpBridge::RequestContextGetter(
+                       baseline_context_getter),
                    io_loop), test_(test) { }
  protected:
   virtual void MakeAsynchronousPost() {
@@ -94,19 +109,22 @@ class ShuntedHttpBridge : public HttpBridge {
 
 TEST_F(HttpBridgeTest, TestUsesSameHttpNetworkSession) {
   scoped_refptr<HttpBridge> http_bridge(this->BuildBridge());
-  EXPECT_TRUE(GetTestRequestContext());
+  EXPECT_TRUE(GetTestRequestContextGetter());
   net::HttpNetworkSession* test_session =
-      GetTestRequestContext()->http_transaction_factory()->GetSession();
+      GetTestRequestContextGetter()->GetURLRequestContext()->
+      http_transaction_factory()->GetSession();
   EXPECT_EQ(test_session,
-            http_bridge->GetRequestContext()->
+            http_bridge->GetRequestContextGetter()->
+                GetURLRequestContext()->
                 http_transaction_factory()->GetSession());
 }
 
 // Test the HttpBridge without actually making any network requests.
 TEST_F(HttpBridgeTest, TestMakeSynchronousPostShunted) {
-  scoped_refptr<TestURLRequestContext> ctx(new TestURLRequestContext());
+  scoped_refptr<URLRequestContextGetter> ctx_getter(
+      new TestURLRequestContextGetter());
   scoped_refptr<HttpBridge> http_bridge(new ShuntedHttpBridge(
-      ctx, io_thread_loop(), this));
+      ctx_getter, io_thread_loop(), this));
   http_bridge->SetUserAgent("bob");
   http_bridge->SetURL("http://www.google.com", 9999);
   http_bridge->SetPostPayload("text/plain", 2, " ");
diff --git a/chrome/browser/sync/glue/sync_backend_host.cc b/chrome/browser/sync/glue/sync_backend_host.cc
index 8432381..4c38711 100644
--- a/chrome/browser/sync/glue/sync_backend_host.cc
+++ b/chrome/browser/sync/glue/sync_backend_host.cc
@@ -40,8 +40,9 @@ SyncBackendHost::~SyncBackendHost() {
   DCHECK(!core_ && !frontend_) << "Must call Shutdown before destructor.";
 }
 
-void SyncBackendHost::Initialize(const GURL& sync_service_url,
-                                 URLRequestContext* baseline_context) {
+void SyncBackendHost::Initialize(
+    const GURL& sync_service_url,
+    URLRequestContextGetter* baseline_context_getter) {
   if (!core_thread_.Start())
     return;
   bookmark_model_worker_ = new BookmarkModelWorker(frontend_loop_);
@@ -49,8 +50,8 @@ void SyncBackendHost::Initialize(const GURL& sync_service_url,
   core_thread_.message_loop()->PostTask(FROM_HERE,
       NewRunnableMethod(core_.get(), &SyncBackendHost::Core::DoInitialize,
                         sync_service_url, bookmark_model_worker_, true,
-                        new HttpBridgeFactory(baseline_context),
-                        new HttpBridgeFactory(baseline_context)));
+                        new HttpBridgeFactory(baseline_context_getter),
+                        new HttpBridgeFactory(baseline_context_getter)));
 }
 
 void SyncBackendHost::Authenticate(const std::string& username,
diff --git a/chrome/browser/sync/glue/sync_backend_host.h b/chrome/browser/sync/glue/sync_backend_host.h
index 3fad2c7..f0a1cd4 100644
--- a/chrome/browser/sync/glue/sync_backend_host.h
+++ b/chrome/browser/sync/glue/sync_backend_host.h
@@ -15,11 +15,11 @@
 #include "base/ref_counted.h"
 #include "base/thread.h"
 #include "base/timer.h"
+#include "chrome/browser/net/url_request_context_getter.h"
 #include "chrome/browser/sync/auth_error_state.h"
 #include "chrome/browser/sync/engine/syncapi.h"
 #include "chrome/browser/sync/glue/bookmark_model_worker.h"
 #include "googleurl/src/gurl.h"
-#include "net/url_request/url_request_context.h"
 
 namespace browser_sync {
 
@@ -85,7 +85,8 @@ class SyncBackendHost {
   ~SyncBackendHost();
 
   // Called on |frontend_loop_| to kick off asynchronous initialization.
-  void Initialize(const GURL& service_url, URLRequestContext* baseline_context);
+  void Initialize(const GURL& service_url,
+                  URLRequestContextGetter* baseline_context_getter);
 
   // Called on |frontend_loop_| to kick off asynchronous authentication.
   void Authenticate(const std::string& username, const std::string& password);