Changeset 30811 in webkit

Timestamp:

Mar 5, 2008, 2:26:34 PM (17 years ago)

Author:

[email protected]

Message:

JavaScriptCore:

Reviewed by Alexey and Mark Rowe

Fix for <rdar://problem/5778247> - Reproducible crash on storage/execute-sql-args.html

DatabaseThread::unscheduleDatabaseTasks() manually filters through a MessageQueue,
removing particular items for Databases that were shutting down.

This filtering operation is not atomic, and therefore causes a race condition with the
MessageQueue waking up and reading from the message queue.

The end result was an attempt to dereference a null DatabaseTask. Timing-wise, this never
seemed to happen in a debug build, otherwise an assertion would've caught it. Replacing that
assertion with a crash in a release build is what revealed this bug.

• wtf/MessageQueue.h: (WTF::::waitForMessage): Tweak the waiting logic to check the queue's empty state then go back to sleep if the queue was empty - checking m_killed each time it wakes up.

WebCore:

Reviewed by Alexey and Mark Rowe

Fix for <rdar://problem/5778247> - Reproducible crash on storage/execute-sql-args.html

DatabaseThread::unscheduleDatabaseTasks() manually filters through a MessageQueue,
removing particular items for Databases that were shutting down.

This filtering operation is not atomic, and therefore causes a race condition with the
database thread waking up and reading from the message queue.

The end result was an attempt to dereference a null DatabaseTask. Timing-wise, this never
seemed to happen in a debug build, otherwise an assertion would've caught it. Replacing that
assertion with a crash in a release build is what revealed this bug.

The fix for the above symptom was entirely in WTF::MessageQueue in JSCore. With this fix in
place, another crash popped up in the layout tests that was related to dereferencing a
deallocated object - simply because SQLTransaction had a raw pointer to it's Database object
when it needed to be a ref pointer.

• storage/SQLTransaction.cpp: (WebCore::SQLTransaction::runCurrentStatement):
• storage/SQLTransaction.h: Change m_database to be a RefPtr (WebCore::SQLTransaction::database):

LayoutTests:

Reviewed by Alexey + Mark Rowe

Fix for <rdar://problem/5778247> - Reproducible crash on storage/execute-sql-args.html

This test takes its best shot at handling two databases on a single database thread at once,
then having one of those databases go away completely (garbage collection and everything)

• storage/multiple-databases-garbage-collection-expected.txt: Added.
• storage/multiple-databases-garbage-collection.html: Added.

Location:

trunk

Files:

• JavaScriptCore/ChangeLog (modified) (1 diff)
• JavaScriptCore/wtf/MessageQueue.h (modified) (1 diff)
• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/storage/multiple-databases-garbage-collection-expected.txt (added)
• LayoutTests/storage/multiple-databases-garbage-collection.html (added)
• WebCore/ChangeLog (modified) (1 diff)
• WebCore/storage/SQLTransaction.cpp (modified) (2 diffs)
• WebCore/storage/SQLTransaction.h (modified) (2 diffs)

trunk/JavaScriptCore/ChangeLog

@@ +1 @@
+2008-03-05  Brady Eidson  <[email protected]>
+
+        Reviewed by Alexey and Mark Rowe
+
+        Fix for <rdar://problem/5778247> - Reproducible crash on storage/execute-sql-args.html
+
+        DatabaseThread::unscheduleDatabaseTasks() manually filters through a MessageQueue,
+        removing particular items for Databases that were shutting down.
+
+        This filtering operation is not atomic, and therefore causes a race condition with the
+        MessageQueue waking up and reading from the message queue.  
+
+        The end result was an attempt to dereference a null DatabaseTask.  Timing-wise, this never
+        seemed to happen in a debug build, otherwise an assertion would've caught it.  Replacing that
+        assertion with a crash in a release build is what revealed this bug.
+
+        * wtf/MessageQueue.h:
+        (WTF::::waitForMessage): Tweak the waiting logic to check the queue's empty state then go back
+          to sleep if the queue was empty - checking m_killed each time it wakes up.
+
 2008-03-05  David D. Kilzer  <[email protected]>
 

trunk/JavaScriptCore/wtf/MessageQueue.h

@@ -76 +76 @@
     {
         MutexLocker lock(m_mutex);
-        if (m_killed)
-            return false;
         
-        if (m_queue.isEmpty())
+        while (!m_killed && m_queue.isEmpty())
             m_condition.wait(m_mutex);
+
         if (m_killed)
             return false;

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2008-03-05  Brady Eidson  <[email protected]>
+
+        Reviewed by Alexey + Mark Rowe
+
+        Fix for <rdar://problem/5778247> - Reproducible crash on storage/execute-sql-args.html
+
+        This test takes its best shot at handling two databases on a single database thread at once,
+        then having one of those databases go away completely (garbage collection and everything)
+
+        * storage/multiple-databases-garbage-collection-expected.txt: Added.
+        * storage/multiple-databases-garbage-collection.html: Added.
+
 2008-03-04  Maciej Stachowiak  <[email protected]>
 

trunk/WebCore/ChangeLog

@@ +1 @@
+2008-03-05  Brady Eidson  <[email protected]>
+
+        Reviewed by Alexey and Mark Rowe
+
+        Fix for <rdar://problem/5778247> - Reproducible crash on storage/execute-sql-args.html
+
+        DatabaseThread::unscheduleDatabaseTasks() manually filters through a MessageQueue,
+        removing particular items for Databases that were shutting down.
+
+        This filtering operation is not atomic, and therefore causes a race condition with the
+        database thread waking up and reading from the message queue.  
+
+        The end result was an attempt to dereference a null DatabaseTask.  Timing-wise, this never
+        seemed to happen in a debug build, otherwise an assertion would've caught it.  Replacing that
+        assertion with a crash in a release build is what revealed this bug.
+
+        The fix for the above symptom was entirely in WTF::MessageQueue in JSCore.  With this fix in 
+        place, another crash popped up in the layout tests that was related to dereferencing a 
+        deallocated object - simply because SQLTransaction had a raw pointer to it's Database object 
+        when it needed to be a ref pointer.
+
+        * storage/SQLTransaction.cpp:
+        (WebCore::SQLTransaction::runCurrentStatement):
+        * storage/SQLTransaction.h: Change m_database to be a RefPtr
+        (WebCore::SQLTransaction::database):
+
 2008-03-05  Mark Rowe  <[email protected]>
 

trunk/WebCore/storage/SQLTransaction.cpp

@@ -309 +309 @@
     m_database->m_databaseAuthorizer->reset();
     
-    if (m_currentStatement->execute(m_database)) {
+    if (m_currentStatement->execute(m_database.get())) {
         if (m_database->m_databaseAuthorizer->lastActionChangedDatabase()) {
             // Flag this transaction as having changed the database for later delegate notification
@@ -317 +317 @@
             Locker<OriginQuotaManager> locker(manager);
             
-            manager.markDatabase(m_database);
+            manager.markDatabase(m_database.get());
         }
             

trunk/WebCore/storage/SQLTransaction.h

@@ -74 +74 @@
     void performPendingCallback();
     
-    Database* database() { return m_database; }
+    Database* database() { return m_database.get(); }
 
 private:
@@ -108 +108 @@
     bool m_executeSqlAllowed;
     
-    Database* m_database;
+    RefPtr<Database> m_database;
     RefPtr<SQLTransactionWrapper> m_wrapper;
     RefPtr<SQLTransactionCallback> m_callback;