Initial CL for fixing some of the proxy auth issues.

Auth_cache is contained in the http session. We need to share
the http session with the parent profile request context in order to retain http authentication.
Weirdly enough, Profile::GetDefaultRequestContext() is not the same as profile_->GetRequestContext(),
It does NOT yet pop up a dialog if the user hasn't done so already.

BUG=19581
TEST=Included.

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

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

6 files changed, 87 insertions, 32 deletions

diff --git a/chrome/browser/sync/glue/http_bridge.cc b/chrome/browser/sync/glue/http_bridge.cc
index 304f91e..ebf0c7a 100644
--- a/chrome/browser/sync/glue/http_bridge.cc
+++ b/chrome/browser/sync/glue/http_bridge.cc
@@ -12,20 +12,20 @@
 #include "chrome/browser/profile.h"
 #include "net/base/cookie_monster.h"
 #include "net/base/load_flags.h"
+#include "net/http/http_cache.h"
 #include "net/http/http_network_layer.h"
 #include "net/proxy/proxy_service.h"
+#include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_status.h"
 #include "webkit/glue/webkit_glue.h"
 
 namespace browser_sync {
 
-HttpBridge::RequestContext* HttpBridgeFactory::GetRequestContext() {
-  if (!request_context_) {
-    request_context_ =
-        new HttpBridge::RequestContext(Profile::GetDefaultRequestContext());
-    request_context_->AddRef();
-  }
-  return request_context_;
+HttpBridgeFactory::HttpBridgeFactory(
+    URLRequestContext* baseline_context) {
+  DCHECK(baseline_context != NULL);
+  request_context_ = new HttpBridge::RequestContext(baseline_context);
+  request_context_->AddRef();
 }
 
 HttpBridgeFactory::~HttpBridgeFactory() {
@@ -38,8 +38,7 @@ HttpBridgeFactory::~HttpBridgeFactory() {
 }
 
 sync_api::HttpPostProviderInterface* HttpBridgeFactory::Create() {
-  // TODO(timsteele): We want the active profile request context.
-  HttpBridge* http = new HttpBridge(GetRequestContext(),
+  HttpBridge* http = new HttpBridge(request_context_,
       ChromeThread::GetMessageLoop(ChromeThread::IO));
   http->AddRef();
   return http;
@@ -49,8 +48,8 @@ void HttpBridgeFactory::Destroy(sync_api::HttpPostProviderInterface* http) {
   static_cast<HttpBridge*>(http)->Release();
 }
 
-HttpBridge::RequestContext::RequestContext(
-    const URLRequestContext* baseline_context) {
+HttpBridge::RequestContext::RequestContext(URLRequestContext* baseline_context)
+    : baseline_context_(baseline_context) {
 
   // Create empty, in-memory cookie store.
   cookie_store_ = new net::CookieMonster();
@@ -59,9 +58,15 @@ HttpBridge::RequestContext::RequestContext(
   host_resolver_ = baseline_context->host_resolver();
   proxy_service_ = baseline_context->proxy_service();
   ssl_config_service_ = baseline_context->ssl_config_service();
-  http_transaction_factory_ =
-      net::HttpNetworkLayer::CreateFactory(host_resolver_, proxy_service_,
-                                           ssl_config_service_);
+
+  // We want to share the HTTP session data with the network layer factory,
+  // which includes auth_cache for proxies.
+  // Session is not refcounted so we need to be careful to not lose the parent
+  // context.
+  net::HttpNetworkSession* session =
+      baseline_context->http_transaction_factory()->GetSession();
+  DCHECK(session);
+  http_transaction_factory_ = net::HttpNetworkLayer::CreateFactory(session);
 
   // TODO(timsteele): We don't currently listen for pref changes of these
   // fields or CookiePolicy; I'm not sure we want to strictly follow the
@@ -189,6 +194,10 @@ 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 b3dfc4f..3055ac8 100644
--- a/chrome/browser/sync/glue/http_bridge.h
+++ b/chrome/browser/sync/glue/http_bridge.h
@@ -47,7 +47,7 @@ class HttpBridge : public base::RefCountedThreadSafe<HttpBridge>,
     // accept-charsets, and proxy service information for bridged requests.
     // Typically |baseline_context| should be the URLRequestContext of the
     // currently active profile.
-    explicit RequestContext(const URLRequestContext* baseline_context);
+    explicit RequestContext(URLRequestContext* baseline_context);
     virtual ~RequestContext();
 
     // Set the user agent for requests using this context. The default is
@@ -68,6 +68,7 @@ class HttpBridge : public base::RefCountedThreadSafe<HttpBridge>,
 
    private:
     std::string user_agent_;
+    scoped_refptr<URLRequestContext> baseline_context_;
 
     DISALLOW_COPY_AND_ASSIGN(RequestContext);
   };
@@ -97,6 +98,8 @@ class HttpBridge : public base::RefCountedThreadSafe<HttpBridge>,
                                   const ResponseCookies& cookies,
                                   const std::string& data);
 
+  URLRequestContext* GetRequestContext() const;
+
  protected:
   // Protected virtual so the unit test can override to shunt network requests.
   virtual void MakeAsynchronousPost();
@@ -162,11 +165,13 @@ class HttpBridge : public base::RefCountedThreadSafe<HttpBridge>,
 class HttpBridgeFactory
   : public sync_api::HttpPostProviderFactory {
  public:
-  HttpBridgeFactory() : request_context_(NULL) { }
+  explicit HttpBridgeFactory(URLRequestContext* baseline_context);
   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();
   // We must Release() this from the IO thread.
   HttpBridge::RequestContext* request_context_;
diff --git a/chrome/browser/sync/glue/http_bridge_unittest.cc b/chrome/browser/sync/glue/http_bridge_unittest.cc
index 1e785dc..b0177d6 100644
--- a/chrome/browser/sync/glue/http_bridge_unittest.cc
+++ b/chrome/browser/sync/glue/http_bridge_unittest.cc
@@ -47,6 +47,12 @@ 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_;
+  }
+
  private:
   // A make-believe "default" request context, as would be returned by
   // Profile::GetDefaultRequestContext().  Created lazily by BuildBridge.
@@ -60,7 +66,7 @@ class HttpBridgeTest : public testing::Test {
 // back with dummy response info.
 class ShuntedHttpBridge : public HttpBridge {
  public:
-  ShuntedHttpBridge(const URLRequestContext* baseline_context,
+  ShuntedHttpBridge(URLRequestContext* baseline_context,
                     MessageLoop* io_loop, HttpBridgeTest* test)
       : HttpBridge(new HttpBridge::RequestContext(baseline_context),
                    io_loop), test_(test) { }
@@ -85,6 +91,16 @@ class ShuntedHttpBridge : public HttpBridge {
   HttpBridgeTest* test_;
 };
 
+TEST_F(HttpBridgeTest, TestUsesSameHttpNetworkSession) {
+  scoped_refptr<HttpBridge> http_bridge(this->BuildBridge());
+  EXPECT_TRUE(GetTestRequestContext());
+  net::HttpNetworkSession* test_session =
+      GetTestRequestContext()->http_transaction_factory()->GetSession();
+  EXPECT_EQ(test_session,
+            http_bridge->GetRequestContext()->
+                http_transaction_factory()->GetSession());
+}
+
 // Test the HttpBridge without actually making any network requests.
 TEST_F(HttpBridgeTest, TestMakeSynchronousPostShunted) {
   scoped_refptr<TestURLRequestContext> ctx(new TestURLRequestContext());
diff --git a/chrome/browser/sync/glue/sync_backend_host.cc b/chrome/browser/sync/glue/sync_backend_host.cc
index 562d280..43b26b1 100644
--- a/chrome/browser/sync/glue/sync_backend_host.cc
+++ b/chrome/browser/sync/glue/sync_backend_host.cc
@@ -8,6 +8,7 @@
 #include "base/file_version_info.h"
 #include "base/file_util.h"
 #include "base/string_util.h"
+#include "chrome/browser/chrome_thread.h"
 #include "chrome/browser/sync/glue/change_processor.h"
 #include "chrome/browser/sync/glue/sync_backend_host.h"
 #include "chrome/browser/sync/glue/http_bridge.h"
@@ -41,11 +42,12 @@ SyncBackendHost::~SyncBackendHost() {
   DCHECK(!core_ && !frontend_) << "Must call Shutdown before destructor.";
 }
 
-void SyncBackendHost::Initialize(const GURL& sync_service_url) {
+void SyncBackendHost::Initialize(const GURL& sync_service_url,
+                                 URLRequestContext* baseline_context) {
   if (!core_thread_.Start())
     return;
-
   bookmark_model_worker_ = new BookmarkModelWorker(frontend_loop_);
+  core_.get()->SetBaseRequestContext(baseline_context);
   core_thread_.message_loop()->PostTask(FROM_HERE,
       NewRunnableMethod(core_.get(), &SyncBackendHost::Core::DoInitialize,
                         sync_service_url, bookmark_model_worker_, true));
@@ -127,9 +129,26 @@ AuthErrorState SyncBackendHost::GetAuthErrorState() const {
   return last_auth_error_;
 }
 
+void SyncBackendHost::Core::SetBaseRequestContext(
+    URLRequestContext* request_context) {
+  DCHECK(base_request_context_ == NULL);
+  base_request_context_ = request_context;
+  // This ref is removed on the IO thread after the core thread is over.
+  base_request_context_->AddRef();
+}
+
 SyncBackendHost::Core::Core(SyncBackendHost* backend)
     : host_(backend),
-      syncapi_(new sync_api::SyncManager()) {
+      syncapi_(new sync_api::SyncManager()),
+      base_request_context_(NULL) {
+}
+
+SyncBackendHost::Core::~Core() {
+  if (base_request_context_) {
+    ChromeThread::GetMessageLoop(ChromeThread::IO)->ReleaseSoon(FROM_HERE,
+        base_request_context_);
+    base_request_context_ = NULL;
+  }
 }
 
 // Helper to construct a user agent string (ASCII) suitable for use by
@@ -183,8 +202,8 @@ void SyncBackendHost::Core::DoInitialize(
       kGaiaServiceId,
       kGaiaSourceForChrome,
       service_url.SchemeIsSecure(),
-      new HttpBridgeFactory(),
-      new HttpBridgeFactory(),
+      new HttpBridgeFactory(base_request_context_),
+      new HttpBridgeFactory(base_request_context_),
       bookmark_model_worker,
       attempt_last_user_authentication,
       MakeUserAgentForSyncapi().c_str());
diff --git a/chrome/browser/sync/glue/sync_backend_host.h b/chrome/browser/sync/glue/sync_backend_host.h
index a15e1a7..89c581e 100644
--- a/chrome/browser/sync/glue/sync_backend_host.h
+++ b/chrome/browser/sync/glue/sync_backend_host.h
@@ -19,6 +19,7 @@
 #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 {
 
@@ -84,7 +85,7 @@ class SyncBackendHost {
   ~SyncBackendHost();
 
   // Called on |frontend_loop_| to kick off asynchronous initialization.
-  void Initialize(const GURL& service_url);
+  void Initialize(const GURL& service_url, URLRequestContext* baseline_context);
 
   // Called on |frontend_loop_| to kick off asynchronous authentication.
   void Authenticate(const std::string& username, const std::string& password);
@@ -139,8 +140,7 @@ class SyncBackendHost {
     // created it, and *always* after core_thread_ has exited. The syncapi
     // watches thread exit events and keeps pointers to objects this dtor will
     // destroy, so this ordering is important.
-    ~Core() {
-    }
+    ~Core();
 
     // SyncManager::Observer implementation.  The Core just acts like an air
     // traffic controller here, forwarding incoming messages to appropriate
@@ -163,7 +163,7 @@ class SyncBackendHost {
     // Called on the SyncBackendHost core_thread_ to perform initialization
     // of the syncapi on behalf of SyncBackendHost::Initialize.
     void DoInitialize(const GURL& service_url,
-                      BookmarkModelWorker* bookmark_model_worker_,
+                      BookmarkModelWorker* bookmark_model_worker,
                       bool attempt_last_user_authentication);
 
     // Called on our SyncBackendHost's core_thread_ to perform authentication
@@ -182,6 +182,13 @@ class SyncBackendHost {
     //    because the thread that was using them has exited (in step 2).
     void DoShutdown(bool stopping_sync);
 
+    // Set the base request context to use when making HTTP calls.
+    // This method will add a reference to the context to persist it
+    // on the IO thread. Must be removed from IO thread.
+    // TODO(chron): After HttpNetworkSession reorganization happens, try
+    //              getting HttpBridgeFactory to initialize earlier.
+    void SetBaseRequestContext(URLRequestContext* request_context);
+
     sync_api::SyncManager* syncapi() { return syncapi_.get(); }
 
 #ifdef UNIT_TEST
@@ -234,6 +241,10 @@ class SyncBackendHost {
     // Our parent SyncBackendHost
     SyncBackendHost* host_;
 
+    // Our request context that we have to keep a ref to.
+    // Contains session data.
+    URLRequestContext* base_request_context_;
+
     // The timer used to periodically call SaveChanges.
     base::RepeatingTimer<Core> save_changes_timer_;
 
diff --git a/chrome/browser/sync/profile_sync_service.cc b/chrome/browser/sync/profile_sync_service.cc
index e90d555..5f35221 100644
--- a/chrome/browser/sync/profile_sync_service.cc
+++ b/chrome/browser/sync/profile_sync_service.cc
@@ -105,7 +105,7 @@ void ProfileSyncService::ClearPreferences() {
 }
 
 void ProfileSyncService::InitializeBackend() {
-  backend_->Initialize(sync_service_url_);
+  backend_->Initialize(sync_service_url_, profile_->GetRequestContext());
 }
 
 void ProfileSyncService::StartUp() {
@@ -126,11 +126,6 @@ void ProfileSyncService::StartUp() {
   model_associator_ = new ModelAssociator(this);
   change_processor_->set_model_associator(model_associator_);
 
-  // TODO(timsteele): HttpBridgeFactory should take a const* to the profile's
-  // URLRequestContext, because it needs it to create HttpBridge objects, and
-  // it may need to do that before the default request context has been set
-  // up. For now, call GetRequestContext lazy-init to force creation.
-  profile_->GetRequestContext();
   InitializeBackend();
 }