Unify thread-assertion macros on CEF_REQUIRE_* and retire src/common/util.h

[linesight]

The codebase mixes two spellings of the same assertion: REQUIRE_UI_THREAD from src/common/util.h, and CEF_REQUIRE_UI_THREAD from the vendored include/wrapper/cef_helpers.h. Same meaning, two names — worth settling on one.

I'd suggest converging on the CEF_REQUIRE_* form rather than the bare one:

• src/common/util.h is a stale verbatim copy of the old cef/tests/cefclient/util.h (the guard is still CEF_TESTS_CEFCLIENT_UTIL_H_); upstream replaced it with cef_helpers.h. cef_helpers.h ships with CEF and stays current.
• util.h has already rotted: REQUIRE_FILE_THREAD() references TID_FILE, removed from CEF years ago. It survives because ASSERT under NDEBUG expands to ((void)0) and never parses its argument, whereas DCHECK keeps referencing the condition — so the CEF_ variants keep type-checking in release builds.
• The current CEF_REQUIRE_UI_THREAD sites live in dialog_handler_gtk.cpp and print_handler_gtk.cpp, both maintained as upstream copies with .patch files. Rewriting them toward the bare form would enlarge the diff we re-sync against.
• util.h also exposes a bare ASSERT macro through cefpython_public_api.h, which is collision-prone.

[cztomczak]

I agree that using a single CEF_REQUIRE_* family throughout the codebase makes sense.

One thing I'm less sure about is whether REQUIRE_* and CEF_REQUIRE_* have always been equivalent. REQUIRE_UI_THREAD() depends on ASSERT, which is a no-op when NDEBUG is defined. That's true with the current CMake Release builds, but I'm not sure whether the pre-CMake build system defined NDEBUG on all operating systems. From what I recall REQUIRE_UI_THREAD assert did work in Release at some point and it was intended.

Also, since cefpython is almost always built and tested in Release, these thread assertions are effectively disabled today. Was that an intentional change? It seems unfortunate that incorrect thread usage now goes completely unnoticed in the builds we actually run.

[linesight]

Thanks — I dug through the history on this, and your recollection is right. I can date it.

On whether they were always equivalent: they weren't.

• Windows: NDEBUG has been in the Release preprocessor defines since at least 2013 — c6178b4:cefpython/cef3/subprocess/libcefpythonapp_py27.vcproj has PreprocessorDefinitions="WIN32;NDEBUG;_LIB;...", and the same holds for the 2015 vcproj files. So REQUIRE_UI_THREAD() has been compiled out on Windows Release for essentially the whole project history.
• Linux/Mac: the makefiles used CCFLAGS = -g -std=gnu++11 -Wall -Werror ... with no -DNDEBUG, so ASSERT expanded to a live assert(false) and REQUIRE_UI_THREAD() really did abort at runtime — in the only builds that existed on those platforms. That's the behavior you remember, and it was real.
• 6b24eb1 (2017-02-24, "Build v55 on Mac Part 1") added -O3 -DNDEBUG for Darwin only, via MACFLAGS.
• eeef09a (2017-02-27, "Build v55 on Mac Part 2") hoisted it into os.environ["CEF_CCFLAGS"] = "-std=gnu++11 -DNDEBUG -Wall -Werror" under if LINUX or MAC. From then on NDEBUG is defined on all three platforms and ASSERT is dead everywhere. --fast only toggled -O0/-O3; NDEBUG was unconditional, so there was never a build config that dropped it.

So: equivalent for the last ~9 years, not equivalent before that. Worth noting the old divergence was silent platform inconsistency — the same REQUIRE_UI_THREAD() was a hard abort on Linux and a no-op on Windows — which is an argument for one well-defined macro family rather than against it.

On whether disabling them was intentional: intentional flag change, unintended consequence. Those commits are about matching upstream cefclient's Release flags — the old tools/build.py even carries the TODO "Set CEF_CCFLAGS and CEF_LINK_FLAGS according to what is in upstream cefclient, see cef/cmake/cef_variables.cmake" — not about assertions. Losing the thread checks was collateral damage. For what it's worth, this predates the CMake migration by nine years; CMake just preserved it faithfully (CMakeLists.txt:195,198).

One correction to my original post, because I don't want to oversell the rename: switching to CEF_REQUIRE_* does not restore runtime checking. DCHECK is compiled out under NDEBUG too —

#define DCHECK(condition) \
  LAZY_STREAM(LOG_STREAM(DCHECK), DCHECK_IS_ON() && !(condition)) ...

— and DCHECK_IS_ON() is 0 under NDEBUG (include/base/cef_logging.h:548), so the condition short-circuits at compile time. All that survives is that condition is still parsed and type-checked, which is precisely why the TID_FILE rot in REQUIRE_FILE_THREAD went unnoticed. So the rename is a maintainability and compile-time-checking win, and behavior-neutral. Your enforcement concern is a separate and legitimate issue.

But the two connect, and this is the part I'd argue matters most:

#if defined(NDEBUG) && !defined(DCHECK_ALWAYS_ON)

DCHECK has an escape hatch. Defining DCHECK_ALWAYS_ON re-enables every thread check while keeping NDEBUG and -O3. util.h's ASSERT is hard-wired to #ifndef NDEBUG with no override, so with the bare macros the only way to get the assertions back is to drop NDEBUG wholesale — which also changes CEF's own headers and CPython's, i.e. far more than intended. That makes unifying on CEF_REQUIRE_* a precondition for fixing the gap you're describing, not an alternative to it. Secondary benefit: DCHECK logs Check failed: CefCurrentlyOn(TID_UI). through CEF's logging, where ASSERT gave a bare DebugBreak()/abort() with no message.

Scope-wise, I'd keep this issue to the mechanical part: move the multiple bare call sites onto CEF_REQUIRE_* and retire src/common/util.h. No behavior change.

Whether to actually re-enable the thread assertions in the builds we ship or test is a bigger call than this cleanup, and it's yours to make — I don't want to presume the shape of it. If you think it's worth doing, it probably deserves its own issue. One fact worth having in hand when you weigh it: libcef_dll_wrapper is built separately in Release by tools/automate.py, and CEF headers use DCHECK in inline code (cef_ref_counted.h, cef_scoped_refptr.h, cef_weak_ptr.h, cef_lock.h, cef_thread_checker.h). So enabling DCHECKs for cefpython's own targets alone would make those inline bodies differ across translation units — a real ODR mismatch. Whichever direction you'd want to go, the wrapper build has to be part of the picture.

[cztomczak]

I agree with unifying on a single macro family.

However, if we want these thread checks in the Release builds we actually ship and test, why not just keep REQUIRE_* and make it work in Release?

DCHECK_ALWAYS_ON affects all DCHECKs across CEF/libcef_dll_wrapper, while we only need these specific thread checks in cefpython. This seems like a much simpler change in src/common/util.h.

I also wouldn't worry about the few CEF_REQUIRE_* uses in the Linux print/dialog code copied from upstream.

[linesight]

There's a third option I'd like your view on: doing the check on the Cython side. We already have IsThread() in utils.pyx:35 wrapping CefCurrentlyOn, and it's already used exactly this way in focus_handler.pyx (3 sites) and resource_handler.pyx (4, TID_IO), plus cookie.pyx and browser.pyx.

This gets you what you're after without touching NDEBUG at all, since Cython's asserts aren't tied to it. They can only be stripped by defining CYTHON_WITHOUT_ASSERTIONS, which we don't do anywhere — so these checks already run in the wheels we ship today. The placement works too: in focus_handler.pyx the assert comes before the if callback: lookup, so it fires even when the app hasn't registered a handler.

The tradeoff is that enforcement is softer than a C++ CHECK. The AssertionError goes through the handler's except: into sys.excepthook — fatal if the app installed cef.ExceptHook, otherwise just printed. So it reports rather than enforces.

Given that, which layer do you think should own the thread contracts: C++, where CEF calls into us, or the binding layer?

[cztomczak]

Cython assertions seem complementary, but they don't replace the C++ thread checks since not every REQUIRE_* site crosses into the binding layer.

For now I would just make an easy fix in util.h so that these checks are running in Release mode for CEF 147. Later we can make it clever if it's worth.