Fix: JSON Based Scrapers Fail With Chrome CDP - #7137
wraithfive wants to merge 6 commits into
Conversation
…r JSON content types When a useCDP scraper navigates directly to a URL that Chrome identifies as JSON content, Chrome's own JSON viewer wraps the raw JSON in an HTML pretty-printer document before it reaches the page DOM. urlFromCDP always extracted content via OuterHTML, so scrapeJson-type scrapers using useCDP would receive this HTML wrapper instead of the JSON itself and fail with "not valid json". Detect when the main document response's MIME type is JSON and, in that case, pull the raw body via Network.getResponseBody instead of reading the rendered DOM. Non-JSON responses are unaffected and continue to use OuterHTML as before.
|
Meant to request changes instead of approve, sorry |
…allback - Guard jsonDoc's requestID/isJSON fields with a mutex — they were written from chromedp's event-processing goroutine and read from the action sequence without synchronization. - Condense the nested resource-type/mime-type checks into a single condition. - Log the OuterHTML fallback (when the CDP response body is no longer available) at Warn instead of Debug so it's visible without verbose logging.
Every other hand-written mutex in this codebase uses a named field (e.g. mutex sync.Mutex) rather than an embedded one, so switch to match rather than promoting Lock/Unlock onto jsonDoc itself.
Extract the racy state (requestID/isJSON) out of urlFromCDP into a small named type, jsonDocumentTracker, with markJSON/get methods guarding access with its mutex. This makes the concurrency behavior independently testable without spinning up chromedp/a real browser. TestJSONDocumentTrackerConcurrentAccess exercises markJSON and get from separate goroutines concurrently and repeatedly, matching how urlFromCDP actually uses it (one goroutine from chromedp's event-processing loop, one from the action sequence passed to chromedp.Run). Verified this test reliably fails under `go test -race` if the mutex is removed, and passes cleanly with it in place. Also adds TestJSONDocumentTrackerMarksOnlyFirstJSONMatch to cover the "only track the first JSON document" behavior directly.
network.ResourceTypeDocument fires for iframe navigations too, not just the top-level one. The tracker previously latched onto the first Document response that happened to be JSON, so an HTML scraper whose page embeds an iframe that loads JSON (an embed widget, an ad frame) could have its extraction hijacked by that iframe's response instead of the page's own HTML - a regression risk for the population this fix was never meant to touch. Since redirects don't produce a separate responseReceived event for the pre-redirect URL (that arrives via requestWillBeSent's redirectResponse on the same request instead), the first Document response chromedp sees reliably corresponds to the top-level navigation. So rather than plumbing through frame IDs, jsonDocumentTracker now records the *first* Document response unconditionally - JSON or not - and ignores every one after it. Renamed markJSON/get to markDocument/mainDocument to match the new semantics, and reworded the OuterHTML-fallback log line to say the scrape will likely still fail as a result, so it doesn't read as an unrelated warning next to the downstream "not valid json" error. TestJSONDocumentTrackerOnlyRecordsFirstDocument replaces the old first-JSON-match test with two cases, including the exact iframe scenario above; verified it fails under the previous logic and passes with this one.
|
Pushed a fix for all three in 253f27e. Then thought to do what I probably should have done in the first place and checked the style of other mutex's in the project and extracted it to a named field. After that I wanted to make sure a test would actually catch this kind of race if it ever came back. The tracked state was living in an anonymous struct scoped inside urlFromCDP itself so I pulled it out into its own small type first (jsonDocumentTracker) so it could be tested without needing chromedp at all. Then added a test that hammers its write and read methods concurrently — confirmed it fails without the mutex and passes with it. A review by Fable pointed out that Network.responseReceived fires for iframe navigations too, not just the top-level one, so the tracker could in theory latch onto an iframe's JSON response and hijack extraction for an unrelated HTML scraper. Fixed that by only ever tracking the first Document response instead of the first JSON one, and added a test covering it. |
The comments added across this PR's commits were overly verbose. Trim them to state the non-obvious why in one or two lines instead of multi-paragraph explanations. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019tbLTybiYH7eZbhXJKoWLY
Gykes
changed the title
Gykes
left a comment
Gykes
left a comment
There was a problem hiding this comment.
Sorry, hopefully this is the last review.
| t.mutex.Lock() | ||
| defer t.mutex.Unlock() | ||
|
|
||
| if t.recorded { |
There was a problem hiding this comment.
Does this need to be the first document or the last main-frame one?
If the URL hits a JS/CF challenge or a redirect page before landing on the JSON the first document is an HTML. That would take us down the "not valid JSON" path.
There was a problem hiding this comment.
Good Point. I didn't think about pages that redirect or sit behind cloudflare browser challenges before landing on the actual JSON. chromedp will get through those on its own (that's what the sleepDuration setting is there for, giving it a second to settle). It won't handle a captcha challenge but I don't want to get into trying to auto solve those in this. So "first document" was the wrong thing to grab, since by the time it settles we're way past that first page.
I'm thinking instead we keep track of the actual top-level page via page.EventFrameNavigated and only pay attention to Document responses on that frame. The top level should be the one where Frame.ParentID == "". Instead of keeping the first one we see, keep overwriting it so we end up with whatever loaded last. That way an iframe's response still can't sneak in (wrong frame, so it's filtered out no matter when it shows up), but a redirect or challenge page doesn't get stuck as the "final" answer either. Whatever the browser actually ends up sitting on wins.
Does that logic sound right before I make the change?
2 participants
Description
When a
useCDPscraper navigates directly to a URL that Chrome identifies as JSON content, Chrome's own built-in JSON viewer wraps the raw JSON in an HTML pretty-printer document before it ever reaches the page DOM.urlFromCDPalways extracted content viachromedp.OuterHTML, soscrapeJson-type scrapers usinguseCDPwould receive this HTML wrapper instead of the JSON itself and fail with "not valid json".Related Issue
Closes #7136
Testing
go build,go vet, andgo test ./pkg/scraper/...all pass, includingnew unit tests for the MIME-type decision logic (
url_test.go)separate config, pointed at the same headless Chrome CDP sidecar) to verify against the real R18.dev scraper:
not valid jsonuseCDPscraper (JavLibrary) and a non-CDP scraper (JavBus) to confirm unaffected behaviorTo verify this fix
--remote-debugging-portexposed.driver: { useCDP: true }to R18.dev.yml. Any scraper usingaction: scrapeJsonwithdriver: { useCDP: true }should work.Before this fix: fails with
not valid jsonregardless of the endpoint actually returning valid JSON. After: succeeds normally.Checklist
AI Usage Disclosure
The root cause was diagnosed and this fix was drafted with Claude Sonnet 5.
Additional Context
Network.getResponseBodyfails (e.g., the response got evicted from Chrome's cache before the fetch), the fix falls back to the original OuterHTML behavior rather than erroring out.