Skip to content

Offer: GitHub Actions CI (build/test/coverage) + 9 bugs found and fixed along the way -- no obligation, cherry-pick as you like #542

Description

@Thrameos

The offer, up front

I didn't see a .github/workflows/ setup here, so while building coverage tooling
on a personal fork I put together a GitHub Actions CI setup (Linux/macOS/Windows
builds, JUnit tests, JaCoCo+LLVM coverage, CodeQL) in case it's useful. Along the
way it surfaced 9 real bugs, all fixed, verified against a live CI run with all
checks green and a processed Codecov report.

No action is required from anyone here. Everything below is in one squashed,
CI-validated commit at Thrameos#35, with the full, unsquashed commit-by-
commit history preserved at the backport/phase1-ci-infra-full-history tag in the
same fork. Whatever's useful is available: merge the PR as-is if you want the whole
thing, hand it to a volunteer to review at whatever pace works, or cherry-pick
whichever individual fixes are wanted straight out of the full-history tag and
ignore the rest. Just flagging what's there.

Enhancements

  • GitHub Actions CI: build jobs for Linux/macOS/Windows, a JUnit test job, a
    coverage job, and CodeQL analysis (C++ and Java).
  • ENABLE_LLVM_COVERAGE: a Clang source-based native coverage CMake option.
    gcov-style instrumentation was tried and rejected during development --
    its .gcda writer isn't fork-safe against CEF's zygote-style process
    model, and silently produces wrong (usually near-zero) numbers for
    whichever files a test happens to hit a fork on -- so it was never carried
    into the branch's shipped configuration.
  • JaCoCo wired into the Java JUnit run (pinned to 0.8.15 for JDK 25 class-file
    support -- 0.8.12 can't parse it).
  • A CI wrapper (tools/run_tests_ci.sh / tools/run_coverage_ci.sh) that tells
    a real, reportable JUnit test failure apart from a known, already-diagnosed
    native crash: a real failure is never silently retried, while a known crash
    signature is retried within a bounded budget, or accepted as passing once
    it's been verified that the crash doesn't lose any test or coverage data.
    The point is to avoid two opposite failure modes -- a flaky job that's
    ignored because it's "always red," and a job that looks green by quietly
    swallowing real failures.
  • A leak-checker / process-isolation test harness (LeakChecker, LeakTargets,
    IsolatedRunner) for RSS-growth-based leak sweeps, plus roughly 25 tests ported
    from CEF's own ceftests suite and other coverage work (value objects, browser/
    context/request/response/download-item coverage, OSR/windowed smoke tests, drag-
    data, cookies, message router, print/PDF settings, etc.).
  • Current baseline on this branch: ~38% combined coverage.

Bugs found and fixed

  1. CefMessageRouter leak on router removal. RemoveMessageRouter() never
    called CancelPending() on outstanding persistent-query callbacks, which hold
    a CefRefPtr back to the router. If a browser is removed mid-query rather than
    actually closing, nothing else ever releases that reference -- the router, and
    transitively the browser context it keeps alive, leaks for the process's life.
    This looks like the same issue Preserve persistent query callbacks #541 and Support persistent & binary queries/callbacks #528 are addressing.
  2. CefRequestContext double-caching leak. getGlobalContextNative() was
    missing a final else, so every call after the first cached (and leaked) a
    second native reference.
  3. Global CefRequestContext/CefCookieManager never disposed at shutdown.
    The cached global instance outlived CefShutdown(), tripping
    DCHECK(all_.empty()) in browser_context.cc's exit-time leak detector
    (Debug builds only -- invisible in Release, since that check is compiled out
    there).
  4. CefClient.onGotFocus() synchronous infinite recursion. setFocus(true)
    synchronously re-fires OnWebContentsFocused -> onGotFocus() again for the
    same browser; CEF's own OnSetFocus() guards itself against this kind of
    reentrancy but OnGotFocus/OnWebContentsFocused has no equivalent guard. An
    already-focused browser recurses until the thread's stack overflows.
  5. ScopedJNIObject<T> mixed locked/unlocked accessors. A real SIGSEGV during
    ordinary handler-removal teardown, plus a related check-then-create race in
    GetOrCreateCefObject().
  6. Two browser-close hangs. OSR mode never fires OnBeforeClose for a browser
    whose renderer already died (e.g. after a deliberate chrome://crash);
    windowed mode had no fallback for the equivalent case either. Both now get a
    bounded fallback instead of hanging indefinitely.
  7. doMessageLoopWork's self-perpetuating Timer outliving native shutdown.
    An independently-scheduled Timer tick (up to ~33ms out) can still fire after
    N_Shutdown() has already destroyed the native Context singleton --
    harmless in Release, a real SIGSEGV in Debug builds where the resulting null-
    this DCHECK actually runs.
  8. CefClientHandler handler-removal methods used the abstract interface type
    instead of the concrete wrapper type.
    Follow-up to the ScopedJNIObject
    crash above.
  9. Two smaller fixes: a null-guard gap causing Debug-only DCHECK aborts in
    several _N.cpp setters (CefRequest/CefResponse/CefPostDataElement and
    others), and a CodeQL medium-severity finding (a world-readable temp file on
    POSIX).

Public API notes

Two new public static methods were added, both purely additive -- no existing
public signature was changed, removed, or had its contract broken:

  • CefRequestContext.disposeGlobalContext()
  • CefCookieManager.disposeGlobalManager()

Both are documented as internal-use-only (called from CefApp's shutdown sequence
to release the cached global instance before native CEF shutdown runs), but are
callable by any embedding app since they're public. Nothing else in the public
org.cef surface changed.

Credits

This work was done with substantial AI assistance (Claude, Anthropic) under my
direction and review -- disclosing that up front since it materially shaped how
this was produced.

Bug #5 above (ScopedJNIObject<T> mixed locked/unlocked accessors) was found by
diffing this project's teardown code against JetBrains/jcef (branch 261,
CEF 137), which independently added a locking mechanism around the same native-
pointer accessor call sites with a comment describing the identical crash
signature we hit. The fix here is adapted from that fork's approach.

Known limitation

The underlying browser_context.cc:44 DCHECK(all_.empty()) shutdown-time leak
detector is not fully fixed by #2/#3 above -- it still fires via at least one
further mechanism, confirmed independent of reference counting (ADDREF/RELEASE and
JNI ref counts both balance perfectly when it fires). The coverage CI job
recognizes this specific, well-understood signature and tolerates it rather than
treating it as a fresh failure, since it only ever fires after every test in a run
has already completed. Root-causing it further is still open.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions