constellation: prevent panic when constellation receives message from closed pipeline#39745
Merged
gterzian merged 3 commits intoOct 14, 2025
Conversation
Signed-off-by: gterzian <[email protected]>
|
🔨 Triggering try run (#18387250089) for Linux (WPT) |
|
Test results for linux-wpt from try job (#18387250089): Flaky unexpected result (34)
Stable unexpected results that are known to be intermittent (29)
Stable unexpected results (1)
|
|
|
Member
|
This is a bit odd. It should be impossible for a popup to be opened in same WebView as the opener. The popup code creates an entirely new top level browsing context which is assigned to a new WebView by the embedding layer. I think there must be something deeper going wrong. |
|
🔨 Triggering try run (#18400862229) for Linux (WPT) |
Member
Author
Defenitely, this PR only fixes the panic. The underlying problem is discussed at #39748 |
|
Test results for linux-wpt from try job (#18400862229): Flaky unexpected result (26)
Stable unexpected results that are known to be intermittent (28)
Stable unexpected results (1)
|
|
|
Member
Author
|
Looks like this one is now persistently not crashing: |
Member
Author
|
Ok I can comfirm that servo/components/constellation/constellation.rs Line 1644 in 12e5d1b which goes away with these changes. |
…e-iframe-contentwindow Signed-off-by: gterzian <[email protected]>
servo-highfive
added
the
S-awaiting-review
Taym95
reviewed
Oct 10, 2025
Comment on lines
+1763
to
+1775
| let webview_id = match self | ||
| .pipelines | ||
| .get(&source_pipeline_id) | ||
| .map(|pipeline| pipeline.webview_id) | ||
| { | ||
| None => { | ||
| return warn!( | ||
| "{}: DiscardTopLevelBrowsingContext from closed pipeline", | ||
| source_pipeline_id | ||
| ); | ||
| }, | ||
| Some(ctx) => ctx, | ||
| }; |
Member
There was a problem hiding this comment.
this looks like lot of code duplication, can we do something about it?
Member
There was a problem hiding this comment.
I recommend a helper at the top of the loop like:
let get_webview_id = || {
let webview_id = self
.pipelines
.get(&source_pipeline_id)
.map(|pipeline| pipeline.webview_id);
if webview_id.is_none() {
warn!(ScriptToConstellationMessage on closed pipeline ({source_pipeline_id})");
}
webview_id
};Then in each match arm this could be:
let Some(webview_id) = get_webview_id() else {
return;
}Signed-off-by: gterzian <[email protected]>
gterzian
force-pushed
the
prevent_panic_in_message_handling_from_closed_pipeline
branch
from
90106ff to
e618012
Compare
Member
Author
|
@mrobinson @Taym95 Thanks for the review; comments have been addressed. Note the the new TIMEOUT is dealt with over at #39833 |
jdm
approved these changes
Oct 13, 2025
servo-highfive
removed
the
S-awaiting-review
servo-highfive
added
the
S-awaiting-merge
gterzian
deleted the
prevent_panic_in_message_handling_from_closed_pipeline
branch
servo-highfive
removed
the
S-awaiting-merge
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.
As part of #31973, I noticed that the intermittent crash of
/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_escaping-3.htmlappears due to the closing of the of the source webview of the message, which prior to these changes would result in an early return and drop the reply sender, resulting in a panic.This change makes the check for whether the webview is still open specific to certain messages that need the webview and
GetBrowsingContextInfois not one of them.As to exactly what allows a webview to be closed and still send a
GetBrowsingContextInfomessage, it appears that the iframe and the pop-up opened by the iframe share the same webview, and when the pop-up callsclose, that closes the webview but the iframe is still running and then receives a post message from a webview that was already closed(intermittently). See #39748Testing: WPT
Fixes: #22647