Modern IDB: Refactor open/delete requests to exist in the same queue.
authorbeidson@apple.com <beidson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 17 Dec 2015 23:29:53 +0000 (23:29 +0000)
committerbeidson@apple.com <beidson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 17 Dec 2015 23:29:53 +0000 (23:29 +0000)
https://bugs.webkit.org/show_bug.cgi?id=152397

Reviewed by Alex Christensen.

No new tests (Refactor, all existing tests continue to pass).

The order between incoming open and delete requests matters, and each request
needs to be handled individually.

This patch does the above without changing behavior on existing passing tests,
while moving many currently skipped tests closer to passing.

* Modules/indexeddb/server/IDBServerOperation.cpp:
(WebCore::IDBServer::IDBServerOperation::notifyDeleteRequestBlocked):
(WebCore::IDBServer::IDBServerOperation::notifyDidDeleteDatabase):
* Modules/indexeddb/server/IDBServerOperation.h:
(WebCore::IDBServer::IDBServerOperation::hasNotifiedDeleteRequestBlocked):

* Modules/indexeddb/server/UniqueIDBDatabase.cpp:
(WebCore::IDBServer::UniqueIDBDatabase::~UniqueIDBDatabase):
(WebCore::IDBServer::UniqueIDBDatabase::openDatabaseConnection):
(WebCore::IDBServer::UniqueIDBDatabase::isVersionChangeInProgress):
(WebCore::IDBServer::UniqueIDBDatabase::performCurrentOpenOperation):
(WebCore::IDBServer::UniqueIDBDatabase::performCurrentDeleteOperation):
(WebCore::IDBServer::UniqueIDBDatabase::handleDatabaseOperations):
(WebCore::IDBServer::UniqueIDBDatabase::handleCurrentOperation):
(WebCore::IDBServer::UniqueIDBDatabase::handleDelete):
(WebCore::IDBServer::UniqueIDBDatabase::invokeOperationAndTransactionTimer):
(WebCore::IDBServer::UniqueIDBDatabase::operationAndTransactionTimerFired):
(WebCore::IDBServer::UniqueIDBDatabase::maybeDeleteDatabase): Deleted.
* Modules/indexeddb/server/UniqueIDBDatabase.h:

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

Source/WebCore/ChangeLog
Source/WebCore/Modules/indexeddb/server/IDBServerOperation.cpp
Source/WebCore/Modules/indexeddb/server/IDBServerOperation.h
Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp
Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.h

index b53b731..b777a4c 100644 (file)
@@ -1,3 +1,38 @@
+2015-12-17  Brady Eidson  <beidson@apple.com>
+
+        Modern IDB: Refactor open/delete requests to exist in the same queue.
+        https://bugs.webkit.org/show_bug.cgi?id=152397
+
+        Reviewed by Alex Christensen.
+
+        No new tests (Refactor, all existing tests continue to pass).
+
+        The order between incoming open and delete requests matters, and each request
+        needs to be handled individually.
+        
+        This patch does the above without changing behavior on existing passing tests,
+        while moving many currently skipped tests closer to passing.
+    
+        * Modules/indexeddb/server/IDBServerOperation.cpp:
+        (WebCore::IDBServer::IDBServerOperation::notifyDeleteRequestBlocked):
+        (WebCore::IDBServer::IDBServerOperation::notifyDidDeleteDatabase):
+        * Modules/indexeddb/server/IDBServerOperation.h:
+        (WebCore::IDBServer::IDBServerOperation::hasNotifiedDeleteRequestBlocked):
+
+        * Modules/indexeddb/server/UniqueIDBDatabase.cpp:
+        (WebCore::IDBServer::UniqueIDBDatabase::~UniqueIDBDatabase):
+        (WebCore::IDBServer::UniqueIDBDatabase::openDatabaseConnection):
+        (WebCore::IDBServer::UniqueIDBDatabase::isVersionChangeInProgress):
+        (WebCore::IDBServer::UniqueIDBDatabase::performCurrentOpenOperation):
+        (WebCore::IDBServer::UniqueIDBDatabase::performCurrentDeleteOperation):
+        (WebCore::IDBServer::UniqueIDBDatabase::handleDatabaseOperations):
+        (WebCore::IDBServer::UniqueIDBDatabase::handleCurrentOperation):
+        (WebCore::IDBServer::UniqueIDBDatabase::handleDelete):
+        (WebCore::IDBServer::UniqueIDBDatabase::invokeOperationAndTransactionTimer):
+        (WebCore::IDBServer::UniqueIDBDatabase::operationAndTransactionTimerFired):
+        (WebCore::IDBServer::UniqueIDBDatabase::maybeDeleteDatabase): Deleted.
+        * Modules/indexeddb/server/UniqueIDBDatabase.h:
+
 2015-12-17  Brent Fulgham  <bfulgham@apple.com>
 
         [Win] Prevent flashing/strobing repaints on certain hardware
index 0195e18..e22e59e 100644 (file)
@@ -28,6 +28,7 @@
 
 #if ENABLE(INDEXED_DATABASE)
 
+#include "IDBResultData.h"
 #include <wtf/RefCounted.h>
 
 namespace WebCore {
@@ -54,6 +55,22 @@ bool IDBServerOperation::isDeleteRequest() const
     return m_requestData.isDeleteRequest();
 }
 
+void IDBServerOperation::notifyDeleteRequestBlocked(uint64_t currentVersion)
+{
+    ASSERT(isDeleteRequest());
+    ASSERT(!m_notifiedDeleteRequestBlocked);
+
+    m_connection.notifyOpenDBRequestBlocked(m_requestData.requestIdentifier(), currentVersion, 0);
+    m_notifiedDeleteRequestBlocked = true;
+}
+
+void IDBServerOperation::notifyDidDeleteDatabase(const IDBDatabaseInfo& info)
+{
+    ASSERT(isDeleteRequest());
+
+    m_connection.didDeleteDatabase(IDBResultData::deleteDatabaseSuccess(m_requestData.requestIdentifier(), info));
+}
+
 } // namespace IDBServer
 } // namespace WebCore
 
index a9f9136..3dea52d 100644 (file)
@@ -34,6 +34,9 @@
 #include <wtf/RefCounted.h>
 
 namespace WebCore {
+
+class IDBDatabaseInfo;
+
 namespace IDBServer {
 
 class IDBServerOperation : public RefCounted<IDBServerOperation> {
@@ -46,11 +49,17 @@ public:
     bool isOpenRequest() const;
     bool isDeleteRequest() const;
 
+    void notifyDeleteRequestBlocked(uint64_t currentVersion);
+    void notifyDidDeleteDatabase(const IDBDatabaseInfo&);
+    bool hasNotifiedDeleteRequestBlocked() const { return m_notifiedDeleteRequestBlocked; }
+
 private:
     IDBServerOperation(IDBConnectionToClient&, const IDBRequestData&);
 
     IDBConnectionToClient& m_connection;
     IDBRequestData m_requestData;
+
+    bool m_notifiedDeleteRequestBlocked { false };
 };
 
 } // namespace IDBServer
index 1fcbb9e..021a084 100644 (file)
@@ -51,6 +51,15 @@ UniqueIDBDatabase::UniqueIDBDatabase(IDBServer& server, const IDBDatabaseIdentif
 {
 }
 
+UniqueIDBDatabase::~UniqueIDBDatabase()
+{
+    LOG(IndexedDB, "UniqueIDBDatabase::~UniqueIDBDatabase()");
+    ASSERT(!hasAnyPendingCallbacks());
+    ASSERT(m_inProgressTransactions.isEmpty());
+    ASSERT(m_pendingTransactions.isEmpty());
+    ASSERT(m_openDatabaseConnections.isEmpty());
+}
+
 const IDBDatabaseInfo& UniqueIDBDatabase::info() const
 {
     RELEASE_ASSERT(m_databaseInfo);
@@ -60,9 +69,9 @@ const IDBDatabaseInfo& UniqueIDBDatabase::info() const
 void UniqueIDBDatabase::openDatabaseConnection(IDBConnectionToClient& connection, const IDBRequestData& requestData)
 {
     auto operation = IDBServerOperation::create(connection, requestData);
-    m_pendingOpenDatabaseOperations.append(WTF::move(operation));
+    m_pendingDatabaseOperations.append(WTF::move(operation));
 
-    // An open operation is already in progress, so this one has to wait.
+    // An open operation is already in progress, so we can't possibly handle this one yet.
     if (m_isOpeningBackingStore)
         return;
 
@@ -83,39 +92,14 @@ bool UniqueIDBDatabase::hasAnyPendingCallbacks() const
         || !m_countCallbacks.isEmpty();
 }
 
-bool UniqueIDBDatabase::maybeDeleteDatabase(IDBServerOperation* newestDeleteOperation)
+bool UniqueIDBDatabase::isVersionChangeInProgress()
 {
-    ASSERT(isMainThread());
-    LOG(IndexedDB, "(main) UniqueIDBDatabase::maybeDeleteDatabase");
-
-    if (hasAnyOpenConnections() || !m_closePendingDatabaseConnections.isEmpty()) {
-        // Exactly once, notify all open connections of the pending deletion.
-        if (!m_hasNotifiedConnectionsOfDelete) {
-            notifyConnectionsOfVersionChange(0);
-            m_hasNotifiedConnectionsOfDelete = true;
-        }
-
-        if (newestDeleteOperation)
-            newestDeleteOperation->connection().notifyOpenDBRequestBlocked(newestDeleteOperation->requestData().requestIdentifier(), m_databaseInfo->version(), 0);
-
-        return false;
-    }
-
-    ASSERT(!hasAnyPendingCallbacks());
-    ASSERT(m_inProgressTransactions.isEmpty());
-    ASSERT(m_pendingTransactions.isEmpty());
-    ASSERT(m_openDatabaseConnections.isEmpty());
-    ASSERT(m_pendingOpenDatabaseOperations.isEmpty());
-
-    for (auto& operation : m_pendingDeleteDatabaseOperations) {
-        ASSERT(m_databaseInfo);
-        ASSERT(operation->isDeleteRequest());
-        operation->connection().didDeleteDatabase(IDBResultData::deleteDatabaseSuccess(operation->requestData().requestIdentifier(), *m_databaseInfo));
-    }
+#ifndef NDEBUG
+    if (m_versionChangeTransaction)
+        ASSERT(m_versionChangeDatabaseConnection);
+#endif
 
-    m_server.deleteUniqueIDBDatabase(*this);
-
-    return true;
+    return m_versionChangeDatabaseConnection;
 }
 
 void UniqueIDBDatabase::performCurrentOpenOperation()
@@ -125,6 +109,14 @@ void UniqueIDBDatabase::performCurrentOpenOperation()
     ASSERT(m_currentOperation);
     ASSERT(m_currentOperation->isOpenRequest());
 
+    // If we previously started a version change operation but were blocked by having open connections,
+    // we might now be unblocked.
+    if (m_versionChangeDatabaseConnection) {
+        if (!m_versionChangeTransaction && !hasAnyOpenConnections())
+            startVersionChangeTransaction();
+        return;
+    }
+
     // 3.3.1 Opening a database
     // If requested version is undefined, then let requested version be 1 if db was created in the previous step,
     // or the current version of db otherwise.
@@ -176,29 +168,67 @@ void UniqueIDBDatabase::performCurrentDeleteOperation()
 {
     LOG(IndexedDB, "(main) UniqueIDBDatabase::performCurrentDeleteOperation");
 
-    // Not used yet.
+    ASSERT(m_databaseInfo);
+    ASSERT(m_currentOperation);
+    ASSERT(m_currentOperation->isDeleteRequest());
+
+    if (hasAnyOpenConnections()) {
+        // Exactly once, notify all open connections of the pending deletion.
+        if (!m_hasNotifiedConnectionsOfDelete) {
+            notifyConnectionsOfVersionChange(0);
+            m_hasNotifiedConnectionsOfDelete = true;
+        }
+
+        if (!m_currentOperation->hasNotifiedDeleteRequestBlocked())
+            m_currentOperation->notifyDeleteRequestBlocked(m_databaseInfo->version());
+
+        return;
+    }
+
+    ASSERT(!hasAnyPendingCallbacks());
+    ASSERT(m_inProgressTransactions.isEmpty());
+    ASSERT(m_pendingTransactions.isEmpty());
+    ASSERT(m_openDatabaseConnections.isEmpty());
+
+    m_currentOperation->notifyDidDeleteDatabase(*m_databaseInfo);
+    m_currentOperation = nullptr;
+    m_hasNotifiedConnectionsOfDelete = false;
+    m_deletePending = false;
+
+    if (m_pendingDatabaseOperations.isEmpty())
+        m_server.deleteUniqueIDBDatabase(*this);
+    else
+        invokeOperationAndTransactionTimer();
 }
 
 void UniqueIDBDatabase::handleDatabaseOperations()
 {
     ASSERT(isMainThread());
-    LOG(IndexedDB, "(main) UniqueIDBDatabase::handleDatabaseOperations");
+    LOG(IndexedDB, "(main) UniqueIDBDatabase::handleDatabaseOperations - There are %zu pending", m_pendingDatabaseOperations.size());
 
-    // If a version change transaction is currently in progress, no new connections can be opened right now.
-    // We will try again later.
-    if (m_versionChangeDatabaseConnection)
+    if (m_pendingDatabaseOperations.isEmpty())
         return;
 
-    if (m_pendingOpenDatabaseOperations.isEmpty())
-        return;
+    if (m_versionChangeDatabaseConnection || m_currentOperation) {
+        // We can't start the next database operation quite yet, but we might need to notify all open connections
+        // about a pending delete.
+        if (m_pendingDatabaseOperations.first()->isDeleteRequest() && !m_hasNotifiedConnectionsOfDelete) {
+            m_hasNotifiedConnectionsOfDelete = true;
+            notifyConnectionsOfVersionChange(0);
+        }
+    }
 
-    if (m_currentOperation)
-        return;
+    m_currentOperation = m_pendingDatabaseOperations.takeFirst();
+    LOG(IndexedDB, "UniqueIDBDatabase::handleDatabaseOperations - Popped an operation, now there are %zu pending", m_pendingDatabaseOperations.size());
 
-    m_currentOperation = m_pendingOpenDatabaseOperations.takeFirst();
+    handleCurrentOperation();
+}
 
-    // FIXME: Once handleDatabaseOperations also handles delete operations, remove this ASSERT.
-    ASSERT(m_currentOperation->isOpenRequest());
+void UniqueIDBDatabase::handleCurrentOperation()
+{
+    ASSERT(m_currentOperation);
+
+    RefPtr<UniqueIDBDatabase> protector(this);
 
     if (m_currentOperation->isOpenRequest())
         performCurrentOpenOperation();
@@ -206,6 +236,9 @@ void UniqueIDBDatabase::handleDatabaseOperations()
         performCurrentDeleteOperation();
     else
         ASSERT_NOT_REACHED();
+
+    if (!m_currentOperation)
+        invokeOperationAndTransactionTimer();
 }
 
 bool UniqueIDBDatabase::hasAnyOpenConnections() const
@@ -256,18 +289,8 @@ void UniqueIDBDatabase::handleDelete(IDBConnectionToClient& connection, const ID
 {
     LOG(IndexedDB, "(main) UniqueIDBDatabase::handleDelete");
 
-    auto operation = IDBServerOperation::create(connection, requestData);
-    auto* rawOperation = &operation.get();
-    m_pendingDeleteDatabaseOperations.append(WTF::move(operation));
-
-    // If a different request has already come in to delete this database, there's nothing left to do.
-    // A delete is already in progress, and this request will be handled along with all the rest.
-    if (m_deletePending)
-        return;
-
-    m_deletePending = true;
-
-    maybeDeleteDatabase(rawOperation);
+    m_pendingDatabaseOperations.append(IDBServerOperation::create(connection, requestData));
+    handleDatabaseOperations();
 }
 
 void UniqueIDBDatabase::startVersionChangeTransaction()
@@ -937,6 +960,7 @@ void UniqueIDBDatabase::enqueueTransaction(Ref<UniqueIDBDatabaseTransaction>&& t
 
 void UniqueIDBDatabase::invokeOperationAndTransactionTimer()
 {
+    LOG(IndexedDB, "UniqueIDBDatabase::invokeOperationAndTransactionTimer()");
     if (!m_operationAndTransactionTimer.isActive())
         m_operationAndTransactionTimer.startOneShot(0);
 }
@@ -945,18 +969,13 @@ void UniqueIDBDatabase::operationAndTransactionTimerFired()
 {
     LOG(IndexedDB, "(main) UniqueIDBDatabase::operationAndTransactionTimerFired");
 
-    handleDatabaseOperations();
-
-    if (m_deletePending && maybeDeleteDatabase(nullptr))
-        return;
+    // The current operation might require multiple attempts to handle, so try to
+    // make further progress on it now.
+    if (m_currentOperation)
+        handleCurrentOperation();
 
-    // If the database was not deleted in the previous step, try to run a transaction now.
-    if (m_pendingTransactions.isEmpty()) {
-        if (!hasAnyOpenConnections() && m_currentOperation) {
-            startVersionChangeTransaction();
-            return;
-        }
-    }
+    if (!m_currentOperation)
+        handleDatabaseOperations();
 
     bool hadDeferredTransactions = false;
     auto transaction = takeNextRunnableTransaction(hadDeferredTransactions);
index 0b23977..2d8edfe 100644 (file)
@@ -71,6 +71,8 @@ public:
         return adoptRef(*new UniqueIDBDatabase(server, identifier));
     }
 
+    ~UniqueIDBDatabase();
+
     void openDatabaseConnection(IDBConnectionToClient&, const IDBRequestData&);
 
     const IDBDatabaseInfo& info() const;
@@ -105,15 +107,16 @@ private:
     UniqueIDBDatabase(IDBServer&, const IDBDatabaseIdentifier&);
     
     void handleDatabaseOperations();
+    void handleCurrentOperation();
     void performCurrentOpenOperation();
     void performCurrentDeleteOperation();
     void addOpenDatabaseConnection(Ref<UniqueIDBDatabaseConnection>&&);
-    bool maybeDeleteDatabase(IDBServerOperation*);
     bool hasAnyOpenConnections() const;
 
     void startVersionChangeTransaction();
     void notifyConnectionsOfVersionChangeForUpgrade();
     void notifyConnectionsOfVersionChange(uint64_t requestedVersion);
+    bool isVersionChangeInProgress();
 
     void activateTransactionInBackingStore(UniqueIDBDatabaseTransaction&);
     void inProgressTransactionCompleted(const IDBResourceIdentifier&);
@@ -173,8 +176,7 @@ private:
     IDBServer& m_server;
     IDBDatabaseIdentifier m_identifier;
     
-    Deque<Ref<IDBServerOperation>> m_pendingOpenDatabaseOperations;
-    Deque<Ref<IDBServerOperation>> m_pendingDeleteDatabaseOperations;
+    Deque<Ref<IDBServerOperation>> m_pendingDatabaseOperations;
     RefPtr<IDBServerOperation> m_currentOperation;
 
     HashSet<RefPtr<UniqueIDBDatabaseConnection>> m_openDatabaseConnections;