Merge r223442 - If an origin doesn't have databases in the Databases …

…table we should still remove its information from disk in DatabaseTracker::deleteOrigin() https://bugs.webkit.org/show_bug.cgi?id=178281 <rdar://problem/34576132> Patch by Maureen Daum <mdaum@apple.com> on 2017-10-16 Reviewed by Brent Fulgham. Source/WebCore: New test: DatabaseTracker.DeleteOriginWithMissingEntryInDatabasesTable * Modules/webdatabase/DatabaseTracker.cpp: (WebCore::DatabaseTracker::deleteOrigin): If databaseNames is empty, don't bail early. Instead, delete everything in the directory containing the databases for this origin. This condition indicates that we previously tried to remove the origin but didn't get all of the way through the deletion process. Because we have lost track of the databases for this origin, we can assume that no other process is accessing them. This means it should be safe to delete them outright. Tools: Verify that if there is an entry in the Origins table but no entries in the Databases table that we still remove the directory for the origin, and that we remove the entry from the Origins table. * TestWebKitAPI/Tests/WebCore/cocoa/DatabaseTrackerTest.mm: (TestWebKitAPI::TEST):
WebKit · Oct 17, 2017 · 6e93d4f · 6e93d4f
1 parent 782857a
commit 6e93d4f
Show file tree

Hide file tree

Showing 4 changed files with 89 additions and 3 deletions.
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,22 @@
+2017-10-16  Maureen Daum  <mdaum@apple.com>
+
+        If an origin doesn't have databases in the Databases table we should still remove its information from disk in DatabaseTracker::deleteOrigin()
+        https://bugs.webkit.org/show_bug.cgi?id=178281
+        <rdar://problem/34576132>
+
+        Reviewed by Brent Fulgham.
+
+        New test:
+        DatabaseTracker.DeleteOriginWithMissingEntryInDatabasesTable
+
+        * Modules/webdatabase/DatabaseTracker.cpp:
+        (WebCore::DatabaseTracker::deleteOrigin):
+        If databaseNames is empty, don't bail early. Instead, delete everything in the directory
+        containing the databases for this origin. This condition indicates that we previously
+        tried to remove the origin but didn't get all of the way through the deletion process.
+        Because we have lost track of the databases for this origin, we can assume that no
+        other process is accessing them. This means it should be safe to delete them outright.
+
 2017-10-16  Ryan Haddad  <ryanhaddad@apple.com>
 
         Unreviewed attempt to fix the Windows debug build.

diff --git a/Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp b/Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp
@@ -823,10 +823,9 @@ bool DatabaseTracker::deleteOrigin(const SecurityOriginData& origin, DeletionMod
             return false;
 
         databaseNames = databaseNamesNoLock(origin);
-        if (databaseNames.isEmpty()) {
+        if (databaseNames.isEmpty())
             LOG_ERROR("Unable to retrieve list of database names for origin %s", origin.databaseIdentifier().utf8().data());
-            return false;
-        }
+
         if (!canDeleteOrigin(origin)) {
             LOG_ERROR("Tried to delete an origin (%s) while either creating database in it or already deleting it", origin.databaseIdentifier().utf8().data());
             ASSERT_NOT_REACHED();
@@ -845,6 +844,20 @@ bool DatabaseTracker::deleteOrigin(const SecurityOriginData& origin, DeletionMod
         }
     }
 
+    // If databaseNames is empty, delete everything in the directory containing the databases for this origin.
+    // This condition indicates that we previously tried to remove the origin but didn't get all of the way
+    // through the deletion process. Because we have lost track of the databases for this origin,
+    // we can assume that no other process is accessing them. This means it should be safe to delete them outright.
+    if (databaseNames.isEmpty()) {
+#if PLATFORM(COCOA)
+        RELEASE_LOG_ERROR(DatabaseTracker, "Unable to retrieve list of database names for origin");
+#endif
+        for (const auto& file : listDirectory(originPath(origin), "*")) {
+            if (!deleteFile(file))
+                failedToDeleteAnyDatabaseFile = true;
+        }
+    }
+
     // If we failed to delete any database file, don't remove the origin from the tracker
     // database because we didn't successfully remove all of its data.
     if (failedToDeleteAnyDatabaseFile) {

diff --git a/Tools/ChangeLog b/Tools/ChangeLog
@@ -1,3 +1,18 @@
+2017-10-16  Maureen Daum  <mdaum@apple.com>
+
+        If an origin doesn't have databases in the Databases table we should still remove its information from disk in DatabaseTracker::deleteOrigin()
+        https://bugs.webkit.org/show_bug.cgi?id=178281
+        <rdar://problem/34576132>
+
+        Reviewed by Brent Fulgham.
+
+        Verify that if there is an entry in the Origins table but no entries in the Databases
+        table that we still remove the directory for the origin, and that we remove the
+        entry from the Origins table.
+
+        * TestWebKitAPI/Tests/WebCore/cocoa/DatabaseTrackerTest.mm:
+        (TestWebKitAPI::TEST):
+
 2017-10-16  Maureen Daum  <mdaum@apple.com>
 
         If we fail to delete any database file, don't remove its information from the tracker database

diff --git a/Tools/TestWebKitAPI/Tests/WebCore/cocoa/DatabaseTrackerTest.mm b/Tools/TestWebKitAPI/Tests/WebCore/cocoa/DatabaseTrackerTest.mm
@@ -228,6 +228,45 @@ static void createFileAtPath(const String& path)
     EXPECT_FALSE(fileExists(databaseDirectoryPath));
 }
 
+TEST(DatabaseTracker, DeleteOriginWithMissingEntryInDatabasesTable)
+{
+    // Test the case where there is an entry in the Origins table but not
+    // the Databases table. There is an actual database on disk.
+    // The information should still be removed from the Origins table, and the
+    // database should be deleted from disk.
+    NSString *webSQLDirectory = createTemporaryDirectory(@"WebSQL");
+    String databaseDirectoryPath(webSQLDirectory.UTF8String);
+
+    std::unique_ptr<DatabaseTracker> databaseTracker = DatabaseTracker::trackerWithDatabasePath(databaseDirectoryPath);
+    SecurityOriginData origin("https", "webkit.org", 443);
+
+    databaseTracker->setQuota(origin, 5242880);
+    EXPECT_EQ((unsigned)1, databaseTracker->origins().size());
+
+    String databasePath = pathByAppendingComponent(databaseDirectoryPath, "Databases.db");
+    EXPECT_TRUE(fileExists(databasePath));
+
+    EXPECT_TRUE(databaseTracker->databaseNames(origin).isEmpty());
+
+    String originPath = pathByAppendingComponent(databaseDirectoryPath, origin.databaseIdentifier());
+    EXPECT_TRUE(makeAllDirectories(originPath));
+    EXPECT_TRUE(fileExists(originPath));
+
+    String webDatabasePath = pathByAppendingComponent(originPath, "database.db");
+    createFileAtPath(webDatabasePath);
+
+    EXPECT_TRUE(databaseTracker->deleteOrigin(origin));
+
+    EXPECT_FALSE(fileExists(webDatabasePath));
+    EXPECT_FALSE(fileExists(originPath));
+
+    EXPECT_TRUE(databaseTracker->origins().isEmpty());
+    EXPECT_TRUE(databaseTracker->databaseNames(origin).isEmpty());
+
+    removeDirectoryAndAllContents(databaseDirectoryPath);
+    EXPECT_FALSE(fileExists(databaseDirectoryPath));
+}
+
 TEST(DatabaseTracker, DeleteDatabase)
 {
     // Test the expected case. There is an entry in the Databases table