1. Changes the order in which some functions are called to match
authordumi@chromium.org <dumi@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 29 Dec 2009 20:51:32 +0000 (20:51 +0000)
committerdumi@chromium.org <dumi@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 29 Dec 2009 20:51:32 +0000 (20:51 +0000)
the pre-r52536 order. Namely, when a new Database object is
created, DatabaseTracker::addOpenDatabase() is called in the
constructor, before doing anything else related to that database
(like trying to get a file handle to the database
file). Chromium's implementation depends on this ordering.
2. Changes Database::performOpenAndVerify() to close the open
handle to the database file immediately if the database version
does not match the expected one. The current behavior is to add
the Database object to a DatabaseThread collection and let the
database thread close the handle when it's destroyed.

Reviewed by Maciej Stachowiak.

https://bugs.webkit.org/show_bug.cgi?id=33005

All LayoutTests/storage tests pass in clean WebKit and Chromium
clients.

* storage/Database.cpp:
(WebCore::Database::openDatabase): Notify DatabaseTracker and
Document that a Database object is about to be destroyed (when a
database file cannot be opened, or its version doesn't match the
expected one).
(WebCore::Database::Database): Notify DatabaseTracker and Document
that a new Database object was created.
(WebCore::Database::performOpenAndVerify): If a database version
does not match the expected one, immediately close the open file
handle to the database file.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@52631 268f45cc-cd09-0410-ab3c-d52691b4dbfc

WebCore/ChangeLog
WebCore/storage/Database.cpp

index 501e34234e6707a5505a42ea29c14d92196fcae9..ce94af891e47ddf633e73b40642d7e6ec4c6b333 100644 (file)
@@ -1,3 +1,35 @@
+2009-12-28  Dumitru Daniliuc  <dumi@chromium.org>
+
+        Reviewed by Maciej Stachowiak.
+
+        1. Changes the order in which some functions are called to match
+        the pre-r52536 order. Namely, when a new Database object is
+        created, DatabaseTracker::addOpenDatabase() is called in the
+        constructor, before doing anything else related to that database
+        (like trying to get a file handle to the database
+        file). Chromium's implementation depends on this ordering.
+        2. Changes Database::performOpenAndVerify() to close the open
+        handle to the database file immediately if the database version
+        does not match the expected one. The current behavior is to add
+        the Database object to a DatabaseThread collection and let the
+        database thread close the handle when it's destroyed.
+
+        https://bugs.webkit.org/show_bug.cgi?id=33005
+
+        All LayoutTests/storage tests pass in clean WebKit and Chromium
+        clients.
+
+        * storage/Database.cpp:
+        (WebCore::Database::openDatabase): Notify DatabaseTracker and
+        Document that a Database object is about to be destroyed (when a
+        database file cannot be opened, or its version doesn't match the
+        expected one).
+        (WebCore::Database::Database): Notify DatabaseTracker and Document
+        that a new Database object was created.
+        (WebCore::Database::performOpenAndVerify): If a database version
+        does not match the expected one, immediately close the open file
+        handle to the database file.
+
 2009-12-29  Nikolas Zimmermann  <nzimmermann@rim.com>
 
         Reviewed by Dirk Schulze.
index aca66b057a285ea1e2b30bd1a351ebfcda23dd75..305f5f9279f3094b0dcab9a984d221d63212e935 100644 (file)
@@ -131,13 +131,12 @@ PassRefPtr<Database> Database::openDatabase(Document* document, const String& na
     RefPtr<Database> database = adoptRef(new Database(document, name, expectedVersion, displayName, estimatedSize));
 
     if (!database->openAndVerifyVersion(e)) {
-       LOG(StorageAPI, "Failed to open and verify version (expected %s) of database %s", expectedVersion.ascii().data(), database->databaseDebugName().ascii().data());
-       return 0;
+        LOG(StorageAPI, "Failed to open and verify version (expected %s) of database %s", expectedVersion.ascii().data(), database->databaseDebugName().ascii().data());
+        document->removeOpenDatabase(database.get());
+        DatabaseTracker::tracker().removeOpenDatabase(database.get());
+        return 0;
     }
 
-    DatabaseTracker::tracker().addOpenDatabase(database.get());
-    document->addOpenDatabase(database.get());
-
     DatabaseTracker::tracker().setDatabaseDetails(document->securityOrigin(), name, displayName, estimatedSize);
 
     document->setHasOpenDatabases();
@@ -189,6 +188,9 @@ Database::Database(Document* document, const String& name, const String& expecte
     ASSERT(m_document->databaseThread());
 
     m_filename = DatabaseTracker::tracker().fullPathForDatabase(m_mainThreadSecurityOrigin.get(), m_name);
+
+    DatabaseTracker::tracker().addOpenDatabase(this);
+    m_document->addOpenDatabase(this);
 }
 
 static void derefDocument(void* document)
@@ -480,6 +482,8 @@ bool Database::performOpenAndVerify(ExceptionCode& e)
                 if (!m_sqliteDatabase.executeCommand("CREATE TABLE " + databaseInfoTableName() + " (key TEXT NOT NULL ON CONFLICT FAIL UNIQUE ON CONFLICT REPLACE,value TEXT NOT NULL ON CONFLICT FAIL);")) {
                     LOG_ERROR("Unable to create table %s in database %s", databaseInfoTableName().ascii().data(), databaseDebugName().ascii().data());
                     e = INVALID_STATE_ERR;
+                    // Close the handle to the database file.
+                    m_sqliteDatabase.close();
                     return false;
                 }
             }
@@ -487,6 +491,8 @@ bool Database::performOpenAndVerify(ExceptionCode& e)
             if (!getVersionFromDatabase(currentVersion)) {
                 LOG_ERROR("Failed to get current version from database %s", databaseDebugName().ascii().data());
                 e = INVALID_STATE_ERR;
+                // Close the handle to the database file.
+                m_sqliteDatabase.close();
                 return false;
             }
             if (currentVersion.length()) {
@@ -496,6 +502,8 @@ bool Database::performOpenAndVerify(ExceptionCode& e)
                 if (!setVersionInDatabase(m_expectedVersion)) {
                     LOG_ERROR("Failed to set version %s in database %s", m_expectedVersion.ascii().data(), databaseDebugName().ascii().data());
                     e = INVALID_STATE_ERR;
+                    // Close the handle to the database file.
+                    m_sqliteDatabase.close();
                     return false;
                 }
                 currentVersion = m_expectedVersion;
@@ -516,9 +524,13 @@ bool Database::performOpenAndVerify(ExceptionCode& e)
         LOG(StorageAPI, "page expects version %s from database %s, which actually has version name %s - openDatabase() call will fail", m_expectedVersion.ascii().data(),
             databaseDebugName().ascii().data(), currentVersion.ascii().data());
         e = INVALID_STATE_ERR;
+        // Close the handle to the database file.
+        m_sqliteDatabase.close();
         return false;
     }
 
+    // All checks passed and we still have a handle to this database file.
+    // Make sure DatabaseThread closes it when DatabaseThread goes away.
     m_opened = true;
     if (m_document->databaseThread())
         m_document->databaseThread()->recordDatabaseOpen(this);