Intermittent SIGSEGV in SetCefForJNIObjectHelper::Release during normal CefClient teardown (Debug build) #22

Activity

1. Thrameos commented on Aug 30, 2026

   

   Thrameos

   on Aug 30, 2026

   OwnerAuthor

   More actions

   Found a strong, evidence-backed root-cause candidate by diffing against JetBrains' own jcef fork (branch 261, CEF 137 -- 9 versions behind ours but architecturally very close, ~154 shared native/ filenames). They independently hit and fixed exactly this class of bug: every Dispose()/native-pointer-clear call site was migrated to a lock-protected accessor pair (lockAndGetNativeRef()/unlock() on the Java side, SetCefForJNIObject_sync/GetCefFromJNIObject_sync on the native side), with an explicit comment matching our crash: "must do it after setNativeRef_safe (otherwise we can create CefRefPtr with killed ptr)".

   Applied to this fork:

   1. Reordered SetCefForJNIObject() (jni_scoped_helpers.h) to update the Java-side pointer before releasing the previous object, not after -- closes a window where a concurrent reader could get a stale, already-released pointer. Applies to all ~150 existing call sites automatically.
   2. Marked the native handle field volatile in CefNativeAdapter/CefRequestContext_N -- a non-volatile 64-bit field read/write isn't guaranteed atomic per JLS 17.7, so a concurrent reader could observe a torn (half-old/half-new) value. This matches your crash signature exactly: a SIGSEGV dereferencing pointer 0x21, which looks exactly like a torn/garbage value rather than a real freed pointer.
   3. Added the full lock-based _sync variants and rolled them out to CefClientHandler.cpp (the exact function in this issue's stack trace, Java_org_cef_handler_CefClientHandler_N_1removeFocusHandler) and CefRequestContext_N.cpp.

   The SIGSEGV did not reproduce across several repeated Debug/ENABLE_COVERAGE full-suite runs after these fixes (previously intermittent). Given the intermittent nature I can't claim it's provably eliminated, but the root cause is now well understood and directly addressed, not just papered over. Full Release suite: 164/164 passing, no regressions.
2. Thrameos

   added a commit that references this issue on Aug 30, 2026

   Mine JetBrains' jcef fork for teardown-race fixes (issues #22/#23)

   ...

   cf73c06
3. Thrameos commented on Aug 30, 2026

   

   Thrameos

   on Aug 30, 2026

   OwnerAuthor

   More actions

   Correction to my earlier comment: this SIGSEGV did reproduce again just now, once, during an otherwise-unrelated coverage run (mid-suite this time, not at shutdown) -- exact same signature (`SIGSEGV at pc=0x21` in `SetCefForJNIObjectHelper::Release`). I should not have implied it was resolved based on a handful of clean runs; this bug is intermittent by nature (as originally documented) and absence of a crash in N runs isn't proof.

   The fixes already applied (reordered `SetCefForJNIObject`, volatile handle fields, scoped `_sync` locking for `CefClientHandler`/`CefRequestContext`) are real, correct, and address a genuine race -- but evidently not the only remaining source of this crash class, since it's now confirmed to fire from a code path that goes through the same `SetCefForJNIObjectHelper::Release` regardless. The _sync rollout so far only covers CefClientHandler.cpp and CefRequestContext_N.cpp -- there are ~150 other call sites still on the plain (non-locking) SetCefForJNIObject/GetCefFromJNIObject, any of which could be the one actually crashing here. Broader rollout (or at minimum, capturing an hs_err_pid*.log stack trace from the next reproduction to identify which specific handler type is involved this time) is the natural next step -- not closing this out yet.
4. Thrameos commented on Aug 30, 2026

   

   Thrameos

   on Aug 30, 2026

   OwnerAuthor

   More actions

   Broadened the fix scope significantly (following up on the earlier CefClientHandler/CefRequestContext-only scope): the _sync locking mechanism (volatile field + ReentrantLock + lockAndGetNativeRef()/unlock()) is now applied to essentially every class implementing CefNative, and every SetCefForJNIObject<T>/GetCefFromJNIObject<T> disposal/creation call site across ~26 native files -- including jni_util.cpp's GetJNIBrowser() (probably the single highest-traffic call site of this kind, used throughout the whole codebase) and life_span_handler.cpp's browser creation/close lifecycle.

   Honest result: the SIGSEGV still reproduced once in 4 repeated Debug/ENABLE_COVERAGE runs after this broader rollout. Reduced exposure, not eliminated.

   A plausible reason this class of fix has a ceiling: most of JCEF's value-object classes (CefRequest, CefResponse, CefPostData, etc.) don't actually use the GetCefFromJNIObject JNI-callback pattern that the lock protects for their per-instance getters/setters -- they use a different pattern where the Java side reads its own long handle field and passes it directly as a jlong self parameter to each native method (e.g. N_GetURL(N_CefHandle)). For that pattern, the race window is between the Java-side field read and the native call actually using that value -- two separate, non-atomic steps with real (if small) time between them for JNI transition overhead. Locking around create/dispose alone doesn't close that window; it would need either a lock held across every individual native call (a much larger, more invasive change touching every one of ~150+ methods) or -- and this may be the more fundamental fix -- moving off the deprecated finalize()-based cleanup all 30-ish of these classes currently use, toward a PhantomReference/java.lang.ref.Cleaner-based approach that can guarantee an object is never finalized while a native call could still be referencing it. finalize() runs on the JVM's own schedule with no coordination with in-flight native use at all, which is a weaker guarantee than what these value objects likely need.

   Not resolved. Documented as the concrete next-level lead for whoever picks this up -- the fix already applied is a real improvement worth keeping regardless.
5. Thrameos

   added a commit that references this issue on Aug 30, 2026

   Broadly roll out the _sync locking mechanism (issue #22) beyond the i…

   ...

   1ee58d2
6. Thrameos commented on Aug 30, 2026

   

   Thrameos

   on Aug 30, 2026

   OwnerAuthor

   More actions

   Root-caused with a live symbolized stack trace (commit 1df1b4f)

   This time the SIGSEGV hit during this session's own coverage-measurement run and left behind a hs_err_pid*.log with a full native stack trace:

   SetCefForJNIObjectHelper::Release(CefBaseRefCounted*)
   SetCefForJNIObject_sync<CefFocusHandler>(...)
   Java_org_cef_handler_CefClientHandler_N_1removeFocusHandler
     <- CefClientHandler.removeFocusHandler()
     <- CefClient.cleanupBrowser()
     <- CefClient.onBeforeClose()
     <- LifeSpanHandler::OnBeforeClose()
     <- ... deep inside WebContentsImpl::~WebContentsImpl() / WebContentsDestroyed()
   

   i.e. this happens during ordinary, clean browser teardown — not a rare edge case.

   Root cause: CefClientHandler.cpp's remove*Handler functions were converted to the lock-protected SetCefForJNIObject_sync()/GetCefFromJNIObject_sync() in an earlier fix pass, but ScopedJNIObject<T> — used by ClientHandler::GetHandler<T>() to lazily create and cache the native wrapper object the first time any Cef*Handler getter (GetFocusHandler(), GetContextMenuHandler(), etc.) is called — still used the plain, non-locking SetCefForJNIObject()/GetCefFromJNIObject() on that exact same underlying native-ref field.

   Mixing a locked writer with an unlocked reader/writer on the same field gives neither side any real protection — the unlocked side never even attempts to acquire the lock the other side relies on. Deep WebContents teardown is reentrant enough (destruction notifies multiple observers, any of which can synchronously query GetFocusHandler() again) for this race to be reachable on a single thread, without needing true cross-thread concurrency.

   Fix: ScopedJNIObject<T>'s constructor, destructor, and GetOrCreateCefObject() now route through the _sync accessors whenever PtrT is the default CefRefPtr<T> (every instantiation except CefSchemeRegistrar, a CefBaseScoped type structurally incompatible with GetCefFromJNIObject_sync()'s CefRefPtr<T> return type — handled via if constexpr, no call-site changes needed). This closes the gap for every Cef*Handler type at once, not just CefFocusHandler.

   Verification: 5/5 repeated full Release-suite runs clean (169/169 passing, zero SIGSEGV mentions) — previously this reproduced within about 4 runs. Debug/coverage-build stress runs still hit the already-documented, unrelated issue #23 DCHECK (browser_context.cc:44, all_.empty()) plus a scoped_refptr.h:292 DCHECK failed: ptr_. in what looks like a child/helper process — confirmed both already appear in this project's saved logs from sessions well before today's fix, so they're pre-existing and belong to #23, not a regression from this change.
7. Thrameos

   added a commit that references this issue on Aug 30, 2026

   Fix issue #22 SIGSEGV root cause: ScopedJNIObject<T> mixed locked/unl…

   ...

   1df1b4f
8. Thrameos commented on Aug 30, 2026

   

   Thrameos

   on Aug 30, 2026

   OwnerAuthor

   More actions

   Fixed in 78d135f. Root cause: every remove*Handler() in native/CefClientHandler.cpp except removeWindowHandler called SetCefForJNIObject_sync<CefXxxHandler> using the abstract CEF interface type, while the corresponding ClientHandler::GetXxxHandler() (native/client_handler.cpp) stores the object via ScopedJNIObject<XxxHandler> using the concrete wrapper type -- a reinterpret_cast<T*> of the same stored pointer under two different static types on either side of the same field. removeWindowHandler was already written correctly (using the concrete WindowHandler type); every other one wasn't, an apparent copy-paste inconsistency rather than an intentional choice.

   Verified by reproducing the exact SIGSEGV live (fresh clean rebuild, same crash offset every time -- ruling out a stale build), fixing all 12 remove*Handler() functions to match removeWindowHandler's pattern, then reproducing the identical repro post-fix: clean run through to shutdown, no SIGSEGV, only the separate, already-tracked #23 DCHECK at the very end.
9. Thrameos

   closed this as completedon Aug 30, 2026
10. Thrameos

    added 13 commits that reference this issue on Aug 31, 2026

    Fix issue #22 follow-up: CefClientHandler.cpp remove*Handler used abs…

    ...

    78d135f

    Add permanent JCEF_TRACE ref/lifecycle tracing facility

    ...

    d014dac

    Partial fix for issue #22: two real double-release causes found

    ...

    7290b2e

    Close a check-then-create race in ScopedJNIObject<T>::GetOrCreateCefO…

    ...

    6ffeb72

    Mine JetBrains' jcef fork for teardown-race fixes (issues #22/#23)

    ...

    959ac23

    Fix issue #22 SIGSEGV root cause: ScopedJNIObject<T> mixed locked/unl…

    ...

    d94f31c

    Fix issue #22 follow-up: CefClientHandler.cpp remove*Handler used abs…

    ...

    ef24b3a

    Close a check-then-create race in ScopedJNIObject<T>::GetOrCreateCefO…

    ...

    95bbe11

    Broadly roll out the _sync locking mechanism (issue #22) beyond the i…

    ...

    9246c01

    Add GitHub Actions CI (build/test/coverage), JaCoCo+LLVM coverage ins…

    ...

    e0fbcef

    Add GitHub Actions CI (build/test/coverage), JaCoCo+LLVM coverage ins…

    ...

    50bc48b

    Fix two real double-release causes in CefClient/render_handler

    ...

    2a721f4

    Merge pull request #42 from Thrameos/backport/cleanupbrowser-double-r…

    ...

    d9acc85