[webkit-changes] [200322] trunk/Source

Mon, 02 May 2016 08:57:30 -0700

Title: [200322] trunk/Source
Revision
200322
Author
[email protected]
Date
2016-05-02 08:57:05 -0700 (Mon, 02 May 2016)

Log Message

DatabaseTracker::closeAllDatabases calls Database::close from the wrong thread
https://bugs.webkit.org/show_bug.cgi?id=147672
<rdar://problem/22357464>

Reviewed by Brady Eidson.

Source/WebCore:

Schedule a DatabaseCloseTask when Database::close() is called from a thread other than the
database thread as the database thread is responsible for interacting with the database.

Currently -[WebDatabaseManager startBackgroundTask] and WebProcess::processWillSuspendImminently()
call DatabaseTracker::closeAllDatabases() indirectly and directly, respectively, from a
thread other than the database thread. In a debug build, this causes an assertion failure
in Database::close(). In a release/production build, this starts a race between the calling
thread and the database thread that can lead to a crash. It is the responsibility of the
database thread to manage the database. We should ensure that calling Database::close()
delegates the responsibility of actually closing the database to the database thread to
avoid interfering with the database thread or the need to synchronize access to data
structures used by the database thread.

* Modules/webdatabase/Database.cpp:
(WebCore::Database::interrupt): Added. Turns around and calls SQLiteDatabase::interrupt().
(WebCore::Database::close): Added. Schedules a DatabaseCloseTask to close the database and
wait for it to complete if we have not already scheduled closing the database.
(WebCore::Database::performClose): Renamed; formerly named close.
(WebCore::Database::markAsDeletedAndClose): Extracted logic to schedule a DatabaseCloseTask
from here to Database::close() and modified this function to call Database::close().
* Modules/webdatabase/Database.h:
* Modules/webdatabase/DatabaseTask.cpp:
(WebCore::DatabaseCloseTask::doPerformTask): Call Database::performClose() instead of Database::close()
as the latter has been repurposed to schedule closing the database.
* Modules/webdatabase/DatabaseThread.cpp:
(WebCore::DatabaseThread::databaseThread): Ditto.
* Modules/webdatabase/DatabaseTracker.cpp:
(WebCore::DatabaseTracker::closeAllDatabases): Added argument currentQueryBehavior (defaults
to CurrentQueryBehavior::RunToCompletion - close every database after completion of the
current database query, if any).
* Modules/webdatabase/DatabaseTracker.h:
* platform/sql/SQLiteDatabase.cpp:
(WebCore::SQLiteDatabase::interrupt): Added. This is safe to call regardless of the state
of the database and thread safe by <https://www.sqlite.org/c3ref/interrupt.html>.
* platform/sql/SQLiteDatabase.h:

Source/WebKit/mac:

Update the background assertion expiration callback to call DatabaseTracker::closeAllDatabases()
with CurrentQueryBehavior::Interrupt so that we abort a long running query and close the database
immediately to avoid holding a locked file when the process is suspended.

* Storage/WebDatabaseManager.mm:
(+[WebDatabaseManager startBackgroundTask]):

Source/WebKit2:

Call DatabaseTracker::closeAllDatabases() with CurrentQueryBehavior::Interrupt so that we abort
a long running query and close the database immediately to avoid holding a locked file when the
process is suspended.

* WebProcess/WebProcess.cpp:
(WebKit::WebProcess::processWillSuspendImminently):

Modified Paths

Diff

Modified: trunk/Source/WebCore/ChangeLog (200321 => 200322)


--- trunk/Source/WebCore/ChangeLog	2016-05-02 12:01:31 UTC (rev 200321)
+++ trunk/Source/WebCore/ChangeLog	2016-05-02 15:57:05 UTC (rev 200322)
@@ -1,3 +1,47 @@
+2016-05-02  Daniel Bates  <[email protected]>
+
+        DatabaseTracker::closeAllDatabases calls Database::close from the wrong thread
+        https://bugs.webkit.org/show_bug.cgi?id=147672
+        <rdar://problem/22357464>
+
+        Reviewed by Brady Eidson.
+
+        Schedule a DatabaseCloseTask when Database::close() is called from a thread other than the
+        database thread as the database thread is responsible for interacting with the database.
+
+        Currently -[WebDatabaseManager startBackgroundTask] and WebProcess::processWillSuspendImminently()
+        call DatabaseTracker::closeAllDatabases() indirectly and directly, respectively, from a
+        thread other than the database thread. In a debug build, this causes an assertion failure
+        in Database::close(). In a release/production build, this starts a race between the calling
+        thread and the database thread that can lead to a crash. It is the responsibility of the
+        database thread to manage the database. We should ensure that calling Database::close()
+        delegates the responsibility of actually closing the database to the database thread to
+        avoid interfering with the database thread or the need to synchronize access to data
+        structures used by the database thread.
+
+        * Modules/webdatabase/Database.cpp:
+        (WebCore::Database::interrupt): Added. Turns around and calls SQLiteDatabase::interrupt().
+        (WebCore::Database::close): Added. Schedules a DatabaseCloseTask to close the database and
+        wait for it to complete if we have not already scheduled closing the database.
+        (WebCore::Database::performClose): Renamed; formerly named close.
+        (WebCore::Database::markAsDeletedAndClose): Extracted logic to schedule a DatabaseCloseTask
+        from here to Database::close() and modified this function to call Database::close().
+        * Modules/webdatabase/Database.h:
+        * Modules/webdatabase/DatabaseTask.cpp:
+        (WebCore::DatabaseCloseTask::doPerformTask): Call Database::performClose() instead of Database::close()
+        as the latter has been repurposed to schedule closing the database.
+        * Modules/webdatabase/DatabaseThread.cpp:
+        (WebCore::DatabaseThread::databaseThread): Ditto.
+        * Modules/webdatabase/DatabaseTracker.cpp:
+        (WebCore::DatabaseTracker::closeAllDatabases): Added argument currentQueryBehavior (defaults
+        to CurrentQueryBehavior::RunToCompletion - close every database after completion of the
+        current database query, if any).
+        * Modules/webdatabase/DatabaseTracker.h:
+        * platform/sql/SQLiteDatabase.cpp:
+        (WebCore::SQLiteDatabase::interrupt): Added. This is safe to call regardless of the state
+        of the database and thread safe by <https://www.sqlite.org/c3ref/interrupt.html>.
+        * platform/sql/SQLiteDatabase.h:
+
 2016-05-02  Yoav Weiss  <[email protected]>
 
         Move ResourceTiming behind a runtime flag

Modified: trunk/Source/WebCore/Modules/webdatabase/Database.cpp (200321 => 200322)


--- trunk/Source/WebCore/Modules/webdatabase/Database.cpp	2016-05-02 12:01:31 UTC (rev 200321)
+++ trunk/Source/WebCore/Modules/webdatabase/Database.cpp	2016-05-02 15:57:05 UTC (rev 200322)
@@ -279,8 +279,33 @@
     return success;
 }
 
+void Database::interrupt()
+{
+    // It is safe to call this from any thread for an opened or closed database.
+    m_sqliteDatabase.interrupt();
+}
+
 void Database::close()
 {
+    if (!databaseContext()->databaseThread())
+        return;
+
+    DatabaseTaskSynchronizer synchronizer;
+    if (databaseContext()->databaseThread()->terminationRequested(&synchronizer)) {
+        LOG(StorageAPI, "Database handle %p is on a terminated DatabaseThread, cannot be marked for normal closure\n", this);
+        return;
+    }
+
+    databaseContext()->databaseThread()->scheduleImmediateTask(std::make_unique<DatabaseCloseTask>(*this, synchronizer));
+
+    // FIXME: iOS depends on this function blocking until the database is closed as part
+    // of closing all open databases from a process assertion expiration handler.
+    // See <https://bugs.webkit.org/show_bug.cgi?id=157184>.
+    synchronizer.waitForTaskCompletion();
+}
+
+void Database::performClose()
+{
     ASSERT(databaseContext()->databaseThread());
     ASSERT(currentThread() == databaseContext()->databaseThread()->getThreadID());
 
@@ -624,15 +649,7 @@
     LOG(StorageAPI, "Marking %s (%p) as deleted", stringIdentifier().ascii().data(), this);
     m_deleted = true;
 
-    DatabaseTaskSynchronizer synchronizer;
-    if (databaseContext()->databaseThread()->terminationRequested(&synchronizer)) {
-        LOG(StorageAPI, "Database handle %p is on a terminated DatabaseThread, cannot be marked for normal closure\n", this);
-        return;
-    }
-
-    auto task = std::make_unique<DatabaseCloseTask>(*this, synchronizer);
-    databaseContext()->databaseThread()->scheduleImmediateTask(WTFMove(task));
-    synchronizer.waitForTaskCompletion();
+    close();
 }
 
 void Database::changeVersion(const String& oldVersion, const String& newVersion, RefPtr<SQLTransactionCallback>&& callback, RefPtr<SQLTransactionErrorCallback>&& errorCallback, RefPtr<VoidCallback>&& successCallback)

Modified: trunk/Source/WebCore/Modules/webdatabase/Database.h (200321 => 200322)


--- trunk/Source/WebCore/Modules/webdatabase/Database.h	2016-05-02 12:01:31 UTC (rev 200321)
+++ trunk/Source/WebCore/Modules/webdatabase/Database.h	2016-05-02 15:57:05 UTC (rev 200322)
@@ -59,6 +59,8 @@
     virtual bool openAndVerifyVersion(bool setVersionInNewDatabase, DatabaseError&, String& errorMessage);
     void close();
 
+    void interrupt();
+
     bool opened() const { return m_opened; }
     bool isNew() const { return m_new; }
 
@@ -113,10 +115,14 @@
 
     void scheduleTransactionCallback(SQLTransaction*);
 
+    void incrementalVacuumIfNeeded();
+
+    // Called from DatabaseTask
     bool performOpenAndVerify(bool shouldSetVersionInNewDatabase, DatabaseError&, String& errorMessage);
     Vector<String> performGetTableNames();
 
-    void incrementalVacuumIfNeeded();
+    // Called from DatabaseTask and DatabaseThread
+    void performClose();
 
 private:
     Database(PassRefPtr<DatabaseContext>, const String& name, const String& expectedVersion, const String& displayName, unsigned long estimatedSize);

Modified: trunk/Source/WebCore/Modules/webdatabase/DatabaseTask.cpp (200321 => 200322)


--- trunk/Source/WebCore/Modules/webdatabase/DatabaseTask.cpp	2016-05-02 12:01:31 UTC (rev 200321)
+++ trunk/Source/WebCore/Modules/webdatabase/DatabaseTask.cpp	2016-05-02 15:57:05 UTC (rev 200322)
@@ -131,7 +131,7 @@
 
 void DatabaseCloseTask::doPerformTask()
 {
-    database().close();
+    database().performClose();
 }
 
 #if !LOG_DISABLED

Modified: trunk/Source/WebCore/Modules/webdatabase/DatabaseThread.cpp (200321 => 200322)


--- trunk/Source/WebCore/Modules/webdatabase/DatabaseThread.cpp	2016-05-02 12:01:31 UTC (rev 200321)
+++ trunk/Source/WebCore/Modules/webdatabase/DatabaseThread.cpp	2016-05-02 15:57:05 UTC (rev 200322)
@@ -127,7 +127,7 @@
     }
 
     for (auto& openDatabase : openSetCopy)
-        openDatabase->close();
+        openDatabase->performClose();
 
     // Detach the thread so its resources are no longer of any concern to anyone else
     detachThread(m_threadID);

Modified: trunk/Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp (200321 => 200322)


--- trunk/Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp	2016-05-02 12:01:31 UTC (rev 200321)
+++ trunk/Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp	2016-05-02 15:57:05 UTC (rev 200322)
@@ -307,7 +307,7 @@
     return maxSize;
 }
 
-void DatabaseTracker::closeAllDatabases()
+void DatabaseTracker::closeAllDatabases(CurrentQueryBehavior currentQueryBehavior)
 {
     Vector<Ref<Database>> openDatabases;
     {
@@ -321,8 +321,11 @@
             }
         }
     }
-    for (auto& database : openDatabases)
+    for (auto& database : openDatabases) {
+        if (currentQueryBehavior == CurrentQueryBehavior::Interrupt)
+            database->interrupt();
         database->close();
+    }
 }
 
 String DatabaseTracker::originPath(SecurityOrigin* origin) const

Modified: trunk/Source/WebCore/Modules/webdatabase/DatabaseTracker.h (200321 => 200322)


--- trunk/Source/WebCore/Modules/webdatabase/DatabaseTracker.h	2016-05-02 12:01:31 UTC (rev 200321)
+++ trunk/Source/WebCore/Modules/webdatabase/DatabaseTracker.h	2016-05-02 15:57:05 UTC (rev 200322)
@@ -46,6 +46,8 @@
 class OriginLock;
 class SecurityOrigin;
 
+enum class CurrentQueryBehavior { Interrupt, RunToCompletion };
+
 class DatabaseTracker {
     WTF_MAKE_NONCOPYABLE(DatabaseTracker); WTF_MAKE_FAST_ALLOCATED;
 public:
@@ -74,7 +76,7 @@
 
     unsigned long long getMaxSizeForDatabase(const Database*);
 
-    WEBCORE_EXPORT void closeAllDatabases();
+    WEBCORE_EXPORT void closeAllDatabases(CurrentQueryBehavior = CurrentQueryBehavior::RunToCompletion);
 
 private:
     explicit DatabaseTracker(const String& databasePath);

Modified: trunk/Source/WebCore/platform/sql/SQLiteDatabase.cpp (200321 => 200322)


--- trunk/Source/WebCore/platform/sql/SQLiteDatabase.cpp	2016-05-02 12:01:31 UTC (rev 200321)
+++ trunk/Source/WebCore/platform/sql/SQLiteDatabase.cpp	2016-05-02 15:57:05 UTC (rev 200322)
@@ -334,6 +334,13 @@
     return lastError();
 }
 
+void SQLiteDatabase::interrupt()
+{
+    LockHolder locker(m_databaseClosingMutex);
+    if (m_db)
+        sqlite3_interrupt(m_db);
+}
+
 int64_t SQLiteDatabase::lastInsertRowID()
 {
     if (!m_db)

Modified: trunk/Source/WebCore/platform/sql/SQLiteDatabase.h (200321 => 200322)


--- trunk/Source/WebCore/platform/sql/SQLiteDatabase.h	2016-05-02 12:01:31 UTC (rev 200321)
+++ trunk/Source/WebCore/platform/sql/SQLiteDatabase.h	2016-05-02 15:57:05 UTC (rev 200322)
@@ -68,7 +68,10 @@
     int runIncrementalVacuumCommand();
     
     bool transactionInProgress() const { return m_transactionInProgress; }
-    
+
+    // Aborts the current database operation. This is thread safe.
+    void interrupt();
+
     int64_t lastInsertRowID();
     int lastChanges();

Modified: trunk/Source/WebKit/mac/ChangeLog (200321 => 200322)


--- trunk/Source/WebKit/mac/ChangeLog	2016-05-02 12:01:31 UTC (rev 200321)
+++ trunk/Source/WebKit/mac/ChangeLog	2016-05-02 15:57:05 UTC (rev 200322)
@@ -1,3 +1,18 @@
+2016-05-02  Daniel Bates  <[email protected]>
+
+        DatabaseTracker::closeAllDatabases calls Database::close from the wrong thread
+        https://bugs.webkit.org/show_bug.cgi?id=147672
+        <rdar://problem/22357464>
+
+        Reviewed by Brady Eidson.
+
+        Update the background assertion expiration callback to call DatabaseTracker::closeAllDatabases()
+        with CurrentQueryBehavior::Interrupt so that we abort a long running query and close the database
+        immediately to avoid holding a locked file when the process is suspended.
+
+        * Storage/WebDatabaseManager.mm:
+        (+[WebDatabaseManager startBackgroundTask]):
+
 2016-04-29  Dean Jackson  <[email protected]>
 
         RTL <select> popup menu is in the wrong location

Modified: trunk/Source/WebKit/mac/Storage/WebDatabaseManager.mm (200321 => 200322)


--- trunk/Source/WebKit/mac/Storage/WebDatabaseManager.mm	2016-05-02 12:01:31 UTC (rev 200321)
+++ trunk/Source/WebKit/mac/Storage/WebDatabaseManager.mm	2016-05-02 15:57:05 UTC (rev 200322)
@@ -269,7 +269,7 @@
         return;
     
     setTransactionBackgroundTaskIdentifier(startBackgroundTask(^ {
-        DatabaseTracker::tracker().closeAllDatabases();
+        DatabaseTracker::tracker().closeAllDatabases(CurrentQueryBehavior::Interrupt);
         [WebDatabaseManager endBackgroundTask];
     }));
 }

Modified: trunk/Source/WebKit2/ChangeLog (200321 => 200322)


--- trunk/Source/WebKit2/ChangeLog	2016-05-02 12:01:31 UTC (rev 200321)
+++ trunk/Source/WebKit2/ChangeLog	2016-05-02 15:57:05 UTC (rev 200322)
@@ -1,3 +1,18 @@
+2016-05-02  Daniel Bates  <[email protected]>
+
+        DatabaseTracker::closeAllDatabases calls Database::close from the wrong thread
+        https://bugs.webkit.org/show_bug.cgi?id=147672
+        <rdar://problem/22357464>
+
+        Reviewed by Brady Eidson.
+
+        Call DatabaseTracker::closeAllDatabases() with CurrentQueryBehavior::Interrupt so that we abort
+        a long running query and close the database immediately to avoid holding a locked file when the
+        process is suspended.
+
+        * WebProcess/WebProcess.cpp:
+        (WebKit::WebProcess::processWillSuspendImminently):
+
 2016-05-01  Darin Adler  <[email protected]>
 
         Update Fetch to use enum class instead of string for enumerations

Modified: trunk/Source/WebKit2/WebProcess/WebProcess.cpp (200321 => 200322)


--- trunk/Source/WebKit2/WebProcess/WebProcess.cpp	2016-05-02 12:01:31 UTC (rev 200321)
+++ trunk/Source/WebKit2/WebProcess/WebProcess.cpp	2016-05-02 15:57:05 UTC (rev 200322)
@@ -1249,7 +1249,7 @@
     }
 
     WEBPROCESS_LOG_ALWAYS("%p - WebProcess::processWillSuspendImminently()", this);
-    DatabaseTracker::tracker().closeAllDatabases();
+    DatabaseTracker::tracker().closeAllDatabases(CurrentQueryBehavior::Interrupt);
     actualPrepareToSuspend(ShouldAcknowledgeWhenReadyToSuspend::No);
     handled = true;
 }

_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to