script: implement render-blocking option for script fetch and load events#43741
Open
elomscansio wants to merge 18 commits intoservo:mainfrom
Open
script: implement render-blocking option for script fetch and load events#43741elomscansio wants to merge 18 commits intoservo:mainfrom
elomscansio wants to merge 18 commits intoservo:mainfrom
Conversation
servo-highfive
added
the
S-awaiting-review
Contributor
Author
|
@jdm |
Contributor
Author
|
@jdm servo/components/script/dom/document/document.rs Line 2311 in 6656061 |
|
🔨 Triggering try run (#23703999107) for Linux (WPT) |
|
Test results for linux-wpt from try job (#23703999107): Flaky unexpected result (39)
Stable unexpected results that are known to be intermittent (21)
Stable unexpected results (206)
|
|
|
Contributor
For module scripts the corresponding of |
Gae24
reviewed
Mar 29, 2026
Gae24
requested changes
Mar 29, 2026
| let blocker = &self.delaying_the_load_event; | ||
| if delay && blocker.borrow().is_none() { | ||
| *blocker.borrow_mut() = Some(LoadBlocker::new( | ||
| &self.owner_document(), |
Contributor
There was a problem hiding this comment.
I think this is not correct, we should use either parser_document or preparation_time_document, which should be determined by the ExternalScriptKind of the fetched script.
Contributor
Author
There was a problem hiding this comment.
What we are doing is just toggle on/off the load blocker.
See comment by @jdm
I think the way to do this correctly is stop using
Document::fetchin htmlscriptelement.rs andDocumentLoader::fetch_async_with_callbackin script_module.rs, and store an explicitDomRefCell<Option<LoadBlocker>>in HTMLScriptElement which we can manipulate when the spec modifies https://html.spec.whatwg.org/multipage/scripting.html#concept-script-delay-load .
#43697 (comment)
Member
There was a problem hiding this comment.
Gae24 means that using owner_doc() here is not correct; we need to use a more specific document when creating the load blocker.
Contributor
There was a problem hiding this comment.
Yep, as a starting point we should introduce a method that implements step 35 which returns ExternalScriptKind, with that we can then identify which document to use.
servo-highfive
added
S-needs-code-changes
Contributor
Author
|
Oh, thanks for clarification.
On it.
…On Sun, 29 Mar 2026, 6:14 pm Gae24, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In components/script/dom/html/htmlscriptelement.rs
<#43741?email_source=notifications&email_token=BG4RHOSIRPAJZ367UXSSQ6L4TFRWXA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIMBSG4YDCOBZHA2KM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2K64DSL5ZGK5TJMV3V6Y3MNFRWW#discussion_r3006486546>
:
> + ///
+ /// Nothing happens if the element was already delaying the load event and
+ /// we pass true to that method again.
+ ///
+ /// <https://html.spec.whatwg.org/multipage/scripting.html#concept-script-delay-load>
+ /// <https://html.spec.whatwg.org/multipage/#delaying-the-load-event-flag>
+ pub(crate) fn delay_load_event(
+ &self,
+ delay: bool,
+ url: ServoUrl,
+ cx: &mut js::context::JSContext,
+ ) {
+ let blocker = &self.delaying_the_load_event;
+ if delay && blocker.borrow().is_none() {
+ *blocker.borrow_mut() = Some(LoadBlocker::new(
+ &self.owner_document(),
Yep, as a starting point we should introduce a method that implements step
35
<https://html.spec.whatwg.org/multipage/scripting.html#script-processing-model:concept-script-type-15>
which returns ExternalScriptKind, with that we can then identify which
document to use.
—
Reply to this email directly, view it on GitHub
<#43741?email_source=notifications&email_token=BG4RHOUXMXLMO44X2GOTFN34TFRWXA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIMBSG4YDCOBZHA2KM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2L24DSL5ZGK5TJMV3V63TPORUWM2LDMF2GS33OONPWG3DJMNVQ#discussion_r3006486546>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BG4RHORZQZQSMRZOHQS2VBL4TFRWXAVCNFSM6AAAAACXDTJGJCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHM2DAMRXGAYTQOJYGQ>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
elomscansio
force-pushed
the
delaying_the_load_event
branch
from
e0bfdeb to
1425e16
Compare
servo-highfive
added
S-awaiting-review
Gae24
requested changes
Mar 29, 2026
| .expect("Script should have an active document when delaying load event"); | ||
|
|
||
| let url = self | ||
| .get_script_url() |
Contributor
There was a problem hiding this comment.
We can simply pass down the url and avoid this method.
Contributor
Author
There was a problem hiding this comment.
Done, but required making ScriptResult.url public so that we won't pollute execute method which are called from document and so on.
servo-highfive
added
S-needs-code-changes
elomscansio
force-pushed
the
delaying_the_load_event
branch
from
c2dbf2b to
615e35c
Compare
Contributor
Author
|
current commit test result Ran 470 tests finished in 714.4 seconds.
• 435 ran as expected.
• 28 tests crashed unexpectedly
• 1 tests timed out unexpectedly
• 6 tests unexpectedly okay
• 1 tests had unexpected subtest results
/home/elomscansio/.local/share/uv/python/cpython-3.11.15-linux-x86_64-gnu/lib/python3.11/multiprocessing/resource_tracker.py:254: UserWarning: resource_tracker: There appear to be 90 leaked semaphore objects to clean up at shutdown
warnings.warn('resource_tracker: There appear to be %d 'Seems i'm still getting fews things wrong. |
Contributor
Author
|
Please whats the difference between
because i can see step to unblock rendering also i can see https://html.spec.whatwg.org/multipage/scripting.html#steps-to-run-when-the-result-is-ready where delay load event is to be set to false but did not mention to unblock. what then is the different between blocking/unblocking rendering vs delay load event |
Member
|
Render-blicked documents are supposed to be skipped when running the "update the rendering" algorithm. For script elements that's tracked with . Delaying the load event means that the onliad event for the document will not be fired, and that's tracked with the LoadBlocker object. Does that answer your question? |
Contributor
|
The correct document was solely used in servo/components/script/dom/html/htmlscriptelement.rs Lines 882 to 889 in 2d29c38 servo/components/script/dom/html/htmlscriptelement.rs Lines 908 to 917 in 2d29c38 servo/components/script/dom/html/htmlscriptelement.rs Lines 961 to 967 in 2d29c38 servo/components/script/script_module.rs Lines 665 to 671 in 2d29c38 |
Contributor
|
Still need to update the test expectations. |
Gae24
reviewed
Apr 3, 2026
elomscansio
force-pushed
the
delaying_the_load_event
branch
from
a0052cb to
aa0cd0b
Compare
…ents Signed-off-by: Emmanuel Paul Elom <[email protected]>
Signed-off-by: Emmanuel Paul Elom <[email protected]>
Signed-off-by: Emmanuel Paul Elom <[email protected]>
…ructs Signed-off-by: Emmanuel Paul Elom <[email protected]>
…ding The HTML spec defines two important documents for script elements: - Preparation-time document: Set when the script is prepared (owner document at that time) - Parser document: Set when the script is parser-inserted Previously, the code would call `owner_document()` each time it needed to dispatch script load events, which could return a different document if the script element was moved between documents. This change: - Adds getter methods `get_preparation_time_document()` and `get_parser_document()` to `HTMLScriptElement` - Updates `script_module.rs` to use these getters instead of calling `owner_document()` directly - Ensures script load events are dispatched to the correct document according to the HTML spec Signed-off-by: Emmanuel Paul Elom <[email protected]>
… && was_parser_inserted` Signed-off-by: Emmanuel Paul Elom <[email protected]>
…ents - Centralize script kind logic into `get_script_kind()` method - Remove duplicated kind determination in classic and module script paths - Call `delay_load_event` after script fetch completion - Clean up unused url variable assignments in script execution Signed-off-by: Emmanuel Paul Elom <[email protected]>
Signed-off-by: Emmanuel Paul Elom <[email protected]>
Co-authored-by: Gae24 <[email protected]> Signed-off-by: elomscansio <[email protected]>
…arting the fetches Signed-off-by: Emmanuel Paul Elom <[email protected]>
…/html/semantics/scripting-1/the-script-element Signed-off-by: Emmanuel Paul Elom <[email protected]>
…nctions Signed-off-by: Emmanuel Paul Elom <[email protected]>
Co-authored-by: Gae24 <[email protected]> Signed-off-by: Emmanuel Paul Elom <[email protected]>
Signed-off-by: Emmanuel Paul Elom <[email protected]>
Co-authored-by: Gae24 <[email protected]> Signed-off-by: Emmanuel Paul Elom <[email protected]>
Signed-off-by: Emmanuel Paul Elom <[email protected]>
elomscansio
force-pushed
the
delaying_the_load_event
branch
from
aa0cd0b to
59f5b6e
Compare
Co-authored-by: Gae24 <[email protected]> Signed-off-by: elomscansio <[email protected]>
elomscansio
force-pushed
the
delaying_the_load_event
branch
from
59f5b6e to
42e79f1
Compare
Signed-off-by: Gae24 <[email protected]>
Gae24
approved these changes
Apr 5, 2026
servo-highfive
removed
the
S-awaiting-review
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implement render-blocking option for script fetch and load events
Testing:
Test summary on main branch
Test summary for this commits
Ran 470 tests finished in 488.0 seconds. • 397 ran as expected. • 15 tests crashed unexpectedly • 27 tests timed out unexpectedly • 11 tests unexpectedly okay • 28 tests had unexpected subtest resultsFixes: #43697
Fixes: #43354