diff --git a/CHANGELOG.md b/CHANGELOG.md index 36f89c10..675ab4ac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,48 @@ cycle-accurate core later replaced. ### Fixed +- **The browser demo applied no per-game header corrections.** Every mapper, + submapper and region fix the vendored game database ships was silently absent + on the web build — Seicross, which needs submapper 4 to clear its protection + loop, hung there exactly as it hung on the CLI before v2.3.4. + + The mechanism is the interesting part, because this is the **third** time the + same correction has been skipped by a load path that does not go through the + File-menu chokepoint: the CLI (fixed v2.3.4), the mapper-coverage harness + (fixed v2.3.4), and now the browser. `apply_load_time_header_overrides` has two + stages — the compiled-in game database, then the per-game `.json` overlay. + Only the *second* needs a filesystem, but the whole function was `cfg`-gated + off wasm on its account, so the first went down with it. **A `cfg` gate + inherited from the strictest of several stages is a gate on the whole feature, + and nothing tells you which stages did not need it.** + + The database stage is now its own function, ungated, called from both browser + ROM entry points (the `wasm-winit` demo's `AppEvent::RomLoaded` and the + `wasm-canvas` embed's file picker). The overlay stage stays native-only, which + is correct rather than a remaining gap: a browser has no `.json` to find. + + Found by the audit v2.3.6 opened after Pixel Provenance — *look for shipped + features whose core logic is tested and whose frontend wiring is not.* And, as + with Pixel Provenance, a comment asserted the opposite of the code: the wasm + path was documented as one that "preprocesses separately" when it preprocessed + nothing at all. That sentence is corrected in place, quoted, rather than + quietly deleted. + + Pinned three ways: the browser stage must produce byte-identical output to the + full native helper when no overlay exists; a premise test proves the correction + is observable at all, so the agreement test cannot pass vacuously (which is + precisely the state the browser was in); and a source-text assertion requires + every wasm ROM entry point to call it — mutation-checked — because those call + sites live in `cfg`-gated code a native test binary cannot link, so an absent + call is the one thing behaviour can never catch. That third test was itself + **vacuous on first writing**, and review caught it: it lives in `app.rs`, so + `include_str!("app.rs")` pulled in the test's own source — which contains the + literal it searches for, making the assertion permanently true. It survived a + mutation check only because the check deleted the *other* file's call. A file + that reads itself has to exclude the part doing the reading; it now truncates + at the test module, asserts that the truncation worked, and is + mutation-checked on both halves. + - **Rad Racer's roadside artifact — the PPU spliced a hybrid address from a stale address bus.** A band of stray pixels flickered in the sand to the right of the road, tracking the horizon, on 1639 of the 1841 frames of the movie the diff --git a/crates/rustynes-frontend/src/app.rs b/crates/rustynes-frontend/src/app.rs index d4ae3617..3f01bedb 100644 --- a/crates/rustynes-frontend/src/app.rs +++ b/crates/rustynes-frontend/src/app.rs @@ -242,8 +242,22 @@ fn extract_rom_from_zip(zip_bytes: &[u8]) -> Option<(String, Vec)> { /// image. Returns the processed bytes and a display label (the inner archive /// entry name when unzipped, else the file name). Used by `App::new` so a ROM /// passed on argv loads identically to one opened from the menu / drag-drop. -/// Native-only (the in-app menu path handles the running-app case; the wasm -/// `AppEvent::RomLoaded` path preprocesses separately). +/// Native-only (the in-app menu path handles the running-app case). The wasm +/// `AppEvent::RomLoaded` path does NOT run this, and the two halves have +/// different reasons: +/// +/// * **Soft-patching** (`.bps` / `.ups` / `.ips`, that precedence) is keyed on a +/// same-stem sibling file. A browser file picker hands over one file's bytes +/// with no directory to look in and no path to derive a stem from, so there is +/// nothing it could match. +/// * **Zip extraction** is NOT sibling-dependent — a `.zip` selected in a +/// browser is as extractable as one on disk. It is simply not wired up on that +/// path yet. +/// +/// The header corrections DO apply there, via +/// [`apply_game_db_header_overrides`] — the half that used to be missing, and +/// which this sentence previously papered over by claiming the wasm path +/// "preprocesses separately" when it preprocessed nothing at all. #[cfg(not(target_arch = "wasm32"))] fn load_and_preprocess_rom(rom_path: &Path) -> std::io::Result<(Vec, String)> { let mut bytes = std::fs::read(rom_path)?; @@ -306,8 +320,9 @@ fn load_and_preprocess_rom(rom_path: &Path) -> std::io::Result<(Vec, String) /// ROM that needs one -- Seicross needs submapper 4 to clear its protection /// loop -- worked one way and hung the other. /// -/// Native-only, like the startup path it exists for: `per_game::resolve` and -/// the filesystem overlay it reads are `cfg`-gated off wasm. +/// Only the SECOND stage is native-only. See +/// [`apply_game_db_header_overrides`] for why that distinction cost the +/// browser build every header correction for three releases. #[cfg(not(target_arch = "wasm32"))] fn apply_load_time_header_overrides(bytes: &mut [u8], path: Option<&std::path::Path>) { // One CRC for both stages. `rom_crc32` hashes PRG+CHR and excludes the @@ -315,12 +330,9 @@ fn apply_load_time_header_overrides(bytes: &mut [u8], path: Option<&std::path::P // key is stable across the first rewrite -- that stability is what lets the // overlay stack on the database correction at all. Recomputing it was an // O(ROM) pass that could not return a different answer. - let Some(crc) = crate::game_db::rom_crc32(bytes) else { + let Some(crc) = apply_game_db_header_overrides(bytes) else { return; }; - if let Some(entry) = crate::game_db::entry_for_crc(crc) { - crate::game_db::apply_header_overrides(bytes, &entry); - } if let Some(cfg) = crate::per_game::resolve(crc, path) && !cfg.overrides.is_empty() { @@ -329,6 +341,35 @@ fn apply_load_time_header_overrides(bytes: &mut [u8], path: Option<&std::path::P } } +/// Stage one on its own: the vendored per-game database's header corrections. +/// +/// Returns the header-excluded CRC32 the lookup used, so a caller that stacks a +/// second correction on top does not pay an O(ROM) pass to recompute a value +/// that cannot have changed. +/// +/// **Available on every target, and that is the point.** The database is a table +/// compiled into `rustynes-gamedb`; nothing about it needs a filesystem. Only the +/// *second* stage — the per-game `.json` overlay — is native-only, and +/// until v2.3.7 that distinction was lost: the whole function was `cfg`-gated +/// off wasm because one of its two halves was, so **the browser build applied no +/// header corrections at all**. Every mapper / submapper / region fix the +/// database ships was silently absent on the web demo — Seicross needs +/// submapper 4 to clear its protection loop and hung there, exactly as it hung +/// on the CLI before v2.3.4 fixed *that* half of the same asymmetry. +/// +/// This is the third time this correction has been skipped by a load path that +/// did not go through the File-menu chokepoint (CLI in v2.3.4, the coverage +/// harness in v2.3.4, the browser here). The lesson recorded with the fix: a +/// `cfg` gate inherited from the strictest of several stages is a gate on the +/// whole feature, and nothing tells you which stages did not need it. +pub(crate) fn apply_game_db_header_overrides(bytes: &mut [u8]) -> Option { + let crc = crate::game_db::rom_crc32(bytes)?; + if let Some(entry) = crate::game_db::entry_for_crc(crc) { + crate::game_db::apply_header_overrides(bytes, &entry); + } + Some(crc) +} + /// Hand the game-DB crate its overlay directory, then apply the load-time header /// corrections to the startup ROM -- in that order, which is the whole point. /// @@ -8940,6 +8981,44 @@ impl ApplicationHandler for App { AppEvent::GfxReady(gfx) => self.on_gfx_ready(*gfx, event_loop), AppEvent::RomLoaded(bytes) => { self.rom_bytes = bytes; + // v2.3.7 — apply the per-game database's header corrections + // BEFORE `start_nes` hands the bytes to `Nes::from_rom`, which + // is the only moment the header still matters. + // + // This arm is the browser's ONLY ROM entry point, and until now + // it did none of this: the helper it needed was `cfg`-gated off + // wasm because its second stage (the `.json` overlay) reads + // a filesystem, so the first stage — a compiled-in table that + // needs nothing — went with it. The native File-menu and CLI + // paths both correct the header; the web demo did not. + // + // Only the database stage runs here. The overlay stage has no + // meaning in a browser (there is no `.json` to find), so it + // is not a gap, and `apply_load_time_header_overrides` stays + // native-only for that reason alone. + // The returned CRC is discarded because nothing stacks on it + // here (the overlay stage is native-only). + // + // `None` MUST NOT abort the load. It means "not a parseable iNES + // image", and that includes the formats this arm legitimately + // goes on to load: `rom_crc32` requires the `NES\x1A` magic, so + // an FDS disk (`FDS\x1A`) returns `None` and is then handled by + // `start_nes`'s wasm FDS branch. Treating `None` as a failure + // would make the browser refuse every FDS image. A genuinely + // malformed cartridge is rejected by `Nes::from_rom` below, with + // a better message than this stage could produce. + // `let _` is deliberate, and it is what clippy permits: an + // explicit `match`/`if let` on this was tried and rejected by + // `clippy::single_match` (the enforced gate), which is right -- + // there is genuinely nothing to do in either arm. + // + // `None` MUST NOT abort the load. `rom_crc32` requires the + // `NES\x1A` magic, so an FDS disk (`FDS\x1A`) returns `None` and + // is then handled by `start_nes`'s wasm FDS branch. Treating it + // as failure would make the browser refuse every FDS image. A + // genuinely malformed cartridge is rejected by `Nes::from_rom` + // below, with a better message than this stage could produce. + let _ = apply_game_db_header_overrides(&mut self.rom_bytes); // Match the AudioContext's actual sample rate (set up // by `wasm_winit::start`'s file-picker gesture) so the // APU output needs no resampling. Falls back to @@ -10868,6 +10947,124 @@ mod tests { "the startup helper must produce the same header as the DB rewrite" ); } + + /// The browser's load path must correct the header the same way the native + /// one does. + /// + /// Regression for the third instance of one defect: a load path that does + /// not go through the File-menu chokepoint skips the per-game database's + /// header corrections. The CLI skipped them until v2.3.4; the coverage + /// harness skipped them until v2.3.4; the **browser** skipped them until + /// v2.3.7, because `apply_load_time_header_overrides` was `cfg`-gated off + /// wasm on account of its SECOND stage reading a filesystem — taking the + /// first stage, a compiled-in table that needs nothing, down with it. + /// + /// A browser never has a per-game `.json` overlay, so the database + /// stage alone must equal the full native helper with no overlay present. + #[test] + fn the_browser_path_applies_the_same_header_corrections_as_the_native_one() { + let rom = synth_ines_185(); + + let mut via_browser = rom.clone(); + let crc_browser = super::apply_game_db_header_overrides(&mut via_browser); + + let mut via_native = rom.clone(); + apply_load_time_header_overrides(&mut via_native, None); + + assert_eq!( + via_browser, via_native, + "the browser stage diverged from the native helper on a ROM with no overlay" + ); + assert_eq!( + crc_browser, + crate::game_db::rom_crc32(&rom), + "the returned CRC must be the header-excluded key the lookup used, so a \ + caller stacking a second correction need not recompute it" + ); + } + + /// Premise for the test above: the correction is observable at all. + /// + /// Without this, `the_browser_path_...` would pass just as happily if both + /// sides were no-ops — which is precisely the state the browser was in. + #[test] + fn a_header_correction_is_observable_so_the_agreement_test_is_not_vacuous() { + let rom = synth_ines_185(); + let crc = crate::game_db::rom_crc32(&rom).expect("iNES header parses"); + let entry = crate::game_db::GameDbEntry { + crc, + region: None, + mapper: Some(4), + submapper: Some(4), + mirroring: None, + title: String::new(), + }; + let mut rewritten = rom.clone(); + assert!( + crate::game_db::apply_header_overrides(&mut rewritten, &entry), + "the DB rewrite reported no change" + ); + assert_ne!(rewritten, rom, "the DB rewrite left the header untouched"); + } + + /// Every wasm ROM entry point must call the correction. + /// + /// A source-text assertion on purpose, in the style of + /// `snapshot_schema_audit.rs`: the two call sites live in `cfg`-gated wasm + /// code that a native test binary cannot execute or even link, so behaviour + /// cannot reach them. What CAN be checked is that the call is present — and + /// the bug being pinned was exactly an absent call, not a wrong one. + /// + /// If a third wasm ROM entry point is added, add it here. + #[test] + fn every_wasm_rom_entry_point_corrects_the_header() { + const APP_SRC: &str = include_str!("app.rs"); + const CANVAS_SRC: &str = include_str!("wasm.rs"); + + // Whitespace-collapsed before matching, so a rustfmt line-wrap cannot + // turn a still-present call into a phantom regression. Matching the + // ARGUMENT too, not just the function name, is deliberate: the name + // appears in doc comments on both files, so a name-only assertion would + // stay green with every call site deleted. + let squash = |src: &str| src.split_whitespace().collect::>().join(" "); + + // CUT THE TEST MODULE OFF FIRST. This test lives in `app.rs`, so + // `include_str!("app.rs")` includes the text of this function -- and + // this function contains the very string literal it searches for. The + // `APP_SRC` assertion was therefore VACUOUS: permanently true, whether + // or not the real call site existed. It survived a mutation check + // because the check deleted the `wasm.rs` call, exercising only the + // other half. Caught in review. + // + // A file that reads itself has to exclude the part doing the reading. + let app_production = APP_SRC + .split_once("\n#[cfg(test)]") + .map_or(APP_SRC, |(before, _)| before); + assert!( + !squash(app_production).contains("fn every_wasm_rom_entry_point_corrects_the_header"), + "the test-module split failed, so this assertion is searching its own source again" + ); + assert!( + squash(app_production).contains("apply_game_db_header_overrides(&mut self.rom_bytes)"), + "the `wasm-winit` demo's AppEvent::RomLoaded arm no longer corrects the header" + ); + assert!( + squash(CANVAS_SRC).contains("apply_game_db_header_overrides(&mut bytes)"), + "the `wasm-canvas` embed's ROM loader no longer corrects the header" + ); + } + + /// Seicross (Japan): iNES 1.0, mapper 185, no submapper field of its own. + /// 32 KiB PRG + 8 KiB CHR so the header-excluded CRC is well defined. + fn synth_ines_185() -> Vec { + let mut rom = vec![0u8; 16 + 0x8000 + 0x2000]; + rom[0..4].copy_from_slice(b"NES\x1A"); + rom[4] = 2; // 32 KiB PRG + rom[5] = 1; // 8 KiB CHR + rom[6] = 0x90; // mapper low nibble 9 + rom[7] = 0xB0; // mapper high nibble B -> 185 + rom + } use super::{ extract_rom_from_zip, is_fds_image, is_nsf_image, load_and_preprocess_rom, nsf_header_strings, resolve_vs_dip, diff --git a/crates/rustynes-frontend/src/wasm.rs b/crates/rustynes-frontend/src/wasm.rs index a623a197..4bd557f4 100644 --- a/crates/rustynes-frontend/src/wasm.rs +++ b/crates/rustynes-frontend/src/wasm.rs @@ -150,7 +150,21 @@ fn install_rom_loader(rom_input: &HtmlInputElement) { return; }; let array = js_sys::Uint8Array::new(&buffer); - let bytes = array.to_vec(); + let mut bytes = array.to_vec(); + // v2.3.7 — the per-game database's header corrections, applied + // before the core parses the header. Same fix as the `wasm-winit` + // demo's `AppEvent::RomLoaded` arm: the helper used to be `cfg`-gated + // off wasm because a LATER stage of it reads a filesystem, so this + // stage — a compiled-in table needing nothing — was lost with it, and + // every mapper / submapper / region fix was absent in the browser. + // CRC discarded: nothing stacks on it here, and `None` just means + // the bytes are not a parseable iNES image -- which `Nes::from_rom` + // reports properly a few lines below. + // `let _` is deliberate; an explicit `match` here is rejected by + // `clippy::single_match`. `None` means "not an iNES image" -- which + // includes formats this path legitimately handles -- so it must not + // abort the load; `Nes::from_rom` reports a genuinely malformed one. + let _ = crate::app::apply_game_db_header_overrides(&mut bytes); // The file-pick is a user gesture, so it's safe to create // the AudioContext here (the browser autoplay policy // requires a gesture). Create the Nes at the audio