Skip to content

script: Reject AudioContext promises when the document is not fully active. - #46611

Draft
amittenak47 wants to merge 9 commits into
servo:mainfrom
amittenak47:issue-46600
Draft

script: Reject AudioContext promises when the document is not fully active.#46611
amittenak47 wants to merge 9 commits into
servo:mainfrom
amittenak47:issue-46600

Conversation

@amittenak47

@amittenak47 amittenak47 commented Jul 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Draft. Blocked on #47486.

After #47486 lands I will rebase this onto main and drop the overlapping work:

What remains here is the original Web Audio change.

Changes

Reject decodeAudioData, resume, suspend, and close with a promise rejected with InvalidStateError when the Document is not fully active, matching the Web Audio API.

Testing

New passing WPT tests.

  • Are there already automated tests that cover the code that is being changed?
    Yes. The WPT check the existence of a detached iframe and discarded browsing context.

  • If not, does this pull request include new automated tests?
    N/A; the existing tests cover the changes.

  • If not, what prevents adding at least one new test?
    N/A; Tests already exist.

Ran locally:
.\mach test-wpt \servo\tests\wpt\tests\webaudio\the-audio-api\the-audiocontext-interface\audiocontext-detached-execution-context.html
.\mach test-wpt \servo\tests\wpt\tests\webaudio\the-audio-api\the-audiocontext-interface\promise-methods-after-discard.html

Fixes #46600.

see WebAudio/web-audio-api#2176

@amittenak47
amittenak47 requested a review from gterzian as a code owner July 18, 2026 11:20
@servo-highfive servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jul 18, 2026
@mrobinson

mrobinson commented Jul 18, 2026

Copy link
Copy Markdown
Member

@amittenak47 Have you used generative AI (such as Codex or Claude Code) to write this PR or PR description?

@amittenak47

amittenak47 commented Jul 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@amittenak47 Have you used generative AI (such as Codex or Claude Code) to write this PR or PR description?

I referenced the guide here https://book.servo.org/contributing/making-a-pull-request.html#title-and-description but only to format my post description. When i made the PR, my commit message and the default template mixed, so I just tried to re-format the PR

gterzian
gterzian reviewed Jul 20, 2026
View reviewed changes
Comment thread components/script/dom/audio/audiocontext.rs Outdated
Comment thread components/script/dom/audio/audiocontext.rs Outdated
Comment thread components/script/dom/audio/baseaudiocontext.rs Outdated
Comment thread components/script/dom/audio/baseaudiocontext.rs Outdated
Comment thread components/script/dom/audio/baseaudiocontext.rs Outdated
@servo-highfive servo-highfive added the S-needs-rebase There are merge conflict errors. label Jul 20, 2026
@servo-highfive servo-highfive removed the S-needs-rebase There are merge conflict errors. label Jul 20, 2026
@amittenak47
amittenak47 requested a review from gterzian July 21, 2026 00:47
@amittenak47
amittenak47 force-pushed the issue-46600 branch 2 times, most recently from aaabeae to 8c13033 Compare July 21, 2026 06:49
jdm
jdm requested changes Jul 21, 2026
View reviewed changes
Comment thread components/script/dom/audio/audiocontext.rs Outdated
Comment thread components/script/dom/audio/audiocontext.rs Outdated
Comment thread components/script/dom/document/document.rs
Comment thread components/script/dom/document/document.rs Outdated
@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 Jul 21, 2026
@jdm

jdm commented Jul 21, 2026

Copy link
Copy Markdown
Member

You can also update the Testing section of the pull request description to read "Testing: New passing WPT tests."

@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 Jul 22, 2026
@amittenak47
amittenak47 requested a review from jdm July 22, 2026 09:27
jdm
jdm approved these changes Jul 22, 2026
View reviewed changes

@jdm jdm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks!

@servo-highfive servo-highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Jul 22, 2026
@jdm
jdm enabled auto-merge July 22, 2026 11:59
@jdm
jdm added this pull request to the merge queue Jul 22, 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 Jul 22, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 22, 2026
@servo-highfive 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 Jul 22, 2026
@mrobinson
mrobinson added this pull request to the merge queue Jul 22, 2026
@amittenak47
amittenak47 marked this pull request as draft July 31, 2026 11:40
@amittenak47
amittenak47 force-pushed the issue-46600 branch 3 times, most recently from 2f1a59f to fb04493 Compare August 7, 2026 11:25
@amittenak47
amittenak47 marked this pull request as ready for review August 7, 2026 11:46
@amittenak47

amittenak47 commented Aug 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Replaced no_script_rval: bool with CompletionValue in create_a_classic_script:

  • CompletionValue::Discarded: normal classic scripts (<script>, timers, workers) where there is no completion value at runtime
  • CompletionValue::Needed: is only required for javascript: URLs, where the completion string is required to build the navigation body https://html.spec.whatwg.org/multipage/#evaluate-a-javascript:-url
  • eval_js_url is the only call that requires Needed + Some(jsval); Everything else (htmlscriptelement, timers, service workers) uses Discarded + None.

Following @jdm feedback, updated eval_js_url to follow the classic-script path

  • Replace evaluate_js_on_global with create a classic script + run a classic script.
  • create_a_classic_script now uses a new parameter, CompletionValue (instead of something like no_script_rval: bool)
  • run_a_classic_script takes an optional rval to capture the completion value.

Reference identical optimizations from Gecko+SM (and Blink, somewhat):

Testing

Ran locally:
.\mach test-wpt webaudio\the-audio-api\the-audiocontext-interface\audiocontext-detached-execution-context.html 
.\mach test-wpt webaudio\the-audio-api\the-audiocontext-interface\promise-methods-after-discard.html 
.\mach test-wpt trusted-types\navigate-to-javascript-url-007-crash.html

jdm
jdm approved these changes Aug 12, 2026
View reviewed changes

@jdm jdm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you! This is great work, and I appreciate the effort to clean up the javascript URL evaluation.

Comment thread components/script/dom/globalscope/script_execution.rs Outdated
@servo-highfive 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 Aug 12, 2026
@servo-highfive 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 Aug 22, 2026
@jdm
jdm added this pull request to the merge queue Aug 22, 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 Aug 22, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 22, 2026
@servo-highfive 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 22, 2026
amittenak47 and others added 9 commits August 22, 2026 20:21
@amittenak47
script: Reject AudioContext promises when the document is not fully a…
6ab577e 
…ctive.

Reject decodeAudioData, resume, suspend, and close with a promise
rejected with InvalidStateError when the associated Document is not fully
active, matching the Web Audio API.

Update Document::is_fully_active so a discarded browsing context (for
example after iframe removal) is treated as not fully active. Without that,
the activity flag can still report FullyActive and the new checks never
run, leaving those promises pending until the tests time out.

Fixes servo#46600.

Signed-off-by: Amit Anilkumar <aanilkumar30@gatech.edu>
@amittenak47
script: Add detailed messages to Error::InvalidState and update docum…
520cc23 
…entation of is_fully_active

Signed-off-by: Amit Anilkumar <aanilkumar30@gatech.edu>
@amittenak47
Update WPT expectations for discarded browsing-context fully-active c…
5e0c6a4 
…hecks.

Signed-off-by: Amit Anilkumar <aanilkumar30@gatech.edu>
@amittenak47
script: Return early rather than assert can_run_script when evaluate_…
de79a03 
…js_on_global cannot run script.

Signed-off-by: Amit Anilkumar <aanilkumar30@gatech.edu>
@amittenak47
script: eval_js_url via create/run classic script; restore can_run_sc…
4e42a04 
…ript assert

Signed-off-by: Amit Anilkumar <aanilkumar30@gatech.edu>
@amittenak47
script: Add CompletionValue enum for classic-script rval handling
34a661a 
Use CompletionValue::Discarded/Needed instead of bare no_script_rval bool. Store on ClassicScript and debug_assert when rval is passed at run time.

Signed-off-by: Amit Anilkumar <aanilkumar30@gatech.edu>
@amittenak47
script: Document CompletionValue and noScriptRval mapping
99eb12d 
Signed-off-by: Amit Anilkumar <aanilkumar30@gatech.edu>
@amittenak47
style: mach fmt on CompletionValue documentation in script_execution.rs
b3b74a3 
Signed-off-by: Amit Anilkumar <aanilkumar30@gatech.edu>
@amittenak47 @jdm
Update components/script/dom/globalscope/script_execution.rs
3f93bf1 
Co-authored-by: Josh Matthews <josh@joshmatthews.net>
Signed-off-by: amittenak47 <55721729+amittenak47@users.noreply.github.com>
@servo-highfive servo-highfive removed the S-tests-failed The changes caused existing tests to fail. label Aug 23, 2026
@jdm jdm mentioned this pull request Aug 25, 2026
Merged
@amittenak47
amittenak47 marked this pull request as draft August 25, 2026 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jdm jdm jdm 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.

BaseAudioContext::DecodeAudioData is missing check for fully active document

5 participants

@amittenak47 @mrobinson @jdm @gterzian @servo-highfive