Follow-up from PR #631 review (findings #4 and #5).
Background
The cross-session pixel-leak fix in plugins/native/servo/src/servo_thread.rs re-binds make_current() before every read_to_image and gates frames on a first-paint flag. This is a correct mitigation but a band-aid over a process-wide shared SoftwareRenderingContext/surfman state model.
#5 — racy shared context (architectural)
make_current()-before-read is inherently racy: any concurrent bind between it and read_to_image re-opens the leak window. The durable fix per AGENTS.md ("generalize the underlying mechanism, don't special-case shared infra") is per-instance isolated rendering contexts/surfaces so reads can't observe another instance's pixels regardless of bind timing. (PR #631 already stopped discarding the make_current error on the read path — it now logs a warning.)
Related observability gap: a page that loads but never paints yields silently-transparent video while the node still reports healthy and there is no timeout. Worth a health/metric signal (e.g. "no paint after N seconds") for arbitrary-URL capture.
#4 — ~8 MB/frame allocation while unpainted
Before first paint, each tick builds a fresh transparent_frame(w,h) (~8 MB at 1080p). The frame Vec is moved through the result channel and copied into a pool buffer in servo_node.rs, so it can't simply be reused without a buffer-return channel. Options: a returnable scratch-buffer channel, or a cheaper "transparent" signal that lets the consumer fill a pooled buffer without the plugin allocating. The allocator recycles the freed block each tick so churn is bounded, hence this is an optimization, not a correctness issue.
Scope: both items are larger than the PR #631 cleanup and benefit from separate review.
Follow-up from PR #631 review (findings #4 and #5).
Background
The cross-session pixel-leak fix in
plugins/native/servo/src/servo_thread.rsre-bindsmake_current()before everyread_to_imageand gates frames on a first-paint flag. This is a correct mitigation but a band-aid over a process-wide sharedSoftwareRenderingContext/surfman state model.#5 — racy shared context (architectural)
make_current()-before-read is inherently racy: any concurrent bind between it andread_to_imagere-opens the leak window. The durable fix per AGENTS.md ("generalize the underlying mechanism, don't special-case shared infra") is per-instance isolated rendering contexts/surfaces so reads can't observe another instance's pixels regardless of bind timing. (PR #631 already stopped discarding themake_currenterror on the read path — it now logs a warning.)Related observability gap: a page that loads but never paints yields silently-transparent video while the node still reports healthy and there is no timeout. Worth a health/metric signal (e.g. "no paint after N seconds") for arbitrary-URL capture.
#4 — ~8 MB/frame allocation while unpainted
Before first paint, each tick builds a fresh
transparent_frame(w,h)(~8 MB at 1080p). The frameVecis moved through the result channel and copied into a pool buffer inservo_node.rs, so it can't simply be reused without a buffer-return channel. Options: a returnable scratch-buffer channel, or a cheaper "transparent" signal that lets the consumer fill a pooled buffer without the plugin allocating. The allocator recycles the freed block each tick so churn is bounded, hence this is an optimization, not a correctness issue.Scope: both items are larger than the PR #631 cleanup and benefit from separate review.