Merge "Fix a bug when cleaning up private browsing"

5 files changed, 76 insertions, 59 deletions

diff --git a/WebKit/android/WebCoreSupport/WebCache.cpp b/WebKit/android/WebCoreSupport/WebCache.cpp
index 9bc148c..d062bdc 100644
--- a/WebKit/android/WebCoreSupport/WebCache.cpp
+++ b/WebKit/android/WebCoreSupport/WebCache.cpp
@@ -36,6 +36,8 @@ using namespace net;
 
 namespace android {
 
+static WTF::Mutex instanceMutex;
+
 static const std::string& rootDirectory()
 {
     // This method may be called on any thread, as the Java method is
@@ -53,34 +55,50 @@ static const std::string& rootDirectory()
     return cacheDirectory;
 }
 
-WebCache* WebCache::get(bool isPrivateBrowsing)
+static std::string storageDirectory(bool isPrivateBrowsing)
 {
     static const char* const kDirectory = "/webviewCacheChromium";
     static const char* const kDirectoryPrivate = "/webviewCacheChromiumPrivate";
 
-    static WebCache* regularCache = 0;
-    static WebCache* privateCache = 0;
+    std::string storageDirectory = rootDirectory();
+    storageDirectory.append(isPrivateBrowsing ? kDirectoryPrivate : kDirectory);
+    return storageDirectory;
+}
 
-    if (isPrivateBrowsing) {
-        if (!privateCache) {
-            std::string storageDirectory = rootDirectory();
-            storageDirectory.append(kDirectoryPrivate);
-            privateCache = new WebCache(storageDirectory);
-        }
-        return privateCache;
-    }
+static scoped_refptr<WebCache>* instance(bool isPrivateBrowsing)
+{
+    static scoped_refptr<WebCache> regularInstance;
+    static scoped_refptr<WebCache> privateInstance;
+    return isPrivateBrowsing ? &privateInstance : &regularInstance;
+}
 
-    if (!regularCache) {
-        std::string storageDirectory = rootDirectory();
-        storageDirectory.append(kDirectory);
-        regularCache = new WebCache(storageDirectory);
-    }
-    return regularCache;
+WebCache* WebCache::get(bool isPrivateBrowsing)
+{
+    MutexLocker lock(instanceMutex);
+    scoped_refptr<WebCache>* instancePtr = instance(isPrivateBrowsing);
+    if (!instancePtr->get())
+        *instancePtr = new WebCache(storageDirectory(isPrivateBrowsing));
+    return instancePtr->get();
+}
+
+WebCache::~WebCache()
+{
+    // We currently leak the HostResolver object to avoid a crash.
+    // TODO: Fix this. See b/3243797
+    m_hostResolver.leakPtr();
+}
+
+void WebCache::cleanup(bool isPrivateBrowsing)
+{
+    // This is called on the UI thread.
+    MutexLocker lock(instanceMutex);
+    scoped_refptr<WebCache>* instancePtr = instance(isPrivateBrowsing);
+    *instancePtr = 0;
+    WebRequestContext::removeFileOrDirectory(storageDirectory(isPrivateBrowsing).c_str());
 }
 
 WebCache::WebCache(const std::string& storageDirectory)
-    : m_storageDirectory(storageDirectory)
-    , m_doomAllEntriesCallback(this, &WebCache::doomAllEntries)
+    : m_doomAllEntriesCallback(this, &WebCache::doomAllEntries)
     , m_doneCallback(this, &WebCache::onClearDone)
     , m_isClearInProgress(false)
 {
@@ -88,7 +106,7 @@ WebCache::WebCache(const std::string& storageDirectory)
     scoped_refptr<base::MessageLoopProxy> cacheMessageLoopProxy = ioThread->message_loop_proxy();
 
     static const int kMaximumCacheSizeBytes = 20 * 1024 * 1024;
-    FilePath directoryPath(m_storageDirectory.c_str());
+    FilePath directoryPath(storageDirectory.c_str());
     net::HttpCache::DefaultBackend* backendFactory = new net::HttpCache::DefaultBackend(net::DISK_CACHE, directoryPath, kMaximumCacheSizeBytes, cacheMessageLoopProxy);
 
     m_hostResolver = net::CreateSystemHostResolver(net::HostResolver::kDefaultParallelism, 0, 0);
@@ -109,11 +127,6 @@ void WebCache::clear()
          thread->message_loop()->PostTask(FROM_HERE, NewRunnableMethod(this, &WebCache::doClear));
 }
 
-void WebCache::cleanupFiles()
-{
-    WebRequestContext::removeFileOrDirectory(m_storageDirectory.c_str());
-}
-
 void WebCache::doClear()
 {
     if (m_isClearInProgress)
diff --git a/WebKit/android/WebCoreSupport/WebCache.h b/WebKit/android/WebCoreSupport/WebCache.h
index 4aa67c0..cde7906 100644
--- a/WebKit/android/WebCoreSupport/WebCache.h
+++ b/WebKit/android/WebCoreSupport/WebCache.h
@@ -38,9 +38,10 @@ namespace android {
 class WebCache : public base::RefCountedThreadSafe<WebCache> {
 public:
     static WebCache* get(bool isPrivateBrowsing);
+    static void cleanup(bool isPrivateBrowsing);
 
+    ~WebCache();
     void clear();
-    void cleanupFiles();
     net::HostResolver* hostResolver() { return m_hostResolver.get(); }
     net::HttpCache* cache() { return m_cache.get(); }
 
@@ -52,7 +53,6 @@ private:
     void doomAllEntries(int);
     void onClearDone(int);
 
-    std::string m_storageDirectory;
     OwnPtr<net::HostResolver> m_hostResolver;
     OwnPtr<net::HttpCache> m_cache;
 
diff --git a/WebKit/android/WebCoreSupport/WebCookieJar.cpp b/WebKit/android/WebCoreSupport/WebCookieJar.cpp
index 91a565c..6cef74b 100644
--- a/WebKit/android/WebCoreSupport/WebCookieJar.cpp
+++ b/WebKit/android/WebCoreSupport/WebCookieJar.cpp
@@ -32,7 +32,9 @@
 
 namespace android {
 
-const std::string& databaseDirectory()
+static WTF::Mutex instanceMutex;
+
+static const std::string& databaseDirectory()
 {
     // This method may be called on any thread, as the Java method is
     // synchronized.
@@ -50,42 +52,48 @@ const std::string& databaseDirectory()
 }
 
 
-WebCookieJar* WebCookieJar::get(bool isPrivateBrowsing)
+static std::string databaseDirectory(bool isPrivateBrowsing)
 {
     static const char* const kDatabaseFilename = "/webviewCookiesChromium.db";
     static const char* const kDatabaseFilenamePrivateBrowsing = "/webviewCookiesChromiumPrivate.db";
 
-    static WebCookieJar* regularCookieManager = 0;
-    static WebCookieJar* privateCookieManager = 0;
+    std::string databaseFilePath = databaseDirectory();
+    databaseFilePath.append(isPrivateBrowsing ? kDatabaseFilenamePrivateBrowsing : kDatabaseFilename);
+    return databaseFilePath;
+}
 
-    WTF::Mutex instanceMutex;
-    MutexLocker lock(instanceMutex);
+scoped_refptr<WebCookieJar>* instance(bool isPrivateBrowsing)
+{
+    static scoped_refptr<WebCookieJar> regularInstance;
+    static scoped_refptr<WebCookieJar> privateInstance;
+    return isPrivateBrowsing ? &privateInstance : &regularInstance;
+}
 
-    if (isPrivateBrowsing) {
-        if (!privateCookieManager) {
-            std::string databaseFilePath = databaseDirectory();
-            databaseFilePath.append(kDatabaseFilenamePrivateBrowsing);
-            privateCookieManager = new WebCookieJar(databaseFilePath);
-        }
-        return privateCookieManager;
-    }
+WebCookieJar* WebCookieJar::get(bool isPrivateBrowsing)
+{
+    MutexLocker lock(instanceMutex);
+    scoped_refptr<WebCookieJar>* instancePtr = instance(isPrivateBrowsing);
+    if (!instancePtr->get())
+        *instancePtr = new WebCookieJar(databaseDirectory(isPrivateBrowsing));
+    return instancePtr->get();
+}
 
-    if (!regularCookieManager) {
-        std::string databaseFilePath = databaseDirectory();
-        databaseFilePath.append(kDatabaseFilename);
-        regularCookieManager = new WebCookieJar(databaseFilePath);
-    }
-    return regularCookieManager;
+void WebCookieJar::cleanup(bool isPrivateBrowsing)
+{
+    // This is called on the UI thread.
+    MutexLocker lock(instanceMutex);
+    scoped_refptr<WebCookieJar>* instancePtr = instance(isPrivateBrowsing);
+    *instancePtr = 0;
+    WebRequestContext::removeFileOrDirectory(databaseDirectory(isPrivateBrowsing).c_str());
 }
 
 WebCookieJar::WebCookieJar(const std::string& databaseFilePath)
     : m_allowCookies(true)
-    , m_databaseFilePath(databaseFilePath)
 {
     // This is needed for the page cycler. See http://b/2944150
     net::CookieMonster::EnableFileScheme();
 
-    FilePath cookiePath(m_databaseFilePath.c_str());
+    FilePath cookiePath(databaseFilePath.c_str());
     scoped_refptr<SQLitePersistentCookieStore> cookieDb = new SQLitePersistentCookieStore(cookiePath);
     m_cookieStore = new net::CookieMonster(cookieDb.get(), 0);
 }
@@ -102,11 +110,6 @@ void WebCookieJar::setAllowCookies(bool allow)
     m_allowCookies = allow;
 }
 
-void WebCookieJar::cleanupFiles()
-{
-    WebRequestContext::removeFileOrDirectory(m_databaseFilePath.c_str());
-}
-
 // From CookiePolicy in chromium
 int WebCookieJar::CanGetCookies(const GURL&, const GURL&, net::CompletionCallback*)
 {
diff --git a/WebKit/android/WebCoreSupport/WebCookieJar.h b/WebKit/android/WebCoreSupport/WebCookieJar.h
index e3bfe02..ecaa228 100644
--- a/WebKit/android/WebCoreSupport/WebCookieJar.h
+++ b/WebKit/android/WebCoreSupport/WebCookieJar.h
@@ -34,9 +34,10 @@ namespace android {
 
 // This class is threadsafe. It is used from the IO, WebCore and Chromium IO
 // threads.
-class WebCookieJar : public net::CookiePolicy {
+class WebCookieJar : public net::CookiePolicy, public base::RefCountedThreadSafe<WebCookieJar> {
 public:
     static WebCookieJar* get(bool isPrivateBrowsing);
+    static void cleanup(bool isPrivateBrowsing);
 
     // CookiePolicy implementation from external/chromium
     virtual int CanGetCookies(const GURL& url, const GURL& first_party_for_cookies, net::CompletionCallback*);
@@ -44,7 +45,6 @@ public:
 
     bool allowCookies();
     void setAllowCookies(bool allow);
-    void cleanupFiles();
 
     // Instead of this it would probably be better to add the cookie methods
     // here so the rest of WebKit doesn't have to know about Chromium classes
@@ -57,7 +57,6 @@ private:
     scoped_refptr<net::CookieStore> m_cookieStore;
     bool m_allowCookies;
     WTF::Mutex m_allowCookiesMutex;
-    std::string m_databaseFilePath;
 };
 
 }
diff --git a/WebKit/android/WebCoreSupport/WebRequestContext.cpp b/WebKit/android/WebCoreSupport/WebRequestContext.cpp
index eed8863..9d0f6c9 100644
--- a/WebKit/android/WebCoreSupport/WebRequestContext.cpp
+++ b/WebKit/android/WebCoreSupport/WebRequestContext.cpp
@@ -167,13 +167,15 @@ void WebRequestContext::removeFileOrDirectory(const char* filename)
 bool WebRequestContext::cleanupPrivateBrowsingFiles()
 {
     // This is called on the UI thread.
+    // TODO: This should be done on a different thread. Moving to the WebKit
+    // thread should be straightforward and safe. See b/3243891.
     MutexLocker lock(privateBrowsingContextMutex);
 
     if (!privateBrowsingContext || privateBrowsingContext->HasOneRef()) {
         privateBrowsingContext = 0;
 
-        WebCookieJar::get(true)->cleanupFiles();
-        WebCache::get(true)->cleanupFiles();
+        WebCookieJar::cleanup(true);
+        WebCache::cleanup(true);
         return true;
     }
     return false;