From 6e93d4fd5d54a478817721f3ce75e7837fac522a Mon Sep 17 00:00:00 2001 From: Maureen Daum Date: Tue, 17 Oct 2017 14:20:09 +0000 Subject: [PATCH] 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 Patch by Maureen Daum 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): --- Source/WebCore/ChangeLog | 19 +++++++++ .../Modules/webdatabase/DatabaseTracker.cpp | 19 +++++++-- Tools/ChangeLog | 15 +++++++ .../WebCore/cocoa/DatabaseTrackerTest.mm | 39 +++++++++++++++++++ 4 files changed, 89 insertions(+), 3 deletions(-) diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog index f809fe7fed6f..3bd325fc3570 100644 --- a/Source/WebCore/ChangeLog +++ b/Source/WebCore/ChangeLog @@ -1,3 +1,22 @@ +2017-10-16 Maureen Daum + + 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 + + + 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 Unreviewed attempt to fix the Windows debug build. diff --git a/Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp b/Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp index f0fec671d02b..552769a41c74 100644 --- 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 index 709f92f191d7..7ef16adaae81 100644 --- a/Tools/ChangeLog +++ b/Tools/ChangeLog @@ -1,3 +1,18 @@ +2017-10-16 Maureen Daum + + 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 + + + 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 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 index 730a0eaf1a84..a20b38315668 100644 --- 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::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