Skip to content

LibRequests: Defer teardown before finish callbacks - #11218

Open
Lubrsi wants to merge 1 commit into
LadybirdBrowser:masterfrom
Lubrsi:download-crash-fx
Open

LibRequests: Defer teardown before finish callbacks#11218
Lubrsi wants to merge 1 commit into
LadybirdBrowser:masterfrom
Lubrsi:download-crash-fx

Conversation

@Lubrsi

@Lubrsi Lubrsi commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Fixes a crash I encountered while downloading a file.

@coderabbitai

coderabbitai Bot commented Aug 19, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 72486a1b-f1fe-472f-8d91-ac9411a45658

📥 Commits

Reviewing files that changed from the base of the PR and between ee72585 and eba49d2.

📒 Files selected for processing (2)
  • Libraries/LibRequests/Request.cpp
  • Tests/LibWebView/TestFileDownloader.cpp

Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

Request::set_up_internal_stream_data now defers request teardown before it invokes the finish callback after response completion or EOF. TestFileDownloader adds coverage for releasing the final external request reference from the finish callback while response-body delivery is paused. The test resumes delivery and verifies that the full 4 KiB non-range response is received.

Possibly related PRs

Suggested reviewers: alimpfard

Merge Risk: ⚪ Minimal · up to eba49

The change is localized to request teardown timing and its related test coverage; no actionable merge-blocking risk remains after normal checks and review.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description check ✅ Passed The description identifies a download-related crash, which matches the teardown change and downloader regression test.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the conflicts Pull request has merge conflicts that need resolution label Aug 28, 2026
@github-actions

Copy link
Copy Markdown

Your pull request has conflicts that need to be resolved before it can be reviewed and merged. Make sure to rebase your branch on top of the latest master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

conflicts Pull request has merge conflicts that need resolution

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant