Ladybird: make a Bazel-only clone build, and pin the last unpinned input - #15
Open
ulfjack wants to merge 59 commits into
Open
Ladybird: make a Bazel-only clone build, and pin the last unpinned input#15ulfjack wants to merge 59 commits into
ulfjack wants to merge 59 commits into
Conversation
added 30 commits
August 1, 2026 17:04
…n inert version pin
Rebasing onto main brought in docs/BAZEL-RULES.md and EXPERIMENTS.md, which the Ladybird ring contradicts in three places. Reconciled each against the actual build rather than by editing prose:
- BAZEL-RULES said of the two Qt rulesets "neither has been used here". kklochkov/rules_qt 2.0.1 now IS used, so that prediction is recorded as a result (it held: qmake -query found the same 6.10.2 SDK find_package does), plus the two things only the use showed -- qt_cc_moc owning the include paths is what makes moc output byte-identical with zero -I flags, and the ruleset's REFUSAL to offer moc -b is what forced the real header fix.
- The exercised-versions table omitted Ladybird entirely; added it, plus rows for rules_shell and rules_qt, and EXPERIMENTS.md gets a Ladybird entry.
- The rules_foreign_cc section's boundary ("not for the code under migration") is walked at scale by the vcpkg/cargo rings, so it now states WHY they are plain actions over separately-pinned fetched inputs, and why a repository_rule was the tempting wrong answer. The sharper form of the rule is: which properties must remain checkable? For a third-party leaf that is hermeticity and pinning, not compile parity.
Applying that doc's own "versions are resolved, not remembered" rule to Ladybird found a real defect: MODULE.bazel declared rules_cc 0.2.17 while MVS resolved 0.2.19 via bazel_tools on Bazel 9.2.0, so the declared version was inert. Corrected to what resolves and added common --check_direct_dependencies=error so the two cannot drift again; all 6 binaries rebuild green after. Suite 126/126.
…ders + Cranelift)
The README's headline claim was false. Every binary depended on 741 targets under Build/full, CMake's build tree, and the overlay shipped 4 BUILD files and zero headers -- allow_empty=True in the shims meant a fresh clone got no error, just `fatal error: LibXML/Export.h: No such file or directory` some 1,600 actions in. Narrowed 741 to 31 true gaps (666 LibWeb bindings headers were merely SHADOWING Bazel's own; deleting them left the build green).
1. The 15 generate_export_header Export.h + AK's 2 configure_file headers. New emitter Meta/emit_export_headers_bazel.py derives the tokens as Meta/CMake/targets.cmake does; all 17 built artifacts are byte-identical to CMake's. AK/Backtrace.h is a host PROBE, not a template -- the genrule compiles find_package(Backtrace)'s test rather than baking my machine's answer.
2. Cranelift was absent from the Bazel graph ENTIRELY. Libraries/LibWasm/CMakeLists.txt was not in CMAKELISTS and build_rust_binary() was not parsed, so LibWasm's dep was silently dropped. The reference build hid it twice: the header sat in Build/full where a global -I reached it, and the compiler binary was named by an absolute path baked into -DWASM_CRANELIFT_COMPILER_PATH=/home/ubuntu/... -- two host escapes covering for one missing target. Now a cargo_binary that also declares its cbindgen header (byte-identical to CMake's), consumed as //:libwasm_cranelift_lib, with the binary as `data` so Ladybird's own sibling-of-self lookup finds it in any checkout.
3. A real include-collision bug, found by removing the shim that masked it. 8 of 10 crates emit a header named RustFFI.h and 4 TUs include it bare, so an unprefixed include dir must not propagate. Splitting it into cargo_bare_include was necessary but NOT sufficient: include dirs propagate along the C++ dep graph too, so LibGfx inherited LibTextCodec's dir and YUVData.cpp compiled against the wrong header. The fix is implementation_deps -- Bazel's name for exactly the scope CMake's PRIVATE FFI_OUTPUT_DIR has.
Also: rust_bare_include_crates() is now IMPORTED from the cargo emitter that derives it instead of a hand-kept tuple beside it; the FFI-header sync is shared by both cargo drivers instead of duplicated; emit_libweb_bazel round-trips its export-header block instead of silently truncating it.
Verified by removal, the only test that counts: Build/full/{Libraries,Services,UI,bin,cargo} moved off the machine (0 headers left), all six binaries rebuilt from scratch (2,704 actions), and --headless=text AND --headless=layout-tree are byte-identical to the CMake reference on three pages including two that execute WebAssembly. Parity harness 1408/1408, all emitters round-trip.
…ead -IBuild/full copts
The three Build/full/**/BUILD.bazel shim packages are gone, and with them the last thing the emitted build read out of CMake's tree. By the end they supplied nothing: of the 709 headers they globbed, 21 are Bazel genrule outputs and the other 688 are LibWeb bindings headers Bazel ALSO generates (all 692 are in LIBWEB_GENERATED_HDRS, checked). So the roots were SHADOWING Bazel's own outputs, winning or losing on include order.
That is why they lasted: glob(["**/*.h"], allow_empty = True) over a tree that is not there yields an empty list and no diagnostic. A shim that cannot fail is indistinguishable from a shim that is not needed.
Also six of the seven per-target -IBuild/full copts were an un-normalized path: CMake emits a target's own binary dir relative to itself, so the five services and WebContent got -IBuild/full/Services/WebContent/../.. -- the same directory as the global -IBuild/full, spelled differently, so the string comparison against globalroots missed it. target_private_includes() now normpaths before comparing. The seventh (ladybird's -IBuild/full/UI{,/Qt}) pointed at CMake's UI gendir, whose only non-autogen contents are the two SPIR-V shader headers Bazel generates itself and carries on :generated_shader_headers.
Verified by removal: shim packages deleted AND Build/full/{Libraries,Services,UI} moved off the machine (0 Ladybird-generated headers left under Build/full -- the 3,532 remaining are the vcpkg tree, a separately closed ring). All six binaries rebuilt from scratch, 2,704 actions, and all 6 render comparisons -- --headless=text and --headless=layout-tree over three pages, two of which execute WebAssembly -- are byte-identical to the CMake reference.
README: the headline clone-and-build claim is now true and says how it was false, the Layout table and gap 4 reflect the deleted shims, and finding 35 documents the two bugs the CMake tree had been masking (the RustFFI.h collision needing implementation_deps, and Cranelift being absent from the graph behind two host escapes).
…d BUILD vcpkg_tree's cache_dir was "/home/ubuntu/.cache/vcpkg-bazel" -- the last absolute host path in the generated build, and meaningless in anyone else's checkout. It is an opt-in resumability affordance and no part of the dependency graph, so the emitter now writes "", which is also the honest default: an empty cache is the genuine from-source build. The rule already supported it. That leaves zero absolute host paths in the emitted BUILD files (the one remaining /home/ubuntu string is inside the comment explaining why it is not there), and zero targets under Build/full in the dependency closure of all six binaries -- down from 741. Both checked with cquery and grep rather than asserted.
…ere right Same bug as the glob that could not fail, one layer up. `for f in tests/test_*.py; do python3 "$f"; done` exited 0 for all ten files -- but test_emit_cargo.py, test_emit_vcpkg.py and test_vcpkg_plumbing.py had no `if __name__` runner at all: Python defined 46 test functions, called none, exited 0. There is no pytest here, so nothing else called them either. Found by counting tests executed instead of trusting the exit code. With runners added, 6 of the 46 failed, and every one was a TRUE report about the finding-35 changes rather than a stale assertion: emit_ring() grew a third parameter (the binary crates); cargo_lib gained an `archive == None` branch because a --bin crate has no archive, so the ring has 11 cargo_lib for 10 cargo_crate; the shared FFI-header lookup moved into cargo_vendor.sh so both cargo drivers use one copy; the ring quotes with ' not "; and one test read Build/full/Libraries/BUILD.bazel -- a file whose DELETION was the fix. That one is now the stronger assertion that examples/ladybird/workspace/Build/full must not exist at all. Plus two regression tests for the two finding-35 bugs, both asserted from CMake rather than from a list: every build_rust_binary() call site reaches the ring with the same two consumable labels a staticlib crate gets (the fixture now carries both shapes -- flapc with no FFI_OUTPUT_DIR, cranelift with one), and every *_bare_include target is reached through implementation_deps and never plain deps, because CcInfo include dirs propagate along the C++ dep graph and that is how LibGfx compiled against LibTextCodec's RustFFI.h. 128 tests over 10 files, all passing. README now documents running the glob rather than a hand-kept list of filenames, since a list of files to run is one more thing that can silently omit an entry; case study finding 35 records the episode.
… silent No -- running individual tests does not scale, and the way it failed here is instructive. SKILL.md named six of the eleven test files and chained them with `&&`, so it stopped at the first failure and never reached the rest; the README named the same six. Three of the omitted files had no `if __name__` block, so running them defined 46 tests, called none, and exited 0. Adding a runner to each file was necessary but is NOT the fix: a per-file runner is precisely the thing nobody notices the absence of. What was missing is one entry point that knows how many test FILES exist and how many tests each contributed. tests/run_all.py discovers tests/test_*.py and fails the run if a file defines no tests, will not import, or if the glob matches nothing at all -- the allow_empty = False of test discovery. 135 tests over 11 files in 0.3s, one exit code, filterable (`run_all.py -v cargo`); each file still runs standalone for a single-file loop. Its three guards are tested by TRIGGERING them (test_run_all.py, 7 tests over a throwaway tests/ dir, checking the exit code a gate would read) -- a guard never seen to fire is back where this started. Writing that test found a real trap: the obvious version ran the runner as a subprocess from inside the directory the runner scans, so the runner ran the test that ran the runner and the suite hung. It now calls test_files() directly, which is also the more precise assertion. The general lesson, recorded in the case study: the unit that must be accounted for is the CONTAINER, not the item. Counting passing tests cannot detect a missing file, counting matched files cannot detect an empty glob, and counting green actions cannot detect a header supplied by a shim. Next step as this grows is a py_test per file under `bazel test //...` for caching, parallelism and a commit gate that runs without anyone remembering to -- this repo has no MODULE.bazel yet, so that is a bigger change and I have left it for Ulf's call.
You asked whether you can clone and build. I stopped answering from the README and ran it: git clone into an empty directory, overlay in, type the command. It fails six times, and five of the six have nothing to do with Build/full — which was the dependency I had been arguing about, so it is the one I verified. Finding 36 is the autopsy; examples/ladybird/README.md now leads with the table instead of the claim. Fixed here: (1) two path bugs in vcpkg_build.sh — the distfile index AND its entries were execroot-relative while vcpkg invokes the asset script from its own cwd, so every lookup missed, reported as `no asset cache hits` and x-block-origin refusing the network, i.e. a message blaming the pin for a path bug. Hidden because the dev checkout already had downloads/tools/cmake, so vcpkg never asked. (2) `pip install ply` in the angle port: not an asset-cache download, so the 76-distfile pin never covered it, and nothing blocked it either — requires-network 0 is a scheduling hint, no-sandbox means there is no namespace to enforce it, and use_default_shell_env handed the action this sandbox's HTTP_PROXY. Now pinned by URL+sha256 as an http_file, declared as an action input, staged into a find-links dir, with PIP_NO_INDEX and the proxy unset; added to use_repo and to the emitter's --use-repo so a regeneration cannot drop the one input no instrument can capture. (3) the git-archive staging was `if [ -d ]; then cp 2>/dev/null || true; fi` — three ways to succeed while copying nothing — and vcpkg_git_archives.bzl is generated, committed, documented and loaded by NOTHING; the four tarballs only existed because I made that directory by hand. Now a hard error in 4s naming them instead of skia dying 20min in on a googlesource URL. (4) vcpkg scratch is cleaned on INT/TERM too and reports free space, since a small /tmp surfaces as `No space left on device` from the asset script. Documented, not fixed: Build/vcpkg is still an allow_empty=True glob over a checkout a clone lacks (finding 35's pattern one tree over, matching exactly one file — its own BUILD.bazel); that checkout's .git is load-bearing for `git read-tree` yet EXCLUDED by the filegroup, which no-sandbox lets the action read anyway — an excluded input the build needs is worse than an undeclared one, because the exclusion looks deliberate; and the HSTS preload table is an unversioned download from Chromium's main with no hash to pin. All three are now step 1a of the recipe with the reason each stayed invisible. Tests: 6 new in test_vcpkg_plumbing (142 total). One of them failed only in the full run — another test points $LADYBIRD_ROOT at a fixture and my subprocess inherited it. run_all.py running everything in one process is what surfaced that; the per-file runners could not have.
The clone-and-build run hit 'No space left on device' reported by the ASSET SCRIPT -- a message blaming the pin for a full disk. vcpkg's buildtrees peak near 3GB and $TMPDIR here is a 7.9GB tmpfs shared with everything else. Fixed by putting the scratch dir next to the declared output (inside bazel-out, i.e. on the same real filesystem as the output base) rather than $TMPDIR. That is also why it is not a flag: this action runs in BOTH the target and exec configurations, so TMPDIR would need --action_env AND --host_action_env (finding 26's duplication again), and the rule's own 'env =' silently beats --action_env anyway. Two ways wrong. It also has to be ABSOLUTE: vcpkg rejects a relative $HOME outright, and $OUT arrives execroot-relative -- the third place in this file where the relative-vs-absolute distinction has bitten, after the asset index and its entries. Scratch is now cleaned on INT/TERM as well as EXIT (abandoned runs leaving multi-GB trees is how the disk filled), and the action reports its free space so a too-small disk says so in its own voice.
…s the seventh blocker With the six clone blockers fixed, the full build on the virgin clone is RC=0 (2,842 actions, all six binaries). Then the documented run command failed with 'mkdir: Permission denied' -- because its last line symlinked $PWD/Build/full/share/Lagom, CMake's build tree, into the resource root. Six findings about a fresh clone not having Build/full, and the recipe for running the result depended on Build/full. The resource root needs no CMake: Base/res (in the clone) plus pdf.js from //:vcpkg_installed, with pdfjs-ladybird-transport.mjs in web/ where UI/cmake/ResourceFiles.cmake puts it. Assembled that way it is diff -rq identical to Build/full/share/Lagom, and the clone's binaries then render --headless=text and --headless=layout-tree byte-identically to the CMake reference on all three test pages. README: staging block rewritten to be clone-correct (with cp --no-preserve=mode, since Bazel's outputs are read-only and the second run would fail), top claim restated as 'builds and renders, three inputs staged by hand', gap 5 updated. Case doc: finding 36 gains the seventh blocker.
Answering 'so what's needed to make it work' by testing each candidate fix rather than estimating. Two of the three staged inputs are one Bazel rule; the third is an upstream change to Ladybird that no rule can substitute for. Rows 1+2 (Build/vcpkg and its .git) need a custom repository_rule that shells out to git clone. Four shortcuts tested and rejected: git_repository/new_git_repository STRIP .git (so the built-in rule for 'get a git repo' cannot deliver the property this dep needs); a custom rule that clones does keep it, and glob(['**']) carries the .git files as declared inputs, which also fixes row 2's undeclared-input lie; --depth 1 fails with vcpkg's own 'Try again with a full vcpkg clone' because the pinned port trees live in history, so it is a 121MB/~1min full clone; and dropping builtin-baseline to avoid .git 'works' while silently moving 10 dep versions (ffmpeg 7.1.1->8.1.2, harfbuzz 10.2->14.2.1, mimalloc 2->3, plus zlib/freetype/dbus/fontconfig/libedit/libwebp/cpptrace). Positive control: a fresh full clone resolves all 78 ports to exactly the versions this dev checkout resolves. Row 6 (the four git archive tarballs) is the same rule's second user. Row 3 (HSTS) is not a Bazel problem: pinning it only on the Bazel side works mechanically but the pinned file differs from the one CMake fetched and the generated table differs, so it trades a hermeticity gap for a parity gap. The fix belongs in Meta/CMake/hsts_preload.cmake. General shape worth keeping: a converter can detect an unpinned input in the foreign build system but cannot pin it unilaterally.
…a script I answered 'what's needed to make it work' by designing a custom repository_rule that clones vcpkg at the baseline. The supporting work was real -- git_repository/new_git_repository do strip .git, a custom rule that shells out to git clone does keep it, and glob(['**']) does carry those files as declared inputs -- but the conclusion was wrong. Ladybird already ships it: Meta/ladybird.py vcpkg (45 lines in Meta/Utils/build_vcpkg.py) clones microsoft/vcpkg, checks out vcpkg.json's builtin-baseline and bootstraps the tool at a pinned tag+SHA512. Run from an empty directory: 75s, RC=0, .git at the right commit. The README's step 1a already said to run it. So blockers 1+2 are a prefetch step to order correctly, not machinery to write -- and a repo rule reimplementing it would be a fork of tooling the project maintains, drifting the moment the baseline moves. Row 6 collapses the same way: clone the pinned URL and `git archive` the ref. Verified by reproducing libyuv's tarball -- SHA512 matches the committed value in vcpkg_git_archives.bzl exactly, so those hashes are checkable rather than decorative. Eight lines of shell instead of a rule. Row 3 (HSTS) stays upstream-only, unchanged. Also records the two shortcuts tested and rejected: --depth 1 (pinned port trees live in history; vcpkg says 'Try again with a full vcpkg clone') and dropping builtin-baseline (silently moves 10 dep versions). Docs, vcpkg_build.sh comment and both workspace copies updated; the general lesson is that the fix I imagine is more available to me than the fix that exists.
…kg pins history Ulf asked why Ladybird doesn't just use a git submodule for Build/vcpkg. It works mechanically -- a submodule's .git is a gitfile (gitdir: ../../.git/modules/...), and vcpkg's `git --git-dir .git read-tree <tree>` follows the indirection: verified READ-TREE OK. So 'submodules break vcpkg' is not the reason. The reason is that a submodule pins the wrong thing. It pins one commit and gives you its checkout; vcpkg's manifest pins a baseline and then reads history BEHIND it. 14 of vcpkg.json's 45 overrides name a version that is not what ports/ holds at the baseline: ffmpeg 7.1.1#5 vs 8.1.2#3, harfbuzz 10.2.0 vs 14.2.1#2, mimalloc 2.2.7 vs 3.4.3, qtbase, freetype, zlib, dbus, fontconfig, libedit, libwebp, libtommath, cpptrace, angle -- and simdutf pins NEWER than the baseline holds. Concretely ffmpeg 7.1.1#5's port is git-tree 0988005f while HEAD:ports/ffmpeg is c40aaa40, so the bytes vcpkg builds exist at no single commit. That is also why --depth 1 fails. Secondary costs, all measured: the same 119MB lands in .git/modules (no saving); Build/vcpkg is inside a Build*/-ignored path that vcpkg fills with ~3GB of downloads/installed/buildtrees, so the submodule reads dirty forever without ignore = dirty; and a submodule still does not produce the vcpkg binary, which is not tracked but bootstrapped from a tag+SHA512 in scripts/vcpkg-tool-metadata.txt. Generalizes: Build/vcpkg is not a vendored dependency, it is a package manager's cache directory that happens to be a git checkout. Every instinct that treats it as a pinned copy of another repo -- submodule, git_repository, http_archive of a tarball, --depth 1 -- pins the checkout and loses the history, and the history is the dependency.
…- they stage nothing Ulf asked how a Ladybird developer actually gets the correct stuff on disk. The answer dissolves two of the three remaining blockers: ./Meta/ladybird.py build produces all three inputs as side effects. (a) build calls build_vcpkg() itself -- not only the `vcpkg` subcommand -- so the checkout and its .git appear (~70s, verified from an empty directory; `vcpkg` alone gives ONLY this, no downloads/, no HSTS table). (b) The CMake configure downloads the HSTS table via Meta/CMake/hsts_preload.cmake, gated on ENABLE_NETWORK_DOWNLOADS, default ON. (c) vcpkg writes the four git-archive tarballs into Build/vcpkg/downloads/ while building skia and angle -- and their SHA512s equal the committed values in vcpkg_git_archives.bzl, so the directory I 'made by hand months ago' was a copy of vcpkg's own download cache. I had not built a pin, I had copied one and forgotten which. So for a Ladybird developer this is a non-problem; it is a problem only for a Bazel-only clone that never runs CMake. Those two sentences carry different debts: two blockers need no code (a recipe that says 'build once with CMake first', plus a clear error when it hasn't), and only the unpinned HSTS fetch is a genuine hermeticity defect needing an upstream change. My three successive answers to 'what is needed' were a repository_rule, then a prefetch script, then a cp. Each was smaller because each time I read further into what the project already does.
…, git-reproduced, verified
Ulf asked three times for the no-CMake answer; the first two replies described the CMake path. This closes two of the three staged inputs for a Bazel-only clone: `Meta/ladybird.py vcpkg` is a standalone subcommand (~70s, no configure), and Meta/fetch_vcpkg_git_archives.py now produces the four vcpkg_from_git tarballs -- 4/4 reproduced from scratch and byte-identical to the committed SHA512s.
The shape matters because I got it wrong twice. A static parse of the portfiles is unsound in both directions at once: my first version found 8 archives for skia where 4 are real, and missed libyuv entirely. declare_external_from_git only DECLARES; get_externals(${required_externals}) picks from it under feature/platform if()s, so the set is decided by CMake evaluation rather than by the text -- and libyuv's archive comes from its own port calling vcpkg_from_git directly. Finding 30's lesson recurring: portfiles are programs, do not re-derive what they compute.
So the list comes from the committed pin, and regenerating the pin uses vcpkg as the instrument: Meta/vcpkg_capture_git_archives.sh runs `vcpkg install --only-downloads` (~6 min, no compilation, no CMake). fetch_vcpkg_git_archives.py then reproduces each pinned tarball with git clone + git -c core.autocrlf=false archive <ref> -- byte-for-byte what vcpkg_from_git.cmake runs -- and fails on any mismatch, so the hashes are checked rather than trusted. Static resolution survives only where sound: name -> clone URL. Recorded asymmetry: --only-downloads yields 3 of 4, because angle's zlib is fetched in angle's BUILD phase.
9 tests (151/151 total) cover the two decisions the script makes alone: the pin is the authority (regression test for the 8-vs-4 bug), and every pinned name must resolve or it fails loudly. Remaining defect is the HSTS table alone -- Chromium's unversioned main, so nothing to pin to; noted that CMake's download_file is a no-op when the file exists, so the upstream fix is additive.
…we should not Measured all four http_file combinations rather than reasoning about them. Unpinned + main builds today; unpinned also means Bazel caches the first fetch forever (proved with a local origin: changed upstream, rebuilt, got VERSION-ONE in 0.3s, no warning). The table moved during this session -- service.gov.scot left the list, 94627 -> 94626 entries -- and the pinned tag 139.0.7258.5 has 168593, so pinning only on the Bazel side diverges from CMake by 74k entries. Pinning has to happen where both build systems read it.
… not a tag Upstream's hsts_preload.cmake fetches Chromium's transport_security_state_static.json from main, and we cannot change upstream. So the overlay pins it for itself: hsts_preload.bzl declares one http_file at an immutable commit + sha256, MODULE.bazel names the repo, and gen_HSTSPreloadData consumes @hsts_preload_json//file instead of the CMake configure's leftovers under Build/caches (wired in the emitter, since codegen_root.bzl is generated). My earlier answer -- 'fix it upstream, stage it until then' -- was wrong because it made someone else's repo a prerequisite for our hermeticity. What misled me was pinning the wrong revision: a release tag (139.0.7258.5) serves 18.7MB and generates 168,593 entries against main's 94,626, so pinning THAT does trade hermeticity for parity. The commit main is serving has bytes identical to CMake's download (cmp, 10,521,748 bytes), so pinning it costs no parity at all. Meta/pin_hsts_preload.py re-pins by measuring, and --expect-same-as refuses to write a pin whose bytes differ from the file the other build system already has. Verified on the fresh clone with the CMake-downloaded file deleted: gen_HSTSPreloadData's outputs are byte-identical to the CMake reference and //:LibHTTP builds (RC=0, 183 actions). 12 new tests, 164/164. The upstream one-liner is filed as a bug, not depended on.
…d by running it
Getting the overlay onto another machine was 'cp -r workspace/. ladybird/', and running that on an empty directory found four defects. Nothing recorded WHICH Ladybird commit the generated BUILD files describe (they name ~1,961 sources by path, so every parity claim was relative to an unnamed commit -- f9e34731, now pinned in one place). bazelrc.txt must be renamed. The two patches must be applied, not just shipped.
The fourth was invisible from either side alone: Build/vcpkg/BUILD.bazel makes the DIRECTORY Build/vcpkg exist, and upstream's build_vcpkg.py infers 'already cloned' from is_dir(), then runs `git -C Build/vcpkg rev-parse HEAD` -- which, with no .git inside, walks up to Ladybird's own repo and returns Ladybird's HEAD. It then tries to check vcpkg's baseline out of the Ladybird repo: fatal: unable to read tree. Two correct programs, one wrong composition. apply_overlay.sh defers that one file past the prefetch and names the failure where someone hitting it will look.
--verify checks what a copy cannot: HEAD is the pinned commit, all 42 files byte-identical, patches APPLIED (git apply --check -R succeeding is the proof), and the .sh files still executable -- the last because scripts once sat 100644 in git while my tree had them +x, so only a fresh clone failed, and only at action time.
Verified end to end: apply_overlay.sh on an empty dir produced a tree byte-identical to the one that renders (diff -rq), fetch_vcpkg_git_archives.py reproduced 4/4 against the pinned SHA512s, and bazel build built the 76 vcpkg ports offline and linked //:LibHTTP (RC=0, 242 actions) with HSTSPreloadData.{h,cpp} byte-identical to the CMake reference. 12 new tests, 176/176 -- two of which caught real defects in this change.
…at my machine lacked Ulf's clone failed with `distfile MISSING FROM INDEX ... ninja-linux.zip` at vcpkg's compiler detection, then x-block-origin correctly refused the network. The 76-distfile pin came from instrumenting vcpkg's downloader, which cannot miss a download vcpkg asked for -- but vcpkg_find_acquire_program probes the HOST first, and this machine has /usr/bin/ninja at exactly the required 1.13.2, so vcpkg never asked. The capture was faithful; it was an observation of my machine. cmake is in the pin only by luck (host 4.2.3 vs required 4.4.0), and that accident made the whole class look covered. An instrument that records what a program did cannot pin what it would do elsewhere, when its behaviour depends on the machine. The comparand is vcpkg's own scripts/vcpkg-tools.json -- versioned at the baseline, url+sha512+archive name for every tool -- which is a pin rather than an observation. Deriving the tools at emit time was the wrong fix and two existing tests caught it: the emitter's promise is that the committed pin alone regenerates everything with no vcpkg, no CMake, no network, and reading vcpkg metadata during emit made a checkout a requirement again. So --capture-tools writes Meta/vcpkg_tool_assets.tsv, committed like the asset capture; emitting unions it in and cross-checks against the live metadata when a checkout happens to be present. Scoped to cmake+ninja (what detect_compiler needs); the 7 skipped tools are reported, not hidden -- dotnet/node/powershell alone are ~400MB no port here invokes. Verified: the ninja sha512 confirmed against upstream independently of the error text, `bazel query deps(//:vcpkg_installed,1)` now lists 78 distfiles including ninja, and the index the asset script reads resolves that hash to the Bazel-fetched file. 182/182, six new tests including the regression test for the report.
…efore the build Two more of Ulf's builds died 20 minutes in, one tool at a time: `Could not find nasm` from inside libvpx, then `gperf currently requires ... autoconf autoconf-archive automake libtoolize` from vcpkg_make.cmake. Same blind spot as finding 38 -- the capturing machine had the tool -- but a different class, and I had assumed the class was closed by pinning vcpkg-tools.json. On Linux vcpkg has NO download for these. nasm's three URLs and sha512 all sit inside `if(CMAKE_HOST_WIN32)`; six ports in the closure need it. So there is nothing to pin, and the honest statement is that this is the boundary of the port. What WAS fixable is how you find out: a preflight over a derived list, reporting every miss at once with the ports that need each and one pasteable apt line, instead of one tool per 20-minute build in an error that names the wrong place. It has two mechanisms, which is why my first attempt missed half of it: vcpkg-make never calls vcpkg_find_acquire_program, it uses bare find_program + FATAL_ERROR with an apt line -- and does it from a HELPER port, so the error names gperf while the requirement lives where gperf does not mention. A scan built from one example is calibrated to one example. Most of the work was suppressing false positives, because a preflight that demands packages you do not need is one the third person deletes. CLANG's call sites are MSVC-only; openssl's NASM/CLANG live in ports/openssl/windows/ guarded only by the parent's include(), so the PATH must be read; dav1d's `if(NOT VCPKG_TARGET_IS_WINDOWS) ... else()` means that else IS the Windows branch; and angle only WARNS about mesa-common-dev while separately FATAL_ERRORing about architectures, so the check anchors on the FATAL_ERROR's own text -- advice is not a requirement. autoconf-archive (m4 macros) and libltdl-dev (headers) have no binary to probe and are reported unverifiable rather than assumed satisfied: finding 35's rule in a third place. Verified negatively, which finding 38 could not be (sudo hangs here, so I could not hide /usr/bin/ninja): the probe is `command -v`, so a restricted $PATH is a machine without the tools -- both missing tools reported in one run, in one second. Confirmed in a real `bazel build //:vcpkg_installed`: aquery shows Meta/vcpkg_host_tools.tsv as a declared input, passed by path as argument 7 rather than found with `dirname $0` (an sh_binary's data lands in .runfiles/, the trap cargo_vendor.sh already documents), and the action ran the preflight and built on past it. Also: the README claimed 42 overlay files while the overlay held 43, stale since finding 38 added one. Now 44 and asserted against the overlay -- finding 39's lesson at the smallest scale. The environment notes have listed autoconf/nasm as apt prerequisites since the beginning; documenting a requirement is not checking it. glslangValidator is the same class and is still open, filed rather than pretended. 194/194, 12 new tests -- one per false positive above, because each was a real bug in my own derivation.
… GUI segfault Ulf's Ubuntu 24.04 box segfaults the Bazel-built GUI in QXcbConnection::initializeScreens. Cause: the binary links Qt from the Bazel repo and then dlopens Qt's PLUGINS from wherever libQt6Core's baked-in prefix points -- on his box the distro Qt's plugin dir. Two Qt builds in one process. Qt 6.9.2's qt_prfxpath is empty, so the prefix falls back to the executable's directory; a Bazel binary's directory has no platforms/, so the search falls through to the compiled-in system path. Which way the skew points decides the failure: an OLDER plugin is rejected by Qt's version gate (clean abort), a NEWER-or-equal-minor one PASSES and then calls into an ABI it was not built against -- the SIGSEGV he sees. On this box both are 6.10.2, so the same wrong lookup has been passing by accident in every green GUI run this project has reported. Pointing qt.local_repo at aqt 6.9.2 here reproduces his backtrace frame for frame. qt_runtime.bzl fixes it in four pieces: qt_plugins reads @qt's OWN generated qtconf.bzl for QT_INSTALL_PLUGINS (so plugins and libraries cannot come from different Qts) and symlinks every plugin type; qt_plugin_tree stages them beside the binary; qt_conf writes [Paths] Prefix=. which REPLACES the compiled-in prefix, so /usr is never scanned; and runtime_libs carries the private libraries an SDK bundles beside Qt. That last piece is where the loader stopped being intuition: no rpath on the binary can find aqt's bundled ICU 73, because libQt6Core resolves it through RUNPATH $ORIGIN (= Bazel's solib dir, since $ORIGIN is the path the loader opened it by), and while DT_RPATH is inherited by transitive loads, an intermediate object with a DT_RUNPATH of its own blocks it. libQt6Core has one. Measured on three generated .so files, all four combinations. So the private libs become real link inputs and Bazel stages them; the list is derived from DT_NEEDED, and is empty for a distro Qt. Also carries the Qt >= 6.9 floor that UI/Qt/CMakeLists.txt declares and the overlay silently dropped -- which is the class this is finding 40 about: all nine failures Ulf's machine has produced are one bug, a host requirement upstream declares and CHECKS, inherited without its check. find_package and pkg_check_modules are the preflight, not ceremony. Verified by removal: with the host plugin dir hidden behind an empty tmpfs and no LD_LIBRARY_PATH, the aqt build loads libqxcb.so from the aqt SDK, zero /usr dirs are scanned, and the GUI opens its window on Xvfb and stays up. Headless still renders. Following the README's staging recipe to do that turned up two more written-down-instead-of-derived paths (the vcpkg tree is exec-config; the resource root is <bindir>/../share/Lagom); both now come from bazel info/cquery. 205/205 tests, 12 new.
… remaining segfault Finding 40 fixed the plugin path and the GUI still segfaulted in QXcbConnection::initializeScreens with the same backtrace, on Ulf's aqt 6.9.2 SDK. QT_DEBUG_PLUGINS showed the plugins now resolving to his own SDK, info sharedlibrary showed one libQt6Core with identical build strings, and the offscreen QPA plugin crashed the same way -- so it was neither the plugins nor a version mix. At the fault rdi = 0 on `mov 0x8(%rdi),%rbx` inside doActivate: Qt emitted screenAdded from a null qApp, mid-QApplication-constructor. readelf explains it. aqt's libQt6Core has NO relocation against QCoreApplication::self (built with reduce_relocations, so it accesses its own BSS PC-relative); libQt6Gui reads it through the GOT; and bazel-bin/ladybird carried an R_X86_64_COPY for it, which moves the definition into the executable and repoints the GOT there. Core wrote one copy, Gui read the other. Debian's Qt has a GLOB_DAT for self, so the bug cannot appear against a distro Qt -- which is why only an official SDK saw it. The cause is one flag: CMake puts -fPIE on every executable target, the capture recorded it, and emit_build_bazel.py copied it into copts on all seven generated cc_binary targets. Bazel appends per-target copts AFTER the .bazelrc's --copt=-fPIC and the last of the pair wins for GCC, so the UI/Qt objects compiled -fPIE while everything around them compiled -fPIC. Qt's headers #error on exactly this, but only when __PIC__ is unset -- both flags were passed, so the check never fired and the build was clean. Fixed in the generator (DROPPED_TARGET_FLAGS), not in the generated file, since BUILD.bazel is output. Dropping it matches CMake's intent: Bazel compiles a cc_binary's objects PIC and links -pie. Reduced to eight lines without Bazel or Ladybird first (-fPIE crashes, -fPIC prints its screen count); -Wl,-z,nocopyreloc is not an alternative, it only converts the fault into an R_X86_64_PC32 overflow at link time. Verified by removal on both Qts and all six executables: 39 R_X86_64_COPY relocations (qApp, stdout, QString::_empty, 20-odd staticMetaObjects) now 0, the GUI starts against aqt 6.9.2 where it previously died under both xcb and offscreen, and the distro-Qt build is unchanged. Guarded by tests/test_pie_copy_relocation.py (5); finding 41 in the case study, with the note that a backtrace surviving a fix is not the same bug. 210/210.
Ulf's browser died with EMFILE. His two /proc/<pid>/fd censuses (17,423 -> 17,497 socket:, flat 18 pipe:) falsified my own MessagePort diagnosis: that leak allocates two pipe2 pairs per socketpair, so it leaks pipes and sockets at 4:1 (reproduced locally at 1,628:409) and cannot produce a sockets-only census. Following the signature instead: every completed HTTP request leaks exactly one socket fd in WebContent (103 requests -> 107 sockets, 1,164 -> 1,157, pipes never move; also 50 <img> -> 57). internals.dumpGCGraph() names the holder, because GC roots carry a source location: 808 roots at Fetching.cpp:2338 after 202 requests = 4 callbacks x 202. The four fetch callbacks are GC::Roots moved onto the refcounted Requests::Request, which Response holds back by RefPtr -- a cycle across the GC heap and the refcount heap that neither collector can break, keeping the response fd open. Only Request::defer_teardown() clears the callbacks, and normal completion never calls it; aborted fetches, which do reach stop(), leak zero. Also recorded: the obvious one-line fix (defer_teardown() in did_finish()) is wrong. I built it and the leak went to zero because loading broke -- blank pages. Reverted, re-verified the clean build renders and leaks, and took the diagnosis upstream without claiming a patch. All measurements are from a CMake build of f9e34731; the code paths are unchanged in upstream master (50eef049). Finding 42 + docs/UPSTREAM-ladybird-fd-leaks.md.
…liveness Ulf applied the equivalent upstream patch and his browser still leaked. That is not a contradiction and not a regression in the patch: the per-request leak is two bugs, and the patch only reaches one of them. The discriminator is whether the OTHER end of the retained response fd is gone -- one column of `ss -np` (peer inode `* 0`), which I can now also probe in-process. Class A: the request completed, RequestServer closed its half, WebContent retains a corpse (peer DEAD) -- the teardown patch takes this from 143 retained sockets to 0. Class B: `on_finish` never ran at all, so no teardown placed in that branch can fire, and RequestServer is still holding its end (peer ALIVE) -- 40 retained, unchanged by the patch. Class B is the GC-root/RefPtr cycle itself and needs the cycle broken at the Response end, not another teardown call site. Adds patches/0003 (the teardown fix, framed as necessary-not-sufficient, with the scope note in its header) and a DIAGNOSTIC census patch that is deliberately NOT matched by apply_overlay.sh's patches/*.patch glob -- pinned by a new test, since only its extension keeps a per-request HashMap, a repeating timer and a poll() probe out of a browser someone is using. The census reports in_flight-vs-retained by age (so a freshly restarted process cannot look like a fix -- a mistake I made earlier here) and peer DEAD/ALIVE per retained request, and carries LADYBIRD_FDLEAK_TEARDOWN so one build A/Bs the fix. Corrects UPSTREAM-ladybird-fd-leaks.md, which overstated the fix's completeness, and adds the postscript to finding 42: the instrument should answer the classification question, so the next report is a count instead of another theory.
…ng my exact bytes Two defects, both mine, both surfaced by Ulf running --verify on his own tree. 1. The diagnostic was a patch to Request.cpp. That is the wrong delivery mechanism for anyone who has their own commits: it assumes a tree at our pinned commit, it conflicts with a tree already carrying upstream's fix, and it makes "run my diagnostic" mean "reset your tree". Everything it computed is visible from outside, so examples/ladybird/fd_census.py now reads /proc and `ss` against a RUNNING browser: category census, peer DEAD/ALIVE per socket, retained-vs-in-flight by age, which process holds the live peers, and a verdict naming the leak class. Verified to agree with the instrumented build on the same workload (143 DEAD / 4 ALIVE either way) and to diagnose the stalled-body class as 44 ALIVE held by RequestServer. The in-process patch is kept for the fields only it can see, with a header pointing at the script first. 2. `git apply --check -R` proves MY EXACT BYTES are in the tree, which is a stronger claim than "the defect is fixed" -- so upstream landing its own fd-leak fix made a CORRECT tree report PATCH NOT APPLIED and advise applying a patch that would then conflict. A patch we carry only until upstream fixes it now has an .effect-grep beside it, and verify falls back to asking whether the effect is present. The first version of that check was a whole-file grep, which PASSED on an unpatched tree because defer_teardown() already occurs in stop() and did_transfer() -- worse than being too strict. It is now anchored to the branch condition with a line window, and the negative case is pinned by a test. Also records what Ulf's census means: 1514 of 1520 dead-peer WITH the fix applied is class A, the class the fix does address -- so on his tree the fd has an owner other than Requests::Request, which the dead-peer verdict now says outright instead of restating the fixed diagnosis.
Three fixes, all from Ulf's first run of the tool. 1. It told a --watch user to "use --watch". Every fd is 'first seen this sample' on attach -- a fact about when the CENSUS started, not the fds' age. Now it says so, and the next sample ages them. 2. It reported a level but not a slope, which is the only thing that answers 'is it still leaking'. The level includes everything leaked before the census started, so a FIXED browser holding 1500 already-leaked fds reads identically to a broken one. It now reports growth per minute since the first sample and the time to a 1024-fd limit. Verified live: 143 dead-peer sockets on a finished page correctly reads NOT GROWING, which is the distinction a single sample cannot make. The first cut of that had a silent middle band -- <0.5/min called flat, >0.5/min flagged, so exactly +0.5/min was reported as neither. That is ~720 fds/day, i.e. the overnight death being investigated. Every positive slope is now named, with an ETA (+0.5/min -> '1024-fd limit in ~34 hours'). Pinned by a test. 3. It listed 'live peers held by: WebContent x2, Compositor x2, ImageDecoder x2, RequestServer x2, ladybird x2' next to the leak counts. That is the ordinary IPC mesh -- each pair of browser processes keeps a couple of long-lived sockets -- and printing it beside a leak diagnosis invites reading plumbing as evidence. Peers are now split: N-of-a-kind at mesh level is labelled not-the-leak; a peer RETAINING many (RequestServer x40 in the stalled-body repro) is called out.
Ulf's census settled the argument: 81 -> 823 sockets in 458s, 815 of them dead-peer, ~97/min, WITH the teardown fix applied. So the fix does not cover his case, and I have now falsified every workload shape I can build locally -- including top-level navigation, the one path with KeepAliveForTransfer::Yes and body_delivery_paused, which stays flat at 5 sockets. The difference is his tree, which I cannot fetch (c0d567aa is not in my remote), so no amount of me reading code will find it. So: an instrument instead of another theory, and one that needs nothing from his tree. fdtrace.c is an LD_PRELOAD shim that records a backtrace for every fd a process acquires and drops it on close; fdtrace_report.py groups whatever is still open by acquisition stack and symbolises it offline with addr2line. The load-bearing detail is that it hooks recvmsg/SCM_RIGHTS. The leaked fd is never opened by WebContent -- RequestServer creates the socketpair and passes a half over IPC, so the kernel materialises it inside recvmsg on the IPC read thread. A tracer that wraps open()/socket() sees literally nothing, which is presumably why this was never traced. Validated against the known leak on an unpatched build: 209 acquisitions, 166 still open, and the top group is 148 fds via recvmsg/SCM_RIGHTS -- exactly the number fd_census independently reports as leaked. Since an attachment's acquisition stack is always the IPC thread by construction, the report leads with the number that does discriminate: how many still-open fds arrived over IPC versus were opened locally, plus the cross-check that peer=DEAD means only this process can still close them.
Ulf's log is 14 consecutive SCM_RIGHTS attachments with byte-identical stacks and no matching close lines. The identical stacks are expected -- I predicted them -- but they mean the tool as shipped could not name a culprit: the kernel materialises an attachment fd on the IPC read thread, so every attachment from every peer shares one stack, and grouping by stack cannot tell RequestServer's response pipes from the Compositor's or ImageDecoder's attachments. SO_PEERCRED on the receiving socket does discriminate, so fdtrace now records sock= and from=NAME(pid=N) per attachment (cached per socket, since this is the IPC hot path) and the report leads with a by-sender breakdown. On the known leak it prints '143 from RequestServer(pid=...), 5 from Ladybird(pid=...)', which matches fd_census's 143 dead-peer sockets exactly, and says which of the two conclusions follows -- response pipes are the class under investigation, anything else is a different bug in whatever decodes IPC::File from that peer. Also: the report parses BOTH log formats, so the log Ulf already captured stays readable (pinned by a test), and the tracer's own frames are filtered out -- they appear in every stack and were pushing the real caller off the end of --frames.
…collectable With 0003's teardown applied, Ulf still measured 97 leaked sockets/min on his tree while every workload I could build locally was flat. Both instruments agreed on what was leaking: fd_census said every one was peer=DEAD (RequestServer had already closed its end, so these were COMPLETED requests -- class A, on a tree carrying the class A fix), and fdtrace said every one was a received SCM_RIGHTS attachment sent by RequestServer. That pins the fd to exactly one thing: the per-request response pipe from RequestPipe::create(), handed over by request_started. The gap is ownership, not liveness. Dropping the callbacks unpins the GC cycle, but the fd is released only by ~Request (or by the ReadStream that teardown merely nulls), so ANY other surviving reference to the Requests::Request retains one fd per completed request even though the teardown ran. That is why one patch zeroes the leak on my tree and leaves 97/min on his: we differ in what else holds a reference, not in what the teardown does. patches/0004 closes the fd explicitly at the point the code has already proven the body is complete (the user_finish_called branch), deregistering the notifier before closing so the event loop never polls a closed descriptor. It deliberately does NOT close on request_finished alone -- that truncates bodies (verified once as blank pages), because RequestServer stops writing long before WebContent drains the pipe. Measured A/B, same binary, same 200-completed-request workload, only this function differing: clean 208 sockets / 203 peer=DEAD -> fixed 6 sockets / 0 peer=DEAD. Body delivery intact: text=10 | stream=10/bytes=48000 | cancel=5, plus a rendered screenshot. Also fixes why his fdtrace log said from=?(pid=2261433): a renderer's landlock policy grants only /proc/self, so reading a peer's /proc/<pid>/comm is denied from inside while SO_PEERCRED (a syscall, not a path) still works -- cmdline and exe would have failed identically. The tracer now snapshots pid->comm in its constructor, before main() installs the sandbox, and logs the raw pid per connection so the report can resolve it from outside, where /proc is readable. ps -p 2261433 confirmed RequestServer. The generalisable lesson: "the object is collectable" and "the fd is closed" are different claims, and for a descriptor received over IPC only the second is the bug -- a fix verified through the object graph can pass while the resource still leaks.
0004 and 0003 edit the same three lines, so a single patch cannot serve both trees. Worse, I generated the on-top patch by diffing against the CLEAN commit, so it silently contained 0003's own hunk and failed on the one tree it was written for -- Ulf hit that immediately as "patch does not apply". Now: 0004-...-on-completion.patch applies to a tree that HAS the teardown fix (upstream's or 0003), 0004-...-on-clean-tree.patch to one that does not, and each is verified to be REJECTED by the other's tree so a mis-pick fails loudly instead of half-applying. The on-top variant is byte-identical to the source I built and measured. Measured in the 0003+0004 configuration (CMake build of f9e34731, 200 completed requests): 5 sockets, 0 peer=DEAD, workload SETTLE ok=200 -- versus 208 sockets / 203 dead on the clean tree. Body delivery re-verified on that same binary, because a fix that removes body delivery looks identical on fd counts: text=10 | stream=10/bytes=48000 | cancel=5 plus a rendered 800x600 screenshot. The lesson to keep: verify a patch APPLIES to the tree you are asking someone else to apply it to, not just to the tree you wrote it on. That is the same class of error as proving my exact bytes instead of the fix's effect.
…ied as a series apply_overlay.sh applies patches/*.patch by glob, so shipping two mutually-exclusive variants of 0004 (one for a tree with the teardown fix, one without) guaranteed that one of them would fail. Ulf hit it on the first run: "tries to apply both patches at the same time". My fault twice over -- I had already broken 0004 once by generating it against the clean commit, and "add a second variant" was the wrong repair for that. The clean-tree variant is deleted, and not only because of the glob: it was strictly weaker. It closed the fd without dropping the callbacks, so it left the GC-root/RefPtr cycle -- and with it the class B retention -- in place. 0004 is simply the next patch in the series, applied on top of 0003. What makes this not recur: a test reconstructs the pinned version of every file the patches touch, straight out of the reference checkout, and applies the whole series in glob order exactly as the script does -- so a patch that conflicts with its predecessor fails in CI, not on a colleague's clone. Plus a checkout-free structural guard: no two patches may share a series number, and no patch may advertise itself as an alternative. Both were verified to FAIL when the deleted variant is put back, and the series test reproduces Ulf's exact error. The script's own failure message now says patches/ is an ordered series and where an alternative belongs (outside the glob, like DIAGNOSTIC-*.patch.txt). Verified the series applies to a reconstruction of the pinned tree and yields source byte-identical to the build I measured (5 sockets / 0 peer=DEAD on 200 completed requests, bodies intact).
added 29 commits
August 17, 2026 15:41
… WebContent Ulf reports still leaking after 0004. Rather than ask for another number, two things about my own method are wrong and both explain how this loop keeps failing. 1. Every census I have requested was of WebContent. That was my HYPOTHESIS, not a finding -- and RequestServer is the process that creates both the response pipes and the cache body files, while the UI process and Compositor hold fds too. If the fds accumulate in any of them, every measurement I have collected is blind to it, and "still leaking" while my numbers say fixed is exactly the expected result. fd_census.py --all now censuses every Ladybird-family process (Ladybird, WebContent, RequestServer, ImageDecoder, Compositor, WebWorker) and ranks by GROWTH RATE, so the data names the process instead of me naming it. Rate not level, because a process can hold many fds legitimately (the IPC mesh). 2. My test server never exercised the disk cache. Ulf runs --http-disk-cache-mode enabled against real sites; my server sent no cache headers at all, so handle_read_cache_state never executed in ANY A/B I ran. That is not cosmetic: the large-cache-hit branch (body_size >= PAGE_SIZE) goes take_body_file() -> send_transferred_body_file_to_client(), which sends a BODY FILE and never creates a response pipe -- a completed-request path release_response_fd() structurally cannot reach. I built that workload (300 hits across small/large/revalidated cacheable entries) and it stays flat at 5 sockets here, so it is not sufficient alone, but it is the first path found that my fix does not cover. 0004 remains verified for the response-pipe class (208 -> 5 sockets, 203 -> 0 dead, bodies intact). It is evidently not all of what he sees. Documented as open in docs/UPSTREAM-ladybird-fd-leaks.md, including both method errors, so the next measurement is --all on his machine rather than another WebContent census shaped by my assumption.
…sking Ulf's latest census settles the process question against my last detour: the fds accumulate in WebContent (7487 sockets, 7479 peer=DEAD, +91.7/min over 4880s), and it is class A -- completed requests, 7428 of 7436 retained. So --all was the right instrument and it pointed back where I started. The number I could not interpret was the rate: 91.7/min against 97/min measured before the fix. That is equally consistent with "0004 does not address this leak" and "0004 was not in that binary", and those need opposite next steps. My instinct was to ask him which it was -- another round trip, about a build that already happened, answered from memory. That would have been the fourth time in this investigation that a number arrived without the identity of the code that produced it. A leak rate without its build provenance is not reproducible by anyone, including me. The answer was never in anyone's memory: it is in the binary, and the binary is still mapped by the process being censused. So the instrument reads it -- a pure-Python ELF reader over .dynstr/.strtab of the executable and every mapped .so (no binutils dependency on someone else's machine), looking for release_response_fd (0004) and defer_teardown (0003). Printed next to every verdict, because a verdict is only interpretable together with the code that produced it, and available alone as --build. The load-bearing part is the negative control. set_up_internal_stream_data exists in every build of that file, patched or not; if it is missing, the symbols were not readable at all (stripped, LTO, statically linked elsewhere) and the probe says "cannot tell" rather than "fix absent". Without it a stripped binary reads as an unpatched one and aims the next round of work at the wrong code -- the same class of error as the two method mistakes already documented, so it gets a guard rather than a caveat. Statically linked builds (Ulf's is one) are covered by probing the executable when no lagom-requests library is mapped. Verified end-to-end against two genuinely different builds of liblagom-requests: release_response_fd renamed away and relinked -> "does NOT have 0004" with the control still present; restored and relinked -> "HAS 0004". A non-Ladybird process -> "cannot tell". Both directions tested, since a probe that can only confirm a fix is present is useless for the question that prompted it. Running the full census on a HEALTHY browser to check the new block also caught a real reporting bug: 0 dead + 5 live sockets satisfied neither 10x-majority branch and fell through to "mixed DEAD/ALIVE -> both classes present", naming a class with no members and reading the ordinary IPC mesh as a leak. Fixed with explicit zero cases. It surfaced only because the healthy case finally got looked at. Also ruled out the lead I was about to chase: HTTP::MemoryCache::Entry stores status, headers and ImmutableBytes and never holds a Requests::Request, so --enable-http-memory-cache cannot retain a descriptor. Documented instead the two paths that CAN: Response::m_request_server_request is copied by clone(), and paused body delivery (set_body_delivery_paused(true) for document navigations, ~8 resume/stop call sites in LocalNavigable) never reaches the completion branch release_response_fd() lives in -- which yields exactly the observed signature of peer=DEAD, retained, and indifferent to 0004. 249/249 tests pass.
…bol it looked for Ulf: "the tool says it's not, but I'm sure it was applied", then "I have all the patches applied." He was right; the probe I shipped last round was wrong, and the reason is a real flaw rather than a mistake of his. Ladybird sets ENABLE_LTO_FOR_RELEASE=ON (Meta/CMake/cmake_options.cmake:46). In a STATIC build -- his -- LTO inlines a small internal-only method like release_response_fd() into its single caller and leaves NO symbol and NO string behind. Reproduced from first principles: a private method called only within its TU, linked -O3 -flto, is absent from both nm and strings; my own build kept it only because a SHARED library must export it. So the probe's answer depended on how the binary was LINKED, not on whether the fix was in it -- and the one build shape it had to get right is the one it got wrong. The negative control did not save it, which is the instructive part. set_up_internal_stream_data is vulnerable to the same optimisation (verified: a larger internal-only function also vanishes under LTO), so it certified readability it had not established. A negative control only rules out "unreadable" if it CANNOT disappear for the same reason as the thing it guards; mine failed in a different mode than the one it was guarding against. I had called it "the load-bearing part" -- it was load-bearing in the wrong direction. Two fixes, both verified. (1) .debug_str is read alongside .dynstr/.strtab: debug info names inlined-away functions, and Ladybird's RelWithDebInfo compiles with -g (-g1 also verified sufficient), so the answer survives exactly the LTO/static case. (2) A fix is never reported MISSING unless a symbol inlining cannot erase is visible (UNINLINABLE_CONTROLS: vtable/IPC-dispatched entry points, plus Request's header-declared out-of-line methods); otherwise the verdict is "cannot tell" and it names inlining as the reason. Tested both ways: an Ulf-shaped blob yields "cannot tell", while a genuinely unpatched-but-readable binary is still called out -- re-verified end-to-end against a rebuilt shared library. With 0004 confirmed applied, the ~92/min class-A leak is NOT the response-pipe path 0004 closes: the fd has a third owner. I then falsified my best remaining hypothesis too -- 330 abandoned top-level navigations to a dribbling body, site isolation and memory cache on, stayed flat at 0 dead. Two falsified hypotheses deep, guessing a third time is the wrong move. So the census now reads the discriminator off the socket instead: Recv-Q, which ss already reports and I was discarding. For retained peer=DEAD fds, unread>0 means the body was NEVER DRAINED (delivery paused and never resumed, so the completion branch that closes the fd is unreachable) while unread==0 means the body was fully read and the fd is merely still OWNED (the RefPtr in Response::m_request_server_request, which clone() copies). Identical in every other column, opposite fixes, one field. Ulf's existing --all run already collects the line it comes from. 254/254 tests pass.
…eam now
Ulf hit a glob failure on a checkout past our pin -- Libraries/LibJS/BytecodeDef/** matched nothing, allow_empty = False. Not a bug in the generated build: upstream a32d9c9f ("LibJS: Derive bytecodes from Flap handlers") DELETED BytecodeDef/ and Bytecode/Bytecode.def, so the overlay was describing a tree that no longer exists. He asked for the build fixed at 71fb301a.
This commit is the half that does not need the reference build: the pin, the patch series and the host-tool list. The emitter regeneration (BUILD.bazel, codegen_root.bzl, cargo_*.bzl, the new flapc --bin generate-libjs-bytecode replacing the deleted Python generator) needs a CMake reference build at the new commit and follows separately.
The patch directory SHRANK, which is the part worth reading. patches/0001 (PYTHONHASHSEED dictionary order) and 0002 (UI/Qt/TabBar.h not self-contained) are both fixed upstream at 71fb301a and are deleted; the fd-leak pair is renumbered 0003/0004 -> 0001/0002 and verified to still apply as a series at the new commit. Upstream's determinism fix is better than mine: it sorts inside dependency_names_for so no caller can get a set, where I sorted at the one call site I had found -- and two of my four patches were the same bug in that function, which I had filed as distinct. Both patches kept applying long after they stopped being needed, so "it still applies" was never evidence the bug was still there.
The repin also cost four failed CMake configures, one per undiscovered host package, and they are now named in the README: upstream turned Qt6 Positioning from OPTIONAL_COMPONENTS into REQUIRED, made GuiPrivate required on Linux, and added pkg_check_modules(GIO) for the new ExternalURLActivationToken/Handler sources. Two of the four are transitive and neither error names a package: Qt6GuiPrivate reports NOT FOUND until libxkbcommon-dev exists, then names a QtGui/6.10.2 include path only qt6-base-private-dev ships. Same shape as finding 39, one layer out -- the preflight covers vcpkg's host tools, not Ladybird's own find_package requirements.
…ke it
Ulf's checkout at 71fb301a failed to LOAD, not to compile:
Error in glob: glob pattern 'Libraries/LibJS/BytecodeDef/**' didn't match
anything, but allow_empty is set to False
Upstream a32d9c9f ("LibJS: Derive bytecodes from Flap handlers") deleted that
directory, replaced the Python bytecode generator with a Rust binary (flapc's
second bin, generate-libjs-bytecode), and dropped flapc's --bytecode-def flag.
The pattern was a hardcoded string in a list literal in emit_cargo_bazel.py, and
the same string appeared in three more places plus emit_libweb_bazel.py's
rust_crate_srcs heredoc (CSS/Rust/** and Layout/Rust/**, one package over).
The fix is structural, not a re-pin of the string. Every crate directory an
allow_empty=False glob names is now DERIVED: Cargo.toml's members closed over the
manifests' `path =` deps (which is also how the derivation reaches outside the
workspace, since libjs_rust build-depends on the excluded flapc), and flapc's
extra inputs come from scanning it for include_str!. A deleted crate now deletes
its own glob pattern. emit_libweb_bazel imports that one list rather than
restating it, so the two sides of the package boundary cannot disagree.
Why this shape of staleness is the worst one in the repo: allow_empty=False is
CORRECT and had to stay (its opposite is why the old Build/full shims matched
nothing for weeks and failed 1,600 actions later), a loading error has no target
to blame so nothing in the build graph can report it, and the emitters that would
have re-derived it all ran green -- an emitter that enumerates KINDS cannot
notice a kind going away. The same repin silently dropped gen_Op from
codegen_root.bzl while the parity harness reported 0 UNHANDLED, because the new
generator's ninja rule is `<tool> ... && cmake -E copy_if_different ...` and the
copy_if_different EXCLUSION matched first: every exclusion in a first-match
classifier can capture a command it was not written for, and the count that
should catch it is computed after the capture.
Also in this repin, all measured against the reference build at 71fb301a:
* libweb_css_rust + libweb_layout_rust are consolidated back INTO libweb_rust,
so 8 crate targets (was 10) and 19 generated FFI files (was 14). Two of
libweb_rust's twelve are .inc, not .h -- Bazel deletes an undeclared .inc
exactly as it deletes an undeclared header, so the suffix must not decide
whether a generated file is declared.
* build_rust_binary() takes FEATURES too, which the parser only read on the
import_rust_crate side. Upstream's new style-replay is built FEATURES
style-recording from the SAME crate as libweb_rust's staticlib, so ignoring it
would have built a different binary than CMake does and said nothing. The
feature-name character class also needed `-` (style-recording), which truncates
rather than failing to match.
* 4 cargo_binary (was 2), 155 crates.io crates (was 154, flapc + foldhash 0.2.0),
MODULE.bazel's use_repo regenerated.
* libjs_rust's build-script input is Interpreter/interpreter.flap now, not the
deleted Bytecode/Bytecode.def.
One bug I introduced and then had to be shown: excluding cargo's outdir as
`**/target/**` is wrong. flapc has a Rust MODULE at src/target/ (the code
generator's per-architecture backends), so it dropped 40 sources and
//:generate-libjs-bytecode died with `error[E0583]: file not found for module
'target'`. A glob exclusion is a pattern over paths and knows nothing about what
a directory MEANS; "the dir cargo writes to" is <crate root>/target, and only the
anchored form says that. Both properties are now tested.
Verified at 71fb301a: parity harness 53 generators / 1418 of 1418 files
byte-identical / 0 UNHANDLED; emit_cargo --check all 4 ok and --report clean;
export headers 15/15 + AK/Debug.h; all five emitters regenerate the checked-in
files byte-identically. On the Bazel side the workspace now LOADS (203 targets),
//:gen_Op builds through the new Rust tool and both Op.h and OpCodes.h are
byte-identical to CMake's, and libweb_rust/libjs_rust/style-replay build with all
12 of libweb_rust's FFI files byte-identical to the reference. tests 256/256.
…e bug The glob failure Ulf reported is fixed and committed (88abd6f). Running the full browser build past it produced four more failures, and a fifth defect that would never have produced one. Every single one was a CAPTURE: a value derived once on the machine that generated it, written down, and thereafter believed. Not one was found by reasoning about the repin; each was found by the next error message. 1. Libraries/LibWeb/generated_srcs.bzl said "AUTO-GENERATED by Meta/emit_libweb_bazel.py" and NO code path in that emitter wrote it. Its two lists were hand-maintained, so upstream's 5 new generated headers and 4 new .cpp were missing, and a GENERATED header included a GENERATED header the cc_library did not declare: Bindings/Window.h:17: fatal error: LibWeb/Bindings/WindowGlobalMixin.h: No such file or directory -- with the file sitting on disk next to the one that included it. A false AUTO-GENERATED line is worse than an honest hand-written file: it tells the next repin that re-running the emitter will refresh it. Both lists are now derived (the reference build's compile list; codegen.bzl's own outs) and emitted by --generated-srcs. Exactly 9 entries had drifted. 2. Fixing that exposed a second bug it had been HIDING. genrule_outputs() regexed the whole of codegen.bzl for anything path-shaped, so a genrule's srcs counted as outputs: four checked-in headers that generate_dom_tree.py READS were classified as generated. That list is the hdrs exclude=, so they were dropped from the source glob and re-added as labels nothing produces. It never broke the build only because the stale capture was consulted instead of the function's answer. One capture concealing a live bug is the strongest argument against keeping one. 3. QT_MAP was a 3-entry dict of the Qt modules Ladybird used when measured. Upstream made Qt6::Positioning REQUIRED; no key, so the dep fell through to UNKNOWN and //:ladybird failed with "QGeoPositionInfo: No such file or directory". CMake's Qt6<Module> -> rules_qt's @qt//:Qt<Module> is a rename, so it is a rule now -- one that still returns None for a non-Qt name, because reporting UNKNOWN is right and inventing a label is not. 4. moc_headers() ended with `not h.endswith("GeolocationProviderQt.h")`, justified in its docstring by "Qt6::Positioning is not found in this configuration" -- a fact about the capturing machine, false at this pin. The condition is now whether the reference build compiles the sibling .cpp. Wrong in both directions: moc'ing a header CMake does not is a target only Bazel builds; skipping one it does is a missing vtable. The fifth produced no error at all and would not have. scripts/extract_cmake turned /usr/lib/libgio-2.0.so into the dep name `gio-2`, cutting the basename at the FIRST DOT instead of the extension. Invisible for libz.so and libQt6Widgets.so.6.10.2; wrong for any library with a dot in its name. It became visible only because the three glib deps upstream added (a new pkg_check_modules(GIO) in UI/Qt) arrived as unresolvable UNKNOWNs. Had the emitter guessed a label instead of reporting, -lgio-2 would have failed at link time far from the cause. An emitter that reports what it could not resolve converts a silent wrong answer into a loud missing one; that property found this bug, and it is the same property that makes allow_empty=False worth keeping. Also fixed, the specimen I was mid-investigation on: emit_vcpkg_bazel's capture REPLACES the static portfile parse (deliberate -- a portfile is a CMake program). That reasoning was checked once against three genuinely Windows-only rows and then frozen INTO THE MESSAGE, which printed every casualty as "vcpkg never asked for on this platform". At this pin vcpkg.json moves sdl3 to 3.2.28, the versions-db derivation gets it right, and the message reported the CORRECT current pin being discarded for a stale captured 3.4.12 as a Windows-only fetch. classify_static_only() now separates the two with no new input: a dropped row whose URL FAMILY the capture also has is the same project at another version, i.e. a stale capture. Re-capturing is filed (todo 37a5e4ee); vcpkg_distfiles.bzl still fetches 3.4.12. A replace-wins rule whose message asserts the reason for the replacement instead of checking it will eventually be confidently wrong. That sentence describes all six items above. Tests 256 -> 265, and each new guard was verified to BITE by temporarily reintroducing the bug it guards. New tests/test_emit_libweb.py (5) covers the derivation from both ends -- a new codegen output reaches hdrs, a genrule's own inputs do not, a generated .cpp CMake does not compile is not a src, and the checked-in file matches what the emitter emits. All six emitters reproduce their checked-in files byte-for-byte at this pin; apply_overlay.sh --verify reports 45/45 identical. LibWeb, the 5 services and all the Rust crates now build; //:ladybird's compile failures are gone and the remaining vcpkg exec-config rebuild is running.
…t lied generated_srcs.bzl was not special. It carried a first line naming an emitter that had no code path writing it, and that false claim is what made the file safe to hand-edit and invisible at repin time: every reader, including me, was told that re-running the emitter would refresh it. The claim turns out to be checkable for the whole tree at once, and cheaply. A file whose header says a script generates it must name a script that exists, that parses every flag the header names, and that writes output at all. That last assertion is the one that fails for the original bug. Deliberately NOT a round-trip check -- proving each file reproduces needs a CMake reference build, which this suite does not have (the repin verified that by hand: all six emitters byte-identical). This is the weaker property that would still have caught it, because the emitter named there could not have produced the file under any argument. It also refuses to pass vacuously: fewer than 10 such files means the walk broke, not that the tree is clean. Verified to bite in both modes: a nonexistent script, and a flag the named script does not parse.
… restate Found by auditing for the shape rather than waiting for failure number seven. GLOBAL_DEFINES and SYSTEM_LIBS describe the BUILD, not a target: a define is global because .bazelrc sets it for every TU, and a lib is a system lib because no vcpkg port supplies it. Both emitters kept their own copy, and they were already out of step -- this repin added glib/gio/gobject/xkbcommon to emit_build_bazel's SYSTEM_LIBS for upstream's new pkg_check_modules(GIO), and emit_libweb_bazel's copy still held the original four. Latent today, purely because LibWeb does not depend on glib. The failure it was holding is an UNKNOWN dep, i.e. a silently dropped link input -- and "happens not to" is the class of claim the rest of this repin was spent disproving. The second copy is now an import; the emitted BUILD file is byte-identical, which is what makes this a deduplication rather than a change. The test asserts all three levels: the source imports rather than restates, the two modules share the same objects at run time, and the shared set actually contains what the repin added (an import pointing at a stale copy would pass the first two).
…ng 3 of 6
I caused this one, immediately after fixing four hand-copied facts, which makes
it the most useful item in the repin.
An absolute include root cannot be a per-target copt (Bazel rejects a path
outside the execution root even as -isystem), so emit_build_bazel dropped it and
.bazelrc carried it globally as CPLUS_INCLUDE_PATH. The drop was a bare
`continue`: a root CMake compiles with and .bazelrc lacks produced NO output at
all. Having just deleted four written-down facts, I then read glib's include
roots off the model BY EYE and wrote down three of them. The build failed ~3,800
actions later:
UI/Qt/ExternalURLHandler.cpp:19:10: fatal error: gio/gdesktopappinfo.h:
No such file or directory
There are six, not three: gio-unix-2.0 is a separate root from glib-2.0 (the
UNIX-only GIO headers), and blkid/libmount/sysprof-6 arrive transitively through
glib's own pkg-config. Re-running the emitter with the check in place named all
four missing roots in one second.
The emitter now records what it drops and reports the shortfall against
.bazelrc's CPLUS_INCLUDE_PATH. Deliberately a WARNING, not a failure: which host
escapes are acceptable is a judgement (README gap 3), and Qt's roots are
correctly absent because rules_qt carries them on the dep edge -- so those are
exempt, because a warning that cries wolf about Qt every run is a warning that
hides a real one. What is not a judgement is whether the emitter says anything.
The generated BUILD.bazel is byte-identical: this adds a diagnostic, not a
change. The test asserts the emitter records and reports, that the dep-carried
roots are exempt, that all six roots are present, and that the target and exec
copies of the variable are equal (a skew there is finding 26's failure mode:
a genrule tool that cannot compile).
--action_env made gcc FIND glibconfig.h; Bazel then rejected the compile anyway: Compiling UI/Qt/ExternalURLHandler.cpp failed: absolute path inclusion(s) found in rule '//:ladybird': ... '/usr/lib/.../glib-2.0/include/glibconfig.h' and it is right to: a header outside the execution root that no toolchain declares is an undeclared input, so Bazel cannot know when it changed. rules_cc derives cxx_builtin_include_directories by running `cc -E -v` in a REPOSITORY rule, and CPLUS_INCLUDE_PATH shows up in that output -- so passing the same value as --repo_env is what DECLARES these roots to the toolchain, rather than sneaking them past the check. Only glibconfig.h needed it; every other root is under /usr/include, which gcc already reports as builtin. The reasoning is in the file, because the two lines look redundant and the next person to tidy one away gets an error that names neither.
The Bazel build at 71fb301a was green and the browser still would not load a
page: ~14,000 lines of
Failed to parse IPC message:
Local endpoint error: Can't read past the end of the stream memory
Peer endpoint error: Endpoint magic number mismatch, not my message!
Everything aimed at the code generator said the build was fine, and was right:
all 20 generated *Endpoint.h are byte-identical to CMake's, the wire magic
7a36a5ff IS WebContentServerEndpoint (correct endpoint, payload won't decode),
the staged binaries' md5s matched the fresh ones, no stale processes. I spent
real time on a C++17/C++23 ABI-skew theory because aquery reports both flags --
and it is a red herring: EVERY CppCompile has -std=c++17 then -std=c++23, gcc
takes the last, 2694/2695 actions identical.
The artifact that ran was not built by the build. `readlink /proc/<pid>/exe`
on the live WebContent -- the one measurement that asks what is EXECUTING
rather than what is built -- pointed at bazel-out/k8-fastbuild/libexec/, an
August 7 binary from the PREVIOUS pin. I had staged into
bazel-out/k8-fastbuild/bin/libexec/; the UI looks one level up. Upstream
inserted IPC messages between the pins, so every message id past the insertion
point shifted by one: right endpoint, wrong layout, and 8000 unparseable
messages per run.
The staging step was never needed AND was actively harmful.
get_paths_for_helper_process() searches <prefix>/libexec/<name> BEFORE
<prefix>/bin/<name>, and under Bazel the services are already siblings of
ladybird in bazel-bin -- so the copy I made only ever shadowed the real thing:
a cache with no invalidation, in a directory Bazel does not own, that no `bazel
clean` clears and no rebuild refreshes. So the fix is to delete the step, not
refresh it. Verified by removal: with no libexec/ anywhere, --headless=text and
--headless=layout-tree (119 lines) and about:version are byte-identical to the
CMake reference at 71fb301a.
tests/test_run_recipe.py (4 tests) guards the recipe, because the recipe is the
interface Ulf runs: no copy into libexec by any spelling, the recipe must clear
a libexec/ an older one left, the lookup order and the IPC symptom must stay
written down (or "don't stage" reads as a style preference and gets re-added),
and the same-class `share`-symlink-into-CMake's-tree trap. Each verified to bite
by reintroducing its bug.
The general shape, and the reason this belongs in the case study: a convenience
copy of a build output, placed where the program looks first, fails as a
MISCOMPILE -- the one direction byte-parity checks on generated code cannot see.
…eleted a crate
cargo_ring.bzl carried, in generated prose, "its own lock with exactly 3
packages (flapc, in-tree bytecode_def, and smallvec from crates.io pinned
=1.15.1)" -- four hand-copied facts under a header that says AUTO-GENERATED.
Upstream's a32d9c9f ("LibJS: Derive bytecodes from Flap handlers") deleted the
bytecode_def crate, and the Flap lock has had 2 packages ever since. Nothing
failed, because prose has no compiler: all three cargo_*.bzl still reproduce
byte-for-byte at 71fb301a, and one of them documents a crate that does not
exist.
emit_ring() now reads the lock: the count, each package, and which are in-tree
(no checksum) vs registry, plus which are shared with the big workspace at the
same version AND hash -- the reason the shared crate collapses to one fetch
rule, which was the point the prose was making and is now derived rather than
asserted. The LOCKS comment no longer names the contents either.
Third instance of one bug in this repin (after SYSTEM_LIBS and the three-of-six
glib include roots), so the rule is worth stating plainly: if a fact is worth
putting in generated output, it is worth reading from the input.
Two guards, each verified to bite by reintroducing the bug, and they fail for
different reasons -- the mechanism on a fixture whose Flap lock has 2 packages
(so a re-hardcoded "3" fails here, not six weeks later on Ulf's machine), and
the checked-in artifacts, where a crate mentioned in prose but never declared as
a rule/label/key is either dead text or a missing rule. The second one
deliberately does NOT read the real Cargo.locks: they live in the Ladybird
checkout, so that version needs a 5,958-file clone and would be skipped in CI
exactly when it matters.
…kind Ulf has
Ulf asked how to get his tree patched. The answer was: you can't. Both failures
are in the path nobody had run -- the script had only ever been tested on a FRESH
clone, and every repin instruction in the README assumed one.
Reproduced by building a replica of his tree (old pin f9e34731 + the 4 old
patches + the old overlay, reconstructed from git history) and running the
current script at it. Two bugs, in order:
1. `git checkout --detach <new pin>` refuses, because the patches modify tracked
files:
error: Your local changes to the following files would be overwritten by
checkout: Meta/Generators/libweb_bindings/to_idl_value.py, UI/Qt/TabBar.h
git is right; the script was wrong to stop there. The reader cannot resolve it
either: two of those four patches were fixed UPSTREAM at 71fb301a and deleted
from the overlay, so their bytes can no longer be reverse-applied from
anything we carry, and they are indistinguishable from his own edits.
Now: stash, with a message naming the script and the pin, and print the
`stash pop` that restores it. Deliberately NOT `reset --hard`/`checkout --`:
a patch we no longer carry looks exactly like a debugging edit made on top,
and a script that silently discards the second kind is one nobody should run
on a tree they care about.
2. The vcpkg prefetch then dies with `fatal: unable to read tree` -- the trap
this script's own header documents. Phase 1 defers Build/vcpkg/BUILD.bazel so
the directory does not exist when upstream's bootstrap looks; but on a repin
the file is ALREADY THERE from the previous run, so deferring it changes
nothing. The bootstrap reads the bare directory as a checkout, walks up to
Ladybird's repo for a HEAD, and tries to check vcpkg's baseline out of it.
Now the stale directory is cleared first: the overlay's own deferred files by
name, then `rmdir` (an EMPTY Build/vcpkg is just as fatal -- the bootstrap
tests for the directory). Never a directory with a .git, never `rm -rf`: if
something we do not own is in there, that should be loud.
Verified end to end on the replica: runs through, `--verify` reports 45
identical with both patch effects present, previous state recoverable in the
stash. README now answers the question where it is asked, with the repin
commands and the stash caveat.
Three guards, each verified to bite. Two of them had to be scoped to CODE with
comments stripped, because my comments explain why `reset --hard` and `rm -rf`
are wrong here -- a test that cannot tell the explanation from the deed forces
you to delete the explanation to go green.
The 71fb301a re-capture printed "All requested installations completed
successfully in: 49 min", exited 0, and wrote 72 rows. The committed capture has
76. Five URLs vanished, and only one of them was mentioned anywhere: angle's
gni-to-cmake.py failed with a transient TLS error (this sandbox's clock was
briefly behind the certificate's validity window -- "certificate is not yet
valid"; the URL and its pinned hash are fine, verified by hand).
The other four are the mechanism worth remembering: a failed download HALTS its
portfile, so the four vcpkg_download_distfile calls after it in angle were never
made, never requested, and so never captured. The recorder logs each tuple
BEFORE fetching, so the one that FAILED is present in the capture and the four
nobody asked for are simply absent. Result: a pin missing five URLs that looks
complete -- the worst shape available, since the emitted rules would fetch
nothing for angle and fail much later inside a port build.
So the capture now judges itself, which it cannot do from vcpkg's exit code. The
recorder appends each failed URL to a sentinel file (a variable cannot work: the
recorder is a separate process per download), and the driver refuses to bless a
run with any failure, naming them. Verified against a fake vcpkg that calls the
recorder with an unreachable URL and then exits 0 like the real one: exit 1, URLs
named. Clean run still exits 0.
Two more fixes to the same script, both found by it failing in front of me:
* `--max-time 120` bounded how long a download may legitimately TAKE, so it
killed OpenGL-Registry at 22MB of a healthy transfer, called it "FAILED to
fetch", and did it again every re-run -- a capture that could not finish,
blaming the mirror. Now --speed-time/--speed-limit, which is the property
actually wanted ("no progress for 60s") and cannot mistake a big file for a
dead one. With it the capture got past that file and completed.
* It truncated its output on start, so every interruption of a ~50-minute
network-bound job cost the whole run (todo c2affe6b). Now append + dedupe: the
re-run resumed from 72 rows instead of 0.
Three guards, each verified to bite. The re-capture is running again to recover
the five URLs; SDL is already confirmed to move 3.4.12 -> 3.2.28, which is the
staleness that started this.
vcpkg_capture_assets.sh passed --only-downloads unconditionally, under a comment claiming it was "enough because the asset hook fires during resolution". I wrote that sentence from intuition, not measurement, and it is false.
Download Mode makes vcpkg refuse to EXECUTE anything, and a portfile that stops executing stops downloading. angle downloads gni-to-cmake.py (overlay-ports/angle/portfile.cmake:79), sets up a python venv to run it (:86, x_vcpkg_get_python_packages), and only THEN downloads four WebKit files: include_CMakeLists.txt, WebKitCompilerFlags.cmake, DetectSSE2.cmake, WebKitMacros.cmake (:123 :129 :144 :151). In Download Mode :86 halts, so those four URLs are unreachable by construction. That is the 71fb301a re-capture writing 72 rows where the committed capture has 76 -- five URLs short, reported as success. The committed 76-row capture therefore cannot have come from this script's default mode; it came from a full build.
So the full build is the default now and --only-downloads is opt-in via CAPTURE_ONLY_DOWNLOADS=1, for refreshing URLs you already know need no execution.
A halt is also the second way a capture loses URLs with no failed download at all, and vcpkg exits 0 for it ("Downloaded sources for angle", then "All requested installations completed successfully"). There is no exit code to read and no asset-script callback for a step never reached, so vcpkg's stdout is the only witness: it is teed to a log the driver greps, and a halt lands in the same $FAILED sentinel that already refuses to bless a capture with a failed fetch. The report names the PORT, from the "Installing N/M <port>:<triplet>" line above the halt -- the halt message itself names a shared helper (vcpkg_execute_required_process.cmake:23, identical for every port) and a versioned bt/versioning_ path, neither of which a reader can act on. The match is case-insensitive because vcpkg spells it two ways ("Halting portfile execution." and "Download failed, halting portfile.").
Note what CANNOT be done: triage halts into safe and unsafe. In the download-only run 58 of 77 ports halted, most at vcpkg_cmake_configure where every download already happened; nothing in the log distinguishes those from angle's mid-sequence halt, because only the portfile knows whether a download follows. So any halt fails the capture.
pipefail was already on, which is what keeps the new `| tee` from swallowing a vcpkg failure -- verified with a fake vcpkg exiting 3. On the failure path the log is kept and its path printed (it is the evidence for which port halted); on success it is deleted.
Three guards in tests/test_emit_vcpkg.py, each verified to bite by mutation: unconditional --only-downloads, a dropped tee/log, a case-sensitive halt match, a halt not attributed to a port, a halt not reaching $FAILED, and a dropped pipefail are all caught. Also loosened the existing failed-download guard, which asserted one exact sentence of the message it was checking.
Third silent-loss mode in this script, and the only one that leaves no trace whatsoever. vcpkg's binary cache is keyed by each port's ABI hash; on a hit it unpacks the archive and does NOT run the portfile, so the port requests none of its downloads. No error, no halt, exit 0, "All requested installations completed successfully". Measured rather than reasoned: a zlib-only manifest run twice against a warm ~/.cache/vcpkg/archives, the second with a fresh install root. Run 1 built zlib and captured 3 rows. Run 2 printed "Restored 3 package(s)" and "completed successfully in: 1.54 ms", exited 0, and captured ZERO. The capture passed no --binarysource at all, so it inherited whatever cache the machine had -- which on its own explains a re-capture that cannot reproduce the committed rows, independently of the Download Mode bug in the previous commit. vcpkg_build.sh has passed --binarysource=clear since it was written, for the adjacent reason (a restore proves nothing about building from source); the capture never did. So --binarysource=clear is now passed, before "$@" so a caller can still override it deliberately. And because the flag states intent while the log states outcome, the outcome is checked too: "Restored N package(s) from <cache>" and "The following packages are already installed" both land in the $FAILED sentinel that already refuses to bless the capture. The second matters as much as the first -- the 71fb301a capture's second run had 7 already-installed ports, whose distfiles it could not have captured. Verified with fake vcpkgs printing each line and exiting 0: the script exits 1, and the already-installed case reports the count. Also retriaged the download-only run properly instead of assuming angle was special. Parsing each halt's own call stack for the portfile frame and grepping that portfile for download calls AFTER the halt line: of the 61 halts, exactly ONE port lost downloads -- angle, halted at :86 with vcpkg_download_distfile calls remaining at 122/129/144/151/165. The rest halted at vcpkg_cmake_configure or in vcpkg's own post-install steps, after every download. That matches the five missing URLs exactly (angle's four WebKit files, plus SDL, which moved 3.4.12 -> 3.2.28 with the pin and is the one intended change). Two guards in tests/test_emit_vcpkg.py; four mutations verified to bite (flag dropped, flag moved after "$@", restore detection removed, already-installed detected but not reported). Also replaced a 0-800-character regex window in the older failed-download guard with a real block extraction -- it passed for the wrong reason once the message grew.
…s intended
Closes the repin's last derived input. The capture is re-taken at 71fb301a and Meta/vcpkg_assets.tsv, vcpkg_distfiles.bzl, vcpkg_index.bzl and MODULE.bazel's use_repo now agree with vcpkg.json: sdl3 is release-3.2.28, not the 3.4.12 the old pin's capture carried.
The result is checkable rather than asserted. Of the 76 rows, 75 reproduce the previous capture's (url, sha512) BYTE-FOR-BYTE and the single difference is the intended sdl3 move -- so the 21 other capture-only URLs (ANGLE/WebKit/curl/expat/freetype/sqlite) that todo 37a5e4ee flagged as possibly stale the same way are confirmed NOT stale, which is the part I could not have concluded from the emitter's diagnostics. Independently, every one of the 76 SHA512s was re-hashed against the bytes actually sitting in the downloads dir: 76 ok, 0 mismatches.
Two runs were needed, and the second one taught me something. angle was the ONE port whose downloads the Download Mode halt had eaten (derived, not guessed: parsing each halt's own call stack for its portfile frame and grepping that portfile for download calls after the halt line gives exactly one port of 61 halts). But a one-port supplementary manifest resolves its dependencies from the vcpkg BASELINE, not from Ladybird's vcpkg.json overrides -- so it fetched zlib 1.3.2 where Ladybird pins 1.3.1 and the merged capture had both.
I deleted that row by hand and then recognised the shape: a hand-fix nothing checks is what the next capture silently repeats, which is the whole subject of this session. classify_static_only could not see it because it only looked one way (derived-but-not-captured). The mirror is just as derivable -- a CAPTURED row in the same URL family at a version the derivation does not pin came from the wrong resolution -- so classify_capture_only now reports it as a LEAKED CAPTURE ROW, and on the 77-row merged file it names zlib 1.3.2 vs the pinned 1.3.1 exactly. Its guard also pins the boundary: a capture-only row with NO derived sibling (angle's expanded ${VAR} URLs) is not a leak, it is the reason the capture replaces the static parse at all.
README: gap 10 no longer claims vcpkg_distfiles.bzl fetches 3.4.12 "to this day", and records the one-directional-check lesson. New gap 11 documents the four ways a capture silently loses rows while vcpkg exits 0, including that the warm downloads/ dir the RESUME rule asks you to share is itself one of them -- the property that makes a 50-minute job restartable is the property that makes its output incomplete.
…f loss modes
Three ways of losing capture rows turned up in one session, each found by hitting it and each guarded individually: a halted portfile, a binary-cache hit, an already-installed port. That is a losing pattern, and the fourth was already sitting in the log I had been reading all along -- "-- Using cached gni-to-cmake.py". A file already in --downloads-root makes vcpkg skip the asset script entirely, so the recorder never sees it and there is no row.
Which means the RESUME rule is itself a way to produce an incomplete capture. Sharing downloads/ across runs is what makes a 50-minute network-bound job restartable (todo c2affe6b, and the reason the script appends rather than truncates); it is also what stops the second run from recording what the first one already fetched. The two are in direct tension and the comment I wrote for it claimed only the upside.
So the check now compares against something the script does not control. vcpkg announces every download it resolves -- "Trying to download <name> using asset cache script" or "-- Using cached <name>" -- so require a captured row per announcement and stop depending on my enumeration being complete. Two subtleties, both derived rather than listed: an ABSOLUTE path in the second form is vcpkg_from_git pre-placing its own archive (libyuv and skia's two), which bypasses asset caching by design and is pinned by vcpkg_git_archives.bzl instead, so relative name = asset download and absolute = git; and the comparison undoes the two {dst} manglings the recorder is deliberately dumb about (the .<pid>.part suffix and vcpkg's 8-hex disambiguator).
Validated against all three real logs before being trusted: the cold 72-row run and the warm resume both come out complete, and the angle-only run reports its four already-cached files as missing against the angle-only tsv but clean against the accumulated capture -- the correct answer in both cases. Five mutations verified to bite, including hardcoding the git ports instead of deriving them from the path shape.
… complete Rebuilt all six binaries against the re-emitted vcpkg rules (RC=0, 242 actions after the 77-port vcpkg action re-ran, which the changed distfile set required) and re-rendered. Verified by removal rather than by assertion: Bazel's own vcpkg tree now carries libSDL3.so.0.2.28 and sdl3_3.2.28_x64-linux-dynamic.list where it carried 3.4.12 before, so the pin the emitted rules fetch is the one vcpkg.json asks for. --headless=text and --headless=layout-tree are byte-identical to the CMake reference (same md5). Two things done deliberately, both from this session's own lessons. The reference output was REGENERATED from Build/full71/bin/Ladybird on the same page in the same session, instead of being compared against the 119-line count in my notes -- that count was for a different test page, and comparing against a remembered number would have manufactured a mismatch or, worse, agreed by luck. And per the f15a410 autopsy I checked what would EXECUTE before trusting the output: no libexec/ exists next to the binaries (the path that shadowed the build with six-week-old services), and all six binaries carry today's build timestamps. apply_overlay.sh --verify: 45/45 overlay files identical (the two PATCH NOT APPLIED lines are the expected state for the build tree). Suite 287/287. Closes the repin master todo and the vcpkg re-capture todo.
…eproduce Ulf handed over upload/11041.patch: three commits by sideshowbarker, in review upstream. All three apply cleanly at our pin 71fb301a, individually and as a series (git apply --check, RC 0). Read against docs/UPSTREAM-ladybird-fd-leaks.md they close the investigation. The mapping, because two of the three are ours and one is the thing we could not build. 1/3 is our 0001's call site. 2/3 -- abort()/terminate() never telling the network layer -- is a class we NEVER diagnosed: it looks identical to any other retained peer=DEAD socket in fd_census.py, so no amount of my census data would have produced it; it is a reading-the-code finding. 3/3 is exactly the open lead 6a311f55, a navigation parked in wait_for_sniff_bytes with no document, so Document::abort() has no controller to stop and only the arrival callback (which never runs) releases the request. Why 3/3 stayed open on my side is the useful part: I built two workloads for that hypothesis (330 abandoned dribbling navigations with site isolation + memory cache, and 300 cache hits) and both came out FLAT. Upstream's reproducer is more specific than either -- an iframe REMOVED while its response has headers but fewer bytes than the sniff threshold, i.e. the navigable must be destroyed while parked, not merely the navigation abandoned. My workloads abandoned navigations without destroying the navigable, so they tested everything except the condition that matters. A falsified workload was evidence about my workload, and I came close to filing it as evidence about the hypothesis. One real difference found by reading rather than assuming, and I had written "same fix as ours" before checking. Both patches add defer_teardown() inside the same if, but on OPPOSITE sides of user_on_finish(): upstream before, ours after. Upstream's is correct, and their message says why -- defer_teardown() captures NonnullRefPtr(*this) inside the deferred lambda, so calling it first pins the Request across the callback. user_on_finish is the fetch completion path, which is where the last ref can go away (Response holds its Request by RefPtr, Responses.h:226), so our ordering can run the teardown on a half-destroyed object or never reach it. It never fired for me because something always outlived the callback on my workloads -- a latent ordering bug that surfaces as a rare crash on someone else's machine, not as a leak on mine. Also: our 0002 (release_response_fd) has NO upstream counterpart. Upstream fixes the leak by ensuring the teardown is REACHED on all three paths; 0002 closes the fd defensively on the theory that a surviving reference pins it. If upstream's three take the rate to zero, that theory was unnecessary and 0002 was a workaround for a missing call site. Carrying both forward would be two mechanisms closing the same fd, one justified by a theory the other falsifies. Nothing deleted yet -- #11041 is unmerged. Both our patches already carry .effect-grep files, so --verify accepts upstream's equivalent instead of our exact bytes, which is what that mechanism was built for; a repin past #11041 deletes both patches rather than breaking verification.
…ting HEAD Ulf, on being handed the previous behaviour: "It just ignores what I have and creates a floating HEAD with a bunch of uncommitted files, which is FUCKING IDIOTIC." He was right. My two previous answers to "how do I get your changes into my ladybird tree?" both missed the point: the objection is not that anything gets lost (nothing did), it is the SHAPE of what the script produces. A detached HEAD is not a place you can work. rebase, merge, cherry-pick and pull --rebase all need a named ref, so the overlay could not be composed with the branch the reader actually has. And 45 untracked files mean `git status` is permanently 45 lines of noise, `git diff` shows nothing (untracked files are not diffed), `git log` says nothing happened, and one stray `git clean -fd` deletes the entire overlay. Worst, it walked past the branch and commits the reader already had to get to a floating checkout. So the overlay is now expressed the way every other change to a git repo is: one commit per patch (keeping the patch's own Subject:) plus one for the ~44 Bazel files, on a branch named after the pin and based on the pin. git status is clean when it finishes. New flags: --branch NAME, --onto-current (base it on YOUR HEAD, with a warning that the generated BUILD files name ~1,961 sources by path and were generated from the pin), --no-commit (the old behaviour, for a throwaway tree). A re-run is idempotent -- it resets the overlay branch to the pin and rebuilds its commits -- but refuses if that branch holds a commit that is not the overlay's, naming the commit and offering --branch/--onto-current instead. Verified: a foreign commit stops the rerun and survives it. Two real bugs fell out of testing this, both of which predate the rewrite: * the already-applied test was asked of each patch instead of the series. 0002 edits lines adjacent to 0001's in the same function, so on a fully patched tree 0001's context no longer exists and `git apply --check -R 0001` fails -- the script then re-applies an applied patch and dies. Invisible until now because every run started from a pristine checkout of the pin. Reverse-checking the concatenation asks the right question; verified it succeeds on the patched tree and fails on the pin. --verify had the same bug and reported PATCH NOT APPLIED for a correct tree. * --verify asked `HEAD == pin`, which was right while the pin was checked out with everything uncommitted and became wrong the moment the overlay became commits: it now tests ancestry, and reports non-overlay commits on top as a note, since only those can move the paths the BUILD files name. Build/vcpkg/BUILD.bazel is deliberately not committed: Ladybird ignores Build*/, and being ignored it makes no status noise anyway. Exercised end to end against a replica of Ulf's tree (pin + his own commit on his own branch): 3 commits, clean status, his branch untouched, and the advertised `git rebase <overlay branch> my-work` replays his commit on top. Suite 292/292 (5 new tests); --verify on the real tree is byte-for-byte the same report as before the change.
Ulf hit: ERROR: .../external/rules_qt++qt+qt/BUILD.bazel: no such target '@@rules_qt++qt+qt//:QtPositioning': target 'QtPositioning' not declared in package '' ... and referenced by '//:ladybird' The cause is not in our BUILD files. rules_qt's qt.local_repo DERIVES its cc_library targets by listing the host Qt's lib directory (_create_libs_symlinks in qt_local_repo.bzl, keyed on libQt6<Module>.so*), so a Qt module the host does not have is simply never declared. QtPositioning ships in qt6-positioning-dev, which upstream made REQUIRED at the 71fb301a repin (UI/Qt/CMakeLists.txt:8, for GeolocationProviderQt.cpp) -- so the missing apt package is reported as a missing target in a GENERATED file inside Bazel's output base, mentioning neither Qt nor apt, pointing at a file the reader did not write and cannot fix. This is finding 38's shape one layer out: a host probe whose absence is reported as a defect in your own code. It is also the same drift that caused the qt_label bug -- a three-entry table of the modules Ladybird used when it was measured, going stale when upstream needed a fourth. qt_runtime.bzl already preflighted the Qt VERSION floor, which was the right idea at the wrong scope: it asked "is the SDK new enough" and never "does the SDK have the parts we link". Both are properties of the discovered SDK, so the module check goes next to it, and reads QT_INSTALL_LIBS -- the same input qt.local_repo derives from, with the same name mangling as _create_lib_name -- because a repository rule cannot query another repo's targets, and any proxy (asking for the target, probing a header, trusting the version) could disagree with what rules_qt actually does. The failure names the module, the apt package, and the fact that @qt is cached so a reader who installs it and re-runs is told to bazel sync --configure rather than concluding it did not work. _QT_MODULES stays an INDEPENDENT list of names rather than being derived from BUILD.bazel: the emitter writes those @qt//:Qt* deps, so deriving from the same source would only prove the generator agrees with itself. A test asserts the two agree, which is what catches a Qt module added by a future repin that skips its preflight entry -- verified to fail when the QtPositioning entry is deleted. Exercised against real Bazel both ways, using a synthetic Qt prefix (every libQt6* symlinked except libQt6Positioning.so*, with a qmake shim reporting that prefix): the check fails with the package named, and resolves @qt_plugins//:runtime_libs once the library is restored. Suite 294/294.
…ice to the SDK found Ulf: "I don't think that'll work. We're using Qt (6.9.2) from a VENV, and system Qt is 6.4.2." Correct on both counts, and my previous answer was actively wrong for his setup. TWO separate defects. 1. `apt install qt6-positioning-dev` is the wrong instruction for a self-contained SDK. apt drops libQt6Positioning.so into /usr/lib/x86_64-linux-gnu, which is NOT the lib directory a venv/aqt Qt reports, so the module stays missing, the error is unchanged, and the reader reasonably concludes the advice was wrong -- because it was. _QT_MODULES now carries both the deb package and the aqt module name, and the message picks the form matching the SDK it actually probed, naming that prefix and lib dir so the claim is checkable. For a self-contained SDK it says explicitly that apt CANNOT fix it and why. Distinguishing the two reuses _SYSTEM_LIB_DIRS rather than a list of "prefixes that mean distro": test_plugins_come_from_the_same_sdk_as_the_libraries forbids absolute /usr literals outside that list and rightly failed on my first version, which hardcoded /usr/lib/qt6 -- the exact class of thing this file exists to remove. 2. The overlay was overwriting his Qt path on every re-apply (todo bd753248). MODULE.bazel's qt.local_repo `paths` line is the one line in the overlay that is a fact about the READER's machine and not about Ladybird at the pin, and the copy phase clobbered it with the capturing machine's /usr/lib/qt6. For him that is not merely different, it is BELOW Ladybird's 6.9 floor -- so a re-apply turned a working tree into a failing one, and the failure surfaced later and elsewhere (he hit the 6.4.2 floor error). It is now RESOLVED, not imposed, first rule winning and every rule reported: --qt-prefix DIR, else the line already in the target's MODULE.bazel (this is what makes a re-apply safe), else the qmake first on PATH (activating a venv is how you say which Qt you mean, and it is what rules_qt would do), else the historical default. --verify normalises that one line and reports the configured prefix instead of DIFFERS, because telling someone their own correct configuration is a defect is how they come to overwrite it. When the prefix is readable the script prints the Qt version at apply time and warns immediately if it is below 6.9, instead of letting it fail minutes into a build inside a repository rule. Verified: two consecutive applies, the second with no flags, leave a fake venv Qt 6.9.2 in place; a fake 6.4.2 SDK is flagged at apply time; --verify reports 45/45 with the Qt line differing; and both install-hint branches were exercised, the self-contained one through real Bazel against a synthetic SDK with libQt6Positioning removed. Suite 297/297 (3 new apply_overlay tests). Also relaxed test_file_list_is_derived_not_hand_maintained: it read `note "...qt_runtime.bzl..."` as a hand-listed file roster. Diagnostics are prose for the same reason comments are -- a test that cannot tell a message from a roster forces the message to be vaguer to go green.
The script ran `Meta/ladybird.py vcpkg` itself but only PRINTED `Meta/fetch_vcpkg_git_archives.py` in its closing message. So it reported done, the obvious next command was `bazel build`, and that died inside the vcpkg action: Building 77 vcpkg ports (no network) failed vcpkg_build: no git-sourced externals at ./Meta/CMake/vcpkg/git-archives vcpkg_build: 4 are required (see VCPKG_GIT_ARCHIVES in vcpkg_git_archives.bzl) Ulf hit exactly this. The diagnostic is the good one I added earlier -- it fails in four seconds naming the directory and the file, rather than ~20 minutes in inside skia's portfile naming a googlesource URL -- but it is still a failure on a tree the script had just called done. The four tarballs are not optional and not derivable from anything else in the tree (vcpkg_from_git shells out to `git fetch`, which no asset source intercepts), so a tree without them cannot build. So phase 3 runs it, guarded on the vcpkg checkout the first prefetch creates (the fetcher resolves each clone URL out of the portfiles, which is the ordering constraint that made it a message in the first place -- and ordering is a thing a script can express). It is idempotent: on an already-fetched tree all four verify from cache in under a second. --verify checks them too, by NAME against the committed pin rather than by counting files (three of four present is the interesting broken case and `ls | wc -l` calls it fine). Ulf's tree passed --verify and then failed the build on this, which is precisely the thing --verify exists to prevent. Hashes are not re-verified there: that is minutes of sha512 over ~200MB and the fetcher already verified them at write time. One test had to be sharpened rather than satisfied. test_file_list_is_derived_not_hand_maintained bans a .bzl filename in code, because a hand-kept roster is how vcpkg_git_archives.bzl came to be generated, committed, documented and loaded by nothing. The new check READS that same file to learn which tarballs to expect -- the opposite of the bug -- so the ban is now on enumeration: a .bzl name that is not the input of a read. A test that forbade the fix would have demanded the hardcoded list. Two new tests: every required prefetch is run and not merely printed (with here-docs stripped, since prose was the bug), and the git-archive prefetch runs after the checkout it reads portfiles from. The first draft of the second one compared t.index() positions and failed on correct code -- it found the MENTION inside --verify's diagnostic, not the invocation.
…_PATH
Ulf's build crashes immediately and he remembers LD_LIBRARY_PATH fixing it.
That is the problem: TWO unrelated failures present as "immediate crash" and
LD_LIBRARY_PATH masks BOTH, so reaching for it destroys the one datum that
says which one he has.
A. the LIBRARIES: an aqt/official SDK bundles its own ICU; libQt6Core finds
it through `RUNPATH $ORIGIN`, and $ORIGIN is the dir the loader opened the
object by -- Bazel's solib dir, which has no ICU. Death before main().
@qt_plugins//:runtime_libs is the fix (SDK private libs as real link
inputs, so OUR runpath resolves them).
B. the PLUGINS: Qt dlopens the QPA plugin from the compiled-in prefix or the
executable's directory; the distro's libqxcb.so in an SDK libQt6Core is
SIGSEGV in QXcbConnection::initializeScreens. //:qt_conf + //:qt_plugins
is the fix.
Both fixes are in the tree, so the useful question is not "which fix does he
need" but "which fix failed to FIRE on his Qt" -- and I have to admit the
likely answer is mine, not his: the private-library derivation has NEVER run
for real on this machine. My Qt is a distro Qt (/usr/lib/x86_64-linux-gnu),
which by design makes runtime_libs an empty target -- confirmed, it is
literally cc_library(name = "runtime_libs") with no deps here. His aqt SDK is
the first genuine self-contained Qt this code has met; the only other exercise
was a synthetic fake SDK in /tmp. That is exactly the finding-35 shape the
header warns about: an empty result is CORRECT for a distro Qt and WRONG for
an SDK, and the two are indistinguishable without asking.
So the script asks, in seven sections: which Qt MODULE.bazel names, which Qt
@qt actually discovered (its own generated qtconf.bzl, not a guess), whether
rocntime_libs has any cc_imports and what the SDK's lib dir ships as the input
to that derivation, whether qt.conf and the 79 plugins are staged and WHICH
SDK the resolved libqxcb.so belongs to, what the binary resolves at load time,
and finally the run itself with LD_LIBRARY_PATH explicitly UNSET (env -u) --
because with it set the whole exercise says nothing. Then it says how to read
the output.
Ran it against my own tree: passes end to end, correctly reports the distro-Qt
case as the legitimate empty one, and --version exits 0. It also caught a bug
in itself on the first run -- `grep -c` prints one count per file, so a glob
gives "0\n0" and `[ -eq 0 ]` dies with "integer expected". Fixed with `grep -h
| wc -l`.
Ulf's crash is not Qt, and I sent him a Qt diagnostic. The evidence was in the first four lines of his paste and in the backtrace's PATHS, not its frames: ladybird -> bazel-out/k8-fastbuild/bin/ladybird (fresh) Compositor -> bazel-out/k8-fastbuild/libexec/Compositor (stale) LibWebView/Utilities.cpp's get_paths_for_helper_process() searches <prefix>/libexec/<name> BEFORE <prefix>/bin/<name>, so any libexec copy wins over what Bazel just built and only the copy ever runs. After a repin those copies are from the OLD pin, whose IPC message ids have shifted, so every message fails with `Endpoint magic number mismatch, not my message!` and the cascade ends in SIGILL with a Qt-flavoured backtrace (QApplicationPrivate, QEventDispatcherGlib, libQt6Core frames). It looks exactly like a Qt runtime bug. It is not one -- and his LD_LIBRARY_PATH=~/Qt/6.9.2/gcc_64/lib was a red herring pointing the same wrong way. This is the SECOND time this failure has cost real time (the README already records the first, where I spent hours on IPC codegen parity and an ABI-skew theory before checking what was executing), and todo 4a93a257 is the lesson written down: for a "built fine, behaves wrong" bug, ask what is EXECUTING before auditing what produced it. A lesson in a todo did not stop me repeating it, so it now runs as code. Section 0b of the diagnostic checks it FIRST, before any Qt question, and prints the shadowing directory, its contents, its newest mtime and bazel-bin's for comparison, then the exact rm -rf. Section 8 answers the question the backtrace answered and no build check does: for each of the five services, which copy is first on the lookup chain. Both verified against my own tree -- negative case clean, then positive case reproduced by planting a single August-dated Compositor, which also reproduces the detail that only SOME services are shadowed (why his ladybird ran fresh while Compositor died). Three tests: the libexec check precedes the Qt checks by string position (a diagnostic that asks about Qt first sends the reader into the wrong subsystem, which is what I did), it states the FIRST/second lookup order so "delete libexec" is not a superstition, and the run step keeps `env -u LD_LIBRARY_PATH` -- with it set, both Qt failures disappear and the run proves nothing. Adapted to test_run_recipe.py's existing os.path style after the first draft used a pathlib REPO the file does not define.
Ulf's diagnosis, and it is the right one. `ls -la` was the whole datum:
-r-xr-xr-x 107107448 Aug 20 00:38 bazel-bin/ladybird
-r-xr-xr-x 99264432 Aug 11 20:36 bazel-bin/WebContent
The services are found by PATH at runtime (get_paths_for_helper_process), not
linked, so NOTHING in the build graph said the browser needs them. `bazel build
//:ladybird` therefore built the browser and left whatever WebContent happened
to be in bazel-bin from a previous build -- here one from the PREVIOUS PIN.
Upstream inserted ~3 IPC messages between f9e34731 and 71fb301a, shifting every
id after them, so every message failed to decode:
Local endpoint error: Can't read past the end of the stream memory
Peer endpoint error: Endpoint magic number mismatch, not my message!
That reads like a codegen or ABI bug, which is why I got it wrong twice before
decoding the bytes. The magic number 0xffa5367a is
AK::string_hash("WebContentServer") -- the CORRECT endpoint, so the mismatch
line is the peer's rejection, not the diagnosis. The message NUMBERING is what
disagreed, and it is decisive: his ids matched f9e34731 7/7 (SetSystemFontFamily
147, SetWindowSize 126, InspectDomTree 41, SetPreferredLanguages 116,
SetBrowsingBehavior 117, ResetZoom 128, SetHasFocus 121) and 71fb301a 0/7. I
confirmed by running the current generator over the old pin's .ipc.
So: the five services become `data` of //:ladybird. `data`, not `deps` --
separate processes, not link inputs -- which is the relationship this emitter
already gets right for cranelift-compiler on LibWasm, and which also puts them
in the runfiles tree for `bazel run`.
The list is DERIVED from the launch_server_process<> string literals in
HelperProcess.cpp, not hand-written: upstream ADDS services (Compositor is new
since the previous pin), and a hand-kept roster is one new service away from
silently reintroducing exactly this bug. An empty parse is a hard failure, since
returning [] would look like success and rebuild nothing.
Verified on my tree, not asserted: all five now appear in
`bazel cquery deps(//:ladybird)`; touching Services/WebContent/main.cpp and
building ONLY //:ladybird rebuilds WebContent (2 sandbox actions, compile +
link) where before it was untouched; and --headless=text renders with no IPC
decode errors at all.
The recipe's six-target `bazel build` line was a WORKAROUND for this missing
edge, and the reason the bug was survivable for so long -- so README and
apply_overlay.sh now say `bazel build //:ladybird`, which is true.
Two tests: //:ladybird declares each service in data and NOT in deps, and the
list is derived from Ladybird's own source with no roster in the function.
Ulf: "We should use the upstream patch". Yes -- and not just because upstream's
is upstream's. Mine was BROKEN, and his crash trace named the line:
VERIFICATION FAILED: m_ptr at ./AK/OwnPtr.h:134
#0 ...CallableWrapper<Requests::Request::set_up_internal_stream_data(
...)::{lambda()#2}>::call()
#1 Core::Notifier::event(Core::Event&)
{lambda()#2} is the read notifier's on_activation -- the frame that CALLS
on_finish. My 0002's release_response_fd(), invoked from inside on_finish's
completion branch, set m_internal_stream_data->read_stream = nullptr while
on_activation was still on the stack and still about to dereference it:
if (m_internal_stream_data->read_stream->is_eof()) // Request.cpp:376
m_internal_stream_data->read_notifier->close();
AK::OwnPtr::operator-> is VERIFY(m_ptr), so the null became a trap, then SIGILL.
A use-after-null ONE STACK FRAME UP from the code I changed: invisible on every
workload I built here, a crash after a few minutes of real browsing on his.
My framing was upside down. I argued 0002 was needed because "collectable" and
"closed" are different claims and for an fd received over IPC only the second is
the bug. True, and the wrong question: it is not WHEN the fd is closed but WHO
owns the state while the stack is still unwound. The completion branch runs
INSIDE the read loop's callback, so anything it destroys, the caller may still
touch. Upstream doesn't close the fd at all -- it makes the existing DEFERRED
teardown reachable on the three paths where it was missed, and deferred is
precisely what makes it safe. My mechanism was in direct conflict with the one
the code already had.
Worse, I had already spotted the same bug in our 0001 (defer_teardown() after
user_on_finish instead of before, so the Request is not pinned across the
callback), written that it was "the kind of latent ordering bug that shows up as a
rare crash on someone else's machine" -- and carried it anyway because it had
never fired for me. "Never observed here" is a statement about my workloads.
So patches/ is now upstream's three commits (63817a66, 2ae78115, 8636af41),
split out of upload/11041.patch, each annotated IN PLACE with its provenance and
the order-matters reasoning, each with an .effect-grep so --verify recognises the
merged fix on a tree newer than our pin instead of demanding a patch that would
conflict. Verified: all three apply in series at 71fb301a (RC 0), the resulting
source has defer_teardown() BEFORE user_on_finish, no `read_stream = nullptr`
anywhere, and no release_response_fd at all.
Built and ran it: patch 1 adds openResponsePipeCount() to Internals.idl, which
correctly invalidated and rebuilt the whole LibWeb binding set through the
overlay's codegen (1571 actions), --headless=text renders, and a 150-fetch +
50-abort workload against a local server produced zero VERIFICATION FAILED /
SIGILL and a flat fd census.
One detour worth recording: the first build died with `ar failed` and NO
diagnostic, on the LibWeb archive. That is todo e91c1905 -- Bazel+binutils report
ENOSPC as an empty tool failure that reads like a code error. Disk was at 95%;
pruning the disk cache fixed it. I recognised the shape this time instead of
debugging the code.
Tests: the two tests that named the deleted patches are replaced by property
tests -- every patch in the series must have an .effect-grep (derived by glob, not
by filename, which is how those tests rotted), the windowed check is found by
CONTENT rather than name, and a new test asserts no patch may null read_stream or
reintroduce release_response_fd. A future "optimisation" back to my version fails
loudly.
I built this page in /tmp to verify upstream #11041 on my tree, and /tmp does not survive a sandbox restart. It is the workload that exercises two of the three paths #11041 fixes -- ordinary completion (150 settled fetches, the branch my own patch got wrong) and cancellation (50 aborts, the class my fd census could not name) -- so it is worth more than the five minutes it took to write. The header records what it does NOT cover and why that matters: the third path needs a navigable DESTROYED while parked for content sniffing, which is exactly the condition my two earlier workloads missed. They came out flat and I nearly read that as evidence against the hypothesis instead of evidence about the workload. Also records the two harness facts that cost me time just now: it needs a real HTTP origin, and --headless=text snapshots before the async loop finishes (so it reports nothing useful -- use --headless=manual plus fd_census.py --watch and read the RATE, not the level).
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.
Branches off
mainat d4a3d24 (PR #10). 16 commits; every claim below was verified by doing the thing, on a fresh clone, not by reading the graph.Why this exists
The end-to-end test nobody had run:
git clone ladybird, drop in the overlay,bazel build. It failed seven times, andBuild/full— the dependency I had removed and was looking for — was only one of them. The lesson, recorded in finding 36 ofdocs/CASE-ladybird-migration.md: the only check that finds all of them is doing the clone.Result now:
//:vcpkg_installedbuilds all 76 ports offline, all six binaries build (2,842 actions, RC=0), and--headless=text/--headless=layout-treeare byte-identical to the CMake reference on all three test pages.What a fresh clone needs, and what closed each gap
Build/vcpkg+ its.git(load-bearing: vcpkg resolves versioned ports withgit read-tree)python3 Meta/ladybird.py vcpkg, ~70 s, no CMakevcpkg_from_gittarballsMeta/fetch_vcpkg_git_archives.py— 4/4 reproduced withgit clone+git archiveand verified against the committed SHA512smain, unversioned)hsts_preload.bzl— pinned downstream to a commit + sha256, fetched by BazelPlus three fixed bugs, of which the middle one matters most:
Meta/vcpkg_build.shpassed the distfile index execroot-relative while vcpkg invokes the asset script from its own cwd — every asset lookup failed andx-block-originthen correctly refused the network.angleport callspip install ply, which never touches the asset cache, so nothing pinned it — and nothing stopped it either:requires-network: "0"is a scheduling hint andno-sandbox: "1"means there is no namespace to enforce it in, so the action inherited this sandbox'sHTTP_PROXYand pip reached PyPI for months. The wheel is now pinned by URL+sha256 and pip runs withPIP_NO_INDEX. A control that is not enforced is indistinguishable from one that is not there.vcpkg_git_archives.bzlwas generated, committed, documented — and loaded by nothing, with staging that had three ways to succeed while copying nothing. Now a hard error in 4 s instead of agit fetch … Error code: 128twenty minutes in.The HSTS pin — the part worth reviewing
Upstream's
hsts_preload.cmakefetches Chromium'smainat configure time, and we do not control upstream. My first answer ("pin it upstream, stage it until then") was wrong because it made someone else's repo a prerequisite for our hermeticity. So it is pinned downstream, and the general shape is:What misled me was pinning the wrong revision. A Chromium release tag looks like the responsible choice and is the one that breaks parity:
139.0.7258.5serves 18.7 MB → 168,593 entries againstmain's 94,626. The commitmainis serving has bytes identical to CMake's download (cmp, 10,521,748 bytes), so pinning it costs no parity at all.Also measured, and the reason this is a real pin rather than a convenience:
http_filedoes work with nosha256on themainURL — and then Bazel caches the first fetch forever (local origin, changed upstream, rebuilt → old content, 0.3 s, no warning). The table moved during this work (service.gov.scotleft the list, 94,627 → 94,626), so an unpinned fetch would have broken byte-parity for a reason unrelated to the migration.Meta/pin_hsts_preload.pyre-pins by measuring — it writes the hash it computed — and--expect-same-asrefuses to write a pin whose bytes differ from the file the other build system already has, so the trap I fell into is now a hard error. Verified on the fresh clone with the CMake-downloaded file deleted: outputs byte-identical to the CMake reference,//:LibHTTPbuilds (RC=0).Residual cost, stated rather than hidden: CMake still tracks
main, so a configure newer than the pin disagrees with Bazel — one pinned input against one unpinned one, a dated disagreement with a sha to look at. Closeable without upstream, sincedownload_fileno-ops when the file exists (verified withENABLE_NETWORK_DOWNLOADS=OFF). The upstream one-liner is filed as a bug, not depended on.Rejected shortcuts (each looked like a simplification)
--depth 1on vcpkg — 8.7 MB not 121 MB, then fails on ffmpeg/harfbuzz with vcpkg's own "Try again with a full vcpkg clone": pinned port trees live in history.builtin-baseline— resolves happily and silently moves 10 dependency versions (ffmpeg 7.1.1#5 → 8.1.2#3, harfbuzz 10.2 → 14.2.1, mimalloc 2 → 3, …)..gitandread-treefollows the indirection: checked), but a submodule pins a checkout while vcpkg pins history behind a baseline. 14 ofvcpkg.json's 45overridesname a version that is not whatports/holds at the baseline.Build/vcpkgis not a vendored dependency; it is a package manager's cache that happens to be a git checkout.Tests
python3 tests/run_all.py→ 164/164 across 13 files. Includes a runner that fails when a test file goes silent — three files had been running zero tests, and only 6 of their 46 assertions were even right.Notes for the reviewer
.bzl/BUILD.bazelfiles underexamples/ladybird/workspace/are generated byMeta/emit_*.py; regeneration was checked to reproduce the committed bytes.docs/CASE-ladybird-migration.mdfinding 36 records four successive wrong answers to "what does a clone need", each corrected by the next, and why each was wrong. That history is the point of the doc, so it is kept rather than tidied.