Skip to content

script: Properly handle messaging errors in components/constellation/constellation.rs#41419

Merged
mrobinson merged 5 commits intoservo:mainfrom
spider-rs:background-hang-panic-fix
Jan 5, 2026
Merged

script: Properly handle messaging errors in components/constellation/constellation.rs#41419
mrobinson merged 5 commits intoservo:mainfrom
spider-rs:background-hang-panic-fix

Conversation

@j-mendez
Copy link
Copy Markdown
Contributor

@j-mendez j-mendez commented Dec 20, 2025

Fix panic on background_monitor_hang send, allowing graceful cleanups.

Url example https://cosihome.com/products/cooling-blanket/luxury-cooling-blanket-multi-season-lightweight-soft/ - causes many redirects.

thread 'Script#4' panicked at /app/third_party/servo/components/script/dom/history.rs:357:24:
Unknown top-level browsing context`
thread 'Script#5' panicked at /app/third_party/servo/components/background_hang_monitor/background_hang_monitor.rs:138:14

@j-mendez j-mendez requested a review from gterzian as a code owner December 20, 2025 04:25
@servo-highfive servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Dec 20, 2025
TimvdLippe
TimvdLippe previously requested changes Dec 20, 2025
View reviewed changes
Copy link
Copy Markdown
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.

Please run ./mach fmt and add a regression test based on the site that you linked.

@servo-highfive servo-highfive added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. S-awaiting-review There is new code that needs to be reviewed. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Dec 20, 2025
@j-mendez j-mendez requested a review from TimvdLippe December 20, 2025 14:27
@mrobinson mrobinson changed the title chore(background_hang_monitor): fix panic send script: Properly handle messaging errors in components/constellation/constellation.rs Dec 20, 2025
mrobinson
mrobinson requested changes Dec 20, 2025
View reviewed changes
@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 Dec 20, 2025
mrobinson
mrobinson requested changes Dec 20, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@mrobinson mrobinson left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Let's stick with gracefully handling closed channels in components/script/dom/history.rs. I'm really not convinced by the changes to the background hang monitor and without a description of what is going wrong I can't tell if they are right or not. Gracefully handling the closed channels looks obviously correct to me, so please just maintain that in this PR.

@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 Dec 20, 2025
@j-mendez j-mendez requested a review from mrobinson December 20, 2025 22:21
@servo-highfive servo-highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Dec 21, 2025
@mrobinson
Copy link
Copy Markdown
Member

mrobinson commented Dec 21, 2025

@j-mendez Look good. Please sign your commits and then this can land properly: https://github.com/servo/servo/pull/41419/checks?check_run_id=58622941490

@mrobinson mrobinson enabled auto-merge December 21, 2025 11:01
auto-merge was automatically disabled December 21, 2025 12:15

Head branch was pushed to by a user without write access

@j-mendez j-mendez force-pushed the background-hang-panic-fix branch from 703ab71 to 6b94532 Compare December 21, 2025 12:15
@servo-highfive servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Dec 21, 2025
@j-mendez
Copy link
Copy Markdown
Contributor Author

j-mendez commented Dec 21, 2025

@j-mendez Look good. Please sign your commits and then this can land properly: https://github.com/servo/servo/pull/41419/checks?check_run_id=58622941490

Done, thank you.

@jdm jdm enabled auto-merge December 21, 2025 13:00
auto-merge was automatically disabled December 21, 2025 14:15

Head branch was pushed to by a user without write access

@j-mendez j-mendez force-pushed the background-hang-panic-fix branch 2 times, most recently from a5088d2 to d31ee2a Compare December 21, 2025 14:19
@j-mendez j-mendez force-pushed the background-hang-panic-fix branch 2 times, most recently from b659779 to 1981bbd Compare December 21, 2025 14:22
@j-mendez
Copy link
Copy Markdown
Contributor Author

j-mendez commented Dec 29, 2025

@mrobinson the PR might get dropped from the fork, is there any changes needed to merge?

@mrobinson mrobinson enabled auto-merge December 29, 2025 14:13
@servo-wpt-sync
Copy link
Copy Markdown
Collaborator

servo-wpt-sync commented Jan 5, 2026

🛠 These changes could not be applied onto the latest upstream WPT. Servo's copy of the Web Platform Tests may be out of sync.

@mrobinson
Copy link
Copy Markdown
Member

mrobinson commented Jan 5, 2026

@j-mendez I've sent this back to the merge queue. Note that if you push to the branch again, GitHub will automatically remove the change from the merge queue.

@TimvdLippe TimvdLippe dismissed their stale review January 5, 2026 15:22

No longer relevant

@mrobinson mrobinson added this pull request to the merge queue Jan 5, 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 Jan 5, 2026
Merged via the queue into servo:main with commit ad6c17d Jan 5, 2026
29 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 Jan 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mrobinson mrobinson mrobinson approved these changes

@gterzian gterzian Awaiting requested review from gterzian gterzian is a code owner

@TimvdLippe TimvdLippe Awaiting requested review from TimvdLippe

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.

5 participants

@j-mendez @mrobinson @servo-wpt-sync @TimvdLippe @servo-highfive