Revert of DatabaseBackend should not post a task to an already termin…

…ated database thread (patchset #4 id:60001 of https://codereview.chromium.org/596083005/) Reason for revert: This causes a lot of database layout test failures on the buildbots. Reverting. Original issue's description: > DatabaseBackend should not post a task to an already terminated database thread > > Currently some methods of DatabaseBackend post a task to a database thread without checking DatabaseThread::terminationRequested(). Consequently, the task can be posted to an already terminated database thread. > > This is problematic but by accident isn't currently causing any real issue (for some reason). This begins to cause an issue when I destruct DatabaseThread::m_thread promptly after the database thread processed a cleanup task (See https://codereview.chromium.org/589363002). > > This CL adds the necessary DatabaseThread::terminationRequested()s before DatabaseBackend posts a task to the database thread. > > BUG=340522 > > Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182552 [email protected],[email protected],[email protected] NOTREECHECKS=true NOTRY=true BUG=340522 Review URL: https://codereview.chromium.org/593423002 git-svn-id: svn://svn.chromium.org/blink/trunk@182564 bbb929c8-8fbe-4397-9dbb-9b2b20218538
yoavweiss · Sep 24, 2014 · fedfe16 · fedfe16
1 parent ddad8a3
commit fedfe16
Show file tree

Hide file tree

Showing 8 changed files with 28 additions and 16 deletions.
diff --git a/Source/modules/webdatabase/Database.cpp b/Source/modules/webdatabase/Database.cpp
@@ -259,7 +259,7 @@ void Database::trace(Visitor* visitor)
 bool Database::openAndVerifyVersion(bool setVersionInNewDatabase, DatabaseError& error, String& errorMessage)
 {
     TaskSynchronizer synchronizer;
-    if (!databaseContext()->databaseThreadAvailable())
+    if (!databaseContext()->databaseThread() || databaseContext()->databaseThread()->terminationRequested(&synchronizer))
         return false;
 
     DatabaseTracker::tracker().prepareToOpenDatabase(this);
@@ -330,7 +330,7 @@ void Database::scheduleTransaction()
     if (m_isTransactionQueueEnabled && !m_transactionQueue.isEmpty())
         transaction = m_transactionQueue.takeFirst();
 
-    if (transaction && databaseContext()->databaseThreadAvailable()) {
+    if (transaction && databaseContext()->databaseThread()) {
         OwnPtr<DatabaseTransactionTask> task = DatabaseTransactionTask::create(transaction);
         WTF_LOG(StorageAPI, "Scheduling DatabaseTransactionTask %p for transaction %p\n", task.get(), task->transaction());
         m_transactionInProgress = true;
@@ -342,7 +342,7 @@ void Database::scheduleTransaction()
 
 void Database::scheduleTransactionStep(SQLTransactionBackend* transaction)
 {
-    if (!databaseContext()->databaseThreadAvailable())
+    if (!databaseContext()->databaseThread())
         return;
 
     OwnPtr<DatabaseTransactionTask> task = DatabaseTransactionTask::create(transaction);
@@ -783,9 +783,10 @@ ExecutionContext* Database::executionContext() const
 void Database::closeImmediately()
 {
     ASSERT(executionContext()->isContextThread());
-    if (databaseContext()->databaseThreadAvailable() && opened()) {
+    DatabaseThread* databaseThread = databaseContext()->databaseThread();
+    if (databaseThread && !databaseThread->terminationRequested() && opened()) {
         logErrorMessage("forcibly closing database");
-        databaseContext()->databaseThread()->scheduleTask(DatabaseCloseTask::create(this, 0));
+        databaseThread->scheduleTask(DatabaseCloseTask::create(this, 0));
     }
 }
 
@@ -912,7 +913,7 @@ Vector<String> Database::tableNames()
     // this may not be true anymore.
     Vector<String> result;
     TaskSynchronizer synchronizer;
-    if (!databaseContext()->databaseThreadAvailable())
+    if (!databaseContext()->databaseThread() || databaseContext()->databaseThread()->terminationRequested(&synchronizer))
         return result;
 
     OwnPtr<DatabaseTableNamesTask> task = DatabaseTableNamesTask::create(this, &synchronizer, result);

diff --git a/Source/modules/webdatabase/DatabaseContext.cpp b/Source/modules/webdatabase/DatabaseContext.cpp
@@ -165,11 +165,6 @@ DatabaseThread* DatabaseContext::databaseThread()
     return m_databaseThread.get();
 }
 
-bool DatabaseContext::databaseThreadAvailable()
-{
-    return m_databaseThread && !m_hasRequestedTermination;
-}
-
 void DatabaseContext::stopDatabases()
 {
     // Though we initiate termination of the DatabaseThread here in
@@ -182,7 +177,7 @@ void DatabaseContext::stopDatabases()
     // m_databaseThread RefPtr destructor will deref and delete the
     // DatabaseThread.
 
-    if (databaseThreadAvailable()) {
+    if (m_databaseThread && !m_hasRequestedTermination) {
         TaskSynchronizer sync;
         m_databaseThread->requestTermination(&sync);
         m_hasRequestedTermination = true;

diff --git a/Source/modules/webdatabase/DatabaseContext.h b/Source/modules/webdatabase/DatabaseContext.h
@@ -59,7 +59,6 @@ class DatabaseContext FINAL
 
     DatabaseContext* backend();
     DatabaseThread* databaseThread();
-    bool databaseThreadAvailable();
 
     void setHasOpenDatabases() { m_hasOpenDatabases = true; }
     // Blocks the caller thread until cleanup tasks are completed.

diff --git a/Source/modules/webdatabase/DatabaseTask.h b/Source/modules/webdatabase/DatabaseTask.h
@@ -56,6 +56,7 @@ class DatabaseTask : public WebThread::Task {
     Database* database() const { return m_database.get(); }
 #if ENABLE(ASSERT)
     bool hasSynchronizer() const { return m_synchronizer; }
+    bool hasCheckedForTermination() const { return m_synchronizer->hasCheckedForTermination(); }
 #endif
 
 protected:

diff --git a/Source/modules/webdatabase/DatabaseThread.cpp b/Source/modules/webdatabase/DatabaseThread.cpp
@@ -91,8 +91,13 @@ void DatabaseThread::requestTermination(TaskSynchronizer *cleanupSync)
     m_thread->postTask(new Task(WTF::bind(&DatabaseThread::cleanupDatabaseThread, this)));
 }
 
-bool DatabaseThread::terminationRequested() const
+bool DatabaseThread::terminationRequested(TaskSynchronizer* taskSynchronizer) const
 {
+#if ENABLE(ASSERT)
+    if (taskSynchronizer)
+        taskSynchronizer->setHasCheckedForTermination();
+#endif
+
     MutexLocker lock(m_terminationRequestedMutex);
     return m_terminationRequested;
 }
@@ -152,7 +157,7 @@ bool DatabaseThread::isDatabaseOpen(Database* database)
 void DatabaseThread::scheduleTask(PassOwnPtr<DatabaseTask> task)
 {
     ASSERT(m_thread);
-    ASSERT(!terminationRequested());
+    ASSERT(!task->hasSynchronizer() || task->hasCheckedForTermination());
     // WebThread takes ownership of the task.
     m_thread->postTask(task.leakPtr());
 }

diff --git a/Source/modules/webdatabase/DatabaseThread.h b/Source/modules/webdatabase/DatabaseThread.h
@@ -59,7 +59,7 @@ class DatabaseThread : public ThreadSafeRefCountedWillBeGarbageCollectedFinalize
 
     void start();
     void requestTermination(TaskSynchronizer* cleanupSync);
-    bool terminationRequested() const;
+    bool terminationRequested(TaskSynchronizer* = 0) const;
 
     void scheduleTask(PassOwnPtr<DatabaseTask>);
 

diff --git a/Source/platform/TaskSynchronizer.cpp b/Source/platform/TaskSynchronizer.cpp
@@ -35,6 +35,9 @@ namespace blink {
 
 TaskSynchronizer::TaskSynchronizer()
     : m_taskCompleted(false)
+#if ENABLE(ASSERT)
+    , m_hasCheckedForTermination(false)
+#endif
 {
 }
 

diff --git a/Source/platform/TaskSynchronizer.h b/Source/platform/TaskSynchronizer.h
@@ -48,12 +48,20 @@ class PLATFORM_EXPORT TaskSynchronizer {
     // Called from a thread that executes the task.
     void taskCompleted();
 
+#if ENABLE(ASSERT)
+    bool hasCheckedForTermination() const { return m_hasCheckedForTermination; }
+    void setHasCheckedForTermination() { m_hasCheckedForTermination = true; }
+#endif
+
 private:
     void waitForTaskCompletionInternal();
 
     bool m_taskCompleted;
     Mutex m_synchronousMutex;
     ThreadCondition m_synchronousCondition;
+#if ENABLE(ASSERT)
+    bool m_hasCheckedForTermination;
+#endif
 };
 
 } // namespace blink