Changeset 92155 in webkit

Timestamp:

Aug 1, 2011 5:21:56 PM (13 years ago)

Author:

Michael Nordman

Message:

[Chromium] WebSQLDatabase version handling is broken in multi-process browsers.
​https://bugs.webkit.org/show_bug.cgi?id=65486

The WebCore::AbstractDatabase class maintains a global in-memory map of
the version numbers associated with open database files, but that map is
not reliable in a multi-process system like Chrome. So instead of relying
on the cached values in that map, we read the value from the database (and
update the cached value) where possible. There are two edge cases where that's
not possible because the scriptable interface requires synchronous access
to the version: the .version attribute getter and the .openDatabase() method.
In those cases, we have no choice but to use the potentially stale cached value.

Reviewed by Darin Fisher.

No new tests. Existing layout tests cover the version handling functionality.

• storage/AbstractDatabase.cpp:

(WebCore::AbstractDatabase::version):
(WebCore::AbstractDatabase::performOpenAndVerify):
(WebCore::AbstractDatabase::getVersionFromDatabase):
(WebCore::AbstractDatabase::setVersionInDatabase):
(WebCore::AbstractDatabase::setExpectedVersion):
(WebCore::AbstractDatabase::getCachedVersion):
(WebCore::AbstractDatabase::setCachedVersion):
(WebCore::AbstractDatabase::getActualVersionForTransaction):

• storage/AbstractDatabase.h:

(WebCore::AbstractDatabase::expectedVersion):

• storage/ChangeVersionWrapper.cpp:

(WebCore::ChangeVersionWrapper::handleCommitFailedAfterPostflight):

• storage/ChangeVersionWrapper.h:
• storage/Database.cpp:

(WebCore::Database::openDatabase):

• storage/DatabaseSync.cpp:

(WebCore::DatabaseSync::openDatabaseSync):
(WebCore::DatabaseSync::changeVersion):

• storage/SQLTransaction.cpp:

(WebCore::SQLTransaction::SQLTransaction):
(WebCore::SQLTransaction::executeSQL):
(WebCore::SQLTransaction::openTransactionAndPreflight):
(WebCore::SQLTransaction::runCurrentStatement):
(WebCore::SQLTransaction::postflightAndCommit):

• storage/SQLTransaction.h:
• storage/SQLTransactionSync.cpp:

(WebCore::SQLTransactionSync::SQLTransactionSync):
(WebCore::SQLTransactionSync::executeSQL):
(WebCore::SQLTransactionSync::begin):

• storage/SQLTransactionSync.h:

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• storage/AbstractDatabase.cpp (modified) (12 diffs)
• storage/AbstractDatabase.h (modified) (3 diffs)
• storage/ChangeVersionWrapper.cpp (modified) (1 diff)
• storage/ChangeVersionWrapper.h (modified) (1 diff)
• storage/Database.cpp (modified) (1 diff)
• storage/DatabaseSync.cpp (modified) (2 diffs)
• storage/SQLTransaction.cpp (modified) (5 diffs)
• storage/SQLTransaction.h (modified) (2 diffs)
• storage/SQLTransactionSync.cpp (modified) (3 diffs)
• storage/SQLTransactionSync.h (modified) (2 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2011-08-01  Michael Nordman  <michaeln@google.com>
+
+        [Chromium] WebSQLDatabase version handling is broken in multi-process browsers.
+        https://bugs.webkit.org/show_bug.cgi?id=65486
+
+        The WebCore::AbstractDatabase class maintains a global in-memory map of
+        the version numbers associated with open database files, but that map is
+        not reliable in a multi-process system like Chrome. So instead of relying
+        on the cached values in that map, we read the value from the database (and
+        update the cached value) where possible. There are two edge cases where that's
+        not possible because the scriptable interface requires synchronous access
+        to the version: the .version attribute getter and the .openDatabase() method.
+        In those cases, we have no choice but to use the potentially stale cached value.
+
+        Reviewed by Darin Fisher.
+
+        No new tests. Existing layout tests cover the version handling functionality.
+
+        * storage/AbstractDatabase.cpp:
+        (WebCore::AbstractDatabase::version):
+        (WebCore::AbstractDatabase::performOpenAndVerify):
+        (WebCore::AbstractDatabase::getVersionFromDatabase):
+        (WebCore::AbstractDatabase::setVersionInDatabase):
+        (WebCore::AbstractDatabase::setExpectedVersion):
+        (WebCore::AbstractDatabase::getCachedVersion):
+        (WebCore::AbstractDatabase::setCachedVersion):
+        (WebCore::AbstractDatabase::getActualVersionForTransaction):
+        * storage/AbstractDatabase.h:
+        (WebCore::AbstractDatabase::expectedVersion):
+        * storage/ChangeVersionWrapper.cpp:
+        (WebCore::ChangeVersionWrapper::handleCommitFailedAfterPostflight):
+        * storage/ChangeVersionWrapper.h:
+        * storage/Database.cpp:
+        (WebCore::Database::openDatabase):
+        * storage/DatabaseSync.cpp:
+        (WebCore::DatabaseSync::openDatabaseSync):
+        (WebCore::DatabaseSync::changeVersion):
+        * storage/SQLTransaction.cpp:
+        (WebCore::SQLTransaction::SQLTransaction):
+        (WebCore::SQLTransaction::executeSQL):
+        (WebCore::SQLTransaction::openTransactionAndPreflight):
+        (WebCore::SQLTransaction::runCurrentStatement):
+        (WebCore::SQLTransaction::postflightAndCommit):
+        * storage/SQLTransaction.h:
+        * storage/SQLTransactionSync.cpp:
+        (WebCore::SQLTransactionSync::SQLTransactionSync):
+        (WebCore::SQLTransactionSync::executeSQL):
+        (WebCore::SQLTransactionSync::begin):
+        * storage/SQLTransactionSync.h:
+
 2011-08-01  Tim Horton  <timothy_horton@apple.com>
 

trunk/Source/WebCore/storage/AbstractDatabase.cpp

@@ -35 +35 @@
 #include "Logging.h"
 #include "SQLiteStatement.h"
+#include "SQLiteTransaction.h"
 #include "ScriptExecutionContext.h"
 #include "SecurityOrigin.h"
@@ -236 +237 @@
 String AbstractDatabase::version() const
 {
-    MutexLocker locker(guidMutex());
-    return guidToVersionMap().get(m_guid).threadsafeCopy();
-}
-
-static const int maxSqliteBusyWaitTime = 30000;
+    // Note: In multi-process browsers the cached value may be accurate, but we cannot read the
+    // actual version from the database without potentially inducing a deadlock.
+    // FIXME: Add an async version getter to the DatabaseAPI.
+    return getCachedVersion();
+}
+
 bool AbstractDatabase::performOpenAndVerify(bool shouldSetVersionInNewDatabase, ExceptionCode& ec)
 {
+    const int maxSqliteBusyWaitTime = 30000;
+
     if (!m_sqliteDatabase.open(m_filename, true)) {
         LOG_ERROR("Unable to open database at path %s", m_filename.ascii().data());
@@ -251 +255 @@
         LOG_ERROR("Unable to turn on incremental auto-vacuum for database %s", m_filename.ascii().data());
 
-    ASSERT(m_databaseAuthorizer);
-    m_sqliteDatabase.setAuthorizer(m_databaseAuthorizer);
     m_sqliteDatabase.setBusyTimeout(maxSqliteBusyWaitTime);
 
@@ -264 +266 @@
             currentVersion = entry->second.isNull() ? String("") : entry->second;
             LOG(StorageAPI, "Current cached version for guid %i is %s", m_guid, currentVersion.ascii().data());
+
+#if PLATFORM(CHROMIUM)
+            // Note: In multi-process browsers the cached value may be inaccurate, but
+            // we cannot read the actual version from the database without potentially
+            // inducing a form of deadlock, a busytimeout error when trying to
+            // access the database. So we'll use the cached value if we're able to read
+            // the value without waiting, and otherwise use the cached value (which may be off).
+            // FIXME: Add an async openDatabase method to the DatabaseAPI.
+            const int noSqliteBusyWaitTime = 0;
+            m_sqliteDatabase.setBusyTimeout(noSqliteBusyWaitTime);
+            String versionFromDatabase;
+            if (getVersionFromDatabase(versionFromDatabase, false)) {
+                currentVersion = versionFromDatabase;
+                updateGuidVersionMap(m_guid, currentVersion);
+            }
+            m_sqliteDatabase.setBusyTimeout(maxSqliteBusyWaitTime);
+#endif
         } else {
             LOG(StorageAPI, "No cached version for guid %i", m_guid);
+
+            SQLiteTransaction transaction(m_sqliteDatabase);
+            transaction.begin();
+            if (!transaction.inProgress()) {
+                LOG_ERROR("Unable to begin transaction while opening %s", databaseDebugName().ascii().data());
+                ec = INVALID_STATE_ERR;
+                m_sqliteDatabase.close();
+                return false;
+            }
 
             if (!m_sqliteDatabase.tableExists(databaseInfoTableName())) {
@@ -273 +301 @@
                     LOG_ERROR("Unable to create table %s in database %s", databaseInfoTableName().ascii().data(), databaseDebugName().ascii().data());
                     ec = INVALID_STATE_ERR;
-                    // Close the handle to the database file.
+                    transaction.rollback();
                     m_sqliteDatabase.close();
                     return false;
                 }
-            }
-
-            if (!getVersionFromDatabase(currentVersion)) {
+            } else if (!getVersionFromDatabase(currentVersion, false)) {
                 LOG_ERROR("Failed to get current version from database %s", databaseDebugName().ascii().data());
                 ec = INVALID_STATE_ERR;
-                // Close the handle to the database file.
+                transaction.rollback();
                 m_sqliteDatabase.close();
                 return false;
             }
+
             if (currentVersion.length()) {
                 LOG(StorageAPI, "Retrieved current version %s from database %s", currentVersion.ascii().data(), databaseDebugName().ascii().data());
             } else if (!m_new || shouldSetVersionInNewDatabase) {
                 LOG(StorageAPI, "Setting version %s in database %s that was just created", m_expectedVersion.ascii().data(), databaseDebugName().ascii().data());
-                if (!setVersionInDatabase(m_expectedVersion)) {
+                if (!setVersionInDatabase(m_expectedVersion, false)) {
                     LOG_ERROR("Failed to set version %s in database %s", m_expectedVersion.ascii().data(), databaseDebugName().ascii().data());
                     ec = INVALID_STATE_ERR;
-                    // Close the handle to the database file.
+                    transaction.rollback();
                     m_sqliteDatabase.close();
                     return false;
@@ -299 +326 @@
                 currentVersion = m_expectedVersion;
             }
-
             updateGuidVersionMap(m_guid, currentVersion);
+            transaction.commit();
         }
     }
@@ -315 +342 @@
             databaseDebugName().ascii().data(), currentVersion.ascii().data());
         ec = INVALID_STATE_ERR;
-        // Close the handle to the database file.
         m_sqliteDatabase.close();
         return false;
     }
 
+    ASSERT(m_databaseAuthorizer);
+    m_sqliteDatabase.setAuthorizer(m_databaseAuthorizer);
+
     m_opened = true;
+
+    if (m_new && !shouldSetVersionInNewDatabase)
+        m_expectedVersion = ""; // The caller provided a creationCallback which will set the expected version.
 
     return true;
@@ -365 +397 @@
 }
 
-bool AbstractDatabase::getVersionFromDatabase(String& version)
+bool AbstractDatabase::getVersionFromDatabase(String& version, bool shouldCacheVersion)
 {
     DEFINE_STATIC_LOCAL(String, getVersionQuery, ("SELECT value FROM " + databaseInfoTableName() + " WHERE key = '" + databaseVersionKey() + "';"));
@@ -372 +404 @@
 
     bool result = retrieveTextResultFromDatabase(m_sqliteDatabase, getVersionQuery.threadsafeCopy(), version);
-    if (!result)
+    if (result) {
+        if (shouldCacheVersion)
+            setCachedVersion(version);
+    } else
         LOG_ERROR("Failed to retrieve version from database %s", databaseDebugName().ascii().data());
 
@@ -380 +415 @@
 }
 
-bool AbstractDatabase::setVersionInDatabase(const String& version)
+bool AbstractDatabase::setVersionInDatabase(const String& version, bool shouldCacheVersion)
 {
     // The INSERT will replace an existing entry for the database with the new version number, due to the UNIQUE ON CONFLICT REPLACE
@@ -389 +424 @@
 
     bool result = setTextValueInDatabase(m_sqliteDatabase, setVersionQuery.threadsafeCopy(), version);
-    if (!result)
+    if (result) {
+        if (shouldCacheVersion)
+            setCachedVersion(version);
+    } else
         LOG_ERROR("Failed to set version %s in database (%s)", version.ascii().data(), setVersionQuery.ascii().data());
 
@@ -397 +435 @@
 }
 
-bool AbstractDatabase::versionMatchesExpected() const
-{
-    if (!m_expectedVersion.isEmpty()) {
-        MutexLocker locker(guidMutex());
-        return m_expectedVersion == guidToVersionMap().get(m_guid);
-    }
-
-    return true;
-}
-
 void AbstractDatabase::setExpectedVersion(const String& version)
 {
     m_expectedVersion = version.threadsafeCopy();
+}
+
+String AbstractDatabase::getCachedVersion() const
+{
+    MutexLocker locker(guidMutex());
+    return guidToVersionMap().get(m_guid).threadsafeCopy();
+}
+
+void AbstractDatabase::setCachedVersion(const String& actualVersion)
+{
     // Update the in memory database version map.
     MutexLocker locker(guidMutex());
-    updateGuidVersionMap(m_guid, version);
+    updateGuidVersionMap(m_guid, actualVersion);
+}
+
+bool AbstractDatabase::getActualVersionForTransaction(String &actualVersion)
+{
+    ASSERT(m_sqliteDatabase.transactionInProgress());
+#if PLATFORM(CHROMIUM)
+    // Note: In multi-process browsers the cached value may be inaccurate.
+    // So we retrieve the value from the database and update the cached value here.
+    return getVersionFromDatabase(actualVersion, true);
+#else
+    actualVersion = getCachedVersion();
+    return true;
+#endif
 }
 

trunk/Source/WebCore/storage/AbstractDatabase.h

@@ -72 +72 @@
     bool isInterrupted();
 
-    // FIXME: move all version-related methods to a DatabaseVersionTracker class
-    bool versionMatchesExpected() const;
-    void setExpectedVersion(const String& version);
-    bool getVersionFromDatabase(String& version);
-    bool setVersionInDatabase(const String& version);
-
     void disableAuthorizer();
     void enableAuthorizer();
@@ -92 +86 @@
 
 protected:
+    friend class SQLTransactionSync;
+    friend class SQLTransaction;
+    friend class ChangeVersionWrapper;
+
     AbstractDatabase(ScriptExecutionContext*, const String& name, const String& expectedVersion,
                      const String& displayName, unsigned long estimatedSize);
@@ -98 +96 @@
 
     virtual bool performOpenAndVerify(bool shouldSetVersionInNewDatabase, ExceptionCode& ec);
+
+    bool getVersionFromDatabase(String& version, bool shouldCacheVersion = true);
+    bool setVersionInDatabase(const String& version, bool shouldCacheVersion = true);
+    void setExpectedVersion(const String&);
+    const String& expectedVersion() const { return m_expectedVersion; }
+    String getCachedVersion()const;
+    void setCachedVersion(const String&);
+    bool getActualVersionForTransaction(String& version);
 
     static const String& databaseInfoTableName();

trunk/Source/WebCore/storage/ChangeVersionWrapper.cpp

@@ -79 +79 @@
 }
 
+void ChangeVersionWrapper::handleCommitFailedAfterPostflight(SQLTransaction* transaction)
+{
+    transaction->database()->setCachedVersion(m_oldVersion);
+}
+
 } // namespace WebCore
 

trunk/Source/WebCore/storage/ChangeVersionWrapper.h

@@ -45 +45 @@
     virtual bool performPreflight(SQLTransaction*);
     virtual bool performPostflight(SQLTransaction*);
-
     virtual SQLError* sqlError() const { return m_sqlError.get(); }
+    virtual void handleCommitFailedAfterPostflight(SQLTransaction*);
 
 private:

trunk/Source/WebCore/storage/Database.cpp

@@ -110 +110 @@
     InspectorInstrumentation::didOpenDatabase(context, database, context->securityOrigin()->host(), name, expectedVersion);
 
-    // If it's a new database and a creation callback was provided, reset the expected
-    // version to "" and schedule the creation callback. Because of some subtle String
-    // implementation issues, we have to reset m_expectedVersion here instead of doing
-    // it inside performOpenAndVerify() which is run on the DB thread.
     if (database->isNew() && creationCallback.get()) {
-        database->m_expectedVersion = "";
         LOG(StorageAPI, "Scheduling DatabaseCreationCallbackTask for database %p\n", database.get());
         database->m_scriptExecutionContext->postTask(DatabaseCreationCallbackTask::create(database, creationCallback));

trunk/Source/WebCore/storage/DatabaseSync.cpp

@@ -68 +68 @@
 
     if (database->isNew() && creationCallback.get()) {
-        database->m_expectedVersion = "";
         LOG(StorageAPI, "Invoking the creation callback for database %p\n", database.get());
         creationCallback->handleEvent(database.get());
@@ -124 +123 @@
     }
 
-    if ((ec = transaction->commit()))
-        return;
+    if ((ec = transaction->commit())) {
+        setCachedVersion(oldVersion);
+        return;
+    }
 
     setExpectedVersion(newVersion);

trunk/Source/WebCore/storage/SQLTransaction.cpp

@@ -79 +79 @@
     , m_lockAcquired(false)
     , m_readOnly(readOnly)
+    , m_hasVersionMismatch(false)
 {
     ASSERT(m_database);
@@ -105 +106 @@
     if (m_database->deleted())
         statement->setDatabaseDeletedError();
-
-    if (!m_database->versionMatchesExpected())
-        statement->setVersionMismatchedError();
 
     enqueueStatement(statement);
@@ -273 +271 @@
     }
 
+    // Note: We intentionally retrieve the actual version even with an empty expected version.
+    // In multi-process browsers, we take this opportinutiy to update the cached value for
+    // the actual version. In single-process browsers, this is just a map lookup.
+    String actualVersion;
+    if (!m_database->getActualVersionForTransaction(actualVersion)) {
+        m_database->disableAuthorizer();
+        m_sqliteTransaction.clear();
+        m_database->enableAuthorizer();
+        m_transactionError = SQLError::create(SQLError::DATABASE_ERR, "unable to read version");
+        handleTransactionError(false);
+        return;
+    }
+    m_hasVersionMismatch = !m_database->expectedVersion().isEmpty()
+                           && (m_database->expectedVersion() != actualVersion);
+
     // Transaction Steps 3 - Peform preflight steps, jumping to the error callback if they fail
     if (m_wrapper && !m_wrapper->performPreflight(this)) {
+        m_database->disableAuthorizer();
         m_sqliteTransaction.clear();
+        m_database->enableAuthorizer();
         m_transactionError = m_wrapper->sqlError();
         if (!m_transactionError)
@@ -370 +385 @@
     m_database->resetAuthorizer();
 
+    if (m_hasVersionMismatch)
+        m_currentStatement->setVersionMismatchedError();
+
     if (m_currentStatement->execute(m_database.get())) {
         if (m_database->lastActionChangedDatabase()) {
@@ -466 +484 @@
     // If the commit failed, the transaction will still be marked as "in progress"
     if (m_sqliteTransaction->inProgress()) {
+        if (m_wrapper)
+            m_wrapper->handleCommitFailedAfterPostflight(this);
         m_successCallbackWrapper.clear();
         m_transactionError = SQLError::create(SQLError::DATABASE_ERR, "failed to commit the transaction");

trunk/Source/WebCore/storage/SQLTransaction.h

@@ -56 +56 @@
     virtual bool performPreflight(SQLTransaction*) = 0;
     virtual bool performPostflight(SQLTransaction*) = 0;
-
     virtual SQLError* sqlError() const = 0;
+    virtual void handleCommitFailedAfterPostflight(SQLTransaction*) = 0;
 };
 
@@ -124 +124 @@
     bool m_lockAcquired;
     bool m_readOnly;
+    bool m_hasVersionMismatch;
 
     Mutex m_statementMutex;

trunk/Source/WebCore/storage/SQLTransactionSync.cpp

@@ -59 +59 @@
     , m_callback(callback)
     , m_readOnly(readOnly)
+    , m_hasVersionMismatch(false)
     , m_modifiedDatabase(false)
     , m_transactionClient(adoptPtr(new SQLTransactionClient()))
@@ -80 +81 @@
     }
 
-    if (!m_database->versionMatchesExpected()) {
+    if (m_hasVersionMismatch) {
         ec = SQLException::VERSION_ERR;
         return 0;
@@ -151 +152 @@
     }
 
+    // Note: We intentionally retrieve the actual version even with an empty expected version.
+    // In multi-process browsers, we take this opportinutiy to update the cached value for
+    // the actual version. In single-process browsers, this is just a map lookup.
+    String actualVersion;
+    if (!m_database->getActualVersionForTransaction(actualVersion)) {
+        rollback();
+        return SQLException::DATABASE_ERR;
+    }
+    m_hasVersionMismatch = !m_database->expectedVersion().isEmpty()
+                           && (m_database->expectedVersion() != actualVersion);
     return 0;
 }

trunk/Source/WebCore/storage/SQLTransactionSync.h

@@ -35 +35 @@
 
 #include "ExceptionCode.h"
+#include "PlatformString.h"
 #include <wtf/Forward.h>
 #include <wtf/RefCounted.h>
@@ -71 +72 @@
     RefPtr<SQLTransactionSyncCallback> m_callback;
     bool m_readOnly;
+    bool m_hasVersionMismatch;
 
     bool m_modifiedDatabase;