From 14091f32c685ada674e9b2b7247607bc1e9022d9 Mon Sep 17 00:00:00 2001
From: Jelle Raaijmakers <jelle@gmta.nl>
Date: Mon, 7 Aug 2023 23:33:01 +0200
Subject: [PATCH] Browser: Synchronize database statements used for cookies

Since SQLServer is inherently asynchronous, database statements can run
in parallel. Our `CookieJar` did not require synchronous actions on the
database for its cookies, resulting in cookies not being set
immediately. This resulted in a bug that could be exposed by setting
`document.cookie` and immediately querying its value, revealing that the
cookie was not yet persisted.

Solve this by requiring all database statements to be executed
synchronously. Ideally SQLServer has a mechanism to determine interquery
dependencies and blocks until dependent queries are fully executed, but
until we have that, this works around that limitation.
---
 Userland/Applications/Browser/CookieJar.cpp | 24 ++++++++++-----------
 Userland/Applications/Browser/Database.h    | 16 ++++++++++++--
 2 files changed, 26 insertions(+), 14 deletions(-)

diff --git a/Userland/Applications/Browser/CookieJar.cpp b/Userland/Applications/Browser/CookieJar.cpp
index 9180495d94d..3644789f91a 100644
--- a/Userland/Applications/Browser/CookieJar.cpp
+++ b/Userland/Applications/Browser/CookieJar.cpp
@@ -2,6 +2,7 @@
  * Copyright (c) 2021-2023, Tim Flynn <trflynn89@serenityos.org>
  * Copyright (c) 2022, the SerenityOS developers.
  * Copyright (c) 2022, Tobias Christiansen <tobyase@serenityos.org>
+ * Copyright (c) 2023, Jelle Raaijmakers <jelle@gmta.nl>
  *
  * SPDX-License-Identifier: BSD-2-Clause
  */
@@ -349,11 +350,14 @@ void CookieJar::store_cookie(Web::Cookie::ParsedCookie const& parsed_cookie, con
     if (source != Web::Cookie::Source::Http && cookie.http_only)
         return;
 
+    // Synchronize persisting the cookie
+    auto sync_promise = Core::Promise<Empty>::construct();
+
     select_cookie_from_database(
         move(cookie),
 
         // 11. If the cookie store contains a cookie with the same name, domain, and path as the newly created cookie:
-        [this, source](auto& cookie, auto old_cookie) {
+        [this, source, sync_promise](auto& cookie, auto old_cookie) {
             // If the newly created cookie was received from a "non-HTTP" API and the old-cookie's http-only-flag is set, abort these
             // steps and ignore the newly created cookie entirely.
             if (source != Web::Cookie::Source::Http && old_cookie.http_only)
@@ -365,12 +369,16 @@ void CookieJar::store_cookie(Web::Cookie::ParsedCookie const& parsed_cookie, con
             // Remove the old-cookie from the cookie store.
             // NOTE: Rather than deleting then re-inserting this cookie, we update it in-place.
             update_cookie_in_database(cookie);
+            sync_promise->resolve({});
         },
 
         // 12. Insert the newly created cookie into the cookie store.
-        [this](auto cookie) {
+        [this, sync_promise](auto cookie) {
             insert_cookie_into_database(cookie);
+            sync_promise->resolve({});
         });
+
+    MUST(sync_promise->await());
 }
 
 Vector<Web::Cookie::Cookie> CookieJar::get_matching_cookies(const URL& url, DeprecatedString const& canonicalized_domain, Web::Cookie::Source source, MatchingCookiesSpecMode mode)
@@ -600,8 +608,6 @@ void CookieJar::select_all_cookies_from_database(OnSelectAllCookiesResult on_res
     // FIXME: Make surrounding APIs asynchronous.
     m_storage.visit(
         [&](PersistedStorage& storage) {
-            auto promise = Core::Promise<Empty>::construct();
-
             storage.database.execute_statement(
                 storage.statements.select_all_cookies,
                 [on_result = move(on_result)](auto row) {
@@ -610,14 +616,8 @@ void CookieJar::select_all_cookies_from_database(OnSelectAllCookiesResult on_res
                     else
                         on_result(cookie.release_value());
                 },
-                [&]() {
-                    promise->resolve({});
-                },
-                [&](auto) {
-                    promise->resolve({});
-                });
-
-            MUST(promise->await());
+                {},
+                {});
         },
         [&](TransientStorage& storage) {
             for (auto const& cookie : storage)
diff --git a/Userland/Applications/Browser/Database.h b/Userland/Applications/Browser/Database.h
index 0ff6a3e216a..d7a7b49599c 100644
--- a/Userland/Applications/Browser/Database.h
+++ b/Userland/Applications/Browser/Database.h
@@ -1,5 +1,6 @@
 /*
  * Copyright (c) 2022, Tim Flynn <trflynn89@serenityos.org>
+ * Copyright (c) 2023, Jelle Raaijmakers <jelle@gmta.nl>
  *
  * SPDX-License-Identifier: BSD-2-Clause
  */
@@ -14,6 +15,7 @@
 #include <AK/RefCounted.h>
 #include <AK/StringView.h>
 #include <AK/Vector.h>
+#include <LibCore/Promise.h>
 #include <LibSQL/SQLClient.h>
 #include <LibSQL/Type.h>
 #include <LibSQL/Value.h>
@@ -34,14 +36,24 @@ public:
     template<typename... PlaceholderValues>
     void execute_statement(SQL::StatementID statement_id, OnResult on_result, OnComplete on_complete, OnError on_error, PlaceholderValues&&... placeholder_values)
     {
+        auto sync_promise = Core::Promise<Empty>::construct();
+
         PendingExecution pending_execution {
             .on_result = move(on_result),
-            .on_complete = move(on_complete),
-            .on_error = move(on_error),
+            .on_complete = [sync_promise, on_complete = move(on_complete)] {
+                if (on_complete)
+                    on_complete();
+                sync_promise->resolve({}); },
+            .on_error = [sync_promise, on_error = move(on_error)](auto message) {
+                if (on_error)
+                    on_error(message);
+                sync_promise->resolve({}); },
         };
 
         Vector<SQL::Value> values { SQL::Value(forward<PlaceholderValues>(placeholder_values))... };
         execute_statement(statement_id, move(values), move(pending_execution));
+
+        MUST(sync_promise->await());
     }
 
 private: