Changeset 195652 in webkit

Timestamp:

Jan 26, 2016 7:45:09 PM (8 years ago)

Author:

Chris Dumez

Message:

fast/history/page-cache-webdatabase-no-transaction-db.html flakily crashes
​https://bugs.webkit.org/show_bug.cgi?id=153525

Reviewed by Andreas Kling.

Source/WebCore:

The test was crashing because DatabaseThread::hasPendingDatabaseActivity()
was accessing m_openDatabaseSet from the main thread without any locking
mechanism. This is an issue because m_openDatabaseSet is altered by the
database thread.

No new tests, already covered by fast/history/page-cache-webdatabase-no-transaction-db.html.

• Modules/webdatabase/DatabaseThread.cpp:

(WebCore::DatabaseThread::databaseThread):
(WebCore::DatabaseThread::recordDatabaseOpen):
(WebCore::DatabaseThread::recordDatabaseClosed):
(WebCore::DatabaseThread::hasPendingDatabaseActivity):

• Modules/webdatabase/DatabaseThread.h:

LayoutTests:

Unskip fast/history/page-cache-webdatabase-no-transaction-db.html now
that it no longer crashes.

• TestExpectations:

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/TestExpectations (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/Modules/webdatabase/DatabaseThread.cpp (modified) (4 diffs)
• Source/WebCore/Modules/webdatabase/DatabaseThread.h (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2016-01-26  Chris Dumez  <cdumez@apple.com>
+
+        fast/history/page-cache-webdatabase-no-transaction-db.html flakily crashes
+        https://bugs.webkit.org/show_bug.cgi?id=153525
+
+        Reviewed by Andreas Kling.
+
+        Unskip fast/history/page-cache-webdatabase-no-transaction-db.html now
+        that it no longer crashes.
+
+        * TestExpectations:
+
 2016-01-26  Brady Eidson  <beidson@apple.com>
 

trunk/LayoutTests/TestExpectations

@@ -849 +849 @@
 http/tests/security/contentSecurityPolicy/1.1/stylehash-default-src.html # Needs expected file.
 
-# This test flakily crashes, skip it until the crash is fixed.
-webkit.org/b/153525 fast/history/page-cache-webdatabase-no-transaction-db.html [ Skip ]
-
 # These state object tests purposefully stress a resource limit, and take multiple seconds to run.
 loader/stateobjects/pushstate-size-iframe.html [ Slow ]

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2016-01-26  Chris Dumez  <cdumez@apple.com>
+
+        fast/history/page-cache-webdatabase-no-transaction-db.html flakily crashes
+        https://bugs.webkit.org/show_bug.cgi?id=153525
+
+        Reviewed by Andreas Kling.
+
+        The test was crashing because DatabaseThread::hasPendingDatabaseActivity()
+        was accessing m_openDatabaseSet from the main thread without any locking
+        mechanism. This is an issue because m_openDatabaseSet is altered by the
+        database thread.
+
+        No new tests, already covered by fast/history/page-cache-webdatabase-no-transaction-db.html.
+
+        * Modules/webdatabase/DatabaseThread.cpp:
+        (WebCore::DatabaseThread::databaseThread):
+        (WebCore::DatabaseThread::recordDatabaseOpen):
+        (WebCore::DatabaseThread::recordDatabaseClosed):
+        (WebCore::DatabaseThread::hasPendingDatabaseActivity):
+        * Modules/webdatabase/DatabaseThread.h:
+
 2016-01-26  Joseph Pecoraro  <pecoraro@apple.com>
 

trunk/Source/WebCore/Modules/webdatabase/DatabaseThread.cpp

@@ -118 +118 @@
     // Close the databases that we ran transactions on. This ensures that if any transactions are still open, they are rolled back and we don't leave the database in an
     // inconsistent or locked state.
-    if (m_openDatabaseSet.size() > 0) {
-        // As the call to close will modify the original set, we must take a copy to iterate over.
-        DatabaseSet openSetCopy;
-        openSetCopy.swap(m_openDatabaseSet);
-        for (auto& openDatabase : openSetCopy)
-            openDatabase->close();
-    }
+    DatabaseSet openSetCopy;
+    {
+        LockHolder lock(m_openDatabaseSetMutex);
+        if (m_openDatabaseSet.size() > 0) {
+            // As the call to close will modify the original set, we must take a copy to iterate over.
+            openSetCopy.swap(m_openDatabaseSet);
+        }
+    }
+
+    for (auto& openDatabase : openSetCopy)
+        openDatabase->close();
 
     // Detach the thread so its resources are no longer of any concern to anyone else
@@ -140 +144 @@
 void DatabaseThread::recordDatabaseOpen(Database* database)
 {
+    LockHolder lock(m_openDatabaseSetMutex);
+
     ASSERT(currentThread() == m_threadID);
     ASSERT(database);
@@ -148 +154 @@
 void DatabaseThread::recordDatabaseClosed(Database* database)
 {
+    LockHolder lock(m_openDatabaseSetMutex);
+
     ASSERT(currentThread() == m_threadID);
     ASSERT(database);
@@ -184 +192 @@
 bool DatabaseThread::hasPendingDatabaseActivity() const
 {
+    LockHolder lock(m_openDatabaseSetMutex);
     for (auto& database : m_openDatabaseSet) {
         if (database->hasPendingCreationEvent() || database->hasPendingTransaction())

trunk/Source/WebCore/Modules/webdatabase/DatabaseThread.h

@@ -81 +81 @@
     // This set keeps track of the open databases that have been used on this thread.
     typedef HashSet<RefPtr<Database>> DatabaseSet;
+    mutable Lock m_openDatabaseSetMutex;
     DatabaseSet m_openDatabaseSet;