scrollTo/scrollBy dispatch a scroll event; direct assignment did not. So the
usual lazy-load idiom
el.addEventListener('scroll', loadMore);
el.scrollTop = el.scrollHeight;
never reached the listener, and scroll-driven feeds stalled after their first
batch with no error to show for it.
Checked against a real Chrome over CDP, same probe on the same page:
Chrome el.scrollTop = 100 -> 1 scroll event
obscura (before) el.scrollTop = 100 -> 0 scroll events
obscura (after) el.scrollTop = 100 -> 1 scroll event
The setters only fire when the offset actually changes, so a loader that
rewrites the same scrollTop each frame does not re-enter. scrollTo/scrollBy
suppress the per-axis events and emit one for the whole movement, the way a
browser reports one scroll per operation rather than one per axis.
The offset stays unclamped on purpose. Without layout, any maximum is a guess,
and a guess derived from the synthetic scroll box pins scrollTop at 0, which
deadlocks the very loaders this unblocks: no scroll, no content, no scroll.
Geometry is left alone here — this is event dispatch only.
Tests: 4 of the 5 new cases fail without this change (the scrollTo coalescing
one passes either way; it guards against this change turning one event into
three). nextest -p obscura-cdp 91/91, obstacle course 33/33.
Connection threads are detached and never joined, so `save_cookies()` ran on
the main thread while they were still serving: a connection writing a
Set-Cookie after that point lost it, and the process then exited and killed the
thread mid-flight. Before the per-connection move this could not happen -- the
single processor saved on its own way out, ordered against all connection work
on one LocalSet. Draining on the live-connection counter restores that ordering.
Scope, honestly: this bounds the loss window rather than closing it. The drain
does not finish early today because a connection's frame reader is parked in a
socket read and never observes `notify_waiters`, so a client that holds a
connection open costs the full 3s deadline and then gets the warning. Making
readers wake on shutdown is a separate change and not #430's business.
Measured: 2ms to shut down with no connections (unchanged), 3005ms with one
held open, against the 10s `docker stop` allows before SIGKILL. Cookies are
persisted in both cases.
Refs #430, #333.
The test's whole value is that its fixture keeps the navigation settle loop
pumping, but nothing checked the fixture was reached. Pointing `page_url` at a
dead port passed in 0.6s instead of 1.25s, and that degraded version no longer
reproduced the #430 abort on the pre-fix tree -- so a fixture regression would
have turned this back into the `data:`-URL test it was written to replace,
silently and while staying green.
Count the requests the fixture answers and assert on it. Two per client, not
three: the engine does not fetch the `<img>`, only the document and /slow.js.
Verified the assertion kills the dead-port mutant (0 requests, fails) and that
the real run is stable across repeats.
Refs #430.
The premise was wrong. deno_core 0.350's run_event_loop is:
pub async fn run_event_loop(&mut self, opts) -> Result<(), CoreError> {
poll_fn(|cx| self.poll_event_loop(cx, opts)).await
}
so replacing `timeout(budget, run_event_loop())` with a hand-rolled
`poll_fn(poll_event_loop)` loop is the same code with the same cancellation
semantics, not safer ones. The mechanism I described cannot happen either:
`poll` is a synchronous `&mut self` call, a future can only be dropped between
polls, and the HandleScope/ContextScope live inside poll_event_loop as stack
locals that are destroyed before it returns. The isolate is exited between
polls both before and after that change.
What actually fixes#430 is confining each connection's isolates to their own
OS thread, which the next commits do. The regression test still reproduces the
abort on the pre-thread-per-connection tree and still passes here without this
commit, so nothing depended on it.
It was not merely a no-op. Two regressions it introduced:
- resolve_promises_until inverted the meaning of tick_ms. It was "pump the
event loop for up to tick_ms, returning early on idle"; it became one single
poll followed by an unconditional sleep(tick_ms). With the 1->50ms backoff
that settles at 20 event-loop turns per second, so an awaited promise chain
needing many turns crawls, and past the budget the caller silently reads a
stale __obscura_await_meta.
- drive_module_eval replaced `e?` with `let _ = poll_event_loop(...)`,
discarding event-loop errors. A module stalled on a top-level await used to
exit immediately with a logged error; it instead spun for the whole budget
logging nothing -- the exact #374 starvation the surrounding comment claims
to prevent.
Keep the drive_module_eval extraction (it genuinely dedupes load_module and
load_inline_module) but restore the original select!-based body.
Refs #430.
Each connection runs on its own OS thread with its own V8 isolates, so
connection count is what drives the server's thread footprint.
MAX_PENDING_WS_HANDOFFS bounds only the handoff queue: once a stream is handed
off, nothing limited how many could be live at once, and a client could grow
the server without limit.
Reserve a slot in the accept loop and release it from a guard that runs when
the connection thread unwinds, on every path including panics and the early
returns before the runtime is built -- a plain decrement at the end of the
closure would leak slots until the cap wedged the server shut. Connections
past the limit get an explicit 503 with X-Obscura-Reason: max-connections
rather than a bare reset, so a client can tell a refusal from a crash. The
refusal happens in the WS dispatch loop, so the HTTP control plane (#62) stays
reachable at the cap.
Default 128, matching the handoff bound and well above any real client fan-out
(Playwright/Puppeteer use one connection per browser). At the cap, with arenas
capped by the previous commit: 146 threads, 33.2 GiB of reserved address space
and 51 MiB resident -- nearly all of that reservation being V8's process-wide
sandbox, which is present at zero connections.
Threads are the honest cost here, not memory: at 100 connections each running
JS this server reserves 83.5 GiB against `main`'s 83.6 GiB, because `main`
pays the same V8 sandbox as soon as it runs any JS. What it does not pay is
118 threads against 17.
The test covers all three properties, and specifically that a closed
connection frees its slot: a leak there would only surface as a server that
silently stops accepting after N lifetime connections.
Refs #430.
glibc hands each new thread its own 64 MiB arena (up to 8x cores). With one
thread per connection that arena is the dominant per-connection memory term,
and freed chunks stay reserved in it rather than returning to the OS: resident
memory tracks the *peak* number of concurrent connections and never comes back
down, which reads as a leak although nothing is leaked and no thread is stuck.
Measured with reliability/conn-scale.py in obscura-benchmark. 100 connections
each running JS reserve 90.0 GiB of address space uncapped versus 83.5 GiB
capped; per idle connection, 65 MiB of reserved address space drops to 2.0 MiB.
`main` at that same 100-connection JS workload reserves 83.6 GiB, so with the
cap this server is level with it -- most of the total is V8's process-wide
sandbox, which `main` pays too the moment it runs any JS.
The resident-set effect is the one that bites in production. Measured in the
container image against Google Maps, four concurrent connections per round:
350 / 619 / 826 MiB over three rounds uncapped and still climbing linearly,
versus 166 / 235 / 269 MiB capped, on a decelerating curve.
Two arenas cost no measurable throughput here -- 8 concurrent connections x 12
navigations, 1.53s uncapped against 1.50s capped over three runs. V8 allocates
the JS heap through its own allocator and the Rust side is dominated by network
I/O rather than malloc traffic. Applied only from `serve`, which owns the
process, and skipped when the caller has already set MALLOC_ARENA_MAX.
Refs #430.
- Delete the now-unused CdpContext::new_full (its only caller switched to
new_with_shared_context); have _new_inner build the shared BrowserContext and
delegate, so the struct's field list lives in one place.
- Extract drive_module_eval, shared verbatim by load_module and
load_inline_module.
- Update comments that still described the deleted process-wide V8 lock and the
single-LocalSet model to the per-connection reality, drop the dangling
reference to the removed obscura-js/src/v8_lock.rs, and soften the
ISOLATE_CREATE_LOCK note now that the main-thread warmup is the real guard
against the InitializeBuiltinJSDispatchTable segfault.
Refs #430.
The per-command V8 watchdog kept a single global slot whose correctness relied
on the old process-wide lock (at most one command armed at a time). With
thread-per-connection several connections arm concurrently, so a second arm
overwrote the first's slot and left that command's isolate unbounded. Key the
armed slots by generation so each connection's command has its own; the worker
fires whichever have overrun and terminates each via its IsolateHandle.
Refs #430.
With every connection on its own thread (previous commit), the process-wide V8
lock served no safety purpose across connections yet forced all of them to
serialize on one mutex, so throughput did not scale with concurrency. Move the
lock into CdpContext so each connection has its own: a connection's nav task and
command dispatch still coordinate on one page's isolate at a time on its thread,
but different connections no longer block each other. Delete the now-unused
process-wide obscura_js::v8_lock.
Measured on a local CPU-bound fixture, page throughput stays flat (~75/s from
concurrency 1 to 8) with the global lock and scales to about 2.5x at concurrency
8 with the per-connection lock, with no crash at any concurrency. Obstacle
course 33/33, obscura-cdp 76/76.
Refs #430.
concurrent_navigations uses data: URLs, which settle instantly and so never
drove the abort. This test serves a local page with slow subresources plus a
setInterval so the navigation settle loop keeps pumping, then drives it from
four independent CDP connections at once. It aborts the process on the old
single-LocalSet server (SIGTRAP, the V8 heap->isolate() == TryGetCurrent()
fatal) and passes on the thread-per-connection server.
Refs #430.
The CDP server ran every connection's pages on one LocalSet thread, so two
pages' V8 isolates shared an OS thread. Under concurrent navigation a cancelled
event-loop future could leave one isolate entered while another page's
execute_script ran, tripping V8's heap->isolate() == Isolate::TryGetCurrent()
check and aborting the process (#430). No single-thread serialization fixes
this reliably: the cancellation-leak pattern is pervasive and partly inside
deno_core.
Give each WebSocket connection its own OS thread with its own current_thread
runtime, LocalSet, and CdpContext, so every page's isolate is confined to one
thread. V8's invariant is per-thread, so two connections' isolates can never
collide. This is the "Option 2" fix named in obscura-js/src/v8_lock.rs, and it
also lets connections run in parallel instead of serializing on one thread.
- CdpContext::new_with_shared_context reuses one Arc<BrowserContext> across all
connections, so they share one cookie jar and one HTTP client.
- A single signal watcher flips the accept flag and wakes every connection's
processor via notify_waiters(); the accept side persists cookies once on
shutdown.
- Serialize isolate construction with a process-wide lock: the leaptiering
JSDispatchTable setup is not safe to run from several threads at once.
- Warm up V8 once on the main thread before any connection thread creates an
isolate; creating the first isolate off the main thread segfaults in
InitializeBuiltinJSDispatchTable.
obstacle course 33/33, obscura-cdp 75/75 including the concurrent-navigation and
control-plane tests. Concurrency 2 and 4 against heavy pages no longer abort.
Fixes#430.
Several event-loop operations bounded their runtime with
tokio::time::timeout(budget, run_event_loop()) or a select! against a
timer. When the timer wins, tokio drops the run_event_loop future
mid-poll; deno_core enters the isolate while polling, so the drop leaves
the isolate entered on the OS thread. Under concurrent CDP work a second
page's execute_script then trips heap->isolate() == TryGetCurrent() and
aborts the process (#430).
Drive the loop with poll_event_loop inside a poll_fn instead. It creates
and drops its own isolate scope per call, so stopping between ticks never
leaves the isolate entered. Convert resolve_promises_until,
run_event_loop_bounded, resolve_promises, the mod_evaluate settle in
load_module and load_inline_module, and the two navigation settle loops
in page.rs. Add poll_event_loop_once as the cancellation-safe single-tick
primitive the settle loops use.
Keeps the obstacle course at 33/33. This is hardening toward #430; the
residual concurrency abort still needs the thread-per-runtime refactor
that v8_lock.rs calls the "Option 2" fix.
Refs #430.
sycatle