[oilpan]: RFC: Change DatabaseBackendBase::performOpenAndVerify locking

Source/modules/webdatabase/DatabaseBackendBase.cpp

 /*
  * Copyright (C) 2011 Google Inc. All rights reserved.
  * Copyright (C) 2013 Apple 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.
@@ skipping 311 matching lines @@
         errorMessage = formatErrorMessage("unable to open database", m_sqliteDatabase.lastError(), m_sqliteDatabase.lastErrorMsg());
         return false;
     }
     if (!m_sqliteDatabase.turnOnIncrementalAutoVacuum())
         WTF_LOG_ERROR("Unable to turn on incremental auto-vacuum (%d %s)", m_sqliteDatabase.lastError(), m_sqliteDatabase.lastErrorMsg());
 
     m_sqliteDatabase.setBusyTimeout(maxSqliteBusyWaitTime);
 
     String currentVersion;
     {
-        MutexLocker locker(guidMutex());

[Vyacheslav Egorov (Google), 2014/06/13 12:49:39]

An alternative less intrusive approach would be to have SafePoint around attempt
to lock this mutex, similar to how we handle thread detachment. 

ThreadState::current()->enterSafePoint(...);
MutexLocker locker(...)
ThreadState::current()->leaveSafePoint();

For the best effect this can be wrapped into the SafePointAwareMutexLocker
class.

We can even have SafePointAwareMutex which would require
SafePointAwareMutexLocker to be used on it and make all database related mutexes
SafePointAware.

-
-        GuidVersionMap::iterator entry = guidToVersionMap().find(m_guid);
-        if (entry != guidToVersionMap().end()) {
+        GuidVersionMap::iterator entry;
+        GuidVersionMap::iterator end;
+        {
+            MutexLocker locker(guidMutex());
+            entry = guidToVersionMap().find(m_guid);
+            end = guidToVersionMap().end();
+        }
+        if (entry != end) {
             // Map null string to empty string (see updateGuidVersionMap()).
             currentVersion = entry->value.isNull() ? emptyString() : entry->value.isolatedCopy();
             WTF_LOG(StorageAPI, "Current cached version for guid %i is %s", m_guid, currentVersion.ascii().data());
 
             // 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 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;
+                MutexLocker locker(guidMutex());
                 updateGuidVersionMap(m_guid, currentVersion);
             }
             m_sqliteDatabase.setBusyTimeout(maxSqliteBusyWaitTime);
         } else {
             WTF_LOG(StorageAPI, "No cached version for guid %i", m_guid);
 
             SQLiteTransaction transaction(m_sqliteDatabase);
             transaction.begin();
             if (!transaction.inProgress()) {
                 reportOpenDatabaseResult(2, InvalidStateError, m_sqliteDatabase.lastError());
@@ skipping 27 matching lines @@
                 WTF_LOG(StorageAPI, "Setting version %s in database %s that was just created", m_expectedVersion.ascii().data(), databaseDebugName().ascii().data());
                 if (!setVersionInDatabase(m_expectedVersion, false)) {
                     reportOpenDatabaseResult(5, InvalidStateError, m_sqliteDatabase.lastError());
                     errorMessage = formatErrorMessage("unable to open database, failed to write current version", m_sqliteDatabase.lastError(), m_sqliteDatabase.lastErrorMsg());
                     transaction.rollback();
                     m_sqliteDatabase.close();
                     return false;
                 }
                 currentVersion = m_expectedVersion;
             }
-            updateGuidVersionMap(m_guid, currentVersion);
-            transaction.commit();
+
+            {
+                MutexLocker locker(guidMutex());
+                entry = guidToVersionMap().find(m_guid);
+                end = guidToVersionMap().end();
+                // We always update the map with the version when at this point.
+                updateGuidVersionMap(m_guid, currentVersion);
+            }
+
+            // Only commit the transaction if no other thread has already created the table, etc.
+            // We use the presence of an entry in the guidToVersionMap to determine if this thread
+            // is first.
+            if (entry != end) {
+                transaction.commit();
+            } else {
+                // Some other thread has already created table, set the version, etc.
+                transaction.rollback();
+            }
         }
     }
 
     if (currentVersion.isNull()) {
         WTF_LOG(StorageAPI, "Database %s does not have its version set", databaseDebugName().ascii().data());
         currentVersion = "";
     }
 
     // If the expected version isn't the empty string, ensure that the current database version we have matches that version. Otherwise, set an exception.
     // If the expected version is the empty string, then we always return with whatever version of the database we have.
@@ skipping 256 matching lines @@
 {
     executionContext()->addConsoleMessage(StorageMessageSource, ErrorMessageLevel, message);
 }
 
 ExecutionContext* DatabaseBackendBase::executionContext() const
 {
     return databaseContext()->executionContext();
 }
 
 } // namespace WebCore