Repository navigation
script: More faithfully implement "evaluate a JavaScript URL" - #47486
Merged
Merged
Conversation
mrobinson
marked this pull request as draft
August 24, 2026 17:18
Member
Author
|
This is a draft because it depends on #47485, but otherwise it is a three line fix that is reviewable as it is. |
jdm
reviewed
Aug 24, 2026
| introduction_type: Option<&'static CStr>, | ||
| rval: Option<MutableHandleValue>, | ||
| ) -> Result<(), JavaScriptEvaluationError> { | ||
| assert!(self.can_run_script()); |
Member
There was a problem hiding this comment.
I pushed back against this change in another PR because this check is intended to be a backstop. If we're hitting this assertion, I'd like to address the callers instead of silently ignoring the attempt to run script.
Member
Author
There was a problem hiding this comment.
Fair enough. I've taken a different approach here. Instead, I've more faithfully implemented the specification around "evaluate a JavaScript URL" which prevents evaluating a script as a side-effect of creating and running a classic script.
Member
Author
There was a problem hiding this comment.
I realized this comment was a bit confusing. What I meant to say is I rewrote the change after your comment above.
mrobinson
force-pushed
the
trusted-types-3
branch
2 times, most recently
from
August 24, 2026 22:09
3cbc1aa to
2f9548a
Compare
evaluate_js_on_global cannot run script|
🔨 Triggering try run (#32783250714) for Linux (WPT) |
Member
Author
|
Let's see what the bots say about this new approach, I've kicked off a WPT job. |
|
Test results for linux-wpt from try job (#32783250714): Flaky unexpected result (28)
Stable unexpected results that are known to be intermittent (14)
Stable unexpected results (6)
|
|
|
Member
|
Those are very promising results! |
This change makes it so that the "evaluate a JavaScript URL" procedure from the specification is more faithfully implemented. In particular, it builds and executes a classic script, which nows grows the ability to return a value, as specified. More specification steps are filled out in general. Signed-off-by: Martin Robinson <mrobinson@igalia.com>
mrobinson
force-pushed
the
trusted-types-3
branch
from
August 25, 2026 05:20
2f9548a to
ec15fac
Compare
mrobinson
marked this pull request as ready for review
August 25, 2026 05:20
Member
Author
|
Okay. This one is ready for a review. |
TimvdLippe
approved these changes
Aug 25, 2026
| introduction_type: Option<&'static CStr>, | ||
| line_number: u32, | ||
| external: bool, | ||
| returns_a_value: bool, |
Contributor
There was a problem hiding this comment.
Nit: introduce an enum ScriptReturnsAValue with members Yes and No. That way the code comments are unnecessary and the compiler will complain when the argument is accidentally used for external
Member
Author
There was a problem hiding this comment.
I've gone ahead and made a ScriptOptions bitflags struct to hold three of the boolean parameters passed to classic script creation. I think this simplifies the code a lot.
|
|
||
| // Step 13. Let finalSandboxFlags be policyContainer's CSP list's CSP-derived | ||
| // sandboxing flags. | ||
| load_data.creation_sandboxing_flag_set |= sandbox_flags; |
Contributor
There was a problem hiding this comment.
I think the spec means this. Otherwise we might have discovered a spec bug?
Suggested change
| load_data.creation_sandboxing_flag_set |= sandbox_flags; | |
| load_data.creation_sandboxing_flag_set = sandbox_flags; |
Member
Author
There was a problem hiding this comment.
In fact, I think this line of code is completely wrong, because the specification talks about the "final sandbox flags", but I'm assigned the "creation sandbox flags" here. I've just removed this entirely.
Member
|
Oh no, I just realized why these changes looked so familiar. I thought #46611 had already merged, but it hasn't yet and there's a huge amount of overlap. |
Member
Author
- Create a `ScriptOptions` to hold options passed to classic scripts - Respond to review comments - Make a variety of small comment / documentation improvements Signed-off-by: Martin Robinson <mrobinson@abandonedwig.info>
|
🔨 Triggering try run (#32815809600) for Linux (WPT) |
|
Test results for linux-wpt from try job (#32815809600): Flaky unexpected result (33)
Stable unexpected results that are known to be intermittent (13)
|
|
✨ Try run (#32815809600) succeeded. |
Contributor
It's not merged, but i think that PR was addressing a separate issue that unnecessarily overlapped with this. I can rebase that after this, so it cleanly separates the two changes |
Member
You must have looked at the comment I left which referenced a different issue before I edited it. #46611 is not merged. |
RichardTjokroutomo
pushed a commit
to RichardTjokroutomo/servo
that referenced
this pull request
Oct 6, 2026
…47486) This change makes it so that the "evaluate a JavaScript URL" procedure from the specification is more faithfully implemented. In particular, it builds and executes a classic script, which nows grows the ability to return a value, as specified. More specification steps are filled out in general. Testing: This results in a progression on a decent number of WPT subtests. --------- Signed-off-by: Martin Robinson <mrobinson@igalia.com> Signed-off-by: Martin Robinson <mrobinson@abandonedwig.info>
Psychpsyo
pushed a commit
to Psychpsyo/servo
that referenced
this pull request
Oct 12, 2026
…47486) This change makes it so that the "evaluate a JavaScript URL" procedure from the specification is more faithfully implemented. In particular, it builds and executes a classic script, which nows grows the ability to return a value, as specified. More specification steps are filled out in general. Testing: This results in a progression on a decent number of WPT subtests. --------- Signed-off-by: Martin Robinson <mrobinson@igalia.com> Signed-off-by: Martin Robinson <mrobinson@abandonedwig.info>
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
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.
This change makes it so that the "evaluate a JavaScript URL" procedure
from the specification is more faithfully implemented. In particular, it
builds and executes a classic script, which nows grows the ability to
return a value, as specified. More specification steps are filled out in
general.
Testing: This results in a progression on a decent number of WPT subtests.