Currently, we use the calling frame when mutating an element's attributes (e.g.
add/delete). This commit changes it to the element's owning frame. This should
fix both some errors and potential uaf.
Fixes the remaining WPT /dom/collections failures around setting,
defining and describing HTMLCollection properties:
- HTMLCollection-own-props.html: assigning to a supported named property
or to any array index must fail (TypeError in strict mode), while
expandos for unsupported names must keep working.
- HTMLCollection-supported-property-indices.html: indexed property
descriptors must be {enumerable: true, configurable: true, writable:
false}, Object.defineProperty on any array index must throw, and
deleting a supported index must fail.
- HTMLCollection-supported-property-names.html: named property
descriptors must be {enumerable: false, configurable: true, writable:
false} ([LegacyUnenumerableNamedProperties]) and defineProperty on a
supported name must throw.
Bridge changes, mirroring the existing interceptor callbacks:
- bridge.Indexed gains a definer callback; bridge.NamedIndexed gains
definer and descriptor callbacks, exposed through new indexedFull and
namedIndexedFull builders so existing indexed/indexedReadWrite/
namedIndexed call sites are untouched. Definer and descriptor
handlers reuse the deleter (name -> bool) and getter (name -> value)
calling shapes. The stale void-returning definer/descriptor typedefs
in the v8 binding are bridged with @ptrCast; v8 actually reads the
returned u32 as v8::Intercepted.
- Caller's query helpers (shared queryReturn) now accept, besides the
existing bool shape, query callbacks returning the
v8.PropertyAttribute bits directly (e.g. ReadOnly), with
error.NotHandled falling through to the ordinary lookup.
HTMLCollection registers:
- an indexed setter and definer that always fail (no indexed setter in
Web IDL), and an indexed query reporting ReadOnly for supported
indices;
- a named definer that fails for supported names, and a named
*descriptor* (not query) reporting {writable: false, enumerable:
false, configurable: true}. Using the descriptor callback instead of
query is deliberate: the ordinary [[Set]] path does not consult it, so
assigning through a derived object (HTMLCollection-as-prototype.html)
still creates an expando on the receiver, as the spec's
ignore-named-props rule requires, while direct assignments fail via
the definer.
Coverage: /dom/collections is now fully green (53/53):
- HTMLCollection-own-props.html 4/8 -> 8/8
- HTMLCollection-supported-property-indices.html 4/7 -> 7/7
- HTMLCollection-supported-property-names.html 5/6 -> 6/6
- HTMLCollection-as-prototype.html stays 2/2
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fixes the Object.getOwnPropertyNames related failures in WPT
/dom/collections:
- HTMLCollection-live-mutations.window.html: "ownPropertyNames" case
- HTMLCollection-supported-property-names.html: 3 getOwnPropertyNames cases
- namednodemap-supported-property-names.html: all 3 cases
- domstringmap-supported-property-names.html: all 5 cases
Object.getOwnPropertyNames returned no interceptor-backed properties
because these collections registered no enumerator callbacks.
Using the bridge's existing indexed and named enumerator support:
- HTMLCollection registers an indexed enumerator (the supported
indices) and a named enumerator implementing the spec's supported
property names: for each element in tree order, its non-empty id and,
for HTML elements, its non-empty name attribute, without duplicates.
- NamedNodeMap registers indexed and named enumerators (attribute
qualified names in order, via List.getNames).
- DOMStringMap registers a named enumerator (camel-cased data-*
attribute names). The previously unused kebabToCamel helper is fixed
to follow the spec conversion: only a '-' followed by an ASCII
lowercase letter is folded to an uppercase letter, a bare "data-"
attribute maps to the empty name, and a trailing '-' is preserved.
(As originally written, this commit also added named enumerator
interceptor support to the js bridge; main gained equivalent support
independently, so the rebase keeps main's bridge API.)
Coverage:
- /dom/collections/HTMLCollection-live-mutations.window.html 4/5 -> 5/5
- /dom/collections/HTMLCollection-supported-property-names.html 2/6 -> 5/6
- /dom/collections/namednodemap-supported-property-names.html 0/3 -> 3/3
- /dom/collections/domstringmap-supported-property-names.html 0/5 -> 5/5
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fixes 2 failing tests in WPT /dom/collections/HTMLCollection-delete.html
("Strict id" and "Strict name"): deleting a supported indexed or named
property of an HTMLCollection must fail (and throw a TypeError in strict
mode), since those properties are backed by the live collection.
HTMLCollection registered no deleter interceptors, so `delete c[0]` and
`delete c.name` fell through to the ordinary [[Delete]] which succeeded
on the (non-own) property.
HTMLCollection now registers an indexed and a named deleter (via the
bridge's indexedReadWrite/namedIndexed deleter callbacks) that return
false for supported properties - which v8 turns into a TypeError in
strict mode - and error.NotHandled for unsupported ones, leaving those
to the ordinary path.
(As originally written, this commit also added indexed setter/deleter
interceptor support to the js bridge; main gained equivalent support
independently, so the rebase keeps main's bridge API.)
Coverage: /dom/collections/HTMLCollection-delete.html 2/4 -> 4/4.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fixes the failing test in WPT /dom/abort/abort-signal-timeout.html:
"Signal returned by AbortSignal.timeout() is not aborted after frame
detach".
The scheduler task registered by AbortSignal.timeout() kept running
after the iframe that created the signal was removed from the document,
so the signal aborted and the abort event fired. Per the DOM spec, the
abort is queued as a global task on the signal's relevant global, and
such tasks must not run once the global's document stops being fully
active.
The timeout callback now walks the signal's frame and its ancestors and
skips the abort when a hosting iframe element is no longer connected.
Coverage: /dom/abort/abort-signal-timeout.html 0/1 -> 1/1.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fixes 1 failing test in WPT /dom/abort/event.any.html (and its .worker
variant): "AbortController abort(null) should set signal.reason".
abort() reason parameters were typed ?js.Value.Global, and the JS->Zig
conversion collapses an explicit JS null into a missing argument, so
controller.abort(null) fell back to the default "AbortError"
DOMException. Per the DOM spec, the abort reason defaults to a new
AbortError only when the reason is not given (undefined); an explicit
null must be stored as-is.
The parameters are now ?js.Value, which preserves the missing vs
explicit-null distinction, and the new AbortSignal.reasonFromJs helper
only falls back to the default for a missing or undefined reason.
Coverage: /dom/abort/event.any.worker.html 15/16 -> 16/16,
/dom/abort/event.any.html 15/16 -> 16/16.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fixes 3 failing tests in WPT /dom/abort/abort-signal-any.any.html (and
its .worker variant):
- "Dependent signals are aborted correctly for reentrant aborts": when a
source signal aborted a second source from within an abort listener,
the shared dependent signal dispatched its abort event twice. Per the
DOM spec, only dependent signals that are not already aborted are
appended to the list of signals to dispatch. Dependend.markAborted now
reports whether the dependent was newly marked, and abort() only
dispatches for those.
- "Dependent signals should use the same DOMException instance" (already
aborted source / source aborted later): Reason.dom held a DOMException
by value, so every .reason access wrapped a fresh JS object and the
identity check source.reason === dependent.reason failed. Reason.dom
is now a *DOMException allocated once at abort time; the JS bridge
identity-maps the pointer so the source and all dependents expose the
very same DOMException instance.
Coverage: /dom/abort/abort-signal-any.any.worker.html 11/14 -> 14/14,
/dom/abort/abort-signal-any.any.html 11/14 -> 14/14.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Instruct the /save synthesis model to annotate the generated script with
short `//` intent comments — one per logical block (navigate, extract,
fan out, aggregate, return) describing what the block accomplishes, so a
later reader (human or agent) grasps the script at a glance.
The save synthesis prompt previously ended with "no commentary", which
discouraged code comments; reword it to forbid only prose/markdown fences
outside the code while explicitly asking for the `//` intent comments. The
same prompt backs the MCP `save` tool, so both surfaces benefit. Also add a
matching best-practice bullet + example to the agent's script-writing skill.
https://github.com/lightpanda-io/browser/pull/2901 introduced optimizations to
how attributes are stored and probed. One of those optimization breaks when
an attribute is probed across iframes.
This commit removes that specific optimization and reverts to a linear scan
comparing the strings (which is still faster after 2901 since it's an array and
not a linked list).
The docs promise triple-quoted values that span multiple lines, but the
REPL dispatched every physical line immediately, so the multi-line
/extract form died with 'unterminated quote' on its first line. A slash
command that leaves a '''...''' block open now keeps reading lines at a
continuation prompt until the block closes; Ctrl-D abandons it.
The one-shot --save flag bypassed save.parseCommand, so '--save test'
wrote a file literally named 'test' while the REPL's /save appended the
extension. Share one ensureJsExtension helper so the paths can't drift.
Commit eliminates a class of UAF on transfer abort due to the abort potentially
JS callbacks within a libcurl callback.
It also introduces a graveyard queue to the HttpClient so that, on deinit,
Transfers become detached but their memory is still valid. In debug, the
transfer is poisoned so that any uaf fails loudly. In release, the graveyard
is simply cleaned up.
A NSFW site was reported as crashing. It had 21 sibling iframes all loading
the same synchronous script. This would overflow the v8 stack. The issue is
that, on a blocking request, we gate other requests from the same frame, but not
from other frames. Typically, that's ok / what we want. BUT, for a document
request, we need to be more careful:
1- We're inside a v8 callback (so some v8 stack is active)
2- A sync request is made
3- while pumping the http client, a document for another frame completes
4- Ok, it's on another frame, we don't block it
5- We start the parser
6- The parser encounters iframs
7- Each of those iframes have a blocking script
8- Those blocking scripts load their own document
See what's happening? We're still in the v8 callback of #1, still on that same
stack, and we're loading more and more heavy parsers and triggering more and
more http callbacks.
It's possible this problem extends beyond document parsing. But this is the
one we observed and it's also the most likely to cause recursive blocking
requests.
Replaces layering with an inline request pipeline, and transfer queue. This is
meant to simplify the code, reduce footguns, and make future enhancements easier
to implement (e.g. speculative parsing (which requires streaming to fully
leverage)).
Previously, HttpClient implemented deferring as a layer which required special
pumping at various callsites (https://github.com/lightpanda-io/browser/pull/2855,
https://github.com/lightpanda-io/browser/pull/2843, ...). In this new approach,
deferring is built-into the HttpClient/Transfer's flow. Specifically, Transfers
now maintain a queue of events (start, header, data, end, err) which are
dispatched in HttpClient.tick. The result is that JS callbacks are never
executed in the same stack that initiated the I/O, without needing guards or any
external intervention.
tTwo other benefits come from this. The first is that reentrant libcurl is
eliminated. Instead of "libcurl -> callback", it's now "libcurl -> transfer
event queue THEN tick -> callback" (we don't have to wait until the NEXT tick, we
can just do it later in the tick). HttpClient still has to guard against libcurl
reentrancy, but only because of how WebSocket is implemented, and we should be
able to unify WebSockets to use an event queue too in a follow up PR (which will
eliminate a bunch of guard code).
The transfer queue should also be useful to re-implement streaming, since a
data chunk is just an event in the transfer's event queue. For now, I kept it
as a single buffered event to minimize the change. But since speculative parsing
depends on this, and speculative parsing seems to be the next major performance
tweak we can make, we need to re-introduce streaming.
The other change is the removal of all other layers in favor of a pipeline. This
works well with the existing Transfer.park mechanism, where a parked Transfer
can restart the pipeline for a transfer in an arbitrary point (not as fancy as
it sounds given how simple the flow is). The fallout from this is that we're no
longer creating/wrapping contexts and callbacks: whatever the request was
configured with is all we need.
Because of this, HttpClient.Response is removed. There are no intermediary
responses and no changing context, everything is just the Transfer.
A smaller change is the addition of newRequest + transfer.submit(). The one-shot
HttpClient.request and HttpClient.requestT still exist, but this explicit create
+ submit has some advantage. First, callers can use the transfer.arena (e.g.
Frame using the transfer's arena to set the Referrer header). Second, callers
can holds Transfer immediately, rather than waiting for their startCallback to
be fired. An abort on an XMLHttpRequest called before the start of the transfer
no longer silently fails.
Adds a new command line argument `--watchdog-ms` which, when set, will terminate
any JS that appears to be hung.
When configured, a new thread is started. Workers heartbeat this thread to
signal activity (e.g. not stuck in a JS loop). However, workers also block for
their own reason (e.g. network polling), so they can signal the watchdog that
they are "entering a wait" and, when complete, that they are "existing a wait".
During such waits, the watchdog will not signal the isolate to terminate.
Obviously, it's important for workers to signal aliveness and whenever they plan
on doing a non-JS wait. So you could say we introduce safe points where the
watchdog (for that browser) is disabled. We could do the opposite: enable the
watchdog whenever we enter JS ("hey, I'm about to execute JS, monitor me). But
there are _a lot_ more place where this happens.
fetch() ignored RequestInit.redirect: both manual and error behaved
like follow. Thread a redirect mode through HttpClient.Request and the
webapi Request:
- manual: don't follow the 3xx; deliver it as the final response, which
Fetch turns into an opaque-redirect filtered response (status 0, type
opaqueredirect, redirected false, empty url/headers/body).
- error: reject the fetch promise with a TypeError.
Default stays .follow, so navigations, XHR and internal requests are
unaffected. Also exposes Request.prototype.redirect.
Fixes#2908
Largely about improving the named indexer storage["blah"] to support setting
and deletion. Also adds support for numeric indexes storage[4] by adding an
indexer (not named) and converting the integer -> string.
A number of other files had minor changes because namedIndexed now takes a
deleter, enumerator and query functions (or null) so all the other namedIndexed
now need to pass 3 other nulls. I assume these should be set on at least some
of the other consumers.
Currently, we give v8 8MB more memory. This new commit increases that to 64MB
up to a total of 256MB (at which point, no extra space is given).
The commit also instructs v8 to eventually go back to its original heap limit
(and not continuing using the higher limit it was given by nearHeapLimit). This
is important since the isolate can be relatively long lived compared to a script
that caused memory pressure. This feature is currently commented and depends on:
https://github.com/lightpanda-io/zig-v8-fork/pull/187
Mirror the wpt/e2e-integration pattern: tee the suite output to a log,
strip the ANSI colors, and upload it to the #ci-agent channel
(AGENT_SLACK_CHANNEL_ID) via the CI slack bot, with the final
pass/fail summary as the comment. The post also happens when the
suite fails, without masking its exit code (explicit bash pipefail).
Add a nightly cron trigger (03:37, offset from wpt's 02:21) so the
suite actually runs unattended; workflow_dispatch stays for manual
runs with a model override.
Adds deduping to attribute names. This has two benefits. First, it results in
fewer dupes/allocations. Second, it allows finding an attribute name by a single
pointer comparison.
Say we need to create an attribute attr1=val1 (from the parser or JS, doesn't
matter). We:
1 - Lookup "attr1" in String.intern, it doesn't exist
2 - We lookup or create "attr1" in Frame._attribute_names
Whether the name was found in String.intern, found in Frame._attribute_names or
created in Frame._attribute_names, any attribute name "attr1" always points to
the same value.
Now say we want to lookup the value "attr1". We _could_ iterate the attribute
list and do a string comparison on each attribute. OR, we could apply the same
canonicalization to that input as we did when building the list. We can do this
via a read-don't-create API. If it doesn't find it, than that attribute was
never canonicalized and thus, cannot exist (early exit!). If it IS found, then
we now have the pointer for "attr1" that all attributes with "attr1" use, so
we can just do a pointer comparison.
We also replace the name: []const u8, value: []const u8 with a packed
representation (where len: u32, instead of u64) meaning every attribute entry
saves an extra 8 bytes (on top of the 16 bytes saved by the previous commit)
An element's attributes are currently stored as a ?*Attribute.List which is an
intrusive doubly linkedlist of Attribute.Entry. This has two small benefits:
1 - Elements with no attributes only grow by 8 bytes
2 - Attribute list mutation (additions/deletion) are linkedlist cheap
This commit embeds an ArrayList-like structure directly in Element. The impact
being:
1 - Elements with no attributes now grow by 16 bytes (+8)
2 - Elements with 1+ attribute shrink from 32 -> 16 bytes (-16)
3 - Mutations are more expensive
4 - Fewer indirections (and better cache locality)
5 - Much fewer allocations (4 attributes go from 5 allocations to 1)
6 - Every Attribute shrinks by 16 bytes (no need for next/prev link)
While looking at a few popular sites (amazon product, redit post, ...), the
majority of elements have 1+ element and most attributes are never accessed in
JS and, when they are, reads are more frequent than writes (in fact, even
internally to support other WebAPIs, reads far outweigh writes).
There's virtually no real world site where this shouldn't reduce memory usage
by hundreds of KB and also improve performance (in a way that isn't significant
to the overall page loading though).
