script: when handling page headers, stop if pipeline closed already #38739
+102
−44
Conversation
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
b1d764f to
fbb173b
Compare
|
🔨 Triggering try run (#17024108843) for Linux (WPT) |
|
Test results for linux-wpt from try job (#17024108843): Flaky unexpected result (25)
Stable unexpected results that are known to be intermittent (22)
Stable unexpected results (3)
|
|
|
jdm
approved these changes
Aug 17, 2025
tests/wpt/mozilla/meta/MANIFEST.json
Outdated
| "192c7235d061b439ef2b57d4b01b170b7412dcdc", | ||
| [] | ||
| ], | ||
| "navigate_loop_crash.html": [ |
There was a problem hiding this comment.
Name this navigate_loop-crash.html: https://web-platform-tests.org/writing-tests/crashtest.html#crashtest-tests
Taym95
reviewed
Aug 17, 2025
| }; | ||
| .position(|load| load.pipeline_id == *id) | ||
| else { | ||
| unreachable!("Pipeline shouldn't have finished loading."); |
There was a problem hiding this comment.
This assumes 'if not closed, then we must always find the load" Is that always the case? I can’t think of any case, but I’m just wondering.
There was a problem hiding this comment.
Yes I'm trying to maintain the same logic as before, but actually first check whether the pipeline is closed. The previous code would assert the pipeline was closed if no load was found, so now I assert a load is found, if the pipeline is not closed.
| .pipeline_to_constellation_sender | ||
| .send((*id, ScriptToConstellationMessage::AbortLoadUrl)) | ||
| .unwrap(); | ||
| return None; |
There was a problem hiding this comment.
I am not familiar with this code but just from reading, in this is early return you are not removing entry from incomplete_loads ?
There was a problem hiding this comment.
Yes that's a good point on the existing code(only moved around here), which I don't want to change too much in this PR. Would be good to file an issue to investigate.
components/script/script_thread.rs
Outdated
| }; | ||
| // https://html.spec.whatwg.org/multipage/#process-a-navigate-response | ||
| // 2. If response's status is 204 or 205, then abort these steps. | ||
| let is20x = match metadata { |
There was a problem hiding this comment.
Suggested change
| let is20x = match metadata { | |
| let is_204_205 = match metadata { |
?
|
Since #38623 touches the same tests, I will rebase that PR after this one merges. Let's see if the tests start passing then 🤞 |
gterzian
commented
Aug 18, 2025
|
|
||
|
|
||
| [trusted-types-navigation.html?01-05] | ||
| expected: CRASH |
There was a problem hiding this comment.
So those crashes would indeed hit the "pipeline is closed, but there is still a pending load" branch, although the crash was at 0:09.87 pid:2652 called Option::unwrap() on a None value (thread Script(1,1), at components/script/links.rs:386)
|
@jdm @Taym95 thank you for your reviews. @TimvdLippe This should merge soon... |
|
|
🔨 Triggering try run (#17067129607) for Linux (WPT) |
|
Test results for linux-wpt from try job (#17067129607): Flaky unexpected result (26)
Stable unexpected results that are known to be intermittent (19)
Stable unexpected results (1)
|
|
|
|
Actually it is expected that the test times out because of the infinite navigation loop it creates, which is addressed at #38676 |
Signed-off-by: gterzian <2792687+gterzian@users.noreply.github.com>
Signed-off-by: gterzian <2792687+gterzian@users.noreply.github.com>
Signed-off-by: gterzian <2792687+gterzian@users.noreply.github.com>
Signed-off-by: gterzian <2792687+gterzian@users.noreply.github.com>
cd36a83 to
f794167
Compare
Signed-off-by: gterzian <2792687+gterzian@users.noreply.github.com>
|
One more change: removed the styling of the form, which would cause the timeout in the latest run. I was wrong it was not the iframe navigation(which is not infinite, since it does not trigger a load even on the main window). |
|
Test results for linux-wpt from try job (#17206535763): Flaky unexpected result (26)
Stable unexpected results that are known to be intermittent (26)
Stable unexpected results (1)
|
|
🔨 Triggering try run (#17207247416) for Linux (WPT) |
Signed-off-by: gterzian <2792687+gterzian@users.noreply.github.com>
|
So since the intermittent timeout came from the form styling, that has been removed, however for good measure I have re-added the iframe part, since that didn't trigger the timeout and to stay as close as possible to the crash test in the issue. I have also filed #38910 to investigate that timeout caused by the styling. |
|
🔨 Triggering try run (#17207427178) for Linux (WPT) |
Signed-off-by: gterzian <2792687+gterzian@users.noreply.github.com>
|
Test results for linux-wpt from try job (#17207427178): Flaky unexpected result (27)
Stable unexpected results that are known to be intermittent (27)
|
|
✨ Try run (#17207427178) succeeded. |
|
🔨 Triggering try run (#17208450578) for Linux (WPT) |
|
Running it one more time just to be sure... |
|
Test results for linux-wpt from try job (#17208450578): Flaky unexpected result (19)
Stable unexpected results that are known to be intermittent (29)
|
|
|
|
Re-trying the merge, failure was #38920 |
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.
Previously, the script-thread would assert the pipeline was closed if no pending load was found, but it did not check whether the pipeline was closed before processing the page headers. Since incomplete loads are removed only when page headers are processed, this means the page headers were processed even if the pipeline had been closed before the page headers were available. If the pipeline had been closed as part of exiting the constellation, it was possible for the constellation to have exited by the time the page headers became available(since the script-thread closes a pipeline independently from ongoing navigation fetches), which would produce a panic on trying to communicate with the constellation to obtain the browsing context info.
Note: due to the nature of the problem, I cannot verify that this fixes the crash test, although logically this appears to make sense, and a couple of days of WPT runs should tell us more.
Testing: A crash test was added; unfortunately the crash was intermittent.
Fixes: #36747