script: Pass down `&mut JSContext` in servoparser and event loop. by sagudev · Pull Request #42635 · servo/servo

sagudev · 2026-02-15T13:24:43Z

I only wanted to get &mut JSContext in microtask chunk and checkpoint, but this in turn needed &mut JSContext in servoparser, which then caused need for even more changes in script.

I tried to limit the size by putting some temp_cx in:

drops of LoadBlocker, GenericAutoEntryScript
methods of VirtualMethods
methods of FetchResponseListener

Testing: Just refactor, but should be covered by WPT tests.
Part of #40600

TimvdLippe

Since these PRs are usually big, most of them are atomic changes and thus I am okay to approve. But this one also introduces several usages of temp_gc. I would prefer to take a slower pace with tackling such blockers, before we make wide-sweeping changes. What do you think?

components/script/dom/audio/mediaelementaudiosourcenode.rs

TimvdLippe · 2026-02-15T17:12:49Z

components/script/dom/html/htmliframeelement.rs

                if self.upcast::<Node>().is_connected_with_browsing_context() {
                    debug!("iframe srcdoc modified while in browsing context.");
-                    self.process_the_iframe_attributes(ProcessingMode::NotFirstTime, can_gc);
+                    let mut cx = unsafe { temp_cx() };


Same here, let's tackle this first and then update the other ones.

sagudev · 2026-02-16T06:32:53Z

Since these PRs are usually big, most of them are atomic changes and thus I am okay to approve. But this one also introduces several usages of temp_gc. I would prefer to take a slower pace with tackling such blockers, before we make wide-sweeping changes. What do you think?

I do not consider temp_cx really as blockers, but more of a todo that allows splitting work in more pieces, so I would still like to get this landed. Some of temp_cx removal will actually depend on this PR or require &mut JSContext in same dom methods which will duplicate work and create conflicts. I've noted all temp_cx that needs to be removed in the issue: #40600.

In the future I will try to make smaller PRs (I think biggest mistake here was that I went into servoparser instead of just putting temp_cx).

TimvdLippe · 2026-02-16T06:37:51Z

In the future I will try to make smaller PRs (I think biggest mistake here was that I went into servoparser instead of just putting temp_cx).

Is it too big of a hassle to do this anyway? E.g. you start over this patch and use temp_cx in this one place? That would make me a lot more comfortable approving such a PR. Right now, I don't feel comfortable approving given its size and impact. Especially since I am still learning about this migration and you pointed out a couple of things in the devtools PR that I missed.

sagudev · 2026-02-16T07:05:20Z

If it were easy enough I would already split off while doing the PR as I usually do. This time unfortunately I went too deep 😞.

Right now, I don't feel comfortable approving given its size and impact.

The changes are mostly mechanical so it's usually enough for me to just skim those, but I am very biased here as this is something I am very familiar with as I wrote these new types we use here.

Especially since I am still learning about this migration and you pointed out a couple of things in the devtools PR that I missed.

Are you aware of https://gist.github.com/sagudev/839382e70631b1eaed5a4d226b830b6e (linked from #40600)? Tho I find it easier to review code I've also write/use, so I invite you try it (if you have time). Here is good starting point: #42638

Gae24 · 2026-02-16T10:22:27Z

I'm interested in this since it quite overlaps with what's needed to continue #42640.
Regarding splitting off this in multiple PR, one can start with post_connection_steps since it has a single usage

servo/components/script/dom/node.rs

Line 2725 in c13d8a2

vtable_for(&node).post_connection_steps(CanGc::from_cx(cx));

Gae24 · 2026-02-16T17:15:15Z

I'm interested in this since it quite overlaps with what's needed to continue #42640.
Regarding splitting off this in multiple PR, one can start with post_connection_steps since it has a single usage

servo/components/script/dom/node.rs

Line 2725 in c13d8a2

vtable_for(&node).post_connection_steps(CanGc::from_cx(cx));

I can open a PR to do it.

sagudev · 2026-02-17T09:10:46Z

I'm interested in this since it quite overlaps with what's needed to continue #42640.
Regarding splitting off this in multiple PR, one can start with post_connection_steps since it has a single usage

servo/components/script/dom/node.rs

Line 2725 in c13d8a2

vtable_for(&node).post_connection_steps(CanGc::from_cx(cx));

I can open a PR to do it.

Feel free to do it.

sagudev · 2026-02-21T15:30:48Z

I did try to port VirtualMethod::attribute_mutate (not done but it's it is very involved and cannot be reasonably splited 😞) and we desperately need changes from here, so I will rebase this (we should be able to remove some temp_cx) and hopefully we can land this now.

components/script/dom/servoparser/mod.rs

sagudev · 2026-02-21T16:38:35Z

We were also able to remove some temp_cx from #42681.

TimvdLippe

I am still torn on what to do with this PR. On the one hand, I still find it unreasonably too large. It makes sense that you want to target the microtask checkpoint and for that a lot needs to happen, but I am not convinced that everything needs to be done in one go.

For example, the changes you made to Range, XMLHttpRequest or Element are all part of #42638 and imo we should tackle that first.

That said, I also know you have spent a considerable amount of time on this already. So there is a sunken cost fallacy here and I don't want to unnecessarily block this work, given that this migration is worthwhile.

Therefore, I propose a middle-ground: Can you separate the following changes from your PR as part of #42638 ? The remaining bits, we then tackle in this PR and merge this in one go.

components/script/dom/audio/audiocontext.rs
components/script/dom/audio/mediaelementaudiosourcenode.rs
components/script/dom/html/htmliframeelement.rs (except the temp_cx changes)
components/script/dom/html/htmlimageelement.rs (except the temp_cx changes)
components/script/dom/html/htmlmediaelement.rs (except the temp_cx changes)
components/script/dom/html/htmlscriptelement.rs
components/script/dom/dissimilaroriginwindow.rs
components/script/dom/document.rs (the DOM facing API's)
components/script/dom/domparser.rs
components/script/dom/element.rs
components/script/dom/range.rs
components/script/dom/shadowroot.rs
components/script/dom/xmlhttprequest.rs

That's still a huge chunk of work that we can commit in isolation of the other changes. Since these are all similar (they are part of the DOM-facing API migration), that should also be a lot easier to review, since its all mechanical. What we are then left with are the ones that relate to the microtask checkpoint queue and thus we can look closely at this, without the cluttering of the rest.

Thanks again for your patience and contributions. Please know that I am trying to figure out a way forward that helps both you as an author and us as reviewers.

sagudev · 2026-02-22T12:50:43Z

TBH I never viewed changes #42638 as standalone (usually they are driven by other changes). I will see what can be done, but I can already tell that dissimilaroriginwindow.rs will not go as it depends on some other window stuff (it is part of separate commit, so we can drop it - that leaves us it one more temp_cx).

split from servo#42635 Signed-off-by: sagudev <[email protected]>

split from #42635, some dom methods are also called from other function, which require passing down cx from many more places :( Testing: Just refactor, but should be covered by WPT. Part of #42638 Signed-off-by: sagudev <[email protected]>

split from servo#42635, some dom methods are also called from other function, which require passing down cx from many more places :( Testing: Just refactor, but should be covered by WPT. Part of servo#42638 Signed-off-by: sagudev <[email protected]>

TimvdLippe

Went through it and I think it's fine. I left some comments for the usages of temp_cx, please file actionable issues for these. We are taking some shortcuts in this PR with the temp_cx to be able to keep on going with the migration. That's fine by me, but I then would urge us to pick up these tasks first for removing the temp_cx, before we chug along with the migration.

Therefore, let's focus on these vtable usages and get the foundation sorted. Once those are tackled, I reckon that unblocking the other DOM methods will be a lot more straightforward as well.

Do you mind filing these issues and add the TODO-comments in this PR? Then feel free to land this PR. Thanks for taking it on!

TimvdLippe · 2026-02-24T08:54:03Z

components/script/dom/html/htmliframeelement.rs

-    fn attribute_mutated(&self, attr: &Attr, mutation: AttributeMutation, can_gc: CanGc) {
+    #[expect(unsafe_code)]
+    fn attribute_mutated(&self, attr: &Attr, mutation: AttributeMutation, _can_gc: CanGc) {
+        let mut cx = unsafe { temp_cx() };


Can you file a new issue explaining the steps required to make this happen? Then we can add it as a // TODO(<issue-number>) here so we don't forget to clean it up.

Btw I consider this part of #42638 since its all touching DOM-facing API's. Therefore, I do think #42638 is an enabler for the kinds of cleanups you do in this PR.

Yeah, but the problem is that we then need to wait for a lot of dom methods to get this and some dom methods are called from those functions behind traits so we get circular problem (one could say chicken and egg problem). Also we generally do not need all dom methods ported, but it's hard to decide which ones we need. The idea with the migration is that I do mostly harder/tricky stuff and left some easier stuff for others.

I understand that and I agree it is a solid strategy for this migration. Especially for those circular problems, temp_cx is an appropriate temporary measure. What I am trying to say is: let's try to keep the number of shortcuts we have at the same time at a manageable level. If we take on too much tech debt with those temp fixes, we might do more damage. It's a balancing act and I would prefer us to take a cautious approach when deciding how many shortcuts to take on. If we have (like in this PR), then we have to.

Btw, I still approve this PR. So feel free to land as-is if you also desire to do so. I would prefer to file the GitHub issues first and add TODO's, but can also do that later. Since this PR touches many points, it has high odds of running into merge conflicts.

Yeah, I will add issues it was just really busy day yesterday so I was only able to do some work for servo during short breaks.

TimvdLippe · 2026-02-24T08:55:24Z

components/script/dom/html/htmliframeelement.rs

    fn unbind_from_tree(&self, context: &UnbindContext, can_gc: CanGc) {
        self.super_type().unwrap().unbind_from_tree(context, can_gc);

+        let mut cx = unsafe { temp_cx() };


Same here, which seems to be local to Node methods, but doesn't look all that bad at a first glance.

TimvdLippe · 2026-02-24T08:57:22Z

components/script/dom/html/htmlimageelement.rs

-        self.update_the_image_data(can_gc);
+    #[expect(unsafe_code)]
+    fn adopting_steps(&self, old_doc: &Document, _can_gc: CanGc) {
+        let mut cx = unsafe { temp_cx() };


components/script/dom/html/htmlmediaelement.rs

TimvdLippe · 2026-02-24T08:59:00Z

components/script/dom/html/htmlmediaelement.rs


+    #[expect(unsafe_code)]
    fn process_response(&mut self, _: RequestId, metadata: Result<FetchMetadata, NetworkError>) {
+        let mut cx = unsafe { temp_cx() };


Same here. Although this one looks very tricky, given it involves fetch and thus the network code?

I did *_eof and it's entrypoints were eventloop and tasks so it is easy enough.

components/script/dom/servoparser/mod.rs

Signed-off-by: sagudev <[email protected]>

this was bigger then I hoped Signed-off-by: sagudev <[email protected]>

Signed-off-by: sagudev <[email protected]>

sagudev requested a review from gterzian as a code owner February 15, 2026 13:24

servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Feb 15, 2026

sagudev mentioned this pull request Feb 15, 2026

Safety from ground up #40600

Open

1 task

sagudev force-pushed the microtask-chunk-step branch from 2f451c9 to 49f7da4 Compare February 15, 2026 14:15

TimvdLippe reviewed Feb 15, 2026

View reviewed changes

sagudev force-pushed the microtask-chunk-step branch 3 times, most recently from d46e4f4 to eb05147 Compare February 21, 2026 16:35

Gae24 reviewed Feb 21, 2026

View reviewed changes

components/script/dom/servoparser/mod.rs Show resolved Hide resolved

TimvdLippe reviewed Feb 22, 2026

View reviewed changes

sagudev added a commit to sagudev/servo that referenced this pull request Feb 22, 2026

script: Pass &mut JSContext to more dom methods

7c0a82c

split from servo#42635 Signed-off-by: sagudev <[email protected]>

sagudev mentioned this pull request Feb 22, 2026

script: Pass &mut JSContext to more dom methods #42751

Merged

sagudev force-pushed the microtask-chunk-step branch from eb05147 to 666419d Compare February 22, 2026 14:57

TimvdLippe approved these changes Feb 24, 2026

View reviewed changes

servo-highfive added S-needs-rebase There are merge conflict errors. and removed S-awaiting-review There is new code that needs to be reviewed. labels Feb 24, 2026

This was referenced Feb 25, 2026

Pass &mut JSContext to VirtualMethods::unbind_from_tree #42837

Open

Decide how to pass &mut JSContext to TreeSink::create:element #42839

Open

script: Pass down &mut JSContext in servoparser and event loop.

3cb79dd

Signed-off-by: sagudev <[email protected]>

sagudev added 2 commits February 25, 2026 07:59

eliminate another temp_cx

ca7b1d9

this was bigger then I hoped Signed-off-by: sagudev <[email protected]>

add TODOs

4459b9a

Signed-off-by: sagudev <[email protected]>

sagudev force-pushed the microtask-chunk-step branch from 666419d to 4459b9a Compare February 25, 2026 07:05

servo-highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-rebase There are merge conflict errors. labels Feb 25, 2026

sagudev enabled auto-merge February 25, 2026 07:06

sagudev added this pull request to the merge queue Feb 25, 2026

servo-highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Feb 25, 2026

Merged via the queue into servo:main with commit 3e2f14c Feb 25, 2026
33 checks passed

sagudev deleted the microtask-chunk-step branch February 25, 2026 08:17

servo-highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Feb 25, 2026

Uh oh!

Conversation

sagudev commented Feb 15, 2026

Uh oh!

TimvdLippe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sagudev commented Feb 16, 2026

Uh oh!

TimvdLippe commented Feb 16, 2026

Uh oh!

sagudev commented Feb 16, 2026

Uh oh!

Gae24 commented Feb 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Gae24 commented Feb 16, 2026

Uh oh!

sagudev commented Feb 17, 2026

Uh oh!

sagudev commented Feb 21, 2026

Uh oh!

Uh oh!

sagudev commented Feb 21, 2026

Uh oh!

TimvdLippe left a comment

Choose a reason for hiding this comment

Uh oh!

sagudev commented Feb 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TimvdLippe 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Gae24 commented Feb 16, 2026 •

edited

Loading

sagudev commented Feb 22, 2026 •

edited

Loading