From c6fab541b773059b6860dd8dec8efbd9d0f0f7bb Mon Sep 17 00:00:00 2001
From: Shannon Booth <shannon@serenityos.org>
Date: Fri, 26 Dec 2025 17:36:55 +0100
Subject: [PATCH] LibWeb: Fix storage set broadcast event never broadcasting
 old value

We had skipped some steps in the spec and were:
 * Always broadcasting an old value of null, instead of what it
   actually was previously.
 * Still broadcasting a storage event even if the value had
   not changed in storage compared to the last value.

Fix both issues by returning what the old value is in the setter and
implementing the missing logic.
---
 Libraries/LibWeb/HTML/Storage.cpp             | 26 ++++++++++---
 Libraries/LibWeb/Page/Page.h                  |  4 +-
 Libraries/LibWeb/StorageAPI/StorageBottle.cpp |  8 ++--
 Libraries/LibWeb/StorageAPI/StorageBottle.h   |  8 ++--
 Libraries/LibWebView/StorageJar.cpp           | 14 ++++---
 Libraries/LibWebView/StorageJar.h             |  8 ++--
 Libraries/LibWebView/StorageOperationError.h  | 16 --------
 Libraries/LibWebView/StorageSetResult.h       | 23 +++++++++++
 Services/WebContent/PageClient.cpp            |  4 +-
 Services/WebContent/PageClient.h              |  4 +-
 Services/WebContent/WebContentClient.ipc      |  4 +-
 .../webstorage/event_session_oldvalue.txt     |  6 +++
 .../webstorage/event_session_oldvalue.html    | 38 +++++++++++++++++++
 .../resources/session_change_item_iframe.html | 18 +++++++++
 14 files changed, 136 insertions(+), 45 deletions(-)
 delete mode 100644 Libraries/LibWebView/StorageOperationError.h
 create mode 100644 Libraries/LibWebView/StorageSetResult.h
 create mode 100644 Tests/LibWeb/Text/expected/wpt-import/webstorage/event_session_oldvalue.txt
 create mode 100644 Tests/LibWeb/Text/input/wpt-import/webstorage/event_session_oldvalue.html
 create mode 100644 Tests/LibWeb/Text/input/wpt-import/webstorage/resources/session_change_item_iframe.html

diff --git a/Libraries/LibWeb/HTML/Storage.cpp b/Libraries/LibWeb/HTML/Storage.cpp
index 5cfa9b3c3b..3d39af7fc7 100644
--- a/Libraries/LibWeb/HTML/Storage.cpp
+++ b/Libraries/LibWeb/HTML/Storage.cpp
@@ -103,18 +103,34 @@ Optional<String> Storage::get_item(String const& key) const
 WebIDL::ExceptionOr<void> Storage::set_item(String const& key, String const& value)
 {
     // 1. Let oldValue be null.
-    Optional<String> old_value;
-
     // 2. Let reorder be true.
-    // 3. If this's map[key] exists:
+    bool reorder = true;
 
+    // 3. If this's map[key] exists:
+    //     1. Set oldValue to this's map[key].
+    //     2. If oldValue is value, then return.
+    //     3. Set reorder to false.
     // 4. If value cannot be stored, then throw a "QuotaExceededError" DOMException.
     // 5. Set this's map[key] to value.
-    auto error = m_storage_bottle->set(key, value);
-    if (error == WebView::StorageOperationError::QuotaExceededError) {
+
+    auto result = m_storage_bottle->set(key, value);
+
+    if (result.has<WebView::StorageOperationError>())
         return WebIDL::QuotaExceededError::create(realm(), Utf16String::formatted("Unable to store more than {} bytes in storage", *m_storage_bottle->quota()));
+
+    auto old_value = result.get<Optional<String>>();
+
+    if (old_value.has_value()) {
+        if (old_value.value() == value)
+            return {};
+
+        reorder = false;
     }
 
+    // 6. If reorder is true, then reorder this.
+    if (reorder)
+        this->reorder();
+
     // 7. Broadcast this with key, oldValue, and value.
     broadcast(key, old_value, value);
 
diff --git a/Libraries/LibWeb/Page/Page.h b/Libraries/LibWeb/Page/Page.h
index e1dd9cca65..675de3f198 100644
--- a/Libraries/LibWeb/Page/Page.h
+++ b/Libraries/LibWeb/Page/Page.h
@@ -45,7 +45,7 @@
 #include <LibWeb/PixelUnits.h>
 #include <LibWeb/StorageAPI/StorageEndpoint.h>
 #include <LibWeb/UIEvents/KeyCode.h>
-#include <LibWebView/StorageOperationError.h>
+#include <LibWebView/StorageSetResult.h>
 
 namespace Web {
 
@@ -385,7 +385,7 @@ public:
     virtual void page_did_update_cookie(Web::Cookie::Cookie const&) { }
     virtual void page_did_expire_cookies_with_time_offset(AK::Duration) { }
     virtual Optional<String> page_did_request_storage_item([[maybe_unused]] Web::StorageAPI::StorageEndpointType storage_endpoint, [[maybe_unused]] String const& storage_key, [[maybe_unused]] String const& bottle_key) { return {}; }
-    virtual WebView::StorageOperationError page_did_set_storage_item([[maybe_unused]] Web::StorageAPI::StorageEndpointType storage_endpoint, [[maybe_unused]] String const& storage_key, [[maybe_unused]] String const& bottle_key, [[maybe_unused]] String const& value) { return WebView::StorageOperationError::None; }
+    virtual WebView::StorageSetResult page_did_set_storage_item([[maybe_unused]] Web::StorageAPI::StorageEndpointType storage_endpoint, [[maybe_unused]] String const& storage_key, [[maybe_unused]] String const& bottle_key, [[maybe_unused]] String const& value) { return WebView::StorageOperationError::QuotaExceededError; }
     virtual void page_did_remove_storage_item([[maybe_unused]] Web::StorageAPI::StorageEndpointType storage_endpoint, [[maybe_unused]] String const& storage_key, [[maybe_unused]] String const& bottle_key) { }
     virtual Vector<String> page_did_request_storage_keys([[maybe_unused]] Web::StorageAPI::StorageEndpointType storage_endpoint, [[maybe_unused]] String const& storage_key) { return {}; }
     virtual void page_did_clear_storage([[maybe_unused]] Web::StorageAPI::StorageEndpointType storage_endpoint, [[maybe_unused]] String const& storage_key) { }
diff --git a/Libraries/LibWeb/StorageAPI/StorageBottle.cpp b/Libraries/LibWeb/StorageAPI/StorageBottle.cpp
index 34aaa6295c..857e41d1e4 100644
--- a/Libraries/LibWeb/StorageAPI/StorageBottle.cpp
+++ b/Libraries/LibWeb/StorageAPI/StorageBottle.cpp
@@ -119,7 +119,7 @@ Optional<String> LocalStorageBottle::get(String const& key) const
     return m_page->client().page_did_request_storage_item(Web::StorageAPI::StorageEndpointType::LocalStorage, m_storage_key.to_string(), key);
 }
 
-WebView::StorageOperationError LocalStorageBottle::set(String const& key, String const& value)
+WebView::StorageSetResult LocalStorageBottle::set(String const& key, String const& value)
 {
     return m_page->client().page_did_set_storage_item(Web::StorageAPI::StorageEndpointType::LocalStorage, m_storage_key.to_string(), key, value);
 }
@@ -151,8 +151,10 @@ Optional<String> SessionStorageBottle::get(String const& key) const
     return OptionalNone {};
 }
 
-WebView::StorageOperationError SessionStorageBottle::set(String const& key, String const& value)
+WebView::StorageSetResult SessionStorageBottle::set(String const& key, String const& value)
 {
+    auto old_value = get(key);
+
     if (m_quota.has_value()) {
         size_t current_size = 0;
         for (auto const& [existing_key, existing_value] : m_map) {
@@ -167,7 +169,7 @@ WebView::StorageOperationError SessionStorageBottle::set(String const& key, Stri
     }
 
     m_map.set(key, value);
-    return WebView::StorageOperationError::None;
+    return old_value;
 }
 
 void SessionStorageBottle::clear()
diff --git a/Libraries/LibWeb/StorageAPI/StorageBottle.h b/Libraries/LibWeb/StorageAPI/StorageBottle.h
index a0aebb4123..9451cf1f14 100644
--- a/Libraries/LibWeb/StorageAPI/StorageBottle.h
+++ b/Libraries/LibWeb/StorageAPI/StorageBottle.h
@@ -15,7 +15,7 @@
 #include <LibWeb/StorageAPI/StorageEndpoint.h>
 #include <LibWeb/StorageAPI/StorageKey.h>
 #include <LibWeb/StorageAPI/StorageType.h>
-#include <LibWebView/StorageOperationError.h>
+#include <LibWebView/StorageSetResult.h>
 
 namespace Web::StorageAPI {
 
@@ -34,7 +34,7 @@ public:
     virtual size_t size() const = 0;
     virtual Vector<String> keys() const = 0;
     virtual Optional<String> get(String const&) const = 0;
-    virtual WebView::StorageOperationError set(String const& key, String const& value) = 0;
+    virtual WebView::StorageSetResult set(String const& key, String const& value) = 0;
     virtual void clear() = 0;
     virtual void remove(String const&) = 0;
 
@@ -62,7 +62,7 @@ public:
     virtual size_t size() const override;
     virtual Vector<String> keys() const override;
     virtual Optional<String> get(String const&) const override;
-    virtual WebView::StorageOperationError set(String const& key, String const& value) override;
+    virtual WebView::StorageSetResult set(String const& key, String const& value) override;
     virtual void clear() override;
     virtual void remove(String const&) override;
 
@@ -93,7 +93,7 @@ public:
     virtual size_t size() const override;
     virtual Vector<String> keys() const override;
     virtual Optional<String> get(String const&) const override;
-    virtual WebView::StorageOperationError set(String const& key, String const& value) override;
+    virtual WebView::StorageSetResult set(String const& key, String const& value) override;
     virtual void clear() override;
     virtual void remove(String const&) override;
 
diff --git a/Libraries/LibWebView/StorageJar.cpp b/Libraries/LibWebView/StorageJar.cpp
index a8f8bb18ec..5a9b60679f 100644
--- a/Libraries/LibWebView/StorageJar.cpp
+++ b/Libraries/LibWebView/StorageJar.cpp
@@ -107,7 +107,7 @@ Optional<String> StorageJar::get_item(StorageEndpointType storage_endpoint, Stri
     return m_transient_storage.get_item(storage_location);
 }
 
-StorageOperationError StorageJar::set_item(StorageEndpointType storage_endpoint, String const& storage_key, String const& bottle_key, String const& bottle_value)
+StorageSetResult StorageJar::set_item(StorageEndpointType storage_endpoint, String const& storage_key, String const& bottle_key, String const& bottle_value)
 {
     StorageLocation storage_location { storage_endpoint, storage_key, bottle_key };
 
@@ -166,8 +166,10 @@ Optional<String> StorageJar::TransientStorage::get_item(StorageLocation const& k
     return {};
 }
 
-StorageOperationError StorageJar::TransientStorage::set_item(StorageLocation const& key, String const& value)
+StorageSetResult StorageJar::TransientStorage::set_item(StorageLocation const& key, String const& value)
 {
+    auto old_value = get_item(key);
+
     u64 current_size = 0;
 
     for (auto const& [existing_key, existing_entry] : m_storage_items) {
@@ -182,7 +184,7 @@ StorageOperationError StorageJar::TransientStorage::set_item(StorageLocation con
         return StorageOperationError::QuotaExceededError;
 
     m_storage_items.set(key, { value, UnixDateTime::now() });
-    return StorageOperationError::None;
+    return old_value;
 }
 
 void StorageJar::TransientStorage::delete_item(StorageLocation const& key)
@@ -262,8 +264,10 @@ Optional<String> StorageJar::PersistedStorage::get_item(StorageLocation const& k
     return result;
 }
 
-StorageOperationError StorageJar::PersistedStorage::set_item(StorageLocation const& key, String const& value)
+StorageSetResult StorageJar::PersistedStorage::set_item(StorageLocation const& key, String const& value)
 {
+    auto old_value = get_item(key);
+
     size_t current_size = 0;
     database.execute_statement(
         statements.calculate_size_excluding_key,
@@ -287,7 +291,7 @@ StorageOperationError StorageJar::PersistedStorage::set_item(StorageLocation con
         value,
         UnixDateTime::now());
 
-    return StorageOperationError::None;
+    return old_value;
 }
 
 void StorageJar::PersistedStorage::delete_item(StorageLocation const& key)
diff --git a/Libraries/LibWebView/StorageJar.h b/Libraries/LibWebView/StorageJar.h
index 60f9dac567..fdca8893ce 100644
--- a/Libraries/LibWebView/StorageJar.h
+++ b/Libraries/LibWebView/StorageJar.h
@@ -14,7 +14,7 @@
 #include <LibRequests/CacheSizes.h>
 #include <LibWeb/StorageAPI/StorageEndpoint.h>
 #include <LibWebView/Forward.h>
-#include <LibWebView/StorageOperationError.h>
+#include <LibWebView/StorageSetResult.h>
 
 namespace WebView {
 
@@ -39,7 +39,7 @@ public:
     ~StorageJar();
 
     Optional<String> get_item(StorageEndpointType storage_endpoint, String const& storage_key, String const& bottle_key);
-    StorageOperationError set_item(StorageEndpointType storage_endpoint, String const& storage_key, String const& bottle_key, String const& bottle_value);
+    StorageSetResult set_item(StorageEndpointType storage_endpoint, String const& storage_key, String const& bottle_key, String const& bottle_value);
     void remove_item(StorageEndpointType storage_endpoint, String const& storage_key, String const& key);
     void remove_items_accessed_since(UnixDateTime);
     void clear_storage_key(StorageEndpointType storage_endpoint, String const& storage_key);
@@ -62,7 +62,7 @@ private:
     class TransientStorage {
     public:
         Optional<String> get_item(StorageLocation const& key);
-        StorageOperationError set_item(StorageLocation const& key, String const& value);
+        StorageSetResult set_item(StorageLocation const& key, String const& value);
         void delete_item(StorageLocation const& key);
         void delete_items_accessed_since(UnixDateTime);
         void clear(StorageEndpointType storage_endpoint, String const& storage_key);
@@ -80,7 +80,7 @@ private:
 
     struct PersistedStorage {
         Optional<String> get_item(StorageLocation const& key);
-        StorageOperationError set_item(StorageLocation const& key, String const& value);
+        StorageSetResult set_item(StorageLocation const& key, String const& value);
         void delete_item(StorageLocation const& key);
         void delete_items_accessed_since(UnixDateTime);
         void clear(StorageEndpointType storage_endpoint, String const& storage_key);
diff --git a/Libraries/LibWebView/StorageOperationError.h b/Libraries/LibWebView/StorageOperationError.h
deleted file mode 100644
index ca77b40df0..0000000000
--- a/Libraries/LibWebView/StorageOperationError.h
+++ /dev/null
@@ -1,16 +0,0 @@
-/*
- * Copyright (c) 2025, Aliaksandr Kalenik <kalenik.aliaksandr@gmail.com>
- *
- * SPDX-License-Identifier: BSD-2-Clause
- */
-
-#pragma once
-
-namespace WebView {
-
-enum class StorageOperationError : u8 {
-    None,
-    QuotaExceededError,
-};
-
-}
diff --git a/Libraries/LibWebView/StorageSetResult.h b/Libraries/LibWebView/StorageSetResult.h
new file mode 100644
index 0000000000..3b7464c31b
--- /dev/null
+++ b/Libraries/LibWebView/StorageSetResult.h
@@ -0,0 +1,23 @@
+/*
+ * Copyright (c) 2025, Aliaksandr Kalenik <kalenik.aliaksandr@gmail.com>
+ * Copyright (c) 2025, Shannon Booth <shannon@serenityos.org>
+ *
+ * SPDX-License-Identifier: BSD-2-Clause
+ */
+
+#pragma once
+
+#include <AK/Optional.h>
+#include <AK/String.h>
+#include <AK/Types.h>
+
+namespace WebView {
+
+enum class StorageOperationError : u8 {
+    QuotaExceededError,
+};
+
+// Error setting the storage item, or the old value if the operation was successful.
+using StorageSetResult = Variant<StorageOperationError, Optional<String>>;
+
+}
diff --git a/Services/WebContent/PageClient.cpp b/Services/WebContent/PageClient.cpp
index 8ed486162e..1c74e087bf 100644
--- a/Services/WebContent/PageClient.cpp
+++ b/Services/WebContent/PageClient.cpp
@@ -542,14 +542,14 @@ Optional<String> PageClient::page_did_request_storage_item(Web::StorageAPI::Stor
     return response->take_value();
 }
 
-WebView::StorageOperationError PageClient::page_did_set_storage_item(Web::StorageAPI::StorageEndpointType storage_endpoint, String const& storage_key, String const& bottle_key, String const& value)
+WebView::StorageSetResult PageClient::page_did_set_storage_item(Web::StorageAPI::StorageEndpointType storage_endpoint, String const& storage_key, String const& bottle_key, String const& value)
 {
     auto response = client().send_sync_but_allow_failure<Messages::WebContentClient::DidSetStorageItem>(storage_endpoint, storage_key, bottle_key, value);
     if (!response) {
         dbgln("WebContent client disconnected during DidSetStorageItem. Exiting peacefully.");
         exit(0);
     }
-    return response->error();
+    return response->result();
 }
 
 void PageClient::page_did_remove_storage_item(Web::StorageAPI::StorageEndpointType storage_endpoint, String const& storage_key, String const& bottle_key)
diff --git a/Services/WebContent/PageClient.h b/Services/WebContent/PageClient.h
index fe20f0c4ee..4a9d632962 100644
--- a/Services/WebContent/PageClient.h
+++ b/Services/WebContent/PageClient.h
@@ -17,7 +17,7 @@
 #include <LibWeb/PixelUnits.h>
 #include <LibWeb/StorageAPI/StorageEndpoint.h>
 #include <LibWebView/Forward.h>
-#include <LibWebView/StorageOperationError.h>
+#include <LibWebView/StorageSetResult.h>
 #include <WebContent/Forward.h>
 
 namespace WebContent {
@@ -160,7 +160,7 @@ private:
     virtual void page_did_update_cookie(Web::Cookie::Cookie const&) override;
     virtual void page_did_expire_cookies_with_time_offset(AK::Duration) override;
     virtual Optional<String> page_did_request_storage_item(Web::StorageAPI::StorageEndpointType storage_endpoint, String const& storage_key, String const& bottle_key) override;
-    virtual WebView::StorageOperationError page_did_set_storage_item(Web::StorageAPI::StorageEndpointType storage_endpoint, String const& storage_key, String const& bottle_key, String const& value) override;
+    virtual WebView::StorageSetResult page_did_set_storage_item(Web::StorageAPI::StorageEndpointType storage_endpoint, String const& storage_key, String const& bottle_key, String const& value) override;
     virtual void page_did_remove_storage_item(Web::StorageAPI::StorageEndpointType storage_endpoint, String const& storage_key, String const& bottle_key) override;
     virtual Vector<String> page_did_request_storage_keys(Web::StorageAPI::StorageEndpointType storage_endpoint, String const& storage_key) override;
     virtual void page_did_clear_storage(Web::StorageAPI::StorageEndpointType storage_endpoint, String const& storage_key) override;
diff --git a/Services/WebContent/WebContentClient.ipc b/Services/WebContent/WebContentClient.ipc
index cebee3b8ae..65d73f28bf 100644
--- a/Services/WebContent/WebContentClient.ipc
+++ b/Services/WebContent/WebContentClient.ipc
@@ -24,7 +24,7 @@
 #include <LibWebView/ConsoleOutput.h>
 #include <LibWebView/DOMNodeProperties.h>
 #include <LibWeb/StorageAPI/StorageEndpoint.h>
-#include <LibWebView/StorageOperationError.h>
+#include <LibWebView/StorageSetResult.h>
 #include <LibWebView/Mutation.h>
 #include <LibWebView/PageInfo.h>
 #include <LibWebView/ProcessHandle.h>
@@ -83,7 +83,7 @@ endpoint WebContentClient
     did_update_cookie(Web::Cookie::Cookie cookie) =|
     did_expire_cookies_with_time_offset(AK::Duration offset) =|
     did_request_storage_item(Web::StorageAPI::StorageEndpointType storage_endpoint, String storage_key, String bottle_key) => (Optional<String> value)
-    did_set_storage_item(Web::StorageAPI::StorageEndpointType storage_endpoint, String storage_key, String bottle_key, String value) => (WebView::StorageOperationError error)
+    did_set_storage_item(Web::StorageAPI::StorageEndpointType storage_endpoint, String storage_key, String bottle_key, String value) => (WebView::StorageSetResult result)
     did_remove_storage_item(Web::StorageAPI::StorageEndpointType storage_endpoint, String storage_key, String bottle_key) => ()
     did_request_storage_keys(Web::StorageAPI::StorageEndpointType storage_endpoint, String storage_key) => (Vector<String> keys)
     did_clear_storage(Web::StorageAPI::StorageEndpointType storage_endpoint, String storage_key) => ()
diff --git a/Tests/LibWeb/Text/expected/wpt-import/webstorage/event_session_oldvalue.txt b/Tests/LibWeb/Text/expected/wpt-import/webstorage/event_session_oldvalue.txt
new file mode 100644
index 0000000000..4e4a844d10
--- /dev/null
+++ b/Tests/LibWeb/Text/expected/wpt-import/webstorage/event_session_oldvalue.txt
@@ -0,0 +1,6 @@
+Harness status: OK
+
+Found 1 tests
+
+1 Pass
+Pass	oldvalue property test of session event - Session event is fired due to an invocation of the setItem(), clear() methods.
\ No newline at end of file
diff --git a/Tests/LibWeb/Text/input/wpt-import/webstorage/event_session_oldvalue.html b/Tests/LibWeb/Text/input/wpt-import/webstorage/event_session_oldvalue.html
new file mode 100644
index 0000000000..c5c204af96
--- /dev/null
+++ b/Tests/LibWeb/Text/input/wpt-import/webstorage/event_session_oldvalue.html
@@ -0,0 +1,38 @@
+<!DOCTYPE HTML>
+<html>
+ <head>
+  <title>WebStorage Test: sessionStorage event - oldValue</title>
+  <script src="../resources/testharness.js"></script>
+  <script src="../resources/testharnessreport.js"></script>
+ </head>
+ <body>
+    <h1>event_session_oldValue</h1>
+    <div id="log"></div>
+    <script>
+        async_test(function(t) {
+            sessionStorage.clear();
+            t.add_cleanup(function() { sessionStorage.clear() });
+
+            self.fail = t.step_func(function(msg) {
+                assert_unreached(msg);
+                t.done();
+            });
+
+            var expected = [null, 'user1', null]
+            function onStorageEvent(event) {
+                assert_equals(event.oldValue, expected.shift());
+                if (!expected.length) {
+                    t.done();
+                }
+            }
+
+            window.addEventListener('storage', t.step_func(onStorageEvent), false);
+
+            var el = document.createElement("iframe");
+            el.setAttribute('id', 'ifrm');
+            el.setAttribute('src', 'resources/session_change_item_iframe.html');
+            document.body.appendChild(el);
+        }, "oldvalue property test of session event - Session event is fired due to an invocation of the setItem(), clear() methods.");
+    </script>
+ </body>
+</html>
diff --git a/Tests/LibWeb/Text/input/wpt-import/webstorage/resources/session_change_item_iframe.html b/Tests/LibWeb/Text/input/wpt-import/webstorage/resources/session_change_item_iframe.html
new file mode 100644
index 0000000000..1e1867e51e
--- /dev/null
+++ b/Tests/LibWeb/Text/input/wpt-import/webstorage/resources/session_change_item_iframe.html
@@ -0,0 +1,18 @@
+<!DOCTYPE HTML>
+<html>
+ <body>
+    <script>
+        if (('sessionStorage' in window) && window.sessionStorage !== null){
+            try {
+                sessionStorage.setItem("name", "user1");
+                sessionStorage.setItem("name", "user2");
+            } catch (e) {
+                parent.fail("setItem method is failed.");
+            }
+            sessionStorage.clear();
+        } else {
+            parent.fail("sessionStorage is not supported.");
+        }
+    </script>
+ </body>
+</html>