Changeset 96554 in webkit


Ignore:
Timestamp:
Oct 3, 2011 4:07:35 PM (13 years ago)
Author:
Michael Nordman
Message:

A little more WebSQLDatabase thread safety.
​https://bugs.webkit.org/show_bug.cgi?id=69277

  • switch to using AtomicallyInitializedStatic where appropiate
  • avoid using some Strings across threads

Reviewed by David Levin.

Existing tests apply.

  • storage/AbstractDatabase.cpp:

(WebCore::guidMutex):
(WebCore::guidToVersionMap):
(WebCore::guidToDatabaseMap):
(WebCore::guidForOriginAndName):
(WebCore::AbstractDatabase::databaseInfoTableName):
(WebCore::AbstractDatabase::AbstractDatabase):
(WebCore::AbstractDatabase::performOpenAndVerify):
(WebCore::AbstractDatabase::getVersionFromDatabase):
(WebCore::AbstractDatabase::setVersionInDatabase):

  • storage/AbstractDatabase.h:
  • storage/chromium/DatabaseTrackerChromium.cpp:

(WebCore::DatabaseTracker::tracker):

  • storage/chromium/QuotaTracker.cpp:

(WebCore::QuotaTracker::instance):

Location:
trunk/Source/WebCore
Files:
5 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r96553 r96554  
     12011-10-03  Michael Nordman  <michaeln@google.com>
     2
     3        A little more WebSQLDatabase thread safety.
     4        https://bugs.webkit.org/show_bug.cgi?id=69277
     5
     6        - switch to using AtomicallyInitializedStatic where appropiate
     7        - avoid using some Strings across threads
     8
     9        Reviewed by David Levin.
     10
     11        Existing tests apply.
     12
     13        * storage/AbstractDatabase.cpp:
     14        (WebCore::guidMutex):
     15        (WebCore::guidToVersionMap):
     16        (WebCore::guidToDatabaseMap):
     17        (WebCore::guidForOriginAndName):
     18        (WebCore::AbstractDatabase::databaseInfoTableName):
     19        (WebCore::AbstractDatabase::AbstractDatabase):
     20        (WebCore::AbstractDatabase::performOpenAndVerify):
     21        (WebCore::AbstractDatabase::getVersionFromDatabase):
     22        (WebCore::AbstractDatabase::setVersionInDatabase):
     23        * storage/AbstractDatabase.h:
     24        * storage/chromium/DatabaseTrackerChromium.cpp:
     25        (WebCore::DatabaseTracker::tracker):
     26        * storage/chromium/QuotaTracker.cpp:
     27        (WebCore::QuotaTracker::instance):
     28
    1292011-10-03  Ryosuke Niwa  <rniwa@webkit.org>
    230

  • trunk/Source/WebCore/storage/AbstractDatabase.cpp

    r95901 r96554  
    11/*
    2  * Copyright (C) 2010 Google Inc. All rights reserved.
     2 * Copyright (C) 2011 Google Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    4949namespace WebCore {
    5050
     51static const char versionKey[] = "WebKitDatabaseVersionKey";
     52static const char infoTableName[] = "__WebKitDatabaseInfoTable__";
     53
    5154static bool retrieveTextResultFromDatabase(SQLiteDatabase& db, const String& query, String& resultString)
    5255{
    … …  
    97100static Mutex& guidMutex()
    98101{
    99     // Note: We don't have to use AtomicallyInitializedStatic here because
    100     // this function is called once in the constructor on the main thread
    101     // before any other threads that call this function are used.
    102     DEFINE_STATIC_LOCAL(Mutex, mutex, ());
     102    AtomicallyInitializedStatic(Mutex&, mutex = *new Mutex);
    103103    return mutex;
    104104}
    … …  
    107107static GuidVersionMap& guidToVersionMap()
    108108{
     109    // Ensure the the mutex is locked.
     110    ASSERT(!guidMutex().tryLock());
    109111    DEFINE_STATIC_LOCAL(GuidVersionMap, map, ());
    110112    return map;
    … …  
    130132static GuidDatabaseMap& guidToDatabaseMap()
    131133{
     134    // Ensure the the mutex is locked.
     135    ASSERT(!guidMutex().tryLock());
    132136    DEFINE_STATIC_LOCAL(GuidDatabaseMap, map, ());
    133137    return map;
    … …  
    136140static int guidForOriginAndName(const String& origin, const String& name)
    137141{
     142    // Ensure the the mutex is locked.
     143    ASSERT(!guidMutex().tryLock());
     144
    138145    String stringID = origin + "/" + name;
    139146
    140     // Note: We don't have to use AtomicallyInitializedStatic here because
    141     // this function is called once in the constructor on the main thread
    142     // before any other threads that call this function are used.
    143     DEFINE_STATIC_LOCAL(Mutex, stringIdentifierMutex, ());
    144     MutexLocker locker(stringIdentifierMutex);
    145147    typedef HashMap<String, int> IDGuidMap;
    146148    DEFINE_STATIC_LOCAL(IDGuidMap, stringIdentifierToGUIDMap, ());
    … …  
    168170
    169171// static
    170 const String& AbstractDatabase::databaseInfoTableName()
    171 {
    172     DEFINE_STATIC_LOCAL(String, name, ("__WebKitDatabaseInfoTable__"));
    173     return name;
     172const char* AbstractDatabase::databaseInfoTableName()
     173{
     174    return infoTableName;
    174175}
    175176
    … …  
    188189    m_contextThreadSecurityOrigin = m_scriptExecutionContext->securityOrigin();
    189190
    190     m_databaseAuthorizer = DatabaseAuthorizer::create(databaseInfoTableName());
     191    m_databaseAuthorizer = DatabaseAuthorizer::create(infoTableName);
    191192
    192193    if (m_name.isNull())
    193194        m_name = "";
    194195
    195     m_guid = guidForOriginAndName(securityOrigin()->toString(), name);
    196196    {
    197197        MutexLocker locker(guidMutex());
    198 
     198        m_guid = guidForOriginAndName(securityOrigin()->toString(), name);
    199199        HashSet<AbstractDatabase*>* hashSet = guidToDatabaseMap().get(m_guid);
    200200        if (!hashSet) {
    … …  
    265265        if (entry != guidToVersionMap().end()) {
    266266            // Map null string to empty string (see updateGuidVersionMap()).
    267             currentVersion = entry->second.isNull() ? String("") : entry->second;
     267            currentVersion = entry->second.isNull() ? String("") : entry->second.threadsafeCopy();
    268268            LOG(StorageAPI, "Current cached version for guid %i is %s", m_guid, currentVersion.ascii().data());
    269269
    … …  
    272272            // we cannot read the actual version from the database without potentially
    273273            // inducing a form of deadlock, a busytimeout error when trying to
    274             // access the database. So we'll use the cached value if we're able to read
    275             // the value without waiting, and otherwise use the cached value (which may be off).
     274            // access the database. So we'll use the cached value if we're unable to read
     275            // the value from the database file without waiting.
    276276            // FIXME: Add an async openDatabase method to the DatabaseAPI.
    277277            const int noSqliteBusyWaitTime = 0;
    … …  
    296296            }
    297297
    298             if (!m_sqliteDatabase.tableExists(databaseInfoTableName())) {
     298            String tableName(infoTableName);
     299            if (!m_sqliteDatabase.tableExists(tableName)) {
    299300                m_new = true;
    300301
    301                 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);")) {
    302                     LOG_ERROR("Unable to create table %s in database %s", databaseInfoTableName().ascii().data(), databaseDebugName().ascii().data());
     302                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);")) {
     303                    LOG_ERROR("Unable to create table %s in database %s", infoTableName, databaseDebugName().ascii().data());
    303304                    ec = INVALID_STATE_ERR;
    304305                    transaction.rollback();
    … …  
    391392}
    392393
    393 // static
    394 const String& AbstractDatabase::databaseVersionKey()
    395 {
    396     DEFINE_STATIC_LOCAL(String, key, ("WebKitDatabaseVersionKey"));
    397     return key;
    398 }
    399 
    400394bool AbstractDatabase::getVersionFromDatabase(String& version, bool shouldCacheVersion)
    401395{
    402     DEFINE_STATIC_LOCAL(String, getVersionQuery, ("SELECT value FROM " + databaseInfoTableName() + " WHERE key = '" + databaseVersionKey() + "';"));
     396    String query(String("SELECT value FROM ") + infoTableName +  " WHERE key = '" + versionKey + "';");
    403397
    404398    m_databaseAuthorizer->disable();
    405399
    406     bool result = retrieveTextResultFromDatabase(m_sqliteDatabase, getVersionQuery.threadsafeCopy(), version);
     400    bool result = retrieveTextResultFromDatabase(m_sqliteDatabase, query, version);
    407401    if (result) {
    408402        if (shouldCacheVersion)
    … …  
    420414    // The INSERT will replace an existing entry for the database with the new version number, due to the UNIQUE ON CONFLICT REPLACE
    421415    // clause in the CREATE statement (see Database::performOpenAndVerify()).
    422     DEFINE_STATIC_LOCAL(String, setVersionQuery, ("INSERT INTO " + databaseInfoTableName() + " (key, value) VALUES ('" + databaseVersionKey() + "', ?);"));
     416    String query(String("INSERT INTO ") + infoTableName +  " (key, value) VALUES ('" + versionKey + "', ?);");
    423417
    424418    m_databaseAuthorizer->disable();
    425419
    426     bool result = setTextValueInDatabase(m_sqliteDatabase, setVersionQuery.threadsafeCopy(), version);
     420    bool result = setTextValueInDatabase(m_sqliteDatabase, query, version);
    427421    if (result) {
    428422        if (shouldCacheVersion)
    429423            setCachedVersion(version);
    430424    } else
    431         LOG_ERROR("Failed to set version %s in database (%s)", version.ascii().data(), setVersionQuery.ascii().data());
     425        LOG_ERROR("Failed to set version %s in database (%s)", version.ascii().data(), query.ascii().data());
    432426
    433427    m_databaseAuthorizer->enable();

  • trunk/Source/WebCore/storage/AbstractDatabase.h

    r95901 r96554  
    11/*
    2  * Copyright (C) 2010 Google Inc. All rights reserved.
     2 * Copyright (C) 2011 Google Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    105105    bool getActualVersionForTransaction(String& version);
    106106
    107     static const String& databaseInfoTableName();
     107    static const char* databaseInfoTableName();
    108108
    109109    RefPtr<ScriptExecutionContext> m_scriptExecutionContext;
    … …  
    121121
    122122private:
    123     static const String& databaseVersionKey();
    124 
    125123    int m_guid;
    126124    bool m_opened;

  • trunk/Source/WebCore/storage/chromium/DatabaseTrackerChromium.cpp

    r95901 r96554  
    11/*
    2  * Copyright (C) 2010 Google Inc. All rights reserved.
     2 * Copyright (C) 2011 Google Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    4848DatabaseTracker& DatabaseTracker::tracker()
    4949{
    50     DEFINE_STATIC_LOCAL(DatabaseTracker, tracker, (""));
     50    AtomicallyInitializedStatic(DatabaseTracker&, tracker = *new DatabaseTracker(""));
    5151    return tracker;
    5252}

  • trunk/Source/WebCore/storage/chromium/QuotaTracker.cpp

    r95901 r96554  
    11/*
    2  * Copyright (C) 2009 Google Inc. All rights reserved.
     2 * Copyright (C) 2011 Google Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    4141QuotaTracker& QuotaTracker::instance()
    4242{
    43     DEFINE_STATIC_LOCAL(QuotaTracker, tracker, ());
     43    AtomicallyInitializedStatic(QuotaTracker&, tracker = *new QuotaTracker);
    4444    return tracker;
    4545}
Note: See TracChangeset for help on using the changeset viewer.