fix(chrome): parse PDFs in offscreen document - #2983
Conversation
|
@16Miku is attempting to deploy a commit to the esokullu's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Right architecture for MV3 (PDF.js out of the service worker). Three concrete issues: Cold-start race. ensureOffscreen() returns before the module host’s onMessage is registered — first read_pdf can get “returned no result.” Add a ready handshake or short retry. |
|
Thanks — addressed all three points in
Added regression coverage for the ready retry, sender and scheme rejection, pre-fetch blocking, preserved byte length, one-request Claude extraction, and byte-for-byte document identity. Validation:
The current WebMCP smoke failure is the same pre-existing |
|
Re-reviewed at high effort against the branch head. (Correcting my own earlier line here: I said the Cold-start race — Open credentialed fetch — Dual fetch — the offscreen host base64-encodes the same buffer it parses, before PDF.js can detach it, and returns it as One thing worth knowing rather than changing: a 16MB PDF now crosses 🤖 Generated with Claude Code |
|
Added the suggested why-comment in commit ee4e104. It documents that the approximately 4/3 base64 message overhead is an intentional tradeoff for preserving the single-fetch, byte-identical guarantee. Validation after the comment-only change: npm run test:pdf-read (4 passed); npm run test:pdf-mime-handler (passed in Chrome 152.0.7977.76); git diff --check (passed). |
Four fixes from a review pass over the offscreen extraction path. - waitForPdfExtractionHost() treated every non-ready response as transient. An "Unauthorized PDF extraction sender." refusal is permanent, but it burned all 40 attempts and then surfaced as "did not become ready", which points a debugger at the wrong subsystem. It also never re-ensured the document, so an offscreen page that went away between ensureOffscreen() and the probe failed read_pdf permanently instead of recreating the host. - isTrustedPdfExtractionSender() hardcoded 'src/background.js', duplicating the manifest. Renaming the service worker entry would have rejected every extraction with that same misleading error, and no test would have caught it. The path now comes from getManifest(). - The scheme allowlist hard-failed on our own viewer tabs. With the native MIME handler on, a PDF tab's URL is src/ui/pdf-handler.html?url=..., and read_pdf falls back to the tab URL, so it returned "PDF URL must use http:, https:, or file:." normalizePdfUrl() now unwraps the inner URL for that page only, and the unwrapped URL still passes the allowlist. - The passthrough base64 was encoded before parsing, so a corrupt PDF that threw in getDocument() still paid for a string nobody reads. The bytes are copied before parsing, preserving the single-fetch guarantee, and encoded only after extraction succeeds. Also waits for the service worker in the MIME-handler e2e: serviceWorkers() is a snapshot, so the lookup could miss a worker Chrome had idled out. node test/run.js: 2198 passed, 0 failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0196wte9FS1SnsrKvrQUbB7m
|
Ran a second review pass scoped to this PR alone — the first one was shared with a much larger PR and I didn't trust "no findings" from it. Five issues, four now fixed in Rebased onto Fixed in
|
|
Thanks for the additional review and for the fixes in Review of
|
…sponse Three follow-ups on the offscreen PDF extraction host: getPdfjs() memoized its rejection as well as its result. The old pdf-tools.js assigned the module only after the await succeeded, so a failed import retried on the next call. Because the offscreen document outlives any single read, one transient failure of the vendor/pdfjs/pdf.mjs import made every later read_pdf fail with the same stale error until the document was torn down. Clear the promise on rejection to restore the retry. PDF_HANDLER_PAGE hardcoded the viewer path, duplicating the manifest's mime_types_handler handler_url — the same duplication we removed for background.service_worker. Renaming the viewer would have made read_pdf on our own PDF tabs fail with a misleading scheme error. Derive it from getManifest(), keeping the literal as a fallback for runtime stubs without getManifest. A throwing sendResponse on the success path fell through to the catch, which responded a second time on a closed channel; that second throw became an unhandled rejection and swallowed the original error. Route both replies through a single-shot respond(). Also hoists the per-message message-type array into a module-level Set, matching vision-inference-host.js. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FrohYGnD3GTHtRXsA1vhh1
Summary
read_pdfresult shape and Claude-compatible native PDF passthroughWhy
Chrome MV3 rejects dynamic
import()fromServiceWorkerGlobalScope, soread_pdffailed whenpdf-tools.jstried to lazily load PDF.js. The offscreen extension page can load PDF.js and its worker normally, while avoiding the startup cost for users who never read PDFs.This change is Chrome-specific because the failure is caused by the MV3 service-worker runtime; Firefox's MV2 background context is not affected.
Testing
npm run test:pdf-read— 3 passednpm run test:pdf-mime-handler— passed with Chrome 152.0.7977.76node test/run.js— passedThe Bitter Lesson;read_pagereturnedsuccess: true,totalPages: 2, andpageCount: 2, with noServiceWorkerGlobalScopeimport error