Skip to content

Commit 7ff0b11

Browse files
exp78claude
andcommitted
fix: review pass on v0.9.2 features + render-notice and interruption bugs
Downloads: - WebKit emits failed THEN finished: flag the failure so finished can't toast a false 'Saved' or import a truncated file into the library - dedupe destination as name-1.ext (WebKit's EEXIST cleanup would delete the user's previous file); library import runs off the worker thread (large files no longer freeze the engine); 'Downloading...' toast; sweep *.wkdownload/zero-byte leftovers at startup; destination fallback and absolute-path guard; import detection by response MIME / %PDF- magic, not only the file extension - library.h: dir fsync after rename, per-point errno, 0-byte reject, UTF-8-safe name cap, O_NOFOLLOW, absolute-dir check; tests for the failure paths, uniqueDownloadName, forcedExt, uuid fuzz TLS continue-anyway (hardened after review): - tls-continue now requires a real tap (m_expectUserNav — all taps here are synthetic JS, WebKit gesture APIs never fire) and the one-shot m_tlsErrorHost armed by the TLS failure itself; a foreign page can no longer open the IGNORE window - the IGNORE window now closes on cancelled superseded loads too, and on WebProcess termination; m_tlsContinueKick disarmed on failure - repeat visits to an approved host flip IGNORE at the decision (no second tap); the never-painting proceed path is gone for good Rendering: - blank-check flags 'failed to render' only when the load never painted anything (m_firstContentLogged); a transient white frame late in the load (SPA re-render / anti-adblock hiccup) no longer whites out the page under the notice - 'Frame load interrupted' (policy error 102: superseded or download-converted load) is swallowed like CANCELLED — a double-tapped Go no longer fakes 'site won't load', and downloads stop flashing an error page under the toast - B&W fast mode: single-pass fused grayscale+LUT conversion (~200 ms off every frame on this CPU); RMWEB_BW_HOOK=0 debug escape hatch; the presentFast hook measured faster than the QPA auto waveform and stays - new debug: RMWEB_DUMP_FRAMES=/dir dumps engine frames as PNG Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 1e64a0e commit 7ff0b11

4 files changed

Lines changed: 382 additions & 89 deletions

File tree

CLAUDE.md

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -401,12 +401,26 @@ xochitl restarts WITH XOVI.
401401
properly (see the Phase 7 Batch 2 form-filling entry above, 2026-07-25); the rest of the Batch 2 claims
402402
stay erroneous, and the project is a **beta, not release-ready**. Full audit: `docs/review-2026-07-18.md` (HIGH#1).
403403

404-
**2026-09-11 — downloads → library + TLS continue-anyway (v0.9.2 candidates, NOT device-verified yet):**
404+
**2026-09-11 — downloads → library + TLS continue-anyway (v0.9.2, device-verified 2026-09-16):**
405405
PDF/EPUB downloads now self-register in the xochitl store (`engine/wpeqt/library.h`: UUIDv4 stem +
406406
`<uuid>.{pdf,epub,metadata,content}` — metadata last, it's the registration marker; xochitl rebuilds
407407
the rest on its post-quit restart). Timestamps are ms-epoch strings; `visibleName` = basename w/o ext.
408+
PDFs are forced to the download path (decide-policy): this build's PDF.js would render inline, but the
409+
native xochitl reader wins on this CPU. Downloads dedupe as `name-1.ext` (WebKit's EEXIST cleanup would
410+
otherwise DELETE the user's previous file), and `failed` is flagged so the trailing `finished` signal
411+
(WebKit emits both) can't toast a false "Saved" or import a truncated file.
408412
TLS: `load-failed` gets TLS errors with domain `g-tls-error-quark` (WPE 2.48.5 has NO WEBKIT_TLS_ERROR
409413
quark — verified against the source tree, not the docs); detected via quark-name substring. The error
410-
page offers `rmweb:tls-continue` which whitelists ONLY the current page's own host in a session-scoped
411-
`m_tlsBypass` set (the command carries no host, so a foreign page can't whitelist another origin), then
412-
re-navigates via `load_uri` with `m_expectUserNav` armed (the auto-refresh guard would eat it otherwise).
414+
page offers `rmweb:tls-continue`, gated on: current URI is https, host matches the one-shot
415+
`m_tlsErrorHost` (armed by the TLS failure itself), and a real tap gesture (`m_expectUserNav` — all
416+
taps here are synthetic JS, so WebKit's own gesture APIs never fire). The bypass = flipping the session
417+
tls-errors-policy to IGNORE for that one load, restoring FAIL on any settle (finished/failed/cancelled/
418+
web-process-terminated). NB: the WebKit "proceed" override (return TRUE from load-failed-with-tls-
419+
errors) loads but NEVER paints the document — device-verified three rounds; that's why the policy flip.
420+
**2026-09-16 — blank-check false positive fixed:** the render check now flags "blank" only if the load
421+
NEVER painted content (`m_firstContentLogged`). Before, a single transient white frame late in the load
422+
(SPA re-render / anti-adblock hiccup — ixbt whites out for a beat at ~+11 s with our filter on) reset
423+
`m_lastNonWhite` and the check whited out the whole page under the notice. Also: `Frame load
424+
interrupted` (WebKit policy error 102 — a superseded/download-converted load, not a failure) is now
425+
swallowed like CANCELLED instead of showing an error page (double-tapped Go used to fake "site won't
426+
load"; downloads flashed an error page under the toast). Frame-dump debug: RMWEB_DUMP_FRAMES=/dir.

engine/wpeqt/library.h

Lines changed: 71 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,26 @@ inline bool isDocumentFile(const std::string &path) {
4141
return e == "pdf" || e == "epub";
4242
}
4343

44+
// A download destination that never clobbers an existing file: "name.ext" -> "name-1.ext",
45+
// "name-2.ext", ... WebKit's EEXIST failure path DELETES the pre-existing file (WebKitDownload.cpp
46+
// cleanDownloadFiles), so uniqueness matters — overwrite is not an option. Pure POSIX (access F_OK)
47+
// -> host-unit-testable.
48+
inline std::string uniqueDownloadName(const std::string &dir, const std::string &name) {
49+
if (access((dir + "/" + name).c_str(), F_OK) != 0) return name;
50+
std::string stem = name, ext;
51+
const size_t dot = name.find_last_of('.');
52+
if (dot != std::string::npos && dot > 0) { // no dot, or dot==0 (".pdf") -> whole name is the stem
53+
stem = name.substr(0, dot); ext = name.substr(dot);
54+
}
55+
for (int i = 1; i < 1000; ++i) {
56+
const std::string cand = stem + "-" + std::to_string(i) + ext;
57+
if (access((dir + "/" + cand).c_str(), F_OK) != 0) return cand;
58+
}
59+
// Saturated (>=1000 collisions): last-resort timestamp suffix.
60+
return stem + "-" + std::to_string(std::chrono::duration_cast<std::chrono::milliseconds>(
61+
std::chrono::system_clock::now().time_since_epoch()).count()) + ext;
62+
}
63+
4464
// Minimal JSON string escaping (quotes, backslash, control chars) — the project has htmlEscape for
4565
// markup but no JSON escaper, so this tiny local one covers the single interpolated JSON value.
4666
inline std::string jsonEscape(const std::string &s) {
@@ -94,44 +114,56 @@ inline bool mkdirs(const std::string &dir, std::string *err) {
94114
}
95115
return true;
96116
}
117+
// fsync the containing directory so a rename inside it survives a power cut (same best-effort tail
118+
// as profile.h's atomicWrite — kept local to stay glib/Qt-free).
119+
inline void fsyncDirOf(const std::string &path) {
120+
std::string dir = path;
121+
const size_t slash = dir.find_last_of('/');
122+
if (slash == std::string::npos) dir = "."; else if (slash == 0) dir = "/"; else dir.erase(slash);
123+
const int dfd = open(dir.c_str(), O_RDONLY);
124+
if (dfd >= 0) { (void)fsync(dfd); close(dfd); }
125+
}
97126
// Chunked binary copy src -> dst with the same atomicity model as atomicWrite (tmp + fsync +
98127
// rename, byte-count verified). The payload can be tens of MB, so it is NOT slurped into memory.
128+
// O_NOFOLLOW on the tmp: a planted symlink must not redirect the write. errno is recaptured at the
129+
// exact failing call, so the message names the real cause.
99130
inline bool copyFileAtomic(const std::string &dst, const std::string &src, std::string *err) {
100131
const std::string tmp = dst + ".tmp";
101132
const int in = open(src.c_str(), O_RDONLY);
102133
if (in < 0) { if (err) *err = "open " + src + " failed: " + std::strerror(errno); return false; }
103134
struct stat st {};
104135
const bool haveSize = fstat(in, &st) == 0;
105-
const int out = open(tmp.c_str(), O_WRONLY | O_CREAT | O_TRUNC, 0644);
136+
const int out = open(tmp.c_str(), O_WRONLY | O_CREAT | O_TRUNC | O_NOFOLLOW, 0644);
106137
if (out < 0) {
107138
if (err) *err = "open " + tmp + " failed: " + std::strerror(errno);
108139
close(in);
109140
return false;
110141
}
111-
bool ok = true;
142+
int failErr = 0;
143+
const char *failWhat = nullptr;
112144
long long total = 0;
113145
char buf[1 << 16];
114146
for (;;) {
115147
const ssize_t n = read(in, buf, sizeof buf);
116148
if (n < 0 && errno == EINTR) continue;
117-
if (n < 0) { ok = false; break; }
149+
if (n < 0) { failErr = errno; failWhat = "read"; break; }
118150
if (n == 0) break;
119151
for (ssize_t off = 0; off < n;) { // write(2) may be partial
120152
const ssize_t w = write(out, buf + off, static_cast<size_t>(n - off));
121153
if (w < 0 && errno == EINTR) continue;
122-
if (w <= 0) { ok = false; break; }
154+
if (w <= 0) { failErr = errno; failWhat = "write"; break; }
123155
off += w;
124156
}
125-
if (!ok) break;
157+
if (failWhat) break;
126158
total += n;
127159
}
128-
const int savedErr = errno;
129-
if (ok && fsync(out) != 0) ok = false;
130-
if (close(out) != 0 && ok) ok = false;
160+
if (!failWhat && fsync(out) != 0) { failErr = errno; failWhat = "fsync"; }
161+
if (close(out) != 0 && !failWhat) { failErr = errno; failWhat = "close"; }
131162
close(in);
132-
if (ok && haveSize && total != static_cast<long long>(st.st_size)) ok = false; // truncated copy
133-
if (!ok) {
134-
if (err) *err = "copy " + src + " -> " + dst + " failed: " + std::strerror(savedErr);
163+
if (!failWhat && haveSize && total != static_cast<long long>(st.st_size)) failWhat = "truncated copy";
164+
if (failWhat) {
165+
if (err) *err = "copy " + src + " -> " + dst + " failed (" + failWhat + ")" +
166+
(failErr ? std::string(": ") + std::strerror(failErr) : std::string());
135167
std::remove(tmp.c_str());
136168
return false;
137169
}
@@ -144,17 +176,23 @@ inline bool copyFileAtomic(const std::string &dst, const std::string &src, std::
144176
}
145177
} // namespace detail
146178

147-
// Import srcPath (a finished download) into the xochitl store at xochitlDir. Returns false with a
179+
// Import srcPath (a finished download) into the xochitl store at xochitlDir. forcedExt, when
180+
// non-empty ("pdf"/"epub"), overrides the extension gate and the payload suffix — for downloads
181+
// whose filename has no extension but whose MIME/magic says document. Returns false with a
148182
// human-readable *err on any failure (no exceptions escape); on a mid-way failure every file it
149183
// already wrote is unlinked again, so a retry starts clean. Each import mints a fresh UUID, so it
150184
// never clashes with (or overwrites) an existing library document.
151185
inline bool importDocument(const std::string &xochitlDir, const std::string &srcPath,
152-
const std::string &visibleName, std::string *err) {
186+
const std::string &visibleName, std::string *err,
187+
const std::string &forcedExt = {}) {
153188
auto fail = [err](const std::string &m) { if (err) *err = m; return false; };
154-
if (!isDocumentFile(srcPath)) return fail("not a pdf/epub: " + srcPath);
155-
const std::string ext = lowerExt(srcPath);
189+
if (xochitlDir.empty() || xochitlDir[0] != '/') return fail("xochitl dir not absolute: " + xochitlDir);
190+
std::string ext = forcedExt.empty() ? lowerExt(srcPath) : forcedExt;
191+
for (auto &c : ext) c = char(std::tolower(static_cast<unsigned char>(c)));
192+
if (ext != "pdf" && ext != "epub") return fail("not a pdf/epub: " + srcPath);
156193
struct stat st {};
157194
if (stat(srcPath.c_str(), &st) != 0) return fail("source not readable: " + srcPath);
195+
if (st.st_size == 0) return fail("empty file: " + srcPath);
158196
if (!detail::mkdirs(xochitlDir, err)) return false;
159197

160198
const std::string uuid = makeUuidV4();
@@ -165,10 +203,26 @@ inline bool importDocument(const std::string &xochitlDir, const std::string &src
165203
auto cleanup = [&] { std::remove(payload.c_str()); std::remove(contentF.c_str()); std::remove(metadataF.c_str()); };
166204

167205
if (!detail::copyFileAtomic(payload, srcPath, err)) return fail(err ? *err : "copy failed");
206+
detail::fsyncDirOf(payload); // make the payload rename durable, like atomicWrite's dir fsync
168207

169-
// visibleName: no control chars (profile.h sanitize), capped (a title bar is narrow), never empty.
208+
// visibleName: no control chars (profile.h sanitize), capped (a title bar is narrow) WITHOUT
209+
// splitting a UTF-8 multi-byte sequence at the cut, never empty.
170210
std::string name = sanitizeField(visibleName);
171-
if (name.size() > 120) name.resize(120);
211+
if (name.size() > 120) {
212+
name.resize(120);
213+
// Back off to a UTF-8 boundary: skip continuation bytes, then if the byte before them is a
214+
// lead byte whose sequence was cut by the cap, drop that lead byte too. (Continuation bytes
215+
// of a COMPLETE trailing sequence are never touched: that sequence's lead sits before them
216+
// with its full length available.)
217+
size_t k = name.size();
218+
while (k > 0 && (static_cast<unsigned char>(name[k - 1]) & 0xC0) == 0x80) --k;
219+
if (k > 0) {
220+
const unsigned char lead = static_cast<unsigned char>(name[k - 1]);
221+
const size_t need = lead < 0x80 ? 1 : lead < 0xE0 ? 2 : lead < 0xF0 ? 3 : 4;
222+
if (name.size() - (k - 1) < need) --k;
223+
}
224+
name.resize(k);
225+
}
172226
if (name.empty()) name = "download";
173227

174228
const long long nowMs = std::chrono::duration_cast<std::chrono::milliseconds>(

0 commit comments

Comments
 (0)