script: Wire `element` for `LargestContentfulPaint` by shubhamg13 · Pull Request #46553 · servo/servo

shubhamg13 · 2026-07-16T05:05:44Z

Wire the logic for element on LargestContentfulPaint using OpaqueNode

While processing the ReflowResult, OpaqueNode is resolved and Element is stored in HashMap located in document.

Testing: New WPT Passed (+ To be Added in #46851)
Fixes: Part of #42000

jschwe · 2026-07-16T06:32:57Z

+            let address = UntrustedNodeAddress(id as *const c_void);
+            let node = unsafe { node::from_untrusted_node_address(address) };
+            DomRoot::downcast::<Element>(node).map(|el| String::from(el.Id()))


How do you know that the node is still alive?
In general one should by all means avoid round-tripping a pointer through an integer, since it loses / exposes provenance. Can't we use a different mechanism to lookup the node?

The element was just painted to screen in same layout so it must be alive.

Please enlighten me on this. This is what I achieved by referencing already implemented components in InspectorHighlight or DocumentShadowRoot.

Here is a open Zulip Topic for same

The ID is sent from layout to the constellation, then back to the script thread. The element could have been removed from the document in that time and GCed.

Please enlighten me with better appraoch to this, i am not familiar with this that much.

In general:

choose a script thread struct that will store the underlying object until the async notification is received

create a new ID that can be sent around that is not the pointer (eg. a uuid)

make it possible to map the ID to the stored object

This could be something like
largest_contentful_paint_candidates: DomRefCell<HashMap<LargestContentfulPaintId, Dom<Element>>>
which could live in ScriptThread, or maybe a single DomRefCell<Option<(LargestContentfulPaintId, Dom<Element>)>> in Document, or maybe there's somewhere else that makes more sense.

Please take a look at the latest changes. Extracted some changes in #46828

Sorry for Earlier, I think I mixed few concepts and created a mess.

I explored that idea of having HashMap of largest_contentful_paint_candidates. But I think we need to come back to script layer in one way or other to resolve the element.

But IMO By using Node::is_connected_with_browsing_context, we make sure Node is still alive.

Let me know your thoughts. I am supporting my point with WPTs as well. Please take a look on #46851 as well.

P.S: I will rebase when WPT is merged.

mrobinson · 2026-07-28T08:29:16Z

            url: url.map(|u| DOMString::from(u.as_str())).unwrap_or_default(),
-            element: None,
+            element_id: element_id.map(DOMString::from).unwrap_or_default(),
+            element: element.map(|el| Dom::from_ref(&*el)),


Do not abbreviate variable names here and everywhere else in this change: https://book.servo.org/contributing/style-guide.html#abbreviation

Suggested change

element: element.map(|el| Dom::from_ref(&*el)),

element: element.map(|element| Dom::from_ref(&*element)),

mrobinson · 2026-07-28T08:32:21Z


+    /// <https://www.w3.org/TR/largest-contentful-paint/#dom-largestcontentfulpaint-id>
+    fn Id(&self) -> DOMString {
+        self.element_id.clone()


Can't this just read the id attribute from self.element. Why must the id be passed into layout and then back out again?

Sure Noted.

mrobinson · 2026-07-28T08:37:35Z

+                candidate_id,
+            } => {
+                let mut element: Option<DomRoot<Element>> = None;
+                if let Some(candidate) = candidate_id {


It's very confusing to rename this, so just call it candidate_id as well:

Suggested change

if let Some(candidate) = candidate_id {

if let Some(candidate_id) = candidate_id {

mrobinson · 2026-07-28T08:38:33Z

+                        // captured during display list construction is still
+                        // valid in the DOM.
+                        let address = UntrustedNodeAddress(candidate.0 as *const c_void);
+                        let node = unsafe { node::from_untrusted_node_address(address) };


This is still an OpaqueNode that comes back to the script from an asynchronous message. How can you be sure that's it's still valid? You updated your change, but you didn't explain how your update responded to the concerns raised before, so it's hard to say what the mechanism is.

By using Node::is_connected_with_browsing_context, we make sure Node is still alive.

IMo, missing WPT test case is the reason for proper testing of raised concerns. So, I added WPT #46851.

The issue is that the Node might have been garbage collected at this point and the pointer you are dereferencing here might point to freed memory or even at some other allocated memory (even another Node). Nothing you can call on the Node after this dereference is reliable or can fix this issue. I think it's actually just impossible to get a reference to a node based something you calculate from the display list (for good reason...the display list shouldn't prevent garbage collection). It may be the case that to implement LCP properly, it needs to move to script from paint.

Acknowledged. Yeah I agree. Let me process this information and I will get back to this.

Now while processing the ReflowResult, OpaqueNode is resolved and Element is stored in HashMap located in document.

Almost identical to how we prcoess handle_pending_images_post_reflow.

servo-wpt-sync · 2026-07-29T04:15:04Z

🤖 Opened new upstream WPT pull request (web-platform-tests/wpt#61595) with upstreamable changes.

servo-wpt-sync · 2026-07-29T10:19:41Z

📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#61595).

Signed-off-by: Shubham Gupta <shubham.gupta@chromium.org>

jdm

Almost there!

jdm · 2026-07-31T11:30:52Z

+    fn process_lcp_candidate_post_reflow(
+        &self,
+        candidate: &LCPCandidate,
+        node_id: usize,


Let's make this the UntrustedNodeAddres instead.

Sure. Done.

jdm · 2026-07-31T11:32:04Z

+    /// The LCP candidate during this layout pass, if any.
+    pub lcp_candidate: Option<LCPCandidate>,
+    /// The OpaqueNode ID for the LCP candidate if any.
+    pub lcp_node_id: Option<usize>,


This should be a UntrustedNodeAddress instead.

Signed-off-by: Shubham Gupta <shubham.gupta@chromium.org>

jdm

Thanks!

Addressed.

shubhamg13 mentioned this pull request Jul 16, 2026

Feature: LargestContentfulPaint #42000

Open

shubhamg13 marked this pull request as ready for review July 16, 2026 05:23

shubhamg13 requested review from Loirooriol, gterzian and mrobinson as code owners July 16, 2026 05:23

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

jschwe reviewed Jul 16, 2026

View reviewed changes

jschwe previously requested changes Jul 16, 2026

View reviewed changes

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 Jul 16, 2026

shubhamg13 force-pushed the lcp_id branch from a647e10 to f694bf9 Compare July 17, 2026 02:30

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 Jul 17, 2026

jdm 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 Jul 21, 2026

shubhamg13 force-pushed the lcp_id branch from f694bf9 to aa98bae Compare July 27, 2026 07:45

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 Jul 27, 2026

shubhamg13 requested review from jdm, jschwe and xiaochengh July 27, 2026 08:03

shubhamg13 marked this pull request as draft July 28, 2026 07:58

shubhamg13 changed the title ~~Expose id on LargestContentfulPaint~~ script: Wire element for LargestContentfulPaint Jul 28, 2026

shubhamg13 force-pushed the lcp_id branch from aa98bae to 739cd47 Compare July 28, 2026 08:11

mrobinson previously requested changes Jul 28, 2026

View reviewed changes

shubhamg13 force-pushed the lcp_id branch from 739cd47 to ea08c2f Compare July 29, 2026 04:10

servo-wpt-sync mentioned this pull request Jul 29, 2026

script: Wire element for LargestContentfulPaint web-platform-tests/wpt#61595

Closed

shubhamg13 force-pushed the lcp_id branch from ea08c2f to c9656df Compare July 29, 2026 10:15

shubhamg13 force-pushed the lcp_id branch 2 times, most recently from 6b161a3 to 712d27f Compare July 31, 2026 07:02

shubhamg13 added 2 commits July 31, 2026 15:28

Wire element

e3a9d9c

Signed-off-by: Shubham Gupta <shubham.gupta@chromium.org>

WPT Expectations

9c9ca07

Signed-off-by: Shubham Gupta <shubham.gupta@chromium.org>

shubhamg13 force-pushed the lcp_id branch from 712d27f to 9c9ca07 Compare July 31, 2026 07:28

shubhamg13 marked this pull request as ready for review July 31, 2026 08:01

jdm requested changes Jul 31, 2026

View reviewed changes

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 Jul 31, 2026

Use UntrustedNodeAddress instead of usize.

d6955d1

Signed-off-by: Shubham Gupta <shubham.gupta@chromium.org>

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 Aug 1, 2026

shubhamg13 requested a review from jdm August 1, 2026 06:58

jdm approved these changes Aug 1, 2026

View reviewed changes

servo-highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Aug 1, 2026

jdm added this pull request to the merge queue Aug 1, 2026

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

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 1, 2026

servo-highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Aug 1, 2026

jdm added this pull request to the merge queue Aug 1, 2026

servo-highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Aug 1, 2026

Merged via the queue into servo:main with commit 3f08ca6 Aug 1, 2026
42 checks passed

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

shubhamg13 deleted the lcp_id branch August 2, 2026 03:09

shubhamg13 mentioned this pull request Aug 14, 2026

servoshell: Add largest_contentful_paint_enabled to EXPERIMENTAL_PREFS #43307

Draft

	element: element.map(\|el\| Dom::from_ref(&*el)),
	element: element.map(\|element\| Dom::from_ref(&*element)),

	if let Some(candidate) = candidate_id {
	if let Some(candidate_id) = candidate_id {

Conversation

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shubhamg13 Jul 17, 2026 • 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!

shubhamg13 Jul 17, 2026 • 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!

jdm Jul 17, 2026 • 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!

shubhamg13 Jul 27, 2026 • 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!

shubhamg13 Jul 29, 2026 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrobinson Jul 28, 2026 • 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!

servo-wpt-sync commented Jul 29, 2026

Uh oh!

servo-wpt-sync commented Jul 29, 2026

Uh oh!

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

jdm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

shubhamg13 commented Jul 16, 2026 •

edited

Loading

shubhamg13 Jul 17, 2026 •

edited

Loading

shubhamg13 Jul 17, 2026 •

edited

Loading

jdm Jul 17, 2026 •

edited

Loading

shubhamg13 Jul 27, 2026 •

edited

Loading

shubhamg13 Jul 29, 2026 •

edited

Loading

mrobinson Jul 28, 2026 •

edited

Loading