DatabaseContext should not prevent entering the back/forward cache
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 5 Nov 2019 19:15:18 +0000 (19:15 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 5 Nov 2019 19:15:18 +0000 (19:15 +0000)
https://bugs.webkit.org/show_bug.cgi?id=203103
<rdar://problem/56592193>

Reviewed by Geoffrey Garen.

Source/WebCore:

Let pages with active webdatabase transactions into the back/forward cache. We make sure
to queue tasks that run script to the Window event loop, so that they get delayed when
the document is suspended.

This patch also makes sure that the transaction's error callback gets called if the database
gets closed while the transaction is going on. We previously did not happen and it was causing
issues because databases get closed on navigation.

No new tests, updated existing test.

* Modules/webdatabase/Database.cpp:
(WebCore::Database::runTransaction):
* Modules/webdatabase/DatabaseContext.cpp:
(WebCore::DatabaseContext::shouldPreventEnteringBackForwardCache_DEPRECATED const): Deleted.
* Modules/webdatabase/DatabaseContext.h:
* Modules/webdatabase/DatabaseManager.cpp:
(WebCore::DatabaseManager::openDatabase):
* Modules/webdatabase/SQLTransaction.cpp:
(WebCore::SQLTransaction::performPendingCallback):
(WebCore::SQLTransaction::notifyDatabaseThreadIsShuttingDown):
(WebCore::SQLTransaction::callErrorCallbackDueToInterruption):
(WebCore::SQLTransaction::checkAndHandleClosedDatabase):
(WebCore::SQLTransaction::deliverTransactionErrorCallback):
(WebCore::SQLTransaction::deliverSuccessCallback):
(WebCore::SQLTransaction::computeNextStateAndCleanupIfNeeded):
* Modules/webdatabase/SQLTransaction.h:

LayoutTests:

* fast/history/page-cache-webdatabase-pending-transaction-expected.txt:
* fast/history/page-cache-webdatabase-pending-transaction.html:
Update existing test to reflect behavior change.

* platform/gtk/TestExpectations:
* platform/mac/TestExpectations:
Unmark test as flaky.

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

12 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/history/page-cache-webdatabase-pending-transaction-expected.txt
LayoutTests/fast/history/page-cache-webdatabase-pending-transaction.html
LayoutTests/platform/gtk/TestExpectations
LayoutTests/platform/mac/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/Modules/webdatabase/Database.cpp
Source/WebCore/Modules/webdatabase/DatabaseContext.cpp
Source/WebCore/Modules/webdatabase/DatabaseContext.h
Source/WebCore/Modules/webdatabase/DatabaseManager.cpp
Source/WebCore/Modules/webdatabase/SQLTransaction.cpp
Source/WebCore/Modules/webdatabase/SQLTransaction.h

index 01446ec..e31d685 100644 (file)
@@ -1,3 +1,19 @@
+2019-11-05  Chris Dumez  <cdumez@apple.com>
+
+        DatabaseContext should not prevent entering the back/forward cache
+        https://bugs.webkit.org/show_bug.cgi?id=203103
+        <rdar://problem/56592193>
+
+        Reviewed by Geoffrey Garen.
+
+        * fast/history/page-cache-webdatabase-pending-transaction-expected.txt:
+        * fast/history/page-cache-webdatabase-pending-transaction.html:
+        Update existing test to reflect behavior change.
+
+        * platform/gtk/TestExpectations:
+        * platform/mac/TestExpectations:
+        Unmark test as flaky.
+
 2019-11-05  Wenson Hsieh  <wenson_hsieh@apple.com>
 
         Native text substitutions interfere with HTML <datalist> options resulting in crash
index 16d022f..c1a3380 100644 (file)
@@ -1,10 +1,14 @@
-Tests that a page with an open WebDatabase that has pending transactions does not go into the page cache.
+CONSOLE MESSAGE: line 54: Web SQL is deprecated. Please use IndexedDB instead.
+Tests that a page with an open WebDatabase that has pending transactions is able to go into the back/forward cache.
 
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
 pageshow - not from cache
-PASS Page was not restored from page cache
+pagehide - entering cache
+pageshow - from cache
+PASS Page did enter and was restored from the back/forward cache
+PASS All done
 PASS successfullyParsed is true
 
 TEST COMPLETE
index 55963bc..69afe29 100644 (file)
@@ -2,58 +2,75 @@
 <!DOCTYPE html>
 <html>
 <body>
-<script src="../../resources/js-test-pre.js"></script>
+<script src="../../resources/js-test.js"></script>
 <script>
-description('Tests that a page with an open WebDatabase that has pending transactions does not go into the page cache.');
-window.jsTestIsAsync = true;
+description('Tests that a page with an open WebDatabase that has pending transactions is able to go into the back/forward cache.');
+jsTestIsAsync = true;
+let restoredFromCache = false;
+let pendingTransactionCount = 0;
 
 if (window.testRunner)
     testRunner.clearAllDatabases();
 
+function checkTestComplete()
+{
+    if (!pendingTransactionCount && restoredFromCache) {
+        testPassed("All done");
+        finishJSTest();
+    }
+}
+
 window.addEventListener("pageshow", function(event) {
     debug("pageshow - " + (event.persisted ? "" : "not ") + "from cache");
-    if (!window.sessionStorage.page_cache_open_webdatabase_test_started)
-        return;
 
-    delete window.sessionStorage.page_cache_open_webdatabase_test_started;
-
-    if (event.persisted)
-        testFailed("Page did enter and was restored from the page cache");
-    else
-        testPassed("Page was not restored from page cache");
-    finishJSTest();
+    if (event.persisted) {
+        testPassed("Page did enter and was restored from the back/forward cache");
+        restoredFromCache = true;
+        checkTestComplete();
+    }
 }, false);
 
 window.addEventListener("pagehide", function(event) {
     debug("pagehide - " + (event.persisted ? "" : "not ") + "entering cache");
-    if (event.persisted) {
-        testFailed("Page entered the page cache.");
+    if (!event.persisted) {
+        testFailed("Page failed to enter the back/forward cache.");
         finishJSTest();
     }
 }, false);
 
+function handleTransactionComplete()
+{
+    if (!pendingTransactionCount) {
+        testFailed("Too many completion handlers");
+        finishJSTest();
+    }
+
+    pendingTransactionCount--;
+    checkTestComplete();
+}
+
 window.addEventListener('load', function() {
-    // Open the database.
-    db = openDatabase("PageCacheTest", "", "Page Cache Test", 32768);
+    setTimeout(() => {
+        db = openDatabase("PageCacheTest", "", "Back Forward Cache Test", 32768);
 
-    db.transaction(function(tx) {
-        // Force a back navigation back to this page.
-        window.sessionStorage.page_cache_open_webdatabase_test_started = true;
-        window.location.href = "resources/page-cache-helper.html";
+        pendingTransactionCount++;
+        db.transaction(function(tx) {
+            window.location.href = "resources/page-cache-helper.html";
 
-        tx.executeSql('CREATE TABLE IF NOT EXISTS LOGS (id unique, log)');
-    });
+            tx.executeSql('CREATE TABLE IF NOT EXISTS LOGS (id unique, log)');
+        }, handleTransactionComplete, handleTransactionComplete);
 
-    db.transaction(function(tx) {
-        tx.executeSql('CREATE TABLE IF NOT EXISTS LOGS2 (id unique, log)');
-    });
+        pendingTransactionCount++;
+        db.transaction(function(tx) {
+            tx.executeSql('CREATE TABLE IF NOT EXISTS LOGS2 (id unique, log)');
+        }, handleTransactionComplete, handleTransactionComplete);
 
-    db.transaction(function(tx) {
-        tx.executeSql('CREATE TABLE IF NOT EXISTS LOGS3 (id unique, log)');
-    });
+        pendingTransactionCount++;
+        db.transaction(function(tx) {
+            tx.executeSql('CREATE TABLE IF NOT EXISTS LOGS3 (id unique, log)');
+        }, handleTransactionComplete, handleTransactionComplete);
+    }, 0);
 }, false);
-
 </script>
-<script src="../../resources/js-test-post.js"></script>
 </body>
 </html>
index bbd6e4d..9875ca5 100644 (file)
@@ -1659,7 +1659,6 @@ webkit.org/b/144864 fast/events/clear-edit-drag-state.html [ Failure Pass ]
 webkit.org/b/144864 fast/events/clear-drag-state.html [ Failure Pass ]
 
 webkit.org/b/145051 media/video-rtl.html [ ImageOnlyFailure Pass ]
-webkit.org/b/145052 fast/history/page-cache-webdatabase-pending-transaction.html [ Failure Pass ]
 
 webkit.org/b/145167 transforms/2d/perspective-not-fixed-container.html [ ImageOnlyFailure Pass ]
 
index 8510529..d0c4fa4 100644 (file)
@@ -1198,8 +1198,6 @@ webkit.org/b/158889 media/video-controls-show-on-kb-or-ax-event.html [ Pass Fail
 # rdar://problem/27141291
 [ Sierra+ ] editing/selection/triple-click-in-pre.html [ Failure ]
 
-webkit.org/b/159379 fast/history/page-cache-webdatabase-pending-transaction.html [ Pass Failure ]
-
 # rdar://problem/31243824
 webkit.org/b/158747 media/restore-from-page-cache.html [ Pass Failure Crash ]
 
index db8dc97..8486afb 100644 (file)
@@ -1,3 +1,38 @@
+2019-11-05  Chris Dumez  <cdumez@apple.com>
+
+        DatabaseContext should not prevent entering the back/forward cache
+        https://bugs.webkit.org/show_bug.cgi?id=203103
+        <rdar://problem/56592193>
+
+        Reviewed by Geoffrey Garen.
+
+        Let pages with active webdatabase transactions into the back/forward cache. We make sure
+        to queue tasks that run script to the Window event loop, so that they get delayed when
+        the document is suspended.
+
+        This patch also makes sure that the transaction's error callback gets called if the database
+        gets closed while the transaction is going on. We previously did not happen and it was causing
+        issues because databases get closed on navigation.
+
+        No new tests, updated existing test.
+
+        * Modules/webdatabase/Database.cpp:
+        (WebCore::Database::runTransaction):
+        * Modules/webdatabase/DatabaseContext.cpp:
+        (WebCore::DatabaseContext::shouldPreventEnteringBackForwardCache_DEPRECATED const): Deleted.
+        * Modules/webdatabase/DatabaseContext.h:
+        * Modules/webdatabase/DatabaseManager.cpp:
+        (WebCore::DatabaseManager::openDatabase):
+        * Modules/webdatabase/SQLTransaction.cpp:
+        (WebCore::SQLTransaction::performPendingCallback):
+        (WebCore::SQLTransaction::notifyDatabaseThreadIsShuttingDown):
+        (WebCore::SQLTransaction::callErrorCallbackDueToInterruption):
+        (WebCore::SQLTransaction::checkAndHandleClosedDatabase):
+        (WebCore::SQLTransaction::deliverTransactionErrorCallback):
+        (WebCore::SQLTransaction::deliverSuccessCallback):
+        (WebCore::SQLTransaction::computeNextStateAndCleanupIfNeeded):
+        * Modules/webdatabase/SQLTransaction.h:
+
 2019-11-05  Sihui Liu  <sihui_liu@apple.com>
 
         REGRESSION (r250754): web page using IDBIndex doesn't load occasionally
index 195e6f7..30d0a22 100644 (file)
@@ -52,6 +52,7 @@
 #include "ScriptExecutionContext.h"
 #include "SecurityOrigin.h"
 #include "VoidCallback.h"
+#include "WindowEventLoop.h"
 #include <wtf/NeverDestroyed.h>
 #include <wtf/RefPtr.h>
 #include <wtf/StdLibExtras.h>
@@ -684,10 +685,11 @@ void Database::resetAuthorizer()
 
 void Database::runTransaction(RefPtr<SQLTransactionCallback>&& callback, RefPtr<SQLTransactionErrorCallback>&& errorCallback, RefPtr<VoidCallback>&& successCallback, RefPtr<SQLTransactionWrapper>&& wrapper, bool readOnly)
 {
+    ASSERT(isMainThread());
     LockHolder locker(m_transactionInProgressMutex);
     if (!m_isTransactionQueueEnabled) {
         if (errorCallback) {
-            callOnMainThread([errorCallback = makeRef(*errorCallback)]() {
+            m_document->eventLoop().queueTask(TaskSource::Networking, m_document, [errorCallback = makeRef(*errorCallback)]() {
                 errorCallback->handleEvent(SQLError::create(SQLError::UNKNOWN_ERR, "database has been closed"));
             });
         }
index 1fb061f..a3ce70c 100644 (file)
@@ -130,15 +130,6 @@ void DatabaseContext::stop()
     stopDatabases();
 }
 
-// FIXME: This should never prevent entering the back/forward cache.
-bool DatabaseContext::shouldPreventEnteringBackForwardCache_DEPRECATED() const
-{
-    if (!hasOpenDatabases() || !m_databaseThread)
-        return false;
-
-    return m_databaseThread->hasPendingDatabaseActivity();
-}
-
 DatabaseThread* DatabaseContext::databaseThread()
 {
     if (!m_databaseThread && !m_hasOpenDatabases) {
index bef5520..1057a50 100644 (file)
@@ -73,7 +73,6 @@ private:
 
     void contextDestroyed() override;
     void stop() override;
-    bool shouldPreventEnteringBackForwardCache_DEPRECATED() const override;
     const char* activeDOMObjectName() const override { return "DatabaseContext"; }
 
     RefPtr<DatabaseThread> m_databaseThread;
index 05dfa37..a015993 100644 (file)
@@ -38,6 +38,7 @@
 #include "ScriptController.h"
 #include "SecurityOrigin.h"
 #include "SecurityOriginData.h"
+#include "WindowEventLoop.h"
 #include <wtf/NeverDestroyed.h>
 
 namespace WebCore {
@@ -192,6 +193,7 @@ void DatabaseManager::removeProposedDatabase(ProposedDatabase& database)
 
 ExceptionOr<Ref<Database>> DatabaseManager::openDatabase(Document& document, const String& name, const String& expectedVersion, const String& displayName, unsigned estimatedSize, RefPtr<DatabaseCallback>&& creationCallback)
 {
+    ASSERT(isMainThread());
     ScriptController::initializeThreading();
 
     bool setVersionInNewDatabase = !creationCallback;
@@ -208,7 +210,7 @@ ExceptionOr<Ref<Database>> DatabaseManager::openDatabase(Document& document, con
     if (database->isNew() && creationCallback.get()) {
         LOG(StorageAPI, "Scheduling DatabaseCreationCallbackTask for database %p\n", database.get());
         database->setHasPendingCreationEvent(true);
-        database->m_document->postTask([creationCallback, database] (ScriptExecutionContext&) {
+        database->m_document->eventLoop().queueTask(TaskSource::Networking, database->m_document, [creationCallback, database]() {
             creationCallback->handleEvent(*database);
             database->setHasPendingCreationEvent(false);
         });
index ac490f5..4e9bd63 100644 (file)
@@ -47,6 +47,7 @@
 #include "SQLTransactionErrorCallback.h"
 #include "SQLiteTransaction.h"
 #include "VoidCallback.h"
+#include "WindowEventLoop.h"
 #include <wtf/Optional.h>
 #include <wtf/StdLibExtras.h>
 #include <wtf/Vector.h>
@@ -109,6 +110,7 @@ void SQLTransaction::performNextStep()
 
 void SQLTransaction::performPendingCallback()
 {
+    ASSERT(isMainThread());
     LOG(StorageAPI, "Callback %s\n", debugStepName(m_nextStep));
 
     ASSERT(m_nextStep == &SQLTransaction::deliverTransactionCallback
@@ -125,9 +127,25 @@ void SQLTransaction::performPendingCallback()
 
 void SQLTransaction::notifyDatabaseThreadIsShuttingDown()
 {
+    callOnMainThread([this, protectedThis = makeRef(*this)]() mutable {
+        callErrorCallbackDueToInterruption();
+    });
+
     m_backend.notifyDatabaseThreadIsShuttingDown();
 }
 
+void SQLTransaction::callErrorCallbackDueToInterruption()
+{
+    ASSERT(isMainThread());
+    auto errorCallback = m_errorCallbackWrapper.unwrap();
+    if (!errorCallback)
+        return;
+
+    m_database->document().eventLoop().queueTask(TaskSource::Networking, m_database->document(), [errorCallback = WTFMove(errorCallback)]() mutable {
+        errorCallback->handleEvent(SQLError::create(SQLError::DATABASE_ERR, "the database was closed"));
+    });
+}
+
 void SQLTransaction::enqueueStatement(std::unique_ptr<SQLStatement> statement)
 {
     LockHolder locker(m_statementMutex);
@@ -180,6 +198,8 @@ void SQLTransaction::checkAndHandleClosedDatabase()
     m_statementQueue.clear();
     m_nextStep = nullptr;
 
+    callErrorCallbackDueToInterruption();
+
     // Release the unneeded callbacks, to break reference cycles.
     m_callbackWrapper.clear();
     m_successCallbackWrapper.clear();
@@ -394,8 +414,11 @@ void SQLTransaction::deliverTransactionErrorCallback()
     // Spec 4.3.2.10: If exists, invoke error callback with the last
     // error to have occurred in this transaction.
     RefPtr<SQLTransactionErrorCallback> errorCallback = m_errorCallbackWrapper.unwrap();
-    if (errorCallback)
-        errorCallback->handleEvent(*m_transactionError);
+    if (errorCallback) {
+        m_database->document().eventLoop().queueTask(TaskSource::Networking, m_database->document(), [errorCallback = WTFMove(errorCallback), transactionError = m_transactionError]() mutable {
+            errorCallback->handleEvent(*transactionError);
+        });
+    }
 
     clearCallbackWrappers();
 
@@ -442,8 +465,11 @@ void SQLTransaction::deliverSuccessCallback()
 {
     // Spec 4.3.2.8: Deliver success callback.
     RefPtr<VoidCallback> successCallback = m_successCallbackWrapper.unwrap();
-    if (successCallback)
-        successCallback->handleEvent();
+    if (successCallback) {
+        m_database->document().eventLoop().queueTask(TaskSource::Networking, m_database->document(), [successCallback = WTFMove(successCallback)]() mutable {
+            successCallback->handleEvent();
+        });
+    }
 
     clearCallbackWrappers();
 
@@ -475,7 +501,8 @@ void SQLTransaction::computeNextStateAndCleanupIfNeeded()
 
         LOG(StorageAPI, "Callback %s\n", nameForSQLTransactionState(m_nextState));
         return;
-    }
+    } else
+        callErrorCallbackDueToInterruption();
 
     clearCallbackWrappers();
     m_backend.requestTransitToState(SQLTransactionState::CleanupAndTerminate);
index e2c387a..540f6e3 100644 (file)
@@ -105,6 +105,8 @@ private:
 
     NO_RETURN_DUE_TO_ASSERT void unreachableState();
 
+    void callErrorCallbackDueToInterruption();
+
     void getNextStatement();
     bool runCurrentStatement();
     void handleCurrentStatementError();