Skip to content

script: Mark document as unsalvageable if unclosed websockets present.#39940

Merged
yezhizhen merged 1 commit intoservo:mainfrom
jdm:ws-unsalvageable
Oct 18, 2025
Merged

script: Mark document as unsalvageable if unclosed websockets present.#39940
yezhizhen merged 1 commit intoservo:mainfrom
jdm:ws-unsalvageable

Conversation

@jdm
Copy link
Member

@jdm jdm commented Oct 17, 2025

Our implementation of https://html.spec.whatwg.org/multipage/#unloading-document-cleanup-steps is incomplete and doesn't consider the existence of websockets. This change corrects that, and adds a little bit more structure to the current document unloading implementation.

Testing: New passing tests.
Fixes: #39872

Copy link
Contributor

@TimvdLippe TimvdLippe left a comment

Choose a reason for hiding this comment

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

LGTM with 2 nits

@servo-highfive servo-highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Oct 17, 2025
@jdm jdm force-pushed the ws-unsalvageable branch from e8a4d19 to e942ee3 Compare October 17, 2025 06:57
@servo-highfive servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 17, 2025
@jdm jdm enabled auto-merge October 17, 2025 06:57
@jdm jdm added this pull request to the merge queue Oct 17, 2025
@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 Oct 17, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 17, 2025
@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 Oct 17, 2025
Signed-off-by: Josh Matthews <josh@joshmatthews.net>
@jdm jdm force-pushed the ws-unsalvageable branch from e942ee3 to ae54fc8 Compare October 17, 2025 18:04
@servo-highfive servo-highfive removed the S-tests-failed The changes caused existing tests to fail. label Oct 17, 2025
@jdm jdm enabled auto-merge October 17, 2025 18:04
@jdm jdm added this pull request to the merge queue Oct 17, 2025
@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 Oct 17, 2025
@yezhizhen
Copy link
Member

yezhizhen commented Oct 18, 2025

Requeue this and #36616 to make sure #39968 merge first to improve entire CI speed.. None of tasks have started anyway, nothing wasted.

image

@yezhizhen yezhizhen removed this pull request from the merge queue due to a manual request Oct 18, 2025
@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 Oct 18, 2025
@yezhizhen yezhizhen added this pull request to the merge queue Oct 18, 2025
@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 Oct 18, 2025
Merged via the queue into servo:main with commit 94bcef5 Oct 18, 2025
33 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 Oct 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Websockets do not prevent bfcache

4 participants