script: initial CookieStore implementation #37968

sebsebmc · 2025-07-09T16:41:06Z

This is a first draft at implementing the required infrastructure for CookieStore, which requires setting up IPC between script and the resource thread to allow for async/"in parallel" handling of cookie changes that have a promise API.

Cookie Store also will need to receive change events when cookies for a url are changed so the architecture needs to support that.

Expect this PR to be reworked once the architecture becomes more settled, cookie change events will be implemented in follow up PRs

Testing: WPT tests exist for this API
Part of #37674

components/shared/net/lib.rs

sebsebmc · 2025-07-09T16:45:40Z

components/shared/net/lib.rs

    DeleteCookies(ServoUrl),
    DeleteCookie(ServoUrl, String),
+    DeleteCookieAsync(CookieStoreId, CookieRequestId, ServoUrl, String),
+    NewCookieListener(CookieStoreId, IpcSender<CookieAsyncResponse>, ServoUrl),


For lifecycle management I believe I will also need a RemoveCookieListener message for when a global navigates to a new page. The spec makes clear that CookieStore's relevant URL is the global's creation_url.

yes that sounds like a good idea.

Are you currently sending that message? I think it could be done on drop of the global.

EDIT: drop of the cookiestore.

Yeah, I noticed that I forgot to wire this up yesterday. I'll look at adding this to the drop

gterzian

Looks great so far, thank you.

How do you want to scope this PR?

components/script/dom/cookiestore.rs

gterzian · 2025-07-10T09:31:25Z

components/script/dom/cookiestore.rs

+        // 1. Perform the steps defined in Cookies § Retrieval Model to compute the "cookie-string from a given cookie
+        // store" with url as request-uri. The cookie-string itself is ignored, but the intermediate cookie-list is
+        // used in subsequent steps.
+        // For the purposes of the steps, the cookie-string is being generated for a "non-HTTP" API.


This also belongs in resource threads, and instead I would just document the method with a note on sending a message to run the "in-parallel" steps.

components/script/dom/cookiestore.rs

components/script/dom/window.rs

gterzian · 2025-07-10T09:36:43Z

components/script/dom/window.rs

            .or_init(|| Storage::new(self, StorageType::Local, CanGc::note()))
    }

+    // https://wicg.github.io/cookie-store/


The correct link is https://wicg.github.io/cookie-store/#dom-window-cookiestore(and format is wrong also).

components/shared/net/lib.rs

gterzian · 2025-07-10T09:40:56Z

components/shared/net/lib.rs

    DeleteCookies(ServoUrl),
    DeleteCookie(ServoUrl, String),
+    DeleteCookieAsync(CookieStoreId, CookieRequestId, ServoUrl, String),
+    NewCookieListener(CookieStoreId, IpcSender<CookieAsyncResponse>, ServoUrl),


yes that sounds like a good idea.

components/shared/net/lib.rs

sebsebmc · 2025-07-10T14:31:22Z

How do you want to scope this PR?

I think the goal for this PR is going to be setting up the infrastructure needed to be able to do the most minimal Set-Get-Delete implementation. The change event listener is definitely going to be in a follow up

gterzian · 2025-07-14T19:12:10Z

I think the goal for this PR is

Sounds good.

sebsebmc · 2025-08-16T18:56:28Z

@gterzian @jdm I don't want to make this PR too much larger, I think this is ready for the next round of review.

gterzian

Some initial commend; I'll take another look later today.

gterzian · 2025-08-17T10:34:29Z

components/script/dom/window.rs

    }

+    // https://cookiestore.spec.whatwg.org/#Window
+    fn CookieStore(&self) -> DomRoot<CookieStore> {


Here you can pass a can_gc argument using Bindings.conf.

gterzian · 2025-08-17T10:39:34Z

components/script/dom/cookiestore.rs

+                self.store_id,
+                global.creation_url().clone(),
+                options.name.to_string(),
+            ));


Here and elsewhere you could log an error if the send fails. In theory it never should.

gterzian · 2025-08-17T10:40:57Z

components/script/dom/cookiestore.rs

+                creation_url.clone(),
+                Serde(cookie.build()),
+                NonHTTP,
+            ));


If it fails you probably want to. remove the inflight promise as well, and reject it with an error.

Ah, yeah probably just going to make a small helper for this then. I wish there was a nice way to make this a type/lifetime error somehow.

gterzian · 2025-08-17T10:42:53Z

components/script/dom/cookiestore.rs

+            p.reject_error(Error::Security, can_gc);
+            return p;
+        }
+        // 4. Let url be settings’s creation URL.


nit: I always add a line between the last line of code and the next comment.

gterzian · 2025-08-17T10:46:25Z

components/script/dom/cookiestore.rs

+fn cookie_to_list_item(cookie: Cookie) -> CookieListItem {
+    CookieListItem {
+        // Let domain be the result of running UTF-8 decode without BOM on cookie’s domain.
+        domain: cookie


We should add a TODO to actually call into UTF-8 decode without BOM

gterzian · 2025-08-17T10:47:09Z

components/net/resource_thread.rs

+            );
+            return;
+        }
+        let _ = sender.unwrap().send(CookieAsyncResponse { data });


It's nicer to do a Let Some() = else

gterzian

Looks good overall, thank you. I might spend some time looking at the spec again and into some things mentioned below, in the meantime let's address the current comments.

gterzian · 2025-08-17T13:12:40Z

components/script/dom/cookiestore.rs

+                        .collect_vec(),
+                    CanGc::note());
+                },
+                _ => {promise.resolve_native(&(), CanGc::note());}


I think it's better yo be explicit here.

gterzian · 2025-08-17T13:13:38Z

components/script/dom/cookiestore.rs

+}
+
+struct CookieListener {
+    task_source: SendableTaskSource,


Let's document which task source this is, and add a link to the spec is such an appropriate link exists

gterzian · 2025-08-17T13:19:42Z

components/shared/net/lib.rs

    DeleteCookies(ServoUrl),
    DeleteCookie(ServoUrl, String),
+    DeleteCookieAsync(CookieStoreId, CookieRequestId, ServoUrl, String),
+    NewCookieListener(CookieStoreId, IpcSender<CookieAsyncResponse>, ServoUrl),


Are you currently sending that message? I think it could be done on drop of the global.

EDIT: drop of the cookiestore.

gterzian · 2025-08-17T13:30:47Z

components/script/dom/cookiestore.rs

+            .send(CoreResourceMsg::GetCookieDataForUrlAsync(
+                self.store_id,
+                creation_url.clone(),
+                Some(name.to_string()),


Note: I'm not quite sure how to make this work with the decoding that happens later in parallel. Should we do the decoding before turning it into a string, which means decoding it here, or can we do it later on the string in parallel. But rust strings are already utf8 I believe. Needs some looking into. Any relevant tests failing at this point?

I believe the only encoding/decoding tests that exist are checking if we strip the BOM from the name and we currently pass the test. (We don't strip it)

There's some UTF8 encoding checks in the tests for the cookie change events, which we haven't implemented at all.

Ok, I guess we can then leave it as it, and revisit when the others parts are implemented.

gterzian

LGTM with one nit; thank you.

gterzian · 2025-08-20T14:09:57Z

components/script/dom/cookiestore.rs

+                Some(name.to_string()),
+            ));
+        if res.is_err() {
+            self.in_flight.borrow_mut().pop_back();


I would it's simpler to only push it back if the send is ok.

Ah, I do like that a bit more, since now the pattern is consistent about pushing back if we send a message. Now anytime we send a message we always have a pattern to check to make sure we also stored a promise.

Signed-off-by: Sebastian C <sebsebmc@gmail.com>

sebsebmc · 2025-08-20T23:39:37Z

Not sure what happened there, is that just a flake on the MacOS runner?

jdm · 2025-08-21T01:00:14Z

  cargo:warning=fatal error: /Applications/Xcode_16.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: can't write to output file (Input/output error)
  cargo:warning=/Applications/Xcode_16.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ar: internal ranlib command failed

Wild. Never seen that before! Let's try again.

sebsebmc commented Jul 9, 2025

View reviewed changes

gterzian reviewed Jul 10, 2025

View reviewed changes

sebsebmc force-pushed the cookie-store-api branch from 3da8cfe to 5bb0b8b Compare July 18, 2025 04:00

sebsebmc force-pushed the cookie-store-api branch from 5bb0b8b to 29d7c76 Compare July 30, 2025 23:27

sebsebmc changed the title ~~wip: cookiestore: setup basics~~ script: initial CookieStore implementation Aug 1, 2025

sebsebmc force-pushed the cookie-store-api branch from 29d7c76 to 803abad Compare August 16, 2025 18:45

gterzian reviewed Aug 17, 2025

View reviewed changes

gterzian approved these changes Aug 20, 2025

View reviewed changes

sebsebmc added 11 commits August 20, 2025 12:40

wip: cookiestore: setup basics

eff277b

Signed-off-by: Sebastian C <sebsebmc@gmail.com>

minor cleanup and some arch improvements from comments

4a1aa7b

Signed-off-by: Sebastian C <sebsebmc@gmail.com>

Move query_cookies to cookie_storage

f09713e

Signed-off-by: Sebastian C <sebsebmc@gmail.com>

Put CookieStore behind preference

cc7557a

Signed-off-by: Sebastian C <sebsebmc@gmail.com>

Update spec links

bf05684

Signed-off-by: Sebastian C <sebsebmc@gmail.com>

Comment out unimplemented change event

09f0392

Signed-off-by: Sebastian C <sebsebmc@gmail.com>

Fill out set(options) and delete(options) basics

a26c6c2

Signed-off-by: Sebastian C <sebsebmc@gmail.com>

update expectations

78b34c7

Signed-off-by: Sebastian C <sebsebmc@gmail.com>

Address review comments

20516e2

Signed-off-by: Sebastian C <sebsebmc@gmail.com>

Refactor in-flight promise storage logic

d078457

Signed-off-by: Sebastian C <sebsebmc@gmail.com>

sebsebmc force-pushed the cookie-store-api branch from 8808ff4 to b28d6a5 Compare August 20, 2025 18:35

sebsebmc marked this pull request as ready for review August 20, 2025 18:38

gterzian added this pull request to the merge queue Aug 20, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 20, 2025

jdm added this pull request to the merge queue Aug 21, 2025

Merged via the queue into servo:main with commit b869b7e Aug 21, 2025
23 checks passed

Uh oh!

script: initial CookieStore implementation #37968

script: initial CookieStore implementation #37968

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!