AppCaches which belong to hosted apps are not protected from deletion

content/browser/appcache/chrome_appcache_service_unittest.cc

Index: content/browser/appcache/chrome_appcache_service_unittest.cc
diff --git a/content/browser/appcache/chrome_appcache_service_unittest.cc b/content/browser/appcache/chrome_appcache_service_unittest.cc
index 7f99286c83e963e1c3ccddd812d54acd42855c65..479bbe430e4e2c3f75f66ee4fb8d9d0ee8154d9f 100644
--- a/content/browser/appcache/chrome_appcache_service_unittest.cc
+++ b/content/browser/appcache/chrome_appcache_service_unittest.cc
@@ -6,17 +6,28 @@
 #include "base/memory/ref_counted.h"
 #include "base/message_loop.h"
 #include "base/scoped_temp_dir.h"
+#include "chrome/test/appcache_util.h"
 #include "chrome/test/testing_browser_process.h"
 #include "chrome/test/testing_browser_process_test.h"
 #include "content/browser/appcache/chrome_appcache_service.h"
 #include "content/browser/browser_thread.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "webkit/appcache/appcache_database.h"
 #include "webkit/appcache/appcache_storage_impl.h"
+#include "webkit/quota/mock_special_storage_policy.h"
+
+#include <set>
 
 namespace {
 const FilePath::CharType kTestingAppCacheDirname[] =
     FILE_PATH_LITERAL("Application Cache");
-}
+
+// Examples of a protected and an unprotected origin, to be used througout the
+// test.
+const GURL kProtectedManifest("http://www.protected.com/cache.manifest");
+const GURL kNormalManifest("http://www.normal.com/cache.manifest");

[michaeln, 2011/07/15 19:15:46]

Can you make these non-primitives data members of ChromeAppCacheServiceTest to
avoid the construction at module load time (or do something else to avoid
running GURL ctors at load time). In production code, we pretty much dissallow
non-primitive globals.

Some ideas...

- make these GURLs data members in contruct them in ChromeAppCacheServiceTest()
- put const char kProtectedManifest[] =
"http://www.protected.com/cache.manifest" as globals and construct
GURL(kProtectedManifest) as needed in the tests code

[marja(google), 2011/07/18 09:55:41]

Done for this file. But I left the GURLs in browsing_data_remover_unittest.cc
untouched (some of them were added by me in a previous cl, some of them were
added by others). I can create a separate cl for that.

+
+}  // namespace
 
 namespace appcache {
 
@@ -31,6 +42,12 @@ class ChromeAppCacheServiceTest : public TestingBrowserProcessTest {
   }
 
  protected:
+  void CreateAppCacheService(
+      scoped_refptr<ChromeAppCacheService>& appache_service,
+      const FilePath& appcache_path,
+      bool init_storage);
+  void InsertDataIntoAppCache(ChromeAppCacheService* appcache_service);
+
   MessageLoop message_loop_;
   ScopedTempDir temp_dir_;
 
@@ -41,68 +58,108 @@ class ChromeAppCacheServiceTest : public TestingBrowserProcessTest {
   BrowserThread io_thread_;
 };
 
-TEST_F(ChromeAppCacheServiceTest, KeepOnDestruction) {
-  ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
-  FilePath appcache_path = temp_dir_.path().Append(kTestingAppCacheDirname);
-  scoped_refptr<ChromeAppCacheService> appcache_service =
-      new ChromeAppCacheService(NULL);
+void ChromeAppCacheServiceTest::CreateAppCacheService(
+    scoped_refptr<ChromeAppCacheService>& appcache_service,

[michaeln, 2011/07/15 19:15:46]

Google style thing... we discourage non-const references

It'd be fine to return a scoped_refptr<T> from this method.

[marja(google), 2011/07/18 09:55:41]

The reason for not returning scoped_refptr was to be able to have ASSERTs in the
function. I made the appcache_service parameter a pointer instead of a non-const
reference, is that ok?

[marja(google), 2011/07/18 15:03:20]

Hmm, it seems that ASSSERT_* in a non-test function didn't do what I expected it
to do; it marked the test as failure, but didn't terminate it. I modified the
return value to be scoped_refptr<ChromeAppCacheService>. I didn't think it's
worth to also "return" (via a pointer or such) a bool which would contain the
outcome of the removed ASSERTs, that the test would've then needed to check. Is
the current version ok?

[marja(google), 2011/07/18 15:03:20]

Done.

+    const FilePath& appcache_path,
+    bool init_storage) {
+  appcache_service = new ChromeAppCacheService(NULL);
   const content::ResourceContext* resource_context = NULL;
+  scoped_refptr<quota::MockSpecialStoragePolicy> mock_policy =
+      new quota::MockSpecialStoragePolicy;
+  mock_policy->AddProtected(kProtectedManifest.GetOrigin());
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       NewRunnableMethod(appcache_service.get(),
                         &ChromeAppCacheService::InitializeOnIOThread,
                         appcache_path,
                         resource_context,
-                        scoped_refptr<quota::SpecialStoragePolicy>(NULL),
-                        false));
+                        mock_policy));
   // Make the steps needed to initialize the storage of AppCache data.
   message_loop_.RunAllPending();
-  appcache::AppCacheStorageImpl* storage =
-      static_cast<appcache::AppCacheStorageImpl*>(appcache_service->storage());
-  ASSERT_TRUE(storage->database_->db_connection());
-  ASSERT_EQ(1, storage->NewCacheId());
-  storage->disk_cache();
-  message_loop_.RunAllPending();
+  if (init_storage) {
+    appcache::AppCacheStorageImpl* storage =
+        static_cast<appcache::AppCacheStorageImpl*>(
+            appcache_service->storage());
+    ASSERT_TRUE(storage->database_->db_connection());
+    ASSERT_EQ(1, storage->NewCacheId());
+    storage->disk_cache();
+    message_loop_.RunAllPending();
+  }
+}
+
+void ChromeAppCacheServiceTest::InsertDataIntoAppCache(
+    ChromeAppCacheService* appcache_service) {
+  AppCacheInserter appcache_inserter;
+  appcache_inserter.AddGroupAndCache(appcache_service, kNormalManifest);
+  appcache_inserter.AddGroupAndCache(appcache_service, kProtectedManifest);
+
+  // Verify that adding the data succeeded
+  AppCacheReader reader(appcache_service);
+  std::set<GURL>* origins = reader.ReadAppCaches();
+  ASSERT_EQ(2UL, origins->size());
+  ASSERT_TRUE(origins->find(kProtectedManifest.GetOrigin()) != origins->end());
+  ASSERT_TRUE(origins->find(kNormalManifest.GetOrigin()) != origins->end());
+}
 
+TEST_F(ChromeAppCacheServiceTest, KeepOnDestruction) {
+  ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+  FilePath appcache_path = temp_dir_.path().Append(kTestingAppCacheDirname);
+
+  // Create a ChromeAppCacheService and insert data into it
+  scoped_refptr<ChromeAppCacheService> appcache_service;
+  CreateAppCacheService(appcache_service, appcache_path, true);
   ASSERT_TRUE(file_util::PathExists(appcache_path));
   ASSERT_TRUE(file_util::PathExists(appcache_path.AppendASCII("Index")));
+  InsertDataIntoAppCache(appcache_service);
 
+  // Test: delete the ChromeAppCacheService
+  appcache_service->set_clear_local_state_on_exit(false);
   appcache_service = NULL;
   message_loop_.RunAllPending();
 
+  // Recreate the appcache (for reading the data back)
+  CreateAppCacheService(appcache_service, appcache_path, false);
+
+  // The directory is still there
   ASSERT_TRUE(file_util::PathExists(appcache_path));
+
+  // The appcache data is also there
+  AppCacheReader reader(appcache_service);
+  std::set<GURL>* origins = reader.ReadAppCaches();
+  EXPECT_EQ(2UL, origins->size());
+  EXPECT_TRUE(origins->find(kProtectedManifest.GetOrigin()) != origins->end());
+  EXPECT_TRUE(origins->find(kNormalManifest.GetOrigin()) != origins->end());
 }
 
 TEST_F(ChromeAppCacheServiceTest, RemoveOnDestruction) {
   ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
   FilePath appcache_path = temp_dir_.path().Append(kTestingAppCacheDirname);
-  scoped_refptr<ChromeAppCacheService> appcache_service =
-      new ChromeAppCacheService(NULL);
-  const content::ResourceContext* resource_context = NULL;
-  BrowserThread::PostTask(
-      BrowserThread::IO, FROM_HERE,
-      NewRunnableMethod(appcache_service.get(),
-                        &ChromeAppCacheService::InitializeOnIOThread,
-                        appcache_path,
-                        resource_context,
-                        scoped_refptr<quota::SpecialStoragePolicy>(NULL),
-                        true));
-  // Make the steps needed to initialize the storage of AppCache data.
-  message_loop_.RunAllPending();
-  appcache::AppCacheStorageImpl* storage =
-      static_cast<appcache::AppCacheStorageImpl*>(appcache_service->storage());
-  ASSERT_TRUE(storage->database_->db_connection());
-  ASSERT_EQ(1, storage->NewCacheId());
-  storage->disk_cache();
-  message_loop_.RunAllPending();
 
+  // Create a ChromeAppCacheService and insert data into it
+  scoped_refptr<ChromeAppCacheService> appcache_service;
+  CreateAppCacheService(appcache_service, appcache_path, true);
   ASSERT_TRUE(file_util::PathExists(appcache_path));
   ASSERT_TRUE(file_util::PathExists(appcache_path.AppendASCII("Index")));
+  InsertDataIntoAppCache(appcache_service);
 
+  // Test: delete the ChromeAppCacheService
+  appcache_service->set_clear_local_state_on_exit(true);
   appcache_service = NULL;
   message_loop_.RunAllPending();
 
-  ASSERT_FALSE(file_util::PathExists(appcache_path));
+  // Recreate the appcache (for reading the data back)
+  CreateAppCacheService(appcache_service, appcache_path, false);
+
+  // The directory is still there
+  ASSERT_TRUE(file_util::PathExists(appcache_path));
+
+  // The appcache data for the protected origin is there, and the data for the
+  // unprotected origin was deleted.
+  AppCacheReader reader(appcache_service);
+  std::set<GURL>* origins = reader.ReadAppCaches();
+  EXPECT_EQ(1UL, origins->size());
+  EXPECT_TRUE(origins->find(kProtectedManifest.GetOrigin()) != origins->end());
+  EXPECT_TRUE(origins->find(kNormalManifest.GetOrigin()) == origins->end());
 }
 
 }  // namespace appcache