Fix appcache_service and request_context referencing.

There is one appcache service per profile and several request context per profile. The profile holds a reference to the appcache service. Those request contexts which are subject to retrieval from appcaches hold a reference to the appcache service too. The appcache service is provided with a pointer back to the 'main' request context, this context is used when updating appcaches.

Initialization is a little tricky because profiles can't be used on the IO thread and request contexts can't be used on the UI thread.

BUG=22597,22125
TEST=many existing tests exercise profile/context creation
Review URL: http://codereview.chromium.org/215024

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

5 files changed, 91 insertions, 52 deletions

diff --git a/chrome/browser/net/chrome_url_request_context.cc b/chrome/browser/net/chrome_url_request_context.cc
index 27d8e82..5de62c1 100644
--- a/chrome/browser/net/chrome_url_request_context.cc
+++ b/chrome/browser/net/chrome_url_request_context.cc
@@ -110,9 +110,15 @@ static net::ProxyService* CreateProxyService(URLRequestContext* context,
 // static
 ChromeURLRequestContext* ChromeURLRequestContext::CreateOriginal(
     Profile* profile, const FilePath& cookie_store_path,
-    const FilePath& disk_cache_path, int cache_size) {
+    const FilePath& disk_cache_path, int cache_size,
+    ChromeAppCacheService* appcache_service) {
   DCHECK(!profile->IsOffTheRecord());
-  ChromeURLRequestContext* context = new ChromeURLRequestContext(profile);
+  ChromeURLRequestContext* context = new ChromeURLRequestContext(
+      profile, appcache_service);
+
+  // The appcache service uses the profile's original context for UpdateJobs.
+  DCHECK(!appcache_service->request_context());
+  appcache_service->set_request_context(context);
 
   // Global host resolver for the context.
   context->host_resolver_ = chrome_browser_net::GetGlobalHostResolver();
@@ -169,17 +175,19 @@ ChromeURLRequestContext* ChromeURLRequestContext::CreateOriginal(
 
 // static
 ChromeURLRequestContext* ChromeURLRequestContext::CreateOriginalForMedia(
-    Profile* profile, const FilePath& disk_cache_path, int cache_size) {
+    Profile* profile, const FilePath& disk_cache_path, int cache_size,
+    ChromeAppCacheService* appcache_service) {
   DCHECK(!profile->IsOffTheRecord());
   return CreateRequestContextForMedia(profile, disk_cache_path, cache_size,
-                                      false);
+                                      false, appcache_service);
 }
 
 // static
 ChromeURLRequestContext* ChromeURLRequestContext::CreateOriginalForExtensions(
     Profile* profile, const FilePath& cookie_store_path) {
   DCHECK(!profile->IsOffTheRecord());
-  ChromeURLRequestContext* context = new ChromeURLRequestContext(profile);
+  ChromeURLRequestContext* context = new ChromeURLRequestContext(
+      profile, NULL);
 
   // All we care about for extensions is the cookie store.
   DCHECK(!cookie_store_path.empty());
@@ -200,9 +208,14 @@ ChromeURLRequestContext* ChromeURLRequestContext::CreateOriginalForExtensions(
 
 // static
 ChromeURLRequestContext* ChromeURLRequestContext::CreateOffTheRecord(
-    Profile* profile) {
+    Profile* profile, ChromeAppCacheService* appcache_service) {
   DCHECK(profile->IsOffTheRecord());
-  ChromeURLRequestContext* context = new ChromeURLRequestContext(profile);
+  ChromeURLRequestContext* context = new ChromeURLRequestContext(
+      profile, appcache_service);
+
+  // The appcache service uses the profile's original context for UpdateJobs.
+  DCHECK(!appcache_service->request_context());
+  appcache_service->set_request_context(context);
 
   // Share the same proxy service and host resolver as the original profile.
   // TODO(eroman): although ProxyService is reference counted, this sharing
@@ -243,7 +256,8 @@ ChromeURLRequestContext* ChromeURLRequestContext::CreateOffTheRecord(
 ChromeURLRequestContext*
 ChromeURLRequestContext::CreateOffTheRecordForExtensions(Profile* profile) {
   DCHECK(profile->IsOffTheRecord());
-  ChromeURLRequestContext* context = new ChromeURLRequestContext(profile);
+  ChromeURLRequestContext* context =
+      new ChromeURLRequestContext(profile, NULL);
   net::CookieMonster* cookie_monster = new net::CookieMonster;
 
   // Enable cookies for extension URLs only.
@@ -257,10 +271,11 @@ ChromeURLRequestContext::CreateOffTheRecordForExtensions(Profile* profile) {
 // static
 ChromeURLRequestContext* ChromeURLRequestContext::CreateRequestContextForMedia(
     Profile* profile, const FilePath& disk_cache_path, int cache_size,
-    bool off_the_record) {
+    bool off_the_record, ChromeAppCacheService* appcache_service) {
   URLRequestContext* original_context =
       profile->GetOriginalProfile()->GetRequestContext();
-  ChromeURLRequestContext* context = new ChromeURLRequestContext(profile);
+  ChromeURLRequestContext* context =
+      new ChromeURLRequestContext(profile, appcache_service);
   context->is_media_ = true;
 
   // Share the same proxy service of the common profile.
@@ -301,8 +316,10 @@ ChromeURLRequestContext* ChromeURLRequestContext::CreateRequestContextForMedia(
   return context;
 }
 
-ChromeURLRequestContext::ChromeURLRequestContext(Profile* profile)
-    : prefs_(profile->GetPrefs()),
+ChromeURLRequestContext::ChromeURLRequestContext(
+    Profile* profile, ChromeAppCacheService* appcache_service)
+    : appcache_service_(appcache_service),
+      prefs_(profile->GetPrefs()),
       is_media_(false),
       is_off_the_record_(profile->IsOffTheRecord()) {
   // Set up Accept-Language and Accept-Charset header values
@@ -358,8 +375,6 @@ ChromeURLRequestContext::ChromeURLRequestContext(Profile* profile)
   }
 
   ssl_config_service_ = profile->GetSSLConfigService();
-
-  appcache_service_ = profile->GetAppCacheService();
 }
 
 ChromeURLRequestContext::ChromeURLRequestContext(
@@ -378,13 +393,13 @@ ChromeURLRequestContext::ChromeURLRequestContext(
   referrer_charset_ = other->referrer_charset_;
 
   // Set ChromeURLRequestContext members
+  appcache_service_ = other->appcache_service_;
   extension_paths_ = other->extension_paths_;
   user_script_dir_path_ = other->user_script_dir_path_;
   prefs_ = other->prefs_;
   blacklist_ = other->blacklist_;
   is_media_ = other->is_media_;
   is_off_the_record_ = other->is_off_the_record_;
-  appcache_service_ = other->appcache_service_;
 }
 
 // NotificationObserver implementation.
@@ -539,6 +554,9 @@ void ChromeURLRequestContext::OnUnloadedExtension(
 ChromeURLRequestContext::~ChromeURLRequestContext() {
   DCHECK(NULL == prefs_);
 
+  if (appcache_service_.get() && appcache_service_->request_context() == this)
+    appcache_service_->set_request_context(NULL);
+
   NotificationService::current()->Notify(
       NotificationType::URL_REQUEST_CONTEXT_RELEASED,
       Source<URLRequestContext>(this),
diff --git a/chrome/browser/net/chrome_url_request_context.h b/chrome/browser/net/chrome_url_request_context.h
index b78e0a7..9eb3c90 100644
--- a/chrome/browser/net/chrome_url_request_context.h
+++ b/chrome/browser/net/chrome_url_request_context.h
@@ -32,13 +32,15 @@ class ChromeURLRequestContext : public URLRequestContext,
   // expected to get called on the UI thread.
   static ChromeURLRequestContext* CreateOriginal(
       Profile* profile, const FilePath& cookie_store_path,
-      const FilePath& disk_cache_path, int cache_size);
+      const FilePath& disk_cache_path, int cache_size,
+      ChromeAppCacheService* appcache_service);
 
   // Create an instance for an original profile for media. This is expected to
   // get called on UI thread. This method takes a profile and reuses the
   // 'original' URLRequestContext for common files.
   static ChromeURLRequestContext* CreateOriginalForMedia(Profile *profile,
-      const FilePath& disk_cache_path, int cache_size);
+      const FilePath& disk_cache_path, int cache_size,
+      ChromeAppCacheService* appcache_service);
 
   // Create an instance for an original profile for extensions. This is expected
   // to get called on UI thread.
@@ -47,7 +49,8 @@ class ChromeURLRequestContext : public URLRequestContext,
 
   // Create an instance for use with an OTR profile. This is expected to get
   // called on the UI thread.
-  static ChromeURLRequestContext* CreateOffTheRecord(Profile* profile);
+  static ChromeURLRequestContext* CreateOffTheRecord(Profile* profile,
+      ChromeAppCacheService* appcache_service);
 
   // Create an instance of request context for OTR profile for extensions.
   static ChromeURLRequestContext* CreateOffTheRecordForExtensions(
@@ -81,16 +84,18 @@ class ChromeURLRequestContext : public URLRequestContext,
   const Blacklist* blacklist() const { return blacklist_; }
 
  protected:
-  // Private constructor, use the static factory methods instead. This is
+  // Private constructors, use the static factory methods instead. This is
   // expected to be called on the UI thread.
-  ChromeURLRequestContext(Profile* profile);
+  ChromeURLRequestContext(
+      Profile* profile, ChromeAppCacheService* appcache_service);
   ChromeURLRequestContext(ChromeURLRequestContext* other);
 
   // Create a request context for media resources from a regular request
   // context. This helper method is called from CreateOriginalForMedia and
   // CreateOffTheRecordForMedia.
   static ChromeURLRequestContext* CreateRequestContextForMedia(Profile* profile,
-      const FilePath& disk_cache_path, int cache_size, bool off_the_record);
+      const FilePath& disk_cache_path, int cache_size, bool off_the_record,
+      ChromeAppCacheService* appache_service);
 
   // NotificationObserver implementation.
   virtual void Observe(NotificationType type,
diff --git a/chrome/browser/profile.cc b/chrome/browser/profile.cc
index c3eea76..8655eae 100644
--- a/chrome/browser/profile.cc
+++ b/chrome/browser/profile.cc
@@ -134,13 +134,14 @@ static void CleanupRequestContext(ChromeURLRequestContext* context) {
   }
 }
 
-static void CleanupAppCacheService(ChromeAppCacheService* service) {
-  if (service) {
-    MessageLoop* io_thread = ChromeThread::GetMessageLoop(ChromeThread::IO);
+static void CleanupAppCacheService(ChromeAppCacheService* appcache_service) {
+  if (appcache_service) {
+    // The I/O thread may be NULL during testing.
+    base::Thread* io_thread = g_browser_process->io_thread();
     if (io_thread)
-      io_thread->ReleaseSoon(FROM_HERE, service);
+      io_thread->message_loop()->ReleaseSoon(FROM_HERE, appcache_service);
     else
-      service->Release();
+      appcache_service->Release();
   }
 }
 
@@ -203,7 +204,13 @@ class OffTheRecordProfileImpl : public Profile,
       : profile_(real_profile),
         extensions_request_context_(NULL),
         start_time_(Time::Now()) {
-    request_context_ = ChromeURLRequestContext::CreateOffTheRecord(this);
+    // Created here but lazily inititialized later, finally released on
+    // on the io thread, see CleanupAppCacheService.
+    appcache_service_ = new ChromeAppCacheService();
+    appcache_service_->AddRef();
+
+    request_context_ = ChromeURLRequestContext::CreateOffTheRecord(
+        this, appcache_service_);
     request_context_->AddRef();
 
     // Register for browser close notifications so we can detect when the last
@@ -216,7 +223,7 @@ class OffTheRecordProfileImpl : public Profile,
   virtual ~OffTheRecordProfileImpl() {
     CleanupRequestContext(request_context_);
     CleanupRequestContext(extensions_request_context_);
-    CleanupAppCacheService(appcache_service_.release());
+    CleanupAppCacheService(appcache_service_);
   }
 
   virtual FilePath GetPath() { return profile_->GetPath(); }
@@ -239,12 +246,10 @@ class OffTheRecordProfileImpl : public Profile,
   }
 
   virtual ChromeAppCacheService* GetAppCacheService() {
-    if (!appcache_service_.get()) {
-      appcache_service_ = new ChromeAppCacheService();
-      appcache_service_->InitializeOnUIThread(
-          GetPath(), GetRequestContext(), true);
-    }
-    return appcache_service_.get();
+    DCHECK(request_context_);  // should be created in ctor
+    if (!appcache_service_->is_initialized())
+      appcache_service_->InitializeOnUIThread(GetPath(), true);
+    return appcache_service_;
   }
 
   virtual VisitedLinkMaster* GetVisitedLinkMaster() {
@@ -526,7 +531,7 @@ class OffTheRecordProfileImpl : public Profile,
   ChromeURLRequestContext* extensions_request_context_;
 
   // Use a seperate appcache service for OTR.
-  scoped_refptr<ChromeAppCacheService> appcache_service_;
+  ChromeAppCacheService* appcache_service_;
 
   // The download manager that only stores downloaded items in memory.
   scoped_refptr<DownloadManager> download_manager_;
@@ -579,6 +584,11 @@ ProfileImpl::ProfileImpl(const FilePath& path)
     extension_devtools_manager_ = new ExtensionDevToolsManager(this);
   }
 
+  // Created here but lazily inititialized later, finally released on
+  // on the io thread, see CleanupAppCacheService.
+  appcache_service_ = new ChromeAppCacheService();
+  appcache_service_->AddRef();
+
   extension_process_manager_.reset(new ExtensionProcessManager(this));
   extension_message_service_ = new ExtensionMessageService(this);
 
@@ -769,7 +779,7 @@ ProfileImpl::~ProfileImpl() {
   CleanupRequestContext(request_context_);
   CleanupRequestContext(media_request_context_);
   CleanupRequestContext(extensions_request_context_);
-  CleanupAppCacheService(appcache_service_.release());
+  CleanupAppCacheService(appcache_service_);
 
   // When the request contexts are gone, the blacklist wont be needed anymore.
   delete blacklist_;
@@ -820,12 +830,11 @@ Profile* ProfileImpl::GetOriginalProfile() {
 }
 
 ChromeAppCacheService* ProfileImpl::GetAppCacheService() {
-  if (!appcache_service_.get()) {
-    appcache_service_ = new ChromeAppCacheService();
-    appcache_service_->InitializeOnUIThread(
-        GetPath(), GetRequestContext(), false);
+  if (!appcache_service_->is_initialized()) {
+    EnsureRequestContextCreated();
+    appcache_service_->InitializeOnUIThread(GetPath(), false);
   }
-  return appcache_service_.get();
+  return appcache_service_;
 }
 
 VisitedLinkMaster* ProfileImpl::GetVisitedLinkMaster() {
@@ -926,7 +935,7 @@ URLRequestContext* ProfileImpl::GetRequestContext() {
 
     cache_path = GetCachePath(cache_path);
     request_context_ = ChromeURLRequestContext::CreateOriginal(
-        this, cookie_path, cache_path, max_size);
+        this, cookie_path, cache_path, max_size, appcache_service_);
     request_context_->AddRef();
 
     // The first request context is always a normal (non-OTR) request context.
@@ -957,7 +966,7 @@ URLRequestContext* ProfileImpl::GetRequestContextForMedia() {
 
     cache_path = GetMediaCachePath(cache_path);
     media_request_context_ = ChromeURLRequestContext::CreateOriginalForMedia(
-        this, cache_path, max_size);
+        this, cache_path, max_size, appcache_service_);
     media_request_context_->AddRef();
 
     DCHECK(media_request_context_->cookie_store());
diff --git a/chrome/browser/profile.h b/chrome/browser/profile.h
index ba4dd87..6d6cfc8 100644
--- a/chrome/browser/profile.h
+++ b/chrome/browser/profile.h
@@ -440,6 +440,10 @@ class ProfileImpl : public Profile,
 
   void StopCreateSessionServiceTimer();
 
+  void EnsureRequestContextCreated() {
+    GetRequestContext();
+  }
+
   void EnsureSessionServiceCreated() {
     GetSessionService();
   }
@@ -457,7 +461,6 @@ class ProfileImpl : public Profile,
 
   FilePath path_;
   FilePath base_cache_path_;
-  scoped_refptr<ChromeAppCacheService> appcache_service_;
   scoped_ptr<VisitedLinkEventListener> visited_link_event_listener_;
   scoped_ptr<VisitedLinkMaster> visited_link_master_;
   scoped_refptr<ExtensionsService> extensions_service_;
@@ -481,6 +484,8 @@ class ProfileImpl : public Profile,
   scoped_ptr<ProfileSyncService> sync_service_;
 #endif
 
+  ChromeAppCacheService* appcache_service_;
+
   ChromeURLRequestContext* request_context_;
 
   ChromeURLRequestContext* media_request_context_;
diff --git a/chrome/common/appcache/chrome_appcache_service.h b/chrome/common/appcache/chrome_appcache_service.h
index 3dd6082..213a347 100644
--- a/chrome/common/appcache/chrome_appcache_service.h
+++ b/chrome/common/appcache/chrome_appcache_service.h
@@ -9,6 +9,7 @@
 #include "base/message_loop.h"
 #include "base/ref_counted.h"
 #include "base/task.h"
+#include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_thread.h"
 #include "chrome/common/chrome_constants.h"
 #include "webkit/appcache/appcache_service.h"
@@ -28,21 +29,21 @@ class ChromeAppCacheService
  public:
 
   explicit ChromeAppCacheService()
-      : was_initialized_with_io_thread_(false) {
-    DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
+      : is_initialized_(false), was_initialized_with_io_thread_(false) {
   }
 
+  bool is_initialized() const { return is_initialized_; }
+
   void InitializeOnUIThread(const FilePath& data_directory,
-                            URLRequestContext* request_context,
                             bool is_incognito) {
-    DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
-    set_request_context(request_context);
+    DCHECK(!is_initialized_);
+    is_initialized_ = true;
 
-    // Some test cases run without an IO thread.
-    MessageLoop* io_thread = ChromeThread::GetMessageLoop(ChromeThread::IO);
+    // The I/O thread may be NULL during testing.
+    base::Thread* io_thread = g_browser_process->io_thread();
     if (io_thread) {
       was_initialized_with_io_thread_ = true;
-      io_thread->PostTask(FROM_HERE,
+      io_thread->message_loop()->PostTask(FROM_HERE,
           NewRunnableMethod(this, &ChromeAppCacheService::InitializeOnIOThread,
               data_directory, is_incognito));
     }
@@ -63,6 +64,7 @@ class ChromeAppCacheService
                             : data_directory.Append(chrome::kAppCacheDirname));
   }
 
+  bool is_initialized_;
   bool was_initialized_with_io_thread_;
 };