Merge 96554 - A little more WebSQLDatabase thread safety.

Source/WebCore/storage/AbstractDatabase.cpp

 /*
- * Copyright (C) 2010 Google Inc. All rights reserved.
+ * Copyright (C) 2011 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  *
  * 1.  Redistributions of source code must retain the above copyright
  *     notice, this list of conditions and the following disclaimer.
  * 2.  Redistributions in binary form must reproduce the above copyright
  *     notice, this list of conditions and the following disclaimer in the
  *     documentation and/or other materials provided with the distribution.
@@ skipping 27 matching lines @@
 #include <wtf/HashMap.h>
 #include <wtf/HashSet.h>
 #include <wtf/PassRefPtr.h>
 #include <wtf/RefPtr.h>
 #include <wtf/StdLibExtras.h>
 #include <wtf/text/CString.h>
 #include <wtf/text/StringHash.h>
 
 namespace WebCore {
 
+static const char versionKey[] = "WebKitDatabaseVersionKey";
+static const char infoTableName[] = "__WebKitDatabaseInfoTable__";
+
 static bool retrieveTextResultFromDatabase(SQLiteDatabase& db, const String& query, String& resultString)
 {
     SQLiteStatement statement(db, query);
     int result = statement.prepare();
 
     if (result != SQLResultOk) {
         LOG_ERROR("Error (%i) preparing statement to read text result from database (%s)", result, query.ascii().data());
         return false;
     }
 
@@ skipping 28 matching lines @@
         LOG_ERROR("Failed to step statement to set value in database (%s)", query.ascii().data());
         return false;
     }
 
     return true;
 }
 
 // FIXME: move all guid-related functions to a DatabaseVersionTracker class.
 static Mutex& guidMutex()
 {
-    // Note: We don't have to use AtomicallyInitializedStatic here because
-    // this function is called once in the constructor on the main thread
-    // before any other threads that call this function are used.
-    DEFINE_STATIC_LOCAL(Mutex, mutex, ());
+    AtomicallyInitializedStatic(Mutex&, mutex = *new Mutex);
     return mutex;
 }
 
 typedef HashMap<int, String> GuidVersionMap;
 static GuidVersionMap& guidToVersionMap()
 {
+    // Ensure the the mutex is locked.
+    ASSERT(!guidMutex().tryLock());
     DEFINE_STATIC_LOCAL(GuidVersionMap, map, ());
     return map;
 }
 
 // NOTE: Caller must lock guidMutex().
 static inline void updateGuidVersionMap(int guid, String newVersion)
 {
     // Ensure the the mutex is locked.
     ASSERT(!guidMutex().tryLock());
 
     // Note: It is not safe to put an empty string into the guidToVersionMap() map.
     // That's because the map is cross-thread, but empty strings are per-thread.
     // The copy() function makes a version of the string you can use on the current
     // thread, but we need a string we can keep in a cross-thread data structure.
     // FIXME: This is a quite-awkward restriction to have to program with.
 
     // Map null string to empty string (see comment above).
     guidToVersionMap().set(guid, newVersion.isEmpty() ? String() : newVersion.threadsafeCopy());
 }
 
 typedef HashMap<int, HashSet<AbstractDatabase*>*> GuidDatabaseMap;
 static GuidDatabaseMap& guidToDatabaseMap()
 {
+    // Ensure the the mutex is locked.
+    ASSERT(!guidMutex().tryLock());
     DEFINE_STATIC_LOCAL(GuidDatabaseMap, map, ());
     return map;
 }
 
 static int guidForOriginAndName(const String& origin, const String& name)
 {
+    // Ensure the the mutex is locked.
+    ASSERT(!guidMutex().tryLock());
+
     String stringID = origin + "/" + name;
 
-    // Note: We don't have to use AtomicallyInitializedStatic here because
-    // this function is called once in the constructor on the main thread
-    // before any other threads that call this function are used.
-    DEFINE_STATIC_LOCAL(Mutex, stringIdentifierMutex, ());
-    MutexLocker locker(stringIdentifierMutex);
     typedef HashMap<String, int> IDGuidMap;
     DEFINE_STATIC_LOCAL(IDGuidMap, stringIdentifierToGUIDMap, ());
     int guid = stringIdentifierToGUIDMap.get(stringID);
     if (!guid) {
         static int currentNewGUID = 1;
         guid = currentNewGUID++;
         stringIdentifierToGUIDMap.set(stringID, guid);
     }
 
     return guid;
 }
 
 static bool isDatabaseAvailable = true;
 
 bool AbstractDatabase::isAvailable()
 {
     return isDatabaseAvailable;
 }
 
 void AbstractDatabase::setIsAvailable(bool available)
 {
     isDatabaseAvailable = available;
 }
 
 // static
-const String& AbstractDatabase::databaseInfoTableName()
+const char* AbstractDatabase::databaseInfoTableName()
 {
-    DEFINE_STATIC_LOCAL(String, name, ("__WebKitDatabaseInfoTable__"));
-    return name;
+    return infoTableName;
 }
 
 AbstractDatabase::AbstractDatabase(ScriptExecutionContext* context, const String& name, const String& expectedVersion,
                                    const String& displayName, unsigned long estimatedSize)
     : m_scriptExecutionContext(context)
     , m_name(name.crossThreadString())
     , m_expectedVersion(expectedVersion.crossThreadString())
     , m_displayName(displayName.crossThreadString())
     , m_estimatedSize(estimatedSize)
     , m_guid(0)
     , m_opened(false)
     , m_new(false)
 {
     ASSERT(context->isContextThread());
     m_contextThreadSecurityOrigin = m_scriptExecutionContext->securityOrigin();
 
-    m_databaseAuthorizer = DatabaseAuthorizer::create(databaseInfoTableName());
+    m_databaseAuthorizer = DatabaseAuthorizer::create(infoTableName);
 
     if (m_name.isNull())
         m_name = "";
 
-    m_guid = guidForOriginAndName(securityOrigin()->toString(), name);
     {
         MutexLocker locker(guidMutex());
-
+        m_guid = guidForOriginAndName(securityOrigin()->toString(), name);
         HashSet<AbstractDatabase*>* hashSet = guidToDatabaseMap().get(m_guid);
         if (!hashSet) {
             hashSet = new HashSet<AbstractDatabase*>;
             guidToDatabaseMap().set(m_guid, hashSet);
         }
 
         hashSet->add(this);
     }
 
     m_filename = DatabaseTracker::tracker().fullPathForDatabase(securityOrigin(), m_name);
@@ skipping 48 matching lines @@
 
     m_sqliteDatabase.setBusyTimeout(maxSqliteBusyWaitTime);
 
     String currentVersion;
     {
         MutexLocker locker(guidMutex());
 
         GuidVersionMap::iterator entry = guidToVersionMap().find(m_guid);
         if (entry != guidToVersionMap().end()) {
             // Map null string to empty string (see updateGuidVersionMap()).
-            currentVersion = entry->second.isNull() ? String("") : entry->second;
+            currentVersion = entry->second.isNull() ? String("") : entry->second.threadsafeCopy();
             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).
+            // access the database. So we'll use the cached value if we're unable to read
+            // the value from the database file without waiting.
             // 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())) {
+            String tableName(infoTableName);
+            if (!m_sqliteDatabase.tableExists(tableName)) {
                 m_new = true;
 
-                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());
+                if (!m_sqliteDatabase.executeCommand("CREATE TABLE " + tableName + " (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", infoTableName, databaseDebugName().ascii().data());
                     ec = INVALID_STATE_ERR;
                     transaction.rollback();
                     m_sqliteDatabase.close();
                     return false;
                 }
             } else if (!getVersionFromDatabase(currentVersion, false)) {
                 LOG_ERROR("Failed to get current version from database %s", databaseDebugName().ascii().data());
                 ec = INVALID_STATE_ERR;
                 transaction.rollback();
                 m_sqliteDatabase.close();
@@ skipping 70 matching lines @@
 {
     return m_estimatedSize;
 }
 
 String AbstractDatabase::fileName() const
 {
     // Return a deep copy for ref counting thread safety
     return m_filename.threadsafeCopy();
 }
 
-// static
-const String& AbstractDatabase::databaseVersionKey()
-{
-    DEFINE_STATIC_LOCAL(String, key, ("WebKitDatabaseVersionKey"));
-    return key;
-}
-
 bool AbstractDatabase::getVersionFromDatabase(String& version, bool shouldCacheVersion)
 {
-    DEFINE_STATIC_LOCAL(String, getVersionQuery, ("SELECT value FROM " + databaseInfoTableName() + " WHERE key = '" + databaseVersionKey() + "';"));
+    String query(String("SELECT value FROM ") + infoTableName +  " WHERE key = '" + versionKey + "';");
 
     m_databaseAuthorizer->disable();
 
-    bool result = retrieveTextResultFromDatabase(m_sqliteDatabase, getVersionQuery.threadsafeCopy(), version);
+    bool result = retrieveTextResultFromDatabase(m_sqliteDatabase, query, version);
     if (result) {
         if (shouldCacheVersion)
             setCachedVersion(version);
     } else
         LOG_ERROR("Failed to retrieve version from database %s", databaseDebugName().ascii().data());
 
     m_databaseAuthorizer->enable();
 
     return result;
 }
 
 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
     // clause in the CREATE statement (see Database::performOpenAndVerify()).
-    DEFINE_STATIC_LOCAL(String, setVersionQuery, ("INSERT INTO " + databaseInfoTableName() + " (key, value) VALUES ('" + databaseVersionKey() + "', ?);"));
+    String query(String("INSERT INTO ") + infoTableName +  " (key, value) VALUES ('" + versionKey + "', ?);");
 
     m_databaseAuthorizer->disable();
 
-    bool result = setTextValueInDatabase(m_sqliteDatabase, setVersionQuery.threadsafeCopy(), version);
+    bool result = setTextValueInDatabase(m_sqliteDatabase, query, version);
     if (result) {
         if (shouldCacheVersion)
             setCachedVersion(version);
     } else
-        LOG_ERROR("Failed to set version %s in database (%s)", version.ascii().data(), setVersionQuery.ascii().data());
+        LOG_ERROR("Failed to set version %s in database (%s)", version.ascii().data(), query.ascii().data());
 
     m_databaseAuthorizer->enable();
 
     return result;
 }
 
 void AbstractDatabase::setExpectedVersion(const String& version)
 {
     m_expectedVersion = version.threadsafeCopy();
 }
@@ skipping 98 matching lines @@
 
 bool AbstractDatabase::isInterrupted()
 {
     MutexLocker locker(m_sqliteDatabase.databaseMutex());
     return m_sqliteDatabase.isInterrupted();
 }
 
 } // namespace WebCore
 
 #endif // ENABLE(DATABASE)