WebCore:
authorbeidson@apple.com <beidson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 16 Feb 2008 00:51:38 +0000 (00:51 +0000)
committerbeidson@apple.com <beidson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 16 Feb 2008 00:51:38 +0000 (00:51 +0000)
        Reviewed by Darin

        Fix for <rdar://problem/5727175> and <rdar://problem/5740495> - Database threads and callback scripts can run after
        a page has closed or loaded a new document

        Deciding to make the Database I/O semantic the same as loaders/XHR when a document is shut down, this patch implements
        a policy of shutting down the databases in a document at the same time.  This includes removing all pending transactions
        in a database, cutting off an queued statements in the current transaction, and preventing further callbacks from being
        made.

        No new layout tests with this patch as the current layout tests were catching this issue in a plethora of ways already
        (crashing, unexpected exceptions and output, etc)

        * dom/Document.cpp:
        (WebCore::Document::~Document): Don't actually stop the database thread here - it better have been stopped already.
          Add an assertion to that effect.
        (WebCore::Document::addOpenDatabase): Add a new database handle to this Document's open database set
        (WebCore::Document::removeOpenDatabase): Remove such a handle
        (WebCore::Document:: stopDatabases): Call "close" on all open Database handles for this document
        * dom/Document.h:

        * loader/FrameLoader.cpp:
        (WebCore::FrameLoader::stopLoading): In addition to canceling all resource loads and XHRs, stop all database I/O

        * platform/MessageQueue.h:
        (WebCore::MessageQueue::killed):

        * platform/sql/SQLiteTransaction.cpp:
        (WebCore::SQLiteTransaction::stop): Added.  Explicit stop to cut off a transaction so it won't try anymore SQL activity
        * platform/sql/SQLiteTransaction.h:

        * storage/Database.cpp:
        (WebCore::Database::Database):
        (WebCore::Database::~Database):
        (WebCore::Database::markAsDeletedAndClose): Check if the thread has terminated before committing to waiting on the
          thread.
        (WebCore::Database::stop):  Stop this database, including all queued transactions and callbacks
        * storage/Database.h:
        (WebCore::Database::stopped):

        * storage/DatabaseThread.cpp:
        (WebCore::DatabaseThread::terminationRequested):
        * storage/DatabaseThread.h:

        * storage/SQLTransaction.cpp:
        (WebCore::SQLTransaction::executeSQL): Throw an exception if a new executeSQL comes in after a database is closed
        (WebCore::SQLTransaction::checkAndHandleClosedDatabase): Added.  Clears queued statements and clear the next step
          when the database has been closed since the last step/callback was run.  Also stops the current SQLite transaction,
          if any
        (WebCore::SQLTransaction::performNextStep):
        (WebCore::SQLTransaction::performPendingCallback):
        * storage/SQLTransaction.h:

LayoutTests:

        Reviewed by Darin

        Fix for <rdar://problem/5727175> and <rdar://problem/5740495> - Database threads and callback scripts can run after
        a page has closed or loaded a new document

        * storage/close-during-stress-test-expected.txt: Update results - this test contained output from a javascript callback
          that never should have taken place

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@30331 268f45cc-cd09-0410-ab3c-d52691b4dbfc

15 files changed:
LayoutTests/ChangeLog
LayoutTests/storage/close-during-stress-test-expected.txt
WebCore/ChangeLog
WebCore/dom/Document.cpp
WebCore/dom/Document.h
WebCore/loader/FrameLoader.cpp
WebCore/platform/MessageQueue.h
WebCore/platform/sql/SQLiteTransaction.cpp
WebCore/platform/sql/SQLiteTransaction.h
WebCore/storage/Database.cpp
WebCore/storage/Database.h
WebCore/storage/DatabaseThread.cpp
WebCore/storage/DatabaseThread.h
WebCore/storage/SQLTransaction.cpp
WebCore/storage/SQLTransaction.h

index 8d4427ce25a3cf09ec6a17be45de8135fd0a175d..5203b53d60e455ec26c021d1e383463fea3b1492 100644 (file)
@@ -1,3 +1,13 @@
+2008-02-15  Brady Eidson  <beidson@apple.com>
+
+        Reviewed by Darin
+
+        Fix for <rdar://problem/5727175> and <rdar://problem/5740495> - Database threads and callback scripts can run after
+        a page has closed or loaded a new document
+
+        * storage/close-during-stress-test-expected.txt: Update results - this test contained output from a javascript callback
+          that never should have taken place
+
 2008-02-15  Adele Peterson  <adele@apple.com>
 
         Reviewed by Darin.
index 2370b9b5010ea1c076376ece8d7c7970539d2124..2aad90c1629e50b0e6052cf8b6e47e13e101c022 100644 (file)
@@ -1,4 +1,3 @@
-CONSOLE MESSAGE: line 37: Can't find variable: loadNotes
 Should not crash or cause an assertion failure.
 
 A JavaScript failure on the console is expected, however, as the global object is cleared when closing a frame. It actually helps to cause database activity by throwing an exception from a callback.
index 2fbebe6eec69385824cbdf13caa28bfe8d60a4f7..36c858939c3304ca24c35b68d3c2922512f16820 100644 (file)
@@ -1,3 +1,58 @@
+2008-02-15  Brady Eidson  <beidson@apple.com>
+
+        Reviewed by Darin
+
+        Fix for <rdar://problem/5727175> and <rdar://problem/5740495> - Database threads and callback scripts can run after
+        a page has closed or loaded a new document
+
+        Deciding to make the Database I/O semantic the same as loaders/XHR when a document is shut down, this patch implements
+        a policy of shutting down the databases in a document at the same time.  This includes removing all pending transactions 
+        in a database, cutting off an queued statements in the current transaction, and preventing further callbacks from being
+        made.
+
+        No new layout tests with this patch as the current layout tests were catching this issue in a plethora of ways already
+        (crashing, unexpected exceptions and output, etc)
+
+        * dom/Document.cpp:
+        (WebCore::Document::~Document): Don't actually stop the database thread here - it better have been stopped already.
+          Add an assertion to that effect.
+        (WebCore::Document::addOpenDatabase): Add a new database handle to this Document's open database set
+        (WebCore::Document::removeOpenDatabase): Remove such a handle
+        (WebCore::Document:: stopDatabases): Call "close" on all open Database handles for this document
+        * dom/Document.h:
+
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::stopLoading): In addition to canceling all resource loads and XHRs, stop all database I/O
+
+        * platform/MessageQueue.h:
+        (WebCore::MessageQueue::killed): 
+
+        * platform/sql/SQLiteTransaction.cpp:
+        (WebCore::SQLiteTransaction::stop): Added.  Explicit stop to cut off a transaction so it won't try anymore SQL activity
+        * platform/sql/SQLiteTransaction.h:
+
+        * storage/Database.cpp:
+        (WebCore::Database::Database):
+        (WebCore::Database::~Database):
+        (WebCore::Database::markAsDeletedAndClose): Check if the thread has terminated before committing to waiting on the
+          thread.
+        (WebCore::Database::stop):  Stop this database, including all queued transactions and callbacks
+        * storage/Database.h:
+        (WebCore::Database::stopped):
+
+        * storage/DatabaseThread.cpp:
+        (WebCore::DatabaseThread::terminationRequested):
+        * storage/DatabaseThread.h:
+
+        * storage/SQLTransaction.cpp:
+        (WebCore::SQLTransaction::executeSQL): Throw an exception if a new executeSQL comes in after a database is closed
+        (WebCore::SQLTransaction::checkAndHandleClosedDatabase): Added.  Clears queued statements and clear the next step
+          when the database has been closed since the last step/callback was run.  Also stops the current SQLite transaction,
+          if any
+        (WebCore::SQLTransaction::performNextStep):
+        (WebCore::SQLTransaction::performPendingCallback):
+        * storage/SQLTransaction.h:
+
 2008-02-15  Adele Peterson  <adele@apple.com>
 
         Reviewed by Darin.
index 26ee467e0f841dda29c275ab5cd02bdcd6e4d597..b74a9ba6421180b6168df98d7832c448262566e6 100644 (file)
@@ -35,6 +35,7 @@
 #include "ClassNodeList.h"
 #include "Comment.h"
 #include "CookieJar.h"
+#include "Database.h"
 #include "DOMImplementation.h"
 #include "DocLoader.h"
 #include "DocumentFragment.h"
@@ -444,9 +445,8 @@ Document::~Document()
 
 #if ENABLE(DATABASE)
     if (m_databaseThread) {
-        RefPtr<DatabaseThread> databaseThread = m_databaseThread;
+        ASSERT(m_databaseThread->terminationRequested());
         m_databaseThread = 0;
-        databaseThread->requestTermination();
     }
 #endif
 
@@ -3761,6 +3761,25 @@ void Document::updateFocusAppearanceTimerFired(Timer<Document>*)
 }
 
 #if ENABLE(DATABASE)
+
+void Document::addOpenDatabase(Database* database)
+{
+    if (!m_openDatabaseSet)
+        m_openDatabaseSet.set(new DatabaseSet);
+
+    ASSERT(!m_openDatabaseSet->contains(database));
+    m_openDatabaseSet->add(database);
+}
+
+void Document::removeOpenDatabase(Database* database)
+{
+    ASSERT(m_openDatabaseSet && m_openDatabaseSet->contains(database));
+    if (!m_openDatabaseSet)
+        return;
+        
+    m_openDatabaseSet->remove(database);
+}
+
 DatabaseThread* Document::databaseThread()
 {
     if (!m_databaseThread && !m_hasOpenDatabases) {
@@ -3773,6 +3792,23 @@ DatabaseThread* Document::databaseThread()
 
     return m_databaseThread.get();
 }
+
+void Document::stopDatabases()
+{
+    if (m_openDatabaseSet) {
+        DatabaseSet::iterator i = m_openDatabaseSet->begin();
+        DatabaseSet::iterator end = m_openDatabaseSet->end();
+        for (; i != end; ++i) {
+            (*i)->stop();
+            if (m_databaseThread)
+                m_databaseThread->unscheduleDatabaseTasks(*i);
+        }
+    }
+    
+    if (m_databaseThread)
+        m_databaseThread->requestTermination();
+}
+
 #endif
 
 } // namespace WebCore
index ad95f5cc4552f8e946ae52abb70eae3ade662390..41bd05c1d0929d70efe85311d1f8836c87f9b2ac 100644 (file)
@@ -58,6 +58,7 @@ namespace WebCore {
     class CSSStyleSelector;
     class CSSStyleSheet;
     class Comment;
+    class Database;
     class DOMImplementation;
     class DOMWindow;
     class DatabaseThread;
@@ -874,9 +875,12 @@ public:
     bool processingLoadEvent() const { return m_processingLoadEvent; }
 
 #if ENABLE(DATABASE)
+    void addOpenDatabase(Database*);
+    void removeOpenDatabase(Database*);
     DatabaseThread* databaseThread();   // Creates the thread as needed, but not if it has been already terminated.
     void setHasOpenDatabases() { m_hasOpenDatabases = true; }
     bool hasOpenDatabases() { return m_hasOpenDatabases; }
+    void stopDatabases();
 #endif
 protected:
     void clearXMLVersion() { m_xmlVersion = String(); }
@@ -944,6 +948,8 @@ private:
 #if ENABLE(DATABASE)
     RefPtr<DatabaseThread> m_databaseThread;
     bool m_hasOpenDatabases;    // This never changes back to false, even as the database thread is closed.
+    typedef HashSet<Database*> DatabaseSet;
+    OwnPtr<DatabaseSet> m_openDatabaseSet;
 #endif
 #if USE(LOW_BANDWIDTH_DISPLAY)
     bool m_inLowBandwidthDisplay;
index 196abdff71a95598880d6d5eb40911231397f6ea..9a5eb99d9c61e1c463b2eed1af370d9ddb4d4b70 100644 (file)
@@ -589,7 +589,10 @@ void FrameLoader::stopLoading(bool sendUnload)
     if (Document* doc = m_frame->document()) {
         if (DocLoader* docLoader = doc->docLoader())
             cache()->loader()->cancelRequests(docLoader);
+        
         XMLHttpRequest::cancelRequests(doc);
+        
+        doc->stopDatabases();
     }
 
     // tell all subframes to stop as well
index b0f1c1a89bf7758b10943915c666ed30c924fcf6..dd35449303401726ead99ad9ba5a249c433e6123 100644 (file)
@@ -46,6 +46,7 @@ namespace WebCore {
         bool waitForMessage(DataType&);
         bool tryGetMessage(DataType&);
         void kill();
+        bool killed() const { return m_killed; }
 
     private:
         Mutex m_mutex;
index 41d27c222456c2efdc8f99fafe970a532c626604..3fb54f15cf79ae3bffc3713edd6397d5a06d59bd 100644 (file)
@@ -70,5 +70,10 @@ void SQLiteTransaction::rollback()
         m_db.m_transactionInProgress = false;
     }
 }
+
+void SQLiteTransaction::stop()
+{
+    m_inProgress = false;
+}
     
 } // namespace WebCore
index de1ebe6e5620107ff5a7092dd56f99db18eb8ec8..cf5a1802bfeb62453f08193662db047311032b0b 100644 (file)
@@ -41,6 +41,7 @@ public:
     void begin();
     void commit();
     void rollback();
+    void stop();
     
     bool inProgress() const { return m_inProgress; }
 private:
index 1706cfe323af360b072076887aaa588d273b50fd..efe3f25b7b697f4f7393aac0a0c7615471bd4693 100644 (file)
@@ -114,7 +114,8 @@ Database::Database(Document* document, const String& name, const String& expecte
     , m_name(name.copy())
     , m_guid(0)
     , m_expectedVersion(expectedVersion)
-    , m_deleted(0)
+    , m_deleted(false)
+    , m_stopped(false)
 {
     ASSERT(document);
     m_securityOrigin = document->securityOrigin();
@@ -143,6 +144,7 @@ Database::Database(Document* document, const String& name, const String& expecte
     m_filename = DatabaseTracker::tracker().fullPathForDatabase(m_securityOrigin.get(), m_name);
 
     DatabaseTracker::tracker().addOpenDatabase(this);
+    m_document->addOpenDatabase(this);
 }
 
 Database::~Database()
@@ -165,6 +167,7 @@ Database::~Database()
         m_document->databaseThread()->unscheduleDatabaseTasks(this);
 
     DatabaseTracker::tracker().removeOpenDatabase(this);
+    m_document->removeOpenDatabase(this);
 }
 
 bool Database::openAndVerifyVersion(ExceptionCode& e)
@@ -275,6 +278,11 @@ void Database::markAsDeletedAndClose()
     LOG(StorageAPI, "Marking %s (%p) as deleted", stringIdentifier().ascii().data(), this);
     m_deleted = true;
 
+    if (m_document->databaseThread()->terminationRequested()) {
+        LOG(StorageAPI, "Database handle %p is on a terminated DatabaseThread, cannot be marked for normal closure\n", this);
+        return;
+    }
+
     document()->databaseThread()->unscheduleDatabaseTasks(this);
 
     RefPtr<DatabaseCloseTask> task = new DatabaseCloseTask(this);
@@ -289,6 +297,23 @@ void Database::close()
     m_sqliteDatabase.close();
 }
 
+void Database::stop()
+{
+    // FIXME: The net effect of the following code is to remove all pending transactions and statements, but allow the current statement
+    // to run to completion.  In the future we can use the sqlite3_progress_handler or sqlite3_interrupt interfaces to cancel the current
+    // statement in response to close(), as well.
+    
+    // This method is meant to be used as an analog to cancelling a loader, and is used when a document is shut down as the result of 
+    // a page load or closing the page
+    m_stopped = true;
+
+    {
+        MutexLocker locker(m_transactionInProgressMutex);
+        m_transactionQueue.kill();
+        m_transactionInProgress = false;
+    }
+}
+
 unsigned long long Database::databaseSize() const
 {
     long long size;
index 426484c6ca211d4bcbd0a005fe820dd6cb3c1cef..99be3703417bb6f0c022633d2bcc226f6ae00569 100644 (file)
@@ -94,6 +94,9 @@ public:
     bool deleted() const { return m_deleted; }
 
     void close();
+    
+    void stop();
+    bool stopped() const { return m_stopped; }
 
     unsigned long long databaseSize() const;
     unsigned long long maximumSize() const;
@@ -129,6 +132,8 @@ private:
     String m_filename;
 
     bool m_deleted;
+    
+    bool m_stopped;
 
     SQLiteDatabase m_sqliteDatabase;
     RefPtr<DatabaseAuthorizer> m_databaseAuthorizer;
index a040bd90763a7ce9caac3caaff0e3c55ba0132b3..3153536aa3a986f4daf7241c7f42b34d1db84c3c 100644 (file)
@@ -62,6 +62,11 @@ void DatabaseThread::requestTermination()
     m_queue.kill();
 }
 
+bool DatabaseThread::terminationRequested() const
+{
+    return m_queue.killed();
+}
+
 void* DatabaseThread::databaseThreadStart(void* vDatabaseThread)
 {
     DatabaseThread* dbThread = static_cast<DatabaseThread*>(vDatabaseThread);
index 1d39adb0724eced28c6b57657c0b1ca0db3ecec9..a95ca37b1d0841e5a387c91b873357fc877e965c 100644 (file)
@@ -49,6 +49,7 @@ public:
 
     bool start();
     void requestTermination();
+    bool terminationRequested() const;
 
     void scheduleTask(DatabaseTask*);
     void scheduleImmediateTask(DatabaseTask*); // This just adds the task to the front of the queue - the caller needs to be extremely careful not to create deadlocks when waiting for completion.
index 9605a29f867ac88a6f01a6e39028864e769b0466..c9711cb0646890592ee00f4afad6d5fa901124b4 100644 (file)
@@ -78,7 +78,7 @@ SQLTransaction::~SQLTransaction()
 
 void SQLTransaction::executeSQL(const String& sqlStatement, const Vector<SQLValue>& arguments, PassRefPtr<SQLStatementCallback> callback, PassRefPtr<SQLStatementErrorCallback> callbackError, ExceptionCode& e)
 {
-    if (!m_executeSqlAllowed) {
+    if (!m_executeSqlAllowed || m_database->stopped()) {
         e = INVALID_STATE_ERR;
         return;
     }
@@ -128,6 +128,25 @@ const char* SQLTransaction::debugStepName(SQLTransaction::TransactionStepMethod
 }
 #endif
 
+void SQLTransaction::checkAndHandleClosedDatabase()
+{
+    if (!m_database->stopped())
+        return;
+        
+    // If the database was stopped, don't do anything and cancel queued work
+    LOG(StorageAPI, "Database was stopped - cancelling work for this transaction");
+    MutexLocker locker(m_statementMutex);
+    m_statementQueue.clear();
+    m_nextStep = 0;
+    
+    // The current SQLite transaction should be stopped, as well
+    if (m_sqliteTransaction) {
+        m_sqliteTransaction->stop();
+        m_sqliteTransaction.clear();
+    }
+}
+
+
 bool SQLTransaction::performNextStep()
 {
     LOG(StorageAPI, "Step %s\n", debugStepName(m_nextStep));
@@ -137,8 +156,11 @@ bool SQLTransaction::performNextStep()
            m_nextStep == &SQLTransaction::postflightAndCommit ||
            m_nextStep == &SQLTransaction::cleanupAfterSuccessCallback ||
            m_nextStep == &SQLTransaction::cleanupAfterTransactionErrorCallback);
-               
-    (this->*m_nextStep)();
+    
+    checkAndHandleClosedDatabase();
+    
+    if (m_nextStep)
+        (this->*m_nextStep)();
 
     // If there is no nextStep after performing the above step, the transaction is complete
     return !m_nextStep;
@@ -153,8 +175,11 @@ void SQLTransaction::performPendingCallback()
            m_nextStep == &SQLTransaction::deliverStatementCallback ||
            m_nextStep == &SQLTransaction::deliverQuotaIncreaseCallback ||
            m_nextStep == &SQLTransaction::deliverSuccessCallback);
-           
-    (this->*m_nextStep)();
+
+    checkAndHandleClosedDatabase();
+    
+    if (m_nextStep)
+        (this->*m_nextStep)();
 }
 
 void SQLTransaction::openTransactionAndPreflight()
index 8da4bb9ef90a6c7d18e4c712ee1598fc54793d97..50dc0d28862616d2b5eb6fe35a1bdcffc2d3e5c1 100644 (file)
@@ -81,6 +81,8 @@ private:
     
     void enqueueStatement(PassRefPtr<SQLStatement>);
     
+    void checkAndHandleClosedDatabase();
+    
     void openTransactionAndPreflight();
     void deliverTransactionCallback();
     void scheduleToRunStatements();