Adds a abortParked (to be used instead of abort for a parked transfer) which
guards against a double-free by setting the state to .completing BEFORE running
the requestFailed callback. Without this, requestFail could itself cause the
transfer to be cleared.
Also added a guard to try to catch double transfer deinit's in Debug. Because
the memory could get re-used between the first and second free, the lack of
failure doesn't prove there is no UAF. But it's cheap to do and debug only.
A fetch() deferred behind a parser-blocking script keeps a DeferredContext
in the DeferringLayer whose forward.ctx points at the Fetch struct, which
lives in a page/session arena. If the transfer completes while deferred,
it is deinited and unlinked from the frame owner, so abortTransfers ->
abortOwner can't reach the now-orphaned context. It lingers in the active
list, and when the page is torn down its Fetch arena is freed; a later
flushFrame (e.g. the next page's parser-blocking script popping) replays
the buffered header callback into the freed Fetch -> use-after-free.
Add DeferringLayer.cancelFrame to drop these orphaned (terminal) contexts
during Frame.abortTransfers. Non-terminal contexts still have a live
transfer that cleans them up through its own callback path, so they are
left alone.
The first is that it can outlive the Transfer and thus has to dupe anything
it'll use (e.g. the frame_id off the request).
The second is that flushFrame is reentrant (flushFrame -> fire -> flushFrame)
and that results in the active list and list pointers becoming invalid.
The intercept state is currently split and hard to keep consistent and even
just reason about. InterceptLayer keeps the `intercepted` count, but CDP's
`BrowserContext` has its intercepted lookup. This isn't a problem per se, but
you BrowserContext.deinit tries to decrement `InterceptLayer.intercepted` which
is only safe if we can guarantee that the two are in sync. Which we can't.
This commit simplifies the upkeep of `InterceptLayer.intercepted` and uses the
Transfer's state on unpark/deinit to decrement it. The CDP layer no longer
cares about / has to maintain the count.
Driven by this crash report:
BrowserContext.deinit.intercepted
---
value: 0
/home/runner/work/browser/browser/src/lightpanda.zig:279:25: 0x2871842 in deinit (lightpanda)
/home/runner/work/browser/browser/src/cdp/CDP.zig:127:18: 0x28c3f45 in deinit (lightpanda)
/home/runner/work/browser/browser/src/Server.zig:186:21: 0x2827997 in handleConnection (lightpanda)
/home/runner/work/_temp/6dc322a8-c74f-4990-9660-4cc6dcfb9352/zig-x86_64-linux-0.15.2/lib/std/Thread.zig:509:13: 0x269c233 in entryFn (lightpanda)
???:?:?: 0x7fce7ccabd57 in ??? (libc.so.6)
Unwind information for `libc.so.6:0x7fce7ccabd57` was not available, trace may be incomplete
on 1.0.0-nightly.6542+94ba0791
Replaces 4 boolean flags with a state. Makes it easier to figure out what the
state of the transfer is, and removes the possibility of inconsistent flags
.e.g queued + loop_owned.
loop_owned -> state != .created
_queued -> state == .queued
_perform -> state == .completing
aborted -> state == .aborted
This is just moving fields around. The end result is that there's a
`transfer.req` and a `transfer.res`.
On the Request side, we use to have a nested `params: RequestParam` resulting
in a lot of `transfer.req.params.url`. This is now `transfer.req.url`. On the
Response side, we had the exact opposite: response fields splattered directly
in the transfer, `transfer.response_header`. This is now `transfer.res.header`.
There is now an HttpClient.Response, which is the actual final response (which
could be for a transfer or something else, e.g the cache). And an
HttpClient.Transfer.Response which captures the inflight response data (and is
one of the polymorphic variants of the HttpClient.Response). Probably still not
ideal, but I'm not sure how to make it cleaner, and even if this is just an
intermediary step, I consider it an small win.
Karl Seguin