Merge r165145 - ASSERT(newestManifest) fails in WebCore::ApplicationC…

…acheGroup::didFinishLoadingManifest()

https://bugs.webkit.org/show_bug.cgi?id=129753
<rdar://problem/12069835>

Reviewed by Alexey Proskuryakov.

Fixes an issue where an assertion failure would occur when visiting a web site whose on-disk
app cache doesn't contain a manifest resource.

For some reason an app cache for a web site may be partially written to disk. In particular, the
app cache may only contain a CacheGroups entry. That is, the manifest resource and origin records
may not be persisted to disk. From looking over the code, we're unclear how such a situation can occur
and hence have been unable to create such an app cache. We were able to reproduce this issue using
an app cache database file that was provided by a person that was affected by this issue.

No test included because it's not straightforward to write a test for this change.

* loader/appcache/ApplicationCacheGroup.cpp:
(WebCore::ApplicationCacheGroup::checkIfLoadIsComplete): Assert that m_cacheBeingUpdated->manifestResource()
is non-null. Currently we only document this assumption in a code comment. Also separated a single assertion
expression into two assertion expressions to make it straightforward to identify the failing sub-expression
on failure.
* loader/appcache/ApplicationCacheStorage.cpp:
(WebCore::ApplicationCacheStorage::store): Modified to call ApplicationCacheStorage::deleteCacheGroupRecord()
to remove a cache group and associated cache records (if applicable) before inserting a cache group entry.
This replacement approach will ultimately repair incomplete app cache data for people affected by this bug.
(WebCore::ApplicationCacheStorage::loadCache): Log an error and return nullptr if the cache we loaded doesn't
have a manifest resource.
(WebCore::ApplicationCacheStorage::deleteCacheGroupRecord): Added.
(WebCore::ApplicationCacheStorage::deleteCacheGroup): Extracted deletion logic for cache group record into
ApplicationCacheStorage::deleteCacheGroupRecord().
* loader/appcache/ApplicationCacheStorage.h:

Source/WebCore/ChangeLog

@@ -1,3 +1,39 @@
+2014-03-05  Daniel Bates  <dabates@apple.com>
+            And Alexey Proskuryakov  <ap@apple.com>
+
+        ASSERT(newestManifest) fails in WebCore::ApplicationCacheGroup::didFinishLoadingManifest()
+        https://bugs.webkit.org/show_bug.cgi?id=129753
+        <rdar://problem/12069835>
+
+        Reviewed by Alexey Proskuryakov.
+
+        Fixes an issue where an assertion failure would occur when visiting a web site whose on-disk
+        app cache doesn't contain a manifest resource.
+
+        For some reason an app cache for a web site may be partially written to disk. In particular, the
+        app cache may only contain a CacheGroups entry. That is, the manifest resource and origin records
+        may not be persisted to disk. From looking over the code, we're unclear how such a situation can occur
+        and hence have been unable to create such an app cache. We were able to reproduce this issue using
+        an app cache database file that was provided by a person that was affected by this issue.
+
+        No test included because it's not straightforward to write a test for this change.
+
+        * loader/appcache/ApplicationCacheGroup.cpp:
+        (WebCore::ApplicationCacheGroup::checkIfLoadIsComplete): Assert that m_cacheBeingUpdated->manifestResource()
+        is non-null. Currently we only document this assumption in a code comment. Also separated a single assertion
+        expression into two assertion expressions to make it straightforward to identify the failing sub-expression
+        on failure.
+        * loader/appcache/ApplicationCacheStorage.cpp:
+        (WebCore::ApplicationCacheStorage::store): Modified to call ApplicationCacheStorage::deleteCacheGroupRecord()
+        to remove a cache group and associated cache records (if applicable) before inserting a cache group entry.
+        This replacement approach will ultimately repair incomplete app cache data for people affected by this bug.
+        (WebCore::ApplicationCacheStorage::loadCache): Log an error and return nullptr if the cache we loaded doesn't
+        have a manifest resource.
+        (WebCore::ApplicationCacheStorage::deleteCacheGroupRecord): Added.
+        (WebCore::ApplicationCacheStorage::deleteCacheGroup): Extracted deletion logic for cache group record into
+        ApplicationCacheStorage::deleteCacheGroupRecord().
+        * loader/appcache/ApplicationCacheStorage.h:
+
 2014-03-05  David Kilzer  <ddkilzer@apple.com>
 
         Fix crash in CompositeEditCommand::cloneParagraphUnderNewElement()

Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp

@@ -898,7 +898,9 @@ void ApplicationCacheGroup::checkIfLoadIsComplete()
             // a failure of the cache storage to save the newest cache due to hitting
             // the maximum size. In such a case, m_manifestResource may be 0, as
             // the manifest was already set on the newest cache object.
-            ASSERT(cacheStorage().isMaximumSizeReached() && m_calledReachedMaxAppCacheSize);
+            ASSERT(m_cacheBeingUpdated->manifestResource());
+            ASSERT(cacheStorage().isMaximumSizeReached());
+            ASSERT(m_calledReachedMaxAppCacheSize);
         }
 
         RefPtr<ApplicationCache> oldNewestCache = (m_newestCache == m_cacheBeingUpdated) ? RefPtr<ApplicationCache>() : m_newestCache;

Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp

@@ -694,6 +694,12 @@ bool ApplicationCacheStorage::store(ApplicationCacheGroup* group, GroupStorageID
     ASSERT(group->storageID() == 0);
     ASSERT(journal);
 
+    // For some reason, an app cache may be partially written to disk. In particular, there may be
+    // a cache group with an identical manifest URL and associated cache entries. We want to remove
+    // this cache group and its associated cache entries so that we can create it again (below) as
+    // a way to repair it.
+    deleteCacheGroupRecord(group->manifestURL());
+
     SQLiteStatement statement(m_database, "INSERT INTO CacheGroups (manifestHostHash, manifestURL, origin) VALUES (?, ?, ?)");
     if (statement.prepare() != SQLResultOk)
         return false;
@@ -1176,7 +1182,12 @@ PassRefPtr<ApplicationCache> ApplicationCacheStorage::loadCache(unsigned storage
 
     if (result != SQLResultDone)
         LOG_ERROR("Could not load cache resources, error \"%s\"", m_database.lastErrorMsg());
-
+
+    if (!cache->manifestResource()) {
+        LOG_ERROR("Could not load application cache because there was no manifest resource");
+        return nullptr;
+    }
+
     // Load the online whitelist
     SQLiteStatement whitelistStatement(m_database, "SELECT url FROM CacheWhitelistURLs WHERE cache=?");
     if (whitelistStatement.prepare() != SQLResultOk)
@@ -1418,6 +1429,36 @@ bool ApplicationCacheStorage::cacheGroupSize(const String& manifestURL, int64_t*
     return true;
 }
 
+bool ApplicationCacheStorage::deleteCacheGroupRecord(const String& manifestURL)
+{
+    ASSERT(SQLiteDatabaseTracker::hasTransactionInProgress());
+    SQLiteStatement idStatement(m_database, "SELECT id FROM CacheGroups WHERE manifestURL=?");
+    if (idStatement.prepare() != SQLResultOk)
+        return false;
+
+    idStatement.bindText(1, manifestURL);
+
+    int result = idStatement.step();
+    if (result != SQLResultRow)
+        return false;
+
+    int64_t groupId = idStatement.getColumnInt64(0);
+
+    SQLiteStatement cacheStatement(m_database, "DELETE FROM Caches WHERE cacheGroup=?");
+    if (cacheStatement.prepare() != SQLResultOk)
+        return false;
+
+    SQLiteStatement groupStatement(m_database, "DELETE FROM CacheGroups WHERE id=?");
+    if (groupStatement.prepare() != SQLResultOk)
+        return false;
+
+    cacheStatement.bindInt64(1, groupId);
+    executeStatement(cacheStatement);
+    groupStatement.bindInt64(1, groupId);
+    executeStatement(groupStatement);
+    return true;
+}
+
 bool ApplicationCacheStorage::deleteCacheGroup(const String& manifestURL)
 {
     SQLiteTransactionInProgressAutoCounter transactionCounter;
@@ -1432,36 +1473,10 @@ bool ApplicationCacheStorage::deleteCacheGroup(const String& manifestURL)
         openDatabase(false);
         if (!m_database.isOpen())
             return false;
-
-        SQLiteStatement idStatement(m_database, "SELECT id FROM CacheGroups WHERE manifestURL=?");
-        if (idStatement.prepare() != SQLResultOk)
-            return false;
-
-        idStatement.bindText(1, manifestURL);
-
-        int result = idStatement.step();
-        if (result == SQLResultDone)
-            return false;
-
-        if (result != SQLResultRow) {
-            LOG_ERROR("Could not load cache group id, error \"%s\"", m_database.lastErrorMsg());
+        if (!deleteCacheGroupRecord(manifestURL)) {
+            LOG_ERROR("Could not delete cache group record, error \"%s\"", m_database.lastErrorMsg());
             return false;
         }
-
-        int64_t groupId = idStatement.getColumnInt64(0);
-
-        SQLiteStatement cacheStatement(m_database, "DELETE FROM Caches WHERE cacheGroup=?");
-        if (cacheStatement.prepare() != SQLResultOk)
-            return false;
-
-        SQLiteStatement groupStatement(m_database, "DELETE FROM CacheGroups WHERE id=?");
-        if (groupStatement.prepare() != SQLResultOk)
-            return false;
-
-        cacheStatement.bindInt64(1, groupId);
-        executeStatement(cacheStatement);
-        groupStatement.bindInt64(1, groupId);
-        executeStatement(groupStatement);
     }
 
     deleteTransaction.commit();

Source/WebCore/loader/appcache/ApplicationCacheStorage.h

@@ -110,6 +110,7 @@ class ApplicationCacheStorage {
     bool store(ApplicationCacheGroup*, GroupStorageIDJournal*);
     bool store(ApplicationCache*, ResourceStorageIDJournal*);
     bool store(ApplicationCacheResource*, unsigned cacheStorageID);
+    bool deleteCacheGroupRecord(const String& manifestURL);
 
     bool ensureOriginRecord(const SecurityOrigin*);
     bool shouldStoreResourceAsFlatFile(ApplicationCacheResource*);