Repository navigation
Fix CefDownloadHandler.onBeforeDownload's Chrome-style cancel trap - #48
Merged
Thrameos merged 2 commits intoSep 6, 2026
Merged
Conversation
… contract Root-caused during task 26 (see plan/tasks/20260905-26-download-shelf-check-crash.md): this project runs CEF's Chrome-style runtime, so returning false from onBeforeDownload does NOT cancel the download -- it falls through to Chrome's own download-shelf default handling, which this project's embedding does not implement. If the browser that started the download is torn down before that deferred handling runs, CEF's own ChooseDownloadPath dereferences an already-gone callback and crashes the process with an internal CHECK failure. The only safe way to cancel is to return true and drop |callback| un-run. This bug pattern was found in a future-branch-only coverage test (CefBrowserApiGapCoverageTest, not present on this branch) and doesn't exist anywhere in this branch's current onBeforeDownload implementations -- there's no code-level regression to fix here. What's worth backporting on its own is protecting this branch's future users/tests from the same trap: a javadoc warning on the actual contract, plus a regression test that locks in the documented-safe cancel path (return true, drop the callback) actually completes cleanly with no crash and writes no target file. Deliberately does not attempt to reproduce the crash itself -- it's inherently timing-dependent on browser teardown racing a posted UI-thread task, and a real process crash is not something to leave as a routine CI test. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01HBFmQs6JDypYP5qdC99yDq
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #48 +/- ##
============================================
+ Coverage 38.01% 38.14% +0.13%
- Complexity 735 736 +1
============================================
Files 243 243
Lines 14863 14906 +43
Branches 2449 2453 +4
============================================
+ Hits 5650 5686 +36
Misses 8011 8011
- Partials 1202 1209 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Thrameos
commented
Sep 6, 2026
Comment on lines
+24
to
+36
| * <p><b>Warning:</b> this project runs CEF's Chrome-style runtime, so | ||
| * returning false here does <i>not</i> cancel the download -- it falls | ||
| * through to Chrome's own download-shelf default handling, which this | ||
| * project's embedding does not implement. That default handling posts a | ||
| * task to the UI thread asynchronously, and if the browser that started | ||
| * the download is torn down before that task runs (a common case if a | ||
| * test or app closes the browser right after starting the download), | ||
| * the underlying CEF implementation dereferences an already-gone | ||
| * download-target callback and crashes the process with an internal | ||
| * {@code CHECK} failure. To actually cancel, return true and either | ||
| * never call {@code callback.Continue(...)}, or call it with an empty | ||
| * path. | ||
| * |
Owner
Author
There was a problem hiding this comment.
Should we implement the missing feature rather than documenting the defect?
…r real
Per review feedback on the earlier doc-only PR ("should we implement the
missing feature rather than documenting the defect?"): implementing CEF's
actual Chrome-style download shelf isn't viable here (it's an internal
Chrome-UI subsystem with no public hook for embedders, and this project's
Java/AWT/OSR embedding doesn't use CEF's Views UI framework it would need
anyway) -- but the underlying ask, "make returning false actually work as
a cancel", is fixable at the JNI boundary.
native/download_handler.cpp's OnBeforeDownload now always returns true to
CEF and treats a false return from Java's onBeforeDownload as "drop the
callback un-run" -- the same already-safe cancel path a true+drop-callback
implementation already used, just applied uniformly instead of leaving a
plain `return false` as a latent crash trap (see
plan/tasks/20260905-26-download-shelf-check-crash.md for the original
root-cause: CEF's own Chrome-style default handling for false posts a
UI-thread task that can crash with an internal CHECK if the browser is
torn down first).
Updated CefDownloadHandler.onBeforeDownload's javadoc to describe the new,
simpler contract (false always safely cancels, no runtime-style caveat).
Replaced the doc-PR's regression test with one that exercises the actual
fix (returns false directly, asserts no crash and no file written), and
kept a second test covering the older true+drop-callback path.
Co-Authored-By: Claude Sonnet 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01HBFmQs6JDypYP5qdC99yDq
Thrameos
added
bug
Thrameos
changed the title
Owner
Author
|
Re the review comment on |
Thrameos
deleted the
backport/download-handler-return-false-crash-contract
branch
September 6, 2026 03:06
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
1 participant
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.
Summary
falsefromonBeforeDownloaddid not cancel the download -- it fell through to Chrome's own download-shelf default handling, which this project's embedding does not implement. If the browser that started the download was torn down before that deferred handling ran, CEF's ownChooseDownloadPathdereferenced an already-gone callback and crashed the process with an internalCHECKfailure. Root-caused via gdb while doing coverage work on a separate branch (seeplan/tasks/20260905-26-download-shelf-check-crash.mdfor the full backtrace and CEF source citations).falseactually work as a cancel" -- is fixable at the JNI boundary:native/download_handler.cpp'sOnBeforeDownloadnow always returnstrueto CEF and treats afalsereturn from Java'sonBeforeDownloadas "drop the callback un-run", the same already-safe cancel path atrue+drop-callback implementation already used, just applied uniformly instead of leaving a plainreturn falseas a latent crash trap.CefDownloadHandler.onBeforeDownload's javadoc for the new, simpler contract:falsenow always safely cancels, no runtime-style caveat needed.rejectingDownloadByReturningFalseDoesNotCrash-- returnsfalsedirectly, asserts no crash and no file written), and kept a second test covering the oldertrue+drop-callback path (rejectingDownloadByReturningTrueAndDroppingCallbackDoesNotCrash).Test plan
ninja -C jcef_build jcefunder-Werror+tools/compile.sh linux64)CefDownloadItemTestregression tests pass; full local suite 125/125 passingAborted (core dumped)after the JUnit summary is the already-tracked, pre-existing GH Release build: SIGSEGV in libc.so.6 during JVM shutdown after all tests pass #10 shutdown SIGSEGV, unrelated to this change🤖 Generated with Claude Code
https://claude.ai/code/session_01HBFmQs6JDypYP5qdC99yDq