LibWeb+LibWebView: On error pages, name the URL that actually failed - #11016
Merged
gmta merged 2 commits intoAug 14, 2026
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe navigation failure path now uses the final redirected URL. Error-document loads preserve the existing view URL. Text and WebDriver tests verify error-page output, displayed URLs, and session history. ChangesFailed redirected navigation
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant BrowserTest
participant LocalNavigable
participant WebContentClient
participant SessionHistory
BrowserTest->>LocalNavigable: navigate through redirect
LocalNavigable->>WebContentClient: complete with about:error
WebContentClient->>SessionHistory: preserve failed destination URL
BrowserTest->>SessionHistory: verify displayed URL and history entries
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
sideshowbarker
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Tests/LibWebView/test-webdriver-session-history.py`:
- Around line 2253-2269: Update the wait_for_session_history predicate in the
failed redirected navigation test to require WebContent history convergence as
well as the existing UI URL match, using webContentHistoryMatchesUI or an
equivalent check that expects [url_a, url_tls_failure]. Keep the subsequent
WebContent assertion unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 965280b4-1479-4280-a6ba-1dec559358dc
📒 Files selected for processing (5)
Libraries/LibWeb/HTML/LocalNavigable.cppLibraries/LibWebView/WebContentClient.cppTests/LibWeb/Text/expected/navigation/error-page-after-failed-redirected-navigation.txtTests/LibWeb/Text/input/navigation/error-page-after-failed-redirected-navigation.htmlTests/LibWebView/test-webdriver-session-history.py
sideshowbarker
force-pushed
the
fix-error-page-url-after-redirect
branch
from
August 6, 2026 00:10
4fb0c96 to
7ffe705
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Tests/LibWeb/Text/input/navigation/error-page-after-failed-redirected-navigation.html`:
- Line 24: Bound the polling loop around dumpContains("Failed to load") with a
deadline or maximum poll count, and call the test’s failure path when the
condition is still false at the limit; preserve the existing polling and done()
flow when the error page appears.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d34acb65-46f8-43f5-b4dd-aef4739e2490
📒 Files selected for processing (5)
Libraries/LibWeb/HTML/LocalNavigable.cppLibraries/LibWebView/WebContentClient.cppTests/LibWeb/Text/expected/navigation/error-page-after-failed-redirected-navigation.txtTests/LibWeb/Text/input/navigation/error-page-after-failed-redirected-navigation.htmlTests/LibWebView/test-webdriver-session-history.py
🚧 Files skipped from review as they are similar to previous changes (4)
- Libraries/LibWebView/WebContentClient.cpp
- Libraries/LibWeb/HTML/LocalNavigable.cpp
- Tests/LibWeb/Text/expected/navigation/error-page-after-failed-redirected-navigation.txt
- Tests/LibWebView/test-webdriver-session-history.py
Problem: When a server redirects a navigation (e.g., from http to https) and the fetch of the redirect target then fails (say, with a TLS cert- verification failure), the error page names the start URL (e.g., the http URL) instead of the URL that actually failed (e.g., the https URL). Cause: populate_session_history_entry_document() moves result-> redirected_url into output->redirected_url right after it allocates output. The subsequent error-page branch then reads result-> redirected_url.value_or(url). But a moved-from Optional is always empty; so, the fallback to the pre-redirect URL is taken unconditionally and the redirect-aware code there is dead by construction. Fix: Read the redirect target from output->redirected_url (the field the transfer populated) and comment the transfer point — so readers know result’s redirect fields must not be read past it. Fixes LadybirdBrowser#11014
Problem: After a failed navigation commits the browser’s inline error page, the URL bar shows “about:error” — the internal URL of the error document — instead of the address that failed to load. The WebDriver “navigate to” flow also treats such loads as never finishing and times out — since no session history entry matches about:error. Seen in issue LadybirdBrowser#11014, where an http-to-https redirect failed TLS verification, and the URL bar ended up not naming the https URL that failed. Cause: When a document finishes loading, WebContent reports the document’s URL, and did_finish_loading() stores it as the view’s URL. Documents created for inline error content have the reserved URL about:error (per the HTML spec), so that internal URL clobbers the session-history-entry URL — the URL that failed to load, after any redirects — which the view already received when the entry committed. Fix: Treat an about:error finish like the suppressed-history case, and keep the URL the view already shows. Firefox/Chromium also never surface their internal error-document URLs (chrome-error://chromewebdata and about:neterror): Both show the URL that actually failed to load.
sideshowbarker
force-pushed
the
fix-error-page-url-after-redirect
branch
from
August 6, 2026 00:29
7ffe705 to
087d60c
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
gmta
approved these changes
Aug 14, 2026
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
2 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.
Problem: When a navigation is redirected (e.g., from http to https) and the fetch of the redirect target then fails (e.g., on a TLS cert-verification failure), we misreport which URL failed — in two places: (1) The error page names the URL the navigation started at (e.g., the http one) — rather than the one that actually failed (e.g., the https one). And (2) once the inline error page commits, the URL bar shows
about:error(the internal URL of the error document), instead of the address that failed to load. That same clobber also makes the WebDriver “navigate to” flow treat every failing load as never finishing — so it times out; no session history entry matchesabout:error.Cause: Two separate defects: (1)
populate_session_history_entry_document()movesresult->redirected_urlintooutput->redirected_urlright after it allocatesoutput; the error-page branch below then readsresult->redirected_url.value_or(url). A moved-fromOptionalis always empty, so the fallback to the pre-redirect URL is taken unconditionally, and the redirect-aware code there is dead by construction. And separately, (2) when a document finishes loading, WebContent reports that document’s URL, anddid_finish_loading()stores it as the view’s URL. Documents created for inline error content carry the reserved URLabout:error(per the HTML spec); so, that internal URL clobbers the session-history-entry URL (the URL that failed to load, after any redirects) — which the view already received when the entry committed.Fix: Read the redirect target from
output->redirected_url, the field the transfer populates, and comment the transfer point so readers knowresult’s redirect fields must not be read past it. Treat anabout:errorfinish like the suppressed-history case, and keep the URL the view already shows. Firefox and Chromium likewise never surface their internal error-document URLs (about:neterrorandchrome-error://chromewebdata): Both show the URL that actually failed to load. Fixes #11014.