script: initial CookieStore implementation by sebsebmc · Pull Request #37968 · servo/servo

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

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

    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?

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

+        // 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.

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

            .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).

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

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


yes that sounds like a good idea.

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

    }

+    // 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

+                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

+                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

+            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

+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

+            );
+            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

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


I think it's better yo be explicit here.

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

+}
+
+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

    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

+            .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

+                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>

resolve new clippy lints

b28d6a5

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

Conversation

sebsebmc commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gterzian Aug 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gterzian left a comment

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!

sebsebmc commented Jul 10, 2025

Uh oh!

gterzian commented Jul 14, 2025

Uh oh!

sebsebmc commented Aug 16, 2025

Uh oh!

gterzian left a comment

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!

gterzian left a comment

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!

gterzian Aug 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

gterzian left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

sebsebmc commented Jul 9, 2025 •

edited

Loading

gterzian Aug 17, 2025 •

edited

Loading

gterzian Aug 17, 2025 •

edited

Loading

gterzian left a comment •

edited

Loading