Changeset 165145 in webkit

Timestamp:

Mar 5, 2014 5:02:45 PM (10 years ago)

Author:

dbates@webkit.org

Message:

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:

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• loader/appcache/ApplicationCacheGroup.cpp (modified) (1 diff)
• loader/appcache/ApplicationCacheStorage.cpp (modified) (4 diffs)
• loader/appcache/ApplicationCacheStorage.h (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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  Oliver Hunt  <oliver@apple.com>
 

trunk/Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp

@@ -903 +903 @@
             // 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);
         }
 

trunk/Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp

@@ -695 +695 @@
     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)
@@ -1176 +1182 @@
     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=?");
@@ -1415 +1426 @@
 
     *size = statement.getColumnInt64(0);
+    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;
 }
@@ -1432 +1473 @@
         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);
     }
 

trunk/Source/WebCore/loader/appcache/ApplicationCacheStorage.h

@@ -111 +111 @@
     bool store(ApplicationCache*, ResourceStorageIDJournal*);
     bool store(ApplicationCacheResource*, unsigned cacheStorageID);
+    bool deleteCacheGroupRecord(const String& manifestURL);
 
     bool ensureOriginRecord(const SecurityOrigin*);