Skip to content

script: Store a Weak handle to ScriptThread in both ScriptThread and Window#40645

Merged
mrobinson merged 6 commits intoservo:mainfrom
Narfinger:script-thread-weak
Jan 5, 2026
Merged

script: Store a Weak handle to ScriptThread in both ScriptThread and Window#40645
mrobinson merged 6 commits intoservo:mainfrom
Narfinger:script-thread-weak

Conversation

@Narfinger
Copy link
Copy Markdown
Contributor

@Narfinger Narfinger commented Nov 14, 2025 •
edited by mrobinson
Loading

We construct ScriptThread in an Rc which itself has the given Weak for it. It then can give it to Window and other elements in the future. We currently only use this for the microtask checkpoint function in Window. This is the first step toward eliminating the usage of thread local storage to access the ScriptThread.

Testing: This should not change behavior so is covered by existing tests.
Fixes: Part of addressing: #37969 (comment)

@Narfinger
Copy link
Copy Markdown
Contributor Author

Narfinger commented Nov 14, 2025

@mrobinson I wanted to grab this before somebody else does. However, I am not quite sure if this is safe.
Theoretically the following could happen:

  • Window upgrades the Weak to an Rc
  • ScriptThread ends, dropping the Rc, making the one in the call in Window the only one alive.
  • The Rc in Window is not found by the JavaScript tracer.
  • The Gc cleans up this object and we are in unsafe theretory.

@Narfinger
Copy link
Copy Markdown
Contributor Author

Narfinger commented Nov 17, 2025

I think this actually can be made safe if we trace the weak by upgrading it.

@Narfinger
Copy link
Copy Markdown
Contributor Author

Narfinger commented Nov 20, 2025

@sagudev Perhaps it is better to explain this in this example (related to servo/mozjs#659) as explained. With the upgraded Weak being the only valid reference that is not reachable by trace.
Now one possibility is to root this value (and remove the allow unrooted lint) on ScriptThread.
But I feel like this defeats the purpose of this change, to remove reliance on thread local variables.
Perhaps you see a different method than allowing trace on Weak?

@sagudev
Copy link
Copy Markdown
Member

sagudev commented Nov 20, 2025

Perhaps you see a different method than allowing trace on Weak?

We have custom_trace:

servo/components/jstraceable_derive/lib.rs

Line 18 in 6b452bd

/// #[custom_trace] // Extern type implements CustomTraceable that is in servo => no problem with orphan rules
, although it's mainly used to bypass orphan rules of rust, we can use it in this case to customize behavior.

@Narfinger Narfinger marked this pull request as ready for review November 20, 2025 12:46
@Narfinger Narfinger requested a review from gterzian as a code owner November 20, 2025 12:46
@servo-highfive servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Nov 20, 2025
@mrobinson
Copy link
Copy Markdown
Member

mrobinson commented Nov 20, 2025

Tracing of the ScriptThread is already handled by Runtime callbacks, so maybe it isn't necessary to trace the entire thing again.

sagudev
sagudev reviewed Nov 20, 2025
View reviewed changes
@sagudev
Copy link
Copy Markdown
Member

sagudev commented Nov 20, 2025

Tracing of the ScriptThread is already handled by Runtime callbacks, so maybe it isn't necessary to trace the entire thing again.

Indeed:

servo/components/script/script_runtime.rs

Line 1116 in bc9e8d2

trace_thread(tr);

@mrobinson
Copy link
Copy Markdown
Member

mrobinson commented Jan 4, 2026

I think we should land this one and land it as an incremental step toward #40864.

@Narfinger Narfinger force-pushed the script-thread-weak branch 2 times, most recently from 1b24a82 to aad2a01 Compare January 5, 2026 08:58
@Narfinger
Copy link
Copy Markdown
Contributor Author

Narfinger commented Jan 5, 2026

@mrobinson Ok this should be ready now.

@Narfinger
Pass a weak reference to the ScriptThread down into it and into window.
aad2a01 
Use this to call microtask checkpoint directly.

Signed-off-by: Narfinger <[email protected]>
mrobinson
mrobinson requested changes Jan 5, 2026
View reviewed changes
@servo-highfive servo-highfive added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jan 5, 2026
@mrobinson
Copy link
Copy Markdown
Member

mrobinson commented Jan 5, 2026

@Narfinger Thanks. A few small requests based on the changes that I made in #40864.

@servo-highfive servo-highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Jan 5, 2026
@Narfinger
incorporate suggestions
49113ef 
Signed-off-by: Narfinger <[email protected]>
mrobinson
mrobinson reviewed Jan 5, 2026
View reviewed changes
mrobinson
mrobinson approved these changes Jan 5, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@mrobinson mrobinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple minor suggestions, but otherwise this looks good. Thanks!

@servo-highfive servo-highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Jan 5, 2026
@Narfinger @mrobinson
Update components/script/dom/window.rs
29e18f7 
Co-authored-by: Martin Robinson <[email protected]>
Signed-off-by: Narfinger <[email protected]>
@servo-highfive servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jan 5, 2026
Narfinger and others added 3 commits January 5, 2026 12:04
@Narfinger @mrobinson
Update components/script/script_thread.rs
b53f8e8 
Co-authored-by: Martin Robinson <[email protected]>
Signed-off-by: Narfinger <[email protected]>
@Narfinger @mrobinson
Update components/script/script_thread.rs
a76de69 
Co-authored-by: Martin Robinson <[email protected]>
Signed-off-by: Narfinger <[email protected]>
@Narfinger @mrobinson
Update components/script/script_thread.rs
ad4035c 
Co-authored-by: Martin Robinson <[email protected]>
Signed-off-by: Narfinger <[email protected]>
@Narfinger
Copy link
Copy Markdown
Contributor Author

Narfinger commented Jan 5, 2026

Thanks for the help getting this done! I really appreciate all the comments.

@mrobinson mrobinson changed the title Script: Pass a weak ref to window script: Store a Weak handle to ScriptThread in both ScriptThread and Window Jan 5, 2026
@mrobinson mrobinson added this pull request to the merge queue Jan 5, 2026
@servo-highfive servo-highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jan 5, 2026
Merged via the queue into servo:main with commit 94eafd4 Jan 5, 2026
31 checks passed
@servo-highfive servo-highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jan 5, 2026
@Narfinger Narfinger deleted the script-thread-weak branch January 5, 2026 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sagudev sagudev sagudev left review comments

@mrobinson mrobinson mrobinson approved these changes

@gterzian gterzian Awaiting requested review from gterzian gterzian is a code owner

Assignees

No one assigned

Labels

S-awaiting-review There is new code that needs to be reviewed.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Narfinger @sagudev @mrobinson @servo-highfive