feat(m4-12): boa→VM PR3 — Event Object + Listener#73
Merged
Conversation
First commit of the Event Object + Listener migration (PR3). Adds the `EventTarget.prototype` intrinsic that every DOM wrapper will inherit from in PR3 C2. Design: a single shared prototype (instead of per-wrapper method registration) matches the WHATWG DOM §2.7 EventTarget interface and aligns with how `Promise.prototype` / `Array.prototype` are structured elsewhere in the VM — one NativeFunction allocation per method regardless of wrapper count. The three method bodies are stubs at C0: - `addEventListener` / `removeEventListener` return `undefined` — real implementations land in PR3 C7 / C8 (ECS EventListeners component + HostData::listener_store wiring). - `dispatchEvent` returns `false` (spec default for "not dispatched") — the real implementation is deferred to PR5a alongside the `Event` constructor family, which is the only meaningful path for JS-constructed events to enter a synchronous dispatch. New files: - `vm/host/mod.rs` — host integration module root. - `vm/host/event_target.rs` — prototype + three native stubs + `VmInner::register_event_target_prototype()`. - `vm/tests/tests_event_target.rs` — three unit tests. VmInner gains `event_target_prototype: Option<ObjectId>`; populated from `register_globals()` after the generator prototype. Tests: 1683 → 1686. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add the `ObjectKind::HostObject { entity_bits: u64 }` variant that
every DOM wrapper will use. `entity_bits` packs `Entity::to_bits()`,
keyed against `HostData::wrapper_cache` so repeated lookups for the
same Entity return the same ObjectId (`el === el` identity, landed
properly in PR3 C2).
Design: the variant stores only a plain `u64`; no ObjectId fields,
so GC has nothing to trace. The variant is still listed explicitly
under the GC match's "no ObjectId references" arm — the existing
`trace_work_list` documentation promises exhaustive matching with no
wildcard, and folding HostObject under a wildcard would silently
defeat the compile-time check if a future field carries an ObjectId
(e.g. a cached child-wrapper list).
The wrapper keeps itself alive via `wrapper_cache`, which is rooted
from `HostData::gc_root_object_ids()` — already wired in PR1.
Tests (`tests_host_object.rs`):
- entity_bits round-trip preserved after allocation.
- Rooted (stack-held) HostObject survives a GC cycle; entity_bits
unchanged.
- Unrooted HostObject is reclaimed by GC (slot `None` afterwards).
Tests: 1686 → 1689.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…dentity
Introduces `VmInner::create_element_wrapper(entity) -> ObjectId`, the
single entry point every host-side DOM API will use to surface an ECS
Entity as a JS object. Enforces two invariants:
- **Identity** (`el === el`) — the wrapper ObjectId is keyed against
`HostData::wrapper_cache` by `Entity::to_bits().get()`. Cache hits
return the existing ObjectId without allocation.
- **EventTarget inheritance** — the wrapper's prototype is set to
`EventTarget.prototype` (landed in C0) so `addEventListener` /
`removeEventListener` / `dispatchEvent` resolve via the prototype
chain instead of requiring per-wrapper method registration.
GC safety: `event_target_prototype` is added to `GcRoots::proto_roots`
(array size 15 → 16) so wrappers' prototype field stays reachable
regardless of cache pressure. `HostData::gc_root_object_ids()` (wired
in PR1) roots the wrapper itself via `wrapper_cache`.
The wrapper carries only `HostObject { entity_bits }` + the prototype
slot — no properties installed at creation. Per-interface methods
(`getAttribute`, `textContent`, tree traversal, etc.) arrive in PR4
when the full DOM method suite lands.
New file: `vm/host/elements.rs` (engine-feature-gated).
Tests (`tests_element_wrapper.rs`, engine-feature-gated, 5 cases):
- Cache identity: `create_element_wrapper(e) === create_element_wrapper(e)`.
- Distinct entities → distinct wrappers.
- Wrapper prototype equals `event_target_prototype`.
- Wrapper ObjectKind is HostObject with matching entity_bits.
- Wrapper survives a GC cycle when its only root is wrapper_cache;
subsequent lookup returns the same ObjectId.
Tests: 1686 → 1689 (default) / 1689 → 1694 (engine feature).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add the `ObjectKind::Event` variant with all three flag fields
(`default_prevented`, `propagation_stopped`,
`immediate_propagation_stopped`) plus the two immutable flags
(`cancelable`, `passive`) and the lazily-cached
`composed_path: Option<ObjectId>`.
Design: flags live in the internal slot — same pattern as
`ObjectKind::Promise` / `ObjectKind::Generator`, where the native
method matches on `this`'s `ObjectKind` to locate state. This avoids
hidden property keys (which would leak through `Reflect.ownKeys`)
and sidesteps the VM's bare-`fn`-pointer limitation that forbids
closure capture.
New native methods (`natives_event.rs`, registered on Event objects
in PR3 C4 `create_event_object`):
- `preventDefault` — §2.9 silent no-op when `!cancelable` or
`passive`; otherwise sets `default_prevented`.
- `stopPropagation` — sets `propagation_stopped`.
- `stopImmediatePropagation` — sets BOTH flags (spec explicitly
performs the outer flag write too).
- `composedPath` — returns the cached Array from the internal slot
or an empty Array if the slot is `None` (slot is populated during
listener dispatch in PR3 C5+).
Detached method invocation (`const pd = e.preventDefault; pd()` with
`this === undefined`) is a silent no-op, matching browser behaviour.
Calling with a non-Event `this` is likewise a silent no-op — the
spec treats these methods as unconditionally callable on any Event.
GC: `trace_work_list` gets an `Event { composed_path, .. }` arm that
marks the cached array when present. Listed explicitly (not under a
wildcard) per the existing convention.
Tests (`tests_event_object.rs`, 8 cases) — driven directly via
`NativeContext` to decouple from the PR3 C4 property-installation
work:
- `preventDefault` flag flip on cancelable+non-passive.
- `preventDefault` no-op when not cancelable.
- `preventDefault` no-op on passive listener.
- `preventDefault` silent no-op on detached `this`.
- `stopPropagation` sets only outer flag.
- `stopImmediatePropagation` sets BOTH flags.
- `composedPath` empty array fallback when slot is None.
- `composedPath` identity — returns the exact cached ObjectId.
Tests: 1689 → 1697 (default) / 1694 → 1702 (engine feature).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add `VmInner::create_event_object(event, target_wrapper, current_target_wrapper, passive)`,
the function every listener invocation will use (in PR3 C5) to build
its per-listener JS event object.
Installs, as own properties on the `ObjectKind::Event` variant:
- Core data props: `type`, `bubbles`, `cancelable`, `eventPhase`,
`target`, `currentTarget`, `timeStamp` (0.0 pending PR4
`performance.now()`), `composed`, `isTrusted`.
- `defaultPrevented` — a live accessor (getter reads the
internal-slot `default_prevented` flag). Required by WHATWG DOM
§2.9 so `e.preventDefault(); e.defaultPrevented` observes `true`
inside the same listener. New native:
`native_event_get_default_prevented`.
- Four method data props: `preventDefault`, `stopPropagation`,
`stopImmediatePropagation`, `composedPath`.
- Payload-specific props per `EventPayload` variant: Mouse /
Keyboard / Transition / Animation / Input / Clipboard /
Composition / Focus / Wheel / Message / CloseEvent / HashChange /
PageTransition / Storage. Focus `relatedTarget` resolves through
`create_element_wrapper` (PR3 C2), using the same wrapper cache
so identity holds against e.g. `el.focus(); e.relatedTarget === el`.
Design decisions:
- No `Event.prototype` — methods go directly on each event obj as
own data properties. `Event.prototype` ships with the Event
constructor in PR5a; until then, per-instance install keeps the
object shape self-contained.
- Attributes: `{¬W, E, C}` for data props (WebIDL `[Reflect]`
default); accessor uses the same triple with `is_accessor: true`.
- GC: suppressed for the duration of construction via the saved_gc
pattern (same as `do_new`). Native fn objects allocated during
install are only reachable via local Rust variables until the
property writes complete.
`EventPayload` is `#[non_exhaustive]`; a trailing `_ => {}` catches
future upstream variants and silently installs no payload props —
individual test coverage in PR3 C12 will surface missing setters.
New files: `vm/host/events.rs`.
Extends `vm/natives_event.rs` with the defaultPrevented getter.
Adds `elidex-plugin` to the `engine` feature's dep set (needed for
`EventPayload` variant destructuring).
Tests (`tests_create_event_object.rs`, engine-feature-gated, 7 cases):
- Core props installed with correct types.
- `defaultPrevented` accessor reflects internal-slot flag live
(pre-mutation false, post-mutation true).
- All four methods installed as NativeFunction own properties.
- Mouse payload: coords + button + 4 modifier keys.
- Keyboard payload: key/code + `repeat`.
- Focus payload: `relatedTarget` resolves to a HostObject wrapper;
`None` → `null`.
- `ObjectKind::Event` carries `default_prevented`/`passive`/`cancelable`
from arguments.
Tests: 1697 → 1697 (default — no new default-feature tests) /
1702 → 1709 (engine feature, +7).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The architectural milestone of PR3. `ScriptEngine::call_listener` was
a stub; this commit wires the full vertical slice:
HostData::listener_store lookup
→ create_element_wrapper(target, current_target)
→ create_event_object(event, target_wrapper, current_wrapper, passive)
→ push event obj on VM stack as temporary GC root
→ vm.call(listener, this=current_wrapper, args=[event_obj])
→ sync ObjectKind::Event flag fields back to event.flags
→ pop temporary root
Listener removed between dispatch-plan freeze and invocation: silent
no-op (matches WHATWG DOM §2.10 step 5.4 "if listener's removed is
true, then continue"). Listener errors are swallowed — there is no
sensible recovery path inside dispatch; routing through host log
channels lands in PR4 alongside `console`.
GC safety: the event object has no other root once
`create_event_object` returns; the temporary stack push covers the
narrow window before `vm.call` enters frame setup (where args are
copied onto the stack and become rooted). Target / currentTarget
wrappers are rooted via `wrapper_cache`.
`remove_listener` becomes a thin forward to
`HostData::remove_listener`.
Tests (`tests_call_listener.rs`, engine-feature-gated, 5 cases):
★ `first_green_listener_calls_prevent_default` — the milestone test.
Compile `function(e) { e.preventDefault(); }`, register, dispatch
a cancelable click, assert `event.flags.default_prevented == true`.
- `passive_listener_prevent_default_is_silently_ignored` — passive
listener's `preventDefault` is gated to no-op via
`ObjectKind::Event.passive`.
- `stop_propagation_syncs_to_dispatch_flags` — both inner and outer
flags propagate from `stopImmediatePropagation`.
- `current_target_is_the_this_binding_for_the_listener` —
`this === e.currentTarget` (WHATWG DOM §2.10 step 5).
- `missing_listener_id_silently_no_ops` — stale ListenerId does not
panic, flags unchanged.
Tests: 1697 → 1697 (default — no new default tests) /
1709 → 1714 (engine feature, +5).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ntegration test The original PR3 plan called for draining microtasks + timers from inside `engine.call_listener`. Re-reading the shared dispatch core (`elidex_script_session::event_dispatch::script_dispatch_event_core` line 463) and boa's `call_listener_impl` (which explicitly notes "Microtask checkpoint is handled by the shared dispatch loop") shows the wiring is already in place at the dispatch-core layer: - `script_dispatch_event_core` calls `engine.run_microtasks(ctx)` after every listener invocation (HTML §8.1.7.3). - `drain_timers` is host-driven by the shell event loop, not per-listener. Adding a second drain inside `call_listener` would be a redundant no-op pass and would entangle this engine impl with the dispatch core's checkpoint policy. Instead this commit: 1. Adds an in-source comment in `engine.rs::call_listener` explaining why the drain is intentionally NOT performed locally. 2. Adds an integration test (`microtask_scheduled_inside_listener_fires_on_next_run_microtasks`) that mirrors the dispatch-core pattern: invoke the listener (which schedules a Promise.then microtask), assert the microtask has not yet fired, call `engine.run_microtasks(ctx)`, assert it now has. This locks down the cross-layer contract so a future refactor that moves the checkpoint elsewhere will break this test rather than silently regress to "microtasks only fire next event-loop tick" behaviour, which would cause hard-to-diagnose timing bugs in WHATWG-conforming Promise / MutationObserver code. Tests: 1714 → 1715 (engine feature, +1). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace the C0 stub for `EventTarget.prototype.addEventListener` with
the full WHATWG DOM §2.6 / §2.7 implementation:
- `this` extraction via `ObjectKind::HostObject { entity_bits }` —
non-HostObject receivers are silent no-ops (matches browser
behaviour for objects that feature-test the prototype method).
- `null` / `undefined` callback returns silently (§2.6 step 2).
- Non-callable callback throws `TypeError`.
- `type` argument is `ToString`-coerced.
- Options third argument:
- boolean → capture flag (legacy form).
- object → `{capture, once, passive}` properties read as booleans;
missing keys default to `false`.
- undefined / missing → all flags false.
- other types → `ToBoolean` → capture flag (browsers accept this).
- Duplicate check (§2.6 step 4) — `(type, callback, capture)` triple
via `EventListeners::matching_all` cross-referenced with
`HostData::listener_store`. `once` and `passive` are NOT part of
the identity tuple per spec.
- Successful registration writes to BOTH the ECS `EventListeners`
component (metadata) AND `HostData::listener_store` (function
ObjectId for dispatch lookup). Both are GC-rooted via
`HostData::gc_root_object_ids()`.
The implementation is `#[cfg(feature = "engine")]`; without the
feature there is no DOM/HostData and the method falls back to a
silent no-op stub, keeping the prototype method resolvable for
feature-test paths.
Helpers (also engine-gated):
- `entity_from_this` — pattern-match HostObject → `Entity`.
- `parse_listener_options` — boolean / object form unpacking.
- `listener_already_registered` — duplicate scan via cross-ref.
- `ListenerOptions` struct.
Deferred (per design D7 / PR3 plan):
- `signal` option (AbortSignal) → PR4 once `AbortController` lands.
- Object-form callback with `handleEvent` method (§2.7 step 8).
Tests (`tests_add_event_listener.rs`, engine-feature-gated, 10 cases):
- ECS component populated on first registration.
- Boolean options form sets capture only.
- Object options form reads all three flags.
- Object missing keys default to false.
- Duplicate (same callback + capture) silently skipped.
- Same callback different capture phase yields TWO entries.
- `null` / `undefined` callback silently ignored.
- Non-callable throws `TypeError`.
- Calls on non-HostObject silently no-op.
- listener_store entry points at the JS function ObjectId.
Tests: 1697 → 1697 (default — engine-only tests) /
1715 → 1725 (engine feature, +10).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace the C0 stub for `EventTarget.prototype.removeEventListener` with the full WHATWG DOM §2.7.7 implementation: - `this` extraction via HostObject (silent no-op otherwise). - `null` / `undefined` callback → silent no-op (§2.7.7 step 2). - Non-callable callback → silent no-op (cannot match anything; unlike addEventListener which throws). - Identity tuple per §2.6 step 4: `(type, callback, capture)` — `once` and `passive` are NOT compared. - Walk the entity's `EventListeners`; pick the matching entry by capture flag, then cross-reference `HostData::listener_store` to confirm the JS function ObjectId really matches before removing. - Both removals (ECS component + listener_store) happen atomically from the JS caller's view. The cross-reference guards against the rare case where a listener was added via direct `HostData::store_listener` bypass (e.g. test helpers) — without the check, we could falsely remove an entry that shares a `ListenerId` but maps to a different ObjectId. Engine-feature-gated; non-engine build keeps the C0 silent-no-op stub so prototype lookups still resolve. Tests (`tests_remove_event_listener.rs`, engine-feature-gated, 5): - Drops matching listener from ECS component. - Clears listener_store entry as well (no orphan ObjectId roots). - Capture-phase removal does not touch the bubble-phase entry. - Different callback identity → no-op (original survives). - `null` / `undefined` callback → no-op. Tests: 1725 → 1730 (engine feature, +5). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Install `document` as a JS global pointing at a `HostObject` wrapper
of the bound document entity. This is the minimum host global PR3
needs to light up scripts that wire `document.addEventListener`
(arguably the most common path for global event handlers).
The wrapper is created via `create_element_wrapper`, so its prototype
chain reaches `EventTarget.prototype` and inherits add/remove/
dispatchEvent. Identity holds across bind/unbind cycles via
`HostData::wrapper_cache`.
Wiring: `Vm::bind` (the existing public entry point that the shell
uses to thread session/dom/document pointers into the VM) now
auto-installs the `document` global immediately after `HostData::bind`
succeeds. This means *every* JS execution that comes through bind
sees a fresh-but-identical `document` reference — no separate setup
step required from the shell.
`window` is **deferred to PR4** alongside the rest of the Document /
Window / Location / History / Navigator surface. In a real browser
the Window has its own ECS entity, separate from the Document;
aliasing `window` to `document` here would silently misroute future
`load` / `hashchange` / `popstate` / `unload` / `message` listeners.
Until the Window entity exists, scripts touching `window` get an
honest `ReferenceError` rather than a misleading-but-functional
shim.
New file: `vm/host/globals.rs` (engine-feature-gated).
Tests (`tests_document_global.rs`, engine-feature-gated, 3 cases):
- `Vm::bind` installs `document` as HostObject wrapping the
document entity (entity_bits round-trip).
- `document.addEventListener('DOMContentLoaded', h)` reaches the
EventListeners ECS component on the document entity (prototype
chain via EventTarget.prototype).
- Wrapper identity preserved across bind/unbind/bind cycles.
Tests: 1730 → 1733 (engine feature, +3).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Wire HTML §8.1.5.5 HostPromiseRejectionTracker — when the microtask
checkpoint detects a rejection with no handler attached at settle
time, dispatch a `PromiseRejectionEvent` to the document's
`unhandledrejection` listeners. If any listener calls
`preventDefault()`, suppress the existing `eprintln!` fallback;
otherwise fall through to stderr (development diagnostic).
Design (per `m4-12-pr3-plan.md` D6): direct invocation, NOT through
the session event queue or shared `script_dispatch_event_core`.
Rationale:
- `unhandledrejection` is non-bubbling and targeted at the global
per spec; the 3-phase dispatch machinery has nothing to add.
- The session event queue's payload (`EventPayload`) lives in
`elidex-plugin` and is engine-agnostic. Carrying VM-specific
`ObjectId` (the rejected promise) and `JsValue` (the reason)
through it would force a `PromiseRejection { engine_handle: u64 }`
variant + a per-engine side map — cross-crate pollution that
this commit avoids.
- The dispatch reads the document's `EventListeners` ECS component
directly, looks up each function via `HostData::listener_store`,
and invokes through `vm.call`. Honours
`stopImmediatePropagation` between listeners.
The event object carries the standard core props (built via
`create_event_object`) plus the spec-required `.promise` and
`.reason` data properties, both `{¬W, E, C}` per WebIDL `[Reflect]`.
GC safety: the event object is rooted on the VM stack across all
listener invocations (same pattern as `engine.rs::call_listener` C5).
Without `feature = "engine"` the dispatch is a no-op stub returning
false — the eprintln fallback path is used for headless / standalone
use.
Tests (`tests_promise_rejection_event.rs`, engine-feature-gated, 4):
- `Promise.reject(new Error('boom'))` fires `unhandledrejection`;
listener captures `.promise` and `.reason`; reason.message === 'boom'.
- Listener observes `event.cancelable === true`; preventDefault
succeeds (suppresses fallback path).
- `event.target === document` (the dispatch root).
- No registered listener → silent stderr fallback, no panic.
Tests: 1733 → 1737 (engine feature, +4).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Two pieces:
1. New `HostData::remove_wrapper(entity) -> Option<ObjectId>` —
drops the cached wrapper for `entity` so its ObjectId becomes
eligible for GC. Called by the DOM-mutation hook (PR4) when an
entity is destroyed; without it, wrappers for removed elements
would stay rooted via `wrapper_cache` indefinitely. PR3
introduces only the API; the call site lands in PR4 alongside
the rest of tree-mutation surface (`removeChild`, etc.).
2. Comprehensive GC test suite (`tests_gc_audit.rs`) covering every
PR3-introduced ObjectId-bearing structure:
- `listener_store` roots a JS function past a collection (no
other JS-side reference held).
- `wrapper_cache` roots an element wrapper.
- `remove_wrapper` makes an entry reclaimable on the next cycle.
- `ObjectKind::Event { composed_path: Some(arr), .. }` traces
the cached path Array — Array survives only via the rooted
Event.
- `ObjectKind::Event { composed_path: None, .. }` does not panic
in the trace path (defensive coverage of the `if let Some` arm).
- Unrooted Event is reclaimed (sanity).
- End-to-end: `document` global HostObject reachable through
`globals` + `wrapper_cache` survives a collection.
Together with the per-commit GC tests already in C1 (HostObject)
and C2 (wrapper identity via cache), this closes the audit gap for
PR3's host-side roots — adding a future ObjectId-bearing field to
`ObjectKind::Event` or `HostData` will fail at least one of these
tests if the trace wiring is forgotten.
Tests: 1737 → 1744 (engine feature, +7).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Mirror the load-bearing scenarios from
`elidex-js-boa/src/runtime/tests/events.rs`, translated to the VM
engine and driven through `script_dispatch_event` (the public
dispatch entry point every shell uses). Confirms PR3's per-commit
unit tests compose into spec-conforming dispatch when the shared
3-phase machinery in `elidex-script-session` plays the lead.
New file: `tests_dispatch_integration.rs` (engine-feature-gated).
Tests (9):
- `listener_fires_on_at_target_phase` — basic dispatch.
- `prevent_default_returns_true_from_dispatch` —
`script_dispatch_event` return value reflects flag.
- `event_bubbles_through_parent` — bubble phase fires parent
listener after at-target.
- `stop_propagation_blocks_bubble_phase` —
`e.stopPropagation()` inside child blocks parent.
- `capture_phase_listener_fires_before_target` — capture-phase
outer fires BEFORE at-target inner (ordering verified via
string concatenation: `"OI"`).
- `once_listener_auto_removed_after_first_invocation` —
`{once: true}` listener fires exactly once across two dispatches.
- `passive_listener_prevent_default_does_not_propagate_to_return` —
`{passive: true}` listener's `e.preventDefault()` is silently
no-op'd, dispatch return is `false`.
- `mouse_event_payload_visible_to_listener` — `e.clientX/clientY`
read inside listener.
- `keyboard_event_key_property_visible` — `e.key` read inside
listener.
Test-helper pattern: `fresh_unbound` returns engine + dom + session
unbound; `bind_after_dom` is called explicitly AFTER all
`dom.create_element` / `append_child` mutations. Binding the VM
before DOM mutation would put the bound raw pointer into stacked-
borrows conflict with the `&mut dom` accesses required for
`create_element` etc. — addressing this concretely as a project
pattern (the bare `bind` API doesn't enforce it).
Tests: 1744 → 1753 (engine feature, +9).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Address the few lint findings that emerged once all PR3 commits
landed:
- `mod natives_event;` → `#[cfg(feature = "engine")] mod natives_event;`
Without the engine feature there is no `create_event_object` to
install the methods, so the four native fns are genuinely unused.
Gating the module silences the clippy `dead_code` errors that
fire under `-D warnings`.
- `tests_event_object.rs` → `#![cfg(feature = "engine")]` to match
the now-gated module.
- `native_event_composed_path` — collapse the nested `if let
ObjectKind::Event { composed_path, .. }` + `if let Some(arr) =
*composed_path` into a single `if let ObjectKind::Event {
composed_path: Some(arr), .. }` pattern (clippy
`collapsible_match`).
`mise run ci` (lint + test-all + deny) green on this commit.
Final test counts: 1697 (default) / 1753 (engine feature).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Wires DOM-style event objects, listener registration/removal, and Promise rejection event dispatch into the elidex-js VM as part of the boa→VM migration, establishing the host-integration patterns used by subsequent PRs.
Changes:
- Introduces
ObjectKind::EventandObjectKind::HostObjectplus GC tracing/support for new roots (wrapper cache, listener store, event composed path). - Adds
EventTarget.prototypeintrinsic and nativeaddEventListener/removeEventListenerplumbing viaHostData+ ECSEventListeners. - Dispatches
PromiseRejectionEvent-like events todocument’sunhandledrejectionlisteners during microtask draining, with stderr fallback.
Reviewed changes
Copilot reviewed 29 out of 30 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/script/elidex-js/src/vm/vm_api.rs | Refreshes document global on bind to reflect newly bound host document entity. |
| crates/script/elidex-js/src/vm/value.rs | Adds ObjectKind::Event internal-slot flags + ObjectKind::HostObject wrapper representation. |
| crates/script/elidex-js/src/vm/tests/tests_remove_event_listener.rs | Integration tests for removeEventListener behavior and listener_store cleanup. |
| crates/script/elidex-js/src/vm/tests/tests_promise_rejection_event.rs | Tests direct unhandledrejection dispatch via microtask drain. |
| crates/script/elidex-js/src/vm/tests/tests_host_object.rs | Tests HostObject allocation/GC survival semantics. |
| crates/script/elidex-js/src/vm/tests/tests_gc_audit.rs | GC audit tests for new PR3 roots and tracing. |
| crates/script/elidex-js/src/vm/tests/tests_event_target.rs | Tests EventTarget.prototype intrinsic allocation and method exposure. |
| crates/script/elidex-js/src/vm/tests/tests_event_object.rs | Tests Event native methods operating on internal slots. |
| crates/script/elidex-js/src/vm/tests/tests_element_wrapper.rs | Tests wrapper caching, prototype chain, and GC rooting via wrapper_cache. |
| crates/script/elidex-js/src/vm/tests/tests_document_global.rs | Tests document global installation and identity across rebinds. |
| crates/script/elidex-js/src/vm/tests/tests_create_event_object.rs | Tests create_event_object property/method/accessor installation and payload mapping. |
| crates/script/elidex-js/src/vm/tests/tests_add_event_listener.rs | Integration tests for addEventListener options/dup/null/non-callable handling and listener_store insert. |
| crates/script/elidex-js/src/vm/tests/mod.rs | Registers newly added VM test modules. |
| crates/script/elidex-js/src/vm/natives_promise.rs | Adds host-side unhandledrejection dispatch during microtask drain with preventDefault suppression. |
| crates/script/elidex-js/src/vm/natives_event.rs | Implements native Event methods and defaultPrevented getter. |
| crates/script/elidex-js/src/vm/mod.rs | Adds host module and event_target_prototype slot; wires natives_event behind feature gate. |
| crates/script/elidex-js/src/vm/host_data.rs | Adds HostData::remove_wrapper API for wrapper_cache lifecycle management. |
| crates/script/elidex-js/src/vm/host/mod.rs | Introduces VM host integration module structure. |
| crates/script/elidex-js/src/vm/host/globals.rs | Adds Vm::install_document_global to expose document HostObject. |
| crates/script/elidex-js/src/vm/host/events.rs | Implements VmInner::create_event_object and payload-specific property installation. |
| crates/script/elidex-js/src/vm/host/event_target.rs | Implements EventTarget.prototype and engine-gated add/remove listener natives. |
| crates/script/elidex-js/src/vm/host/elements.rs | Implements create_element_wrapper with identity caching and prototype linkage. |
| crates/script/elidex-js/src/vm/globals.rs | Registers EventTarget.prototype intrinsic during global registration. |
| crates/script/elidex-js/src/vm/gc.rs | Updates GC proto roots and traces Event.composed_path; explicitly lists HostObject. |
| crates/script/elidex-js/src/tests_dispatch_integration.rs | End-to-end dispatch tests via shared 3-phase dispatch core. |
| crates/script/elidex-js/src/tests_call_listener.rs | Integration tests for ScriptEngine::call_listener and event-flag sync-back. |
| crates/script/elidex-js/src/lib.rs | Adds engine-feature test modules to crate-level tests. |
| crates/script/elidex-js/src/engine.rs | Implements listener invocation, event object creation, flag sync-back, and listener removal. |
| crates/script/elidex-js/Cargo.toml | Extends engine feature to include elidex-plugin. |
| Cargo.lock | Adds elidex-plugin to elidex-js dependency set in lockfile. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Consolidate the most-impactful findings from a 3-agent simplify review of PR3 (reuse + quality + efficiency). ## Performance — biggest wins **Internal `event_methods_prototype` intrinsic** (Efficiency #2 + #3): Methods (`preventDefault` / `stopPropagation` / `stopImmediatePropagation` / `composedPath`) and the `defaultPrevented` accessor now live on a single shared prototype populated once at `register_globals` time, instead of being allocated and installed as own properties per event. Drops **5 NativeFunction allocations + 5 shape transitions per event object construction** to zero — this is on the per-listener- invocation hot path (15+ create_event_object calls for a 5-ancestor 3-listener-each dispatch). NOT exposed as the spec `Event.prototype` global (constructor + visible global ship in PR5a alongside `new Event(...)`); pure VM intrinsic. When PR5a lands this can become parent of (or be replaced by) the spec prototype. **Cache event property names in `WellKnownStrings`** (Efficiency #1 + #5; Reuse #5): `EventStrings::intern(self)` had been re-interning 14 property names on every `create_event_object` call. All 14 plus the 3 listener-options keys (`capture` / `once` / `passive`) plus `document` / `unhandledrejection` / `promise` move to `WellKnownStrings`, allocated once at VM construction. Read sites take the StringId directly from `self.well_known.X` — no HashMap lookup per dispatch. **Cheap pre-check before `Vec` alloc in `dispatch_unhandled_rejection_event`** (Efficiency #4): the no-listener case now bails before allocating any `Vec<ListenerId>` or building the synthetic event. **`install_document_global` reads pre-interned StringId** (Efficiency #9): `Vm::bind` runs this on every JS execution boundary; bypassing the intern lookup matters. ## Code quality / dedup **`PropertyAttrs::WEBIDL_RO` / `WEBIDL_RO_ACCESSOR` constants** (Reuse #3): replaced the same `{¬W, E, C}` literal previously duplicated in `vm/host/events.rs::EVENT_RO` and `vm/natives_promise.rs::PropertyAttrs { … }` inline. Single source of truth in `shape.rs`. **`Vm::with_temp_root(value, |vm| …)` helper** (Quality #1): replaces the bare `self.vm.inner.stack.push(JsValue::Object(id))` / `self.vm.inner.stack.pop()` pair in `engine.rs::call_listener` — engine.rs no longer reaches into the VM's internal stack representation. RAII-ish: pop happens on early-return paths through the closure. natives_promise.rs keeps the direct stack push/pop since it's in-module to the VM and the leak concern was specifically the cross-crate boundary. **`debug_assert!` → `assert!` for HostData duplicate-key writes** (Quality #15): `store_listener` and `cache_wrapper` now panic in release as well as debug. Silent overwrite would orphan the prior ObjectId — dropping it from `gc_root_object_ids` while live JS references may still reach it (use-after-free recipe). Better to panic loudly than corrupt silently. **Drop `EventStrings` struct + `intern` fn** (Reuse #5 follow-up): no longer needed once the keys live in `WellKnownStrings`. Drops ~40 lines. **Drop `alloc_native_fn` / `install_method` / `EVENT_RO` / `EVENT_ACCESSOR` from events.rs** (Reuse #1 + #2): per-event method installation is gone (methods on prototype), so these helpers are unused. Also fixes a latent spec regression flagged in review: `alloc_native_fn` skipped the `.name` data property that `create_native_function` installs (`e.preventDefault.name === ""` under the old code; now correctly `"preventDefault"`). **`#[non_exhaustive]` debug_assert in `set_payload_properties`** (Quality #6, Efficiency #11): the trailing `_ => {}` arm now debug-asserts so a new upstream `EventPayload` variant landing without a matching arm fails tests in debug builds, instead of silently producing an event with no payload props. ## Tests `tests_create_event_object`: two tests updated to look up the four methods + `defaultPrevented` accessor on `event_methods_prototype` instead of as own properties (the assertion semantically is "reachable via prototype chain", which is identical from JS but different at the storage layer). `mise run lint` + `mise run test-all` both green. `deny` step fails on a pre-existing webpki advisory (RUSTSEC-2026-0098) — unrelated to this PR. Test counts: 1697 → 1689 (default; 8 tests moved to engine-feature gate as part of the natives_event module-level cfg) / 1753 → 1753 (engine feature, unchanged). ## Deferred (out of /simplify scope) - Per-payload precomputed shapes (Efficiency #3 second half) — ~14 fixed shape paths per `EventPayload` variant. Bigger refactor. - Cross-crate StringId for `EventListeners` (Efficiency #7) — needs `elidex-script-session` API change. - `performance.now()` for `timeStamp` (Quality #12) — coupled with PR4's full Window surface. - Test-helper consolidation into `vm/tests/common.rs` (Reuse #9) — ~50 lines saved across 7 files; defer to follow-up. - `cancelable` property `configurable: false` per WebIDL `[Reflect]` (Quality #5) — needs spec audit on which event props are `[LegacyUnforgeable]`. - `TypeError` on detached `defaultPrevented` getter (Quality #14) — minor spec deviation, follow-up. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…mposedPath identity Address 4 findings from PR3 Copilot review (PR #73): ## CRITICAL — `removeEventListener` correctness (event_target.rs) `removeEventListener` was picking the **first** entry matching (type, capture) and bailing if its `listener_store` callback didn't match the candidate. Two listeners on the same (type, capture) with different callbacks → removing the second one would silently fail. Fix: refactor `listener_already_registered` into a single `find_listener_id(ctx, entity, type, capture, candidate)` helper that does a true (type, capture, callback) search by collecting all (type, capture) candidates and finding the one whose listener_store entry resolves to the candidate. Both addEventListener (duplicate check via `.is_some()`) and removeEventListener (the actual id to drop) now share the lookup, preventing future drift on what counts as a "match". Regression test: `tests_remove_event_listener:: remove_finds_correct_entry_among_multiple_same_type_capture` — registers `h1` and `h2` both on `'click'` bubble phase, removes `h2`, asserts `h1` survives and the listener_store entry still points at `h1`. ## IMPORTANT — `composedPath()` identity (natives_event.rs) WHATWG DOM §2.9: `e.composedPath() === e.composedPath()` must hold. The previous impl returned a fresh empty Array on each call when the slot was None, breaking identity. Fix: lazy-allocate the empty Array on first call AND write it back into the `composed_path` slot. Subsequent calls hit the cached fast path. GC is suppressed across the alloc + writeback so direct native invocation (in tests) doesn't see use-after-free on the event id — the interpreter's per-native gc gate (`interpreter.rs:71`) covers production calls but tests bypass it. Test rewritten: `composed_path_lazy_caches_empty_array_on_first_call` calls twice and asserts identity, then verifies the slot is populated. ## IMPORTANT — Test didn't exercise stated behavior (tests_add_event_listener.rs) `calls_on_non_host_object_silently_no_op` constructed a plain object but never actually invoked `addEventListener` on it; the test would have passed even if the non-HostObject path panicked. Fix: invoke via `document.addEventListener.call({}, 'click', function(){})` — pulls the native off the EventTarget prototype chain (since `EventTarget` global doesn't exist until PR5a) and threads `{}` in as `this`, exercising `entity_from_this`'s None return + the native's silent-no-op fallback. ## MINOR — Comment / code mismatch (tests_call_listener.rs) The helper docstring claimed "the unbind happens at the end so subsequent call_listener uses a fresh bind" but never called `unbind()`. Comment updated to match actual lifecycle: VM stays bound on return, callsite is responsible for cleanup. `mise run lint` + `cargo test -p elidex-js --features engine` (1754 tests passing). `deny` continues to fail on pre-existing RUSTSEC-2026-0098 webpki advisory — unrelated. Resolves PR #73 review threads: - crates/script/elidex-js/src/vm/host/event_target.rs:346 - crates/script/elidex-js/src/vm/natives_event.rs:143 - crates/script/elidex-js/src/vm/tests/tests_add_event_listener.rs:246 - crates/script/elidex-js/src/tests_call_listener.rs:43 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 31 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The PR3 CI's `Licenses & Vulnerabilities` job started failing on 2026-04-15 when **RUSTSEC-2026-0098** (webpki name-constraints bug, GHSA-965h-392x-2mh5) was published. Unrelated to PR3's changes — the advisory landed today and main's last green run was 2026-04-14. `rustls-webpki 0.103.12` (patched release) is available; the update is a transparent semver-compatible Cargo.lock bump pulled in via the rustls / hyper-rustls / reqwest / rustls-platform-verifier chain. `mise run ci` (lint + test-all + deny) all green on this commit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Single round-2 finding: `Vm::with_temp_root` (vm_api.rs:50) does an unconditional `stack.pop()` without verifying that the pushed root is still on top. If the closure (or anything it calls) leaks a push or over-pops, the bare `stack.pop()` either removes the wrong value or underflows — silently corrupting GC roots and making later GC behaviour unpredictable. ## Fix Replace the bare push/pop with a length-tracked truncate: 1. Save `stack.len()` before push. 2. After the closure returns, `assert_eq!` the stack is at `saved_len + 1` (release-enforced — a stack imbalance here is a GC root corruption hazard, panicking is strictly safer than silent corruption). 3. `debug_assert_eq!` the top is the value we pushed (catches the "closure replaced the top" case in tests). 4. `stack.truncate(saved_len)` to restore — robust against any minor leak the assert may not catch in release. Panic semantics: if `f` panics, truncate is skipped — but the VM doesn't `catch_unwind`, so a panic aborts the process and leaked roots are irrelevant. ## Same-pattern audit Same shape lived inline in `vm/natives_promise.rs::dispatch_ unhandled_rejection_event` (the `vm.stack.push(...) → listener loop → vm.stack.pop()` sequence). Hardened with the identical length-check + truncate pattern. Comment now points at `Vm::with_temp_root` as the canonical implementation; the inline version stays only because `natives_promise.rs` operates on `&mut VmInner`, not `&mut Vm`. `mise run ci` (lint + test-all) green on this commit (1754 tests passing). `deny` step pre-existing-fail from RUSTSEC-2026-0098 is already fixed in commit 18d121a (rustls-webpki bump). Resolves PR #73 round-2 thread: - crates/script/elidex-js/src/vm/vm_api.rs:54 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 31 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…NaN-safe)
Round 3 found 3 issues that all stem from the closure-based
`with_temp_root` design landed in R2:
1. **Doc claims "regardless of f's result" but panic skips cleanup**
(vm_api.rs:60). Bare push → assert → truncate is not unwind-
safe; if any caller upstream introduces `catch_unwind`, GC roots
leak after a panic in the rooted region.
2. **`debug_assert_eq!(stack.last().copied(), Some(value))` is
NaN-unsafe + release-blind** (vm_api.rs:77). `JsValue::PartialEq`
uses JS strict equality, so `Number(NaN) != Number(NaN)` —
spurious failure when rooting NaN. Plus `debug_assert!` is
debug-only, so release builds miss the "right length, wrong
top" case (closure pop'd then re-push'd a different value at
the slot, length unchanged but the original root is gone).
3. **Same pattern repeated** (natives_promise.rs:477) without
shared abstraction — drift risk between sites.
## Fix: single RAII guard `VmTempRoot`
New `VmInner::push_temp_root(value) -> VmTempRoot<'_>` returns an
RAII guard that:
- Holds `&mut VmInner` and exposes it via `Deref` / `DerefMut` —
rooted region is written as method calls on the guard.
- On `Drop`: truncates the stack to the saved length **including
during panic unwinding** (`std::thread::panicking()` short-
circuits the asserts to avoid double-panic process abort, but
the truncate runs unconditionally).
- Outside panic: `assert!` (release-enforced) the stack is at
exactly `saved_len + 1`, AND that `stack[saved_len]` still
holds the pushed value (using `same_value` for NaN-safety).
Catches both length corruption and pop-then-push-different.
`Vm::push_temp_root` is a thin `pub(crate)` wrapper that delegates
to `VmInner::push_temp_root` — engine.rs uses the Vm-level entry,
natives_promise.rs uses the VmInner-level entry directly (operates
on `&mut VmInner`).
Both items are `#[cfg(feature = "engine")]` since rooting matters
only when host code can produce un-rooted intermediates (event
objects, PromiseRejection synthetic events, …). Without engine
there is no host bridge and no caller.
## Call-site simplification
engine.rs:
```rust
// Before (closure):
self.vm.with_temp_root(JsValue::Object(id), |vm| { vm.call(...); ... });
// After (RAII):
let mut g = self.vm.push_temp_root(JsValue::Object(id));
let _ = g.call(...);
match g.get_object(id).kind { ... }
// g drops here; stack restored
```
natives_promise.rs: same shape, drops the inline assert + truncate
duplication.
## Tests / CI
`mise run ci` (lint + test-all + deny) all green on this commit.
1754 tests passing. Both default and engine feature builds clean.
Resolves PR #73 round-3 threads:
- crates/script/elidex-js/src/vm/vm_api.rs:60
- crates/script/elidex-js/src/vm/vm_api.rs:77
- crates/script/elidex-js/src/vm/natives_promise.rs:477
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… capture (spec-correct getter observability)
Round 7 found a single MINOR but spec-correctness issue:
## MINOR — `removeEventListener` reads `once`/`passive` from options bag
`removeEventListener` was using `parse_listener_options` (the
addEventListener helper that reads all three flags) to extract
`capture` from the third argument. WHATWG DOM §2.7.7's
"flatten options" step for removal is narrower: only `capture`
should be touched.
Reading `once` / `passive` is observable via user getters (and
once Proxy lands in P4, via Proxy traps). Browsers do NOT trigger
those getters during removeEventListener — pages relying on the
spec invariant would observe extra side effects.
## Fix
Extract `parse_capture_only(ctx, val) -> Result<bool, VmError>`
that reads ONLY `capture` from object-form options (boolean form
unchanged). Replace the `parse_listener_options(...).capture` call
in `removeEventListener` with `parse_capture_only(...)`.
`parse_listener_options` stays as-is for `addEventListener`, where
all three flags are legitimately needed.
## Regression test
`remove_does_not_read_once_or_passive_from_options` builds an
options bag with sentinel-setting getters on all three properties.
After `el.removeEventListener('click', fn, opts)`:
- `capture_read === true` (must be flattened per spec)
- `once_read === false` (MUST NOT fire — pre-fix this was true)
- `passive_read === false` (MUST NOT fire — pre-fix this was true)
Verified to fail against the pre-fix code via stash-and-run.
`mise run ci` (lint + test-all + deny) all green. 1760 → 1761
tests passing (engine feature, +1).
Resolves PR #73 round-7 thread:
- crates/script/elidex-js/src/vm/host/event_target.rs:352
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 31 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… during dispatch
Round 8 — single IMPORTANT GC safety finding in
`warn_unhandled_rejections`:
The previous impl did `let pending = std::mem::take(&mut vm.pending_rejections);`
before iterating and dispatching each rejected Promise. But
`pending_rejections` is part of the GC root set (see
`gc.rs::GcRoots::pending_rejections`); moving the Vec out unroots
the Promise objects.
`dispatch_unhandled_rejection_event` allocates wrappers + an event
object internally — any of these can trigger a GC cycle. In the
common case (`Promise.reject('x')` with no JS-side reference), the
Promise's only reachability is the `pending_rejections` entry; the
GC during dispatch reclaims its slot, leaving `e.promise`
referencing a freed slot when the listener body dereferences it
(panic: "object already freed").
## Fix
Replace `mem::take` + iter with index-based iteration over the
live `pending_rejections` Vec:
```rust
let initial_count = vm.pending_rejections.len();
for i in 0..initial_count {
let id = vm.pending_rejections[i]; // index read drops borrow
// ... dispatch (alloc-safe: id is GC-rooted via Vec) ...
}
vm.pending_rejections.drain(..initial_count);
```
`initial_count` snapshots the boundary so any rejections pushed
*during* this drain (a listener creating + rejecting another
Promise) survive for the next outer drain — matches the
microtask-checkpoint semantics in `script_dispatch_event_core`.
## Same-pattern audit
5 other `mem::take` sites (4 in `natives_promise_combinator.rs`,
2 in `natives_promise.rs::settle_promise`) all operate on Promise
state fields rooted via the Promise object itself; they don't span
external allocations. Only `pending_rejections` had the
"unroot Vec, then alloc inside loop" pattern.
## Test
`pending_rejections_drained_after_dispatch` — smoke test that
verifies the drain semantics (listener fires, `pending_rejections`
empty after). We can't trivially force the GC-race window
without test-only infrastructure (no JS `gc()` global), so this
test guards the structural invariant rather than the precise
use-after-free symptom. The fix's correctness comes from the
type-system + borrow-checker narrative: index iteration cannot
unroot the Vec.
`mise run ci` (lint + test-all + deny) all green. 1761 → 1762
tests passing (engine feature, +1).
Resolves PR #73 round-8 thread:
- crates/script/elidex-js/src/vm/natives_promise.rs:335
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 31 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Round 9 — 4 MINOR findings, all cosmetic:
## Style — replace `let _ = dom;` with block scopes (event_target.rs)
Two sites used the `let _ = dom;` workaround to force-end the
borrow before re-grabbing `host`. Under NLL this is technically
redundant, but more importantly it doesn't communicate intent —
a future reader would wonder what the no-op statement is doing.
Replaced with explicit `{ let dom = ...; ... }` block scopes that
make the borrow lifetime visually obvious:
- `find_listener_id` (the candidate-collection pass)
- `removeEventListener` (the ECS-component removal step)
Also drops the now-unnecessary `let host = ctx.host();` between
the two phases since the block scope releases the borrow naturally.
## Naming — `warn_unhandled_rejections` → `process_pending_rejections`
The function was originally a stderr-only warner; PR3 C10 grew it
into the full `PromiseRejectionEvent` dispatch path with stderr
fallback. The name no longer reflects the responsibility.
Renamed to `process_pending_rejections` (matches the field name
`pending_rejections`). 1 callsite in natives_promise.rs + 2
references in test comments updated.
## Doc — module header in event_target.rs out of date
The "Stub status" bullets still listed `removeEventListener` as a
stub (lands in PR3 C8) — but C8 landed in this PR. Updated the
section to "Method status" with current state of all three
methods, including the per-PR cross-reference (R7's
`parse_capture_only` for spec-correct option-bag handling).
`mise run ci` (lint + test-all + deny) all green. 1762 tests
unchanged. Pure cleanup commit, no behavioural changes.
Resolves PR #73 round-9 threads:
- crates/script/elidex-js/src/vm/host/event_target.rs:18
- crates/script/elidex-js/src/vm/host/event_target.rs:328
- crates/script/elidex-js/src/vm/host/event_target.rs:402
- crates/script/elidex-js/src/vm/natives_promise.rs:309
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 31 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…sistent rejection event phase Round 10 found 2 IMPORTANT issues — confirms the value of one more review pass after R9's pure-cleanup batch. ## IMPORTANT — addEventListener silently leaks listener_store on insert_one failure `addEventListener` registered a listener in two ECS branches: existing `EventListeners` component (mutate via `world_mut().get`) or absent (new component via `world_mut().insert_one`). The absent-branch path silently ignored `insert_one` failures (`let _ = dom.world_mut().insert_one(entity, listeners)`) but still stored the `ListenerId → ObjectId` mapping in `HostData::listener_store`. If the entity has been despawned between `entity_from_this` extraction and the insert (concurrent DOM mutation observer, etc.), this creates an orphan `listener_store` entry that: - ECS dispatch can never reach (no `EventListeners` component on the entity). - `removeEventListener` can never clean up (the find_listener_id path also bails on missing component). - Holds the JS function ObjectId GC-rooted via `gc_root_object_ids()` for the VM's lifetime. Fix: track the registration result as `Option<ListenerId>`, return `Ok(JsValue::Undefined)` if `insert_one` fails (matches the silent no-op pattern for non-HostObject receivers, etc.). No orphan, no leak. ## IMPORTANT — Synthetic DispatchEvent had stale phase / current_target / dispatch_flag `dispatch_unhandled_rejection_event` built a synthetic `DispatchEvent` for `unhandledrejection` but left `phase === EventPhase::None`, `current_target === None`, `dispatch_flag === false`. JS observers saw `e.eventPhase === 0` even though the event was actively being dispatched at the document target — diverging from regular dispatch semantics where `script_dispatch_event_core` threads `AT_TARGET` (2), `current_target = Some(target)`, `dispatch_flag = true` before each listener call. Fix: initialise the synthetic event to spec-consistent at-target state. Also seeds `composed_path = vec![document]` for forward compatibility — `create_event_object` doesn't yet wire DispatchEvent.composed_path into the Event obj's slot (returns empty array per `composedPath()`); when that wiring lands, this site is already correct. ## Regression tests Two new tests in `tests_promise_rejection_event`: - For #1: not added (would require rare-condition setup reproducing despawn race; the structural fix is straightforward to verify by reading). - For #2: `rejection_event_phase_is_at_target_and_current_target_is_document` — listener observes `e.eventPhase === 2` and `e.currentTarget === document`. Verified to fail pre-fix (`e.eventPhase === 0`). `mise run ci` all green. 1762 → 1763 tests passing (engine feature, +1). Resolves PR #73 round-10 threads: - crates/script/elidex-js/src/vm/host/event_target.rs:177 - crates/script/elidex-js/src/vm/natives_promise.rs:444 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 31 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… → Event slot Round 11 — 2 doc-correctness findings about composed_path overpromising what R10 actually wired. The right fix isn't to weaken the docs but to make them accurate: actually thread `DispatchEvent.composed_path` into the Event obj's `composed_path` slot so `composedPath()` returns the real propagation list. ## Fix `create_event_object` (after `push_temp_root` for GC safety) now: - Translates each Entity in `event.composed_path` to a `HostObject` wrapper via `create_element_wrapper`. - Allocates an Array of those wrappers. - Writes `Some(arr_id)` into the Event's `composed_path` slot. Empty `composed_path` (the common case for non-composed events) leaves the slot as `None`; `composedPath()`'s lazy-allocate path provides an empty Array on first call and caches it (preserving identity per WHATWG DOM §2.9). Hot-path cost: only events with non-empty `composed_path` pay the allocation; `script_dispatch_event_core` populates this only for composed events (and `dispatch_unhandled_rejection_event` which explicitly sets `vec![document]`). For typical mouse/keyboard events on non-composed elements: zero overhead. ## Doc updates - `natives_event.rs::native_event_composed_path` — reword the module-doc paragraph: "create_event_object populates the slot from DispatchEvent.composed_path when non-empty" (replacing the vague "dispatch machinery writes..."). - `natives_promise.rs` — the existing comment about `e.composedPath() === [document]` becomes accurate; cleaned up wording slightly. ## Test Extended `rejection_event_phase_is_at_target_and_current_target_is_document` to also assert: - `e.composedPath().length === 1` - `e.composedPath()[0] === document` Both fail without the wiring (composedPath returns empty Array). `mise run ci` all green. 1763 tests passing (unchanged count; the existing test gained 2 new assertions). Resolves PR #73 round-11 threads: - crates/script/elidex-js/src/vm/natives_event.rs:127 - crates/script/elidex-js/src/vm/natives_promise.rs:446 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 31 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…cro-opt Round 12 — 2 MINOR findings + 1 audit: ## MINOR (doc) — natives_event module doc overpromised spec compliance Module-level comment claimed silent-no-op for non-Event receivers "matches browser behaviour" and that the spec treats methods as "unconditionally callable on any Event instance". WebIDL bindings are non-generic and throw `TypeError: Illegal invocation` when the receiver fails the [[Brand]] check. Reworded the section to be honest about the deviation: PR3 chose silent-no-op pragmatically (real-world detached-method-handle escapes are exceedingly rare; linters block them), spec-correct TypeError enforcement is deferred to PR5a alongside the Event constructor + the `defaultPrevented` getter (also a /simplify Quality #14 deferral note). WPT alignment for §2.9 brand-checks will surface the gap when run in Phase 4 late. ## MINOR (perf) — `find_listener_id` matching_all → iter_matching `EventListeners::matching_all` returns `Vec<&ListenerEntry>` — allocates an intermediate vector that is immediately consumed by the subsequent `.iter()` chain. `iter_matching` returns a borrow- backed iterator with the same filter, no allocation. addEventListener (duplicate check) + removeEventListener (target location) both go through `find_listener_id` on every invocation, so the alloc savings add up on event-heavy pages. ## AUDIT — same pattern in dispatch_unhandled_rejection_event `PendingListener` collection in `natives_promise.rs::dispatch_ unhandled_rejection_event` had the identical `matching_all → iter → map → collect` shape. Switched to `iter_matching` for consistency. Skipped the `.iter()` step that's now redundant on the iterator return. `mise run ci` all green. 1763 tests unchanged (pure cleanup, no behavioural change). Resolves PR #73 round-12 threads: - crates/script/elidex-js/src/vm/natives_event.rs:17 - crates/script/elidex-js/src/vm/host/event_target.rs:346 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 31 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…rror on non-callable
Round 13 — 1 spec compliance finding:
`removeEventListener` was silently no-op'ing on any non-Object /
non-function callback (e.g. `el.removeEventListener('click', 42)`),
masking user bugs. WebIDL `EventListener? callback` allows null /
undefined as a silent no-op (§2.7.7 step 2), but any other
non-callable value is a type-conversion failure that throws
TypeError — exactly what `addEventListener` already does for the
same input.
## Fix
Align removeEventListener's callback validation with
addEventListener's:
```rust
let listener_obj_id = match callback_arg {
JsValue::Null | JsValue::Undefined => return Ok(JsValue::Undefined),
JsValue::Object(id) if ctx.vm.get_object(id).kind.is_callable() => id,
_ => return Err(VmError::type_error(
"Failed to execute 'removeEventListener' on 'EventTarget': \
parameter 2 is not of type 'EventListener'.",
)),
};
```
Both endpoints now share identical conversion semantics.
## Regression test
`remove_with_non_callable_callback_throws_type_error` —
`el.removeEventListener('click', 42)` must `Err(_)`. Verified
pre-fix would `Ok(_)` (silent no-op).
`mise run ci` all green. 1763 → 1764 tests passing (engine
feature, +1).
Resolves PR #73 round-13 thread:
- crates/script/elidex-js/src/vm/host/event_target.rs:395
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 31 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+376
to
+377
| /// - Non-callable callback → silent no-op (cannot match anything, | ||
| /// spec just no-ops here too — only addEventListener throws). |
There was a problem hiding this comment.
The doc comment for removeEventListener claims non-callable callbacks are a silent no-op and that only addEventListener throws, but the implementation below intentionally throws a TypeError for any non-null/undefined non-callable value (and tests assert this). Please update the comment to match the actual conversion/throw behavior so callers aren’t misled.
Suggested change
| /// - Non-callable callback → silent no-op (cannot match anything, | |
| /// spec just no-ops here too — only addEventListener throws). | |
| /// - Any other non-callable callback value is rejected during callback | |
| /// conversion and throws `TypeError`. |
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PR3 of the boa→VM migration — wires the Event Object + Listener calling
machinery into the VM, lighting up
addEventListener/removeEventListener/PromiseRejectionEventend-to-end through theshared 3-phase dispatch core. Critical-path PR that establishes the
host-integration patterns every subsequent PR4-7 will reuse.
13 commits, +39 engine-feature tests (1714 → 1753), 1697 default tests
unchanged. CI green (lint + test-all + deny).
Design (per
m4-12-pr3-plan.md)ObjectKind::Event { ... }internal slot(Promise/Generator pattern), NOT hidden JS properties.
ObjectKind::HostObject { entity_bits: u64 }variantfor every DOM wrapper.
EventTarget.prototypeintrinsic — methods inherited viaprototype chain instead of per-wrapper allocation.
dispatchEventJS-side complete impl deferred to PR5a (needsnew Event(...)).bypassing
EventPayloadto avoid cross-crate VM type leakage.is the first consumer).
Commits
Deferred to later PRs (per design)
dispatchEventfrom JS (full) → PR5a alongside Event constructorswindowhost global → PR4 (separate ECS entity from Document)GC audit
7 dedicated GC tests in
tests_gc_audit.rscover every PR3-introducedObjectId-bearing structure: listener_store roots, wrapper_cache roots,
ObjectKind::Event { composed_path: Some(...), .. }tracing, andHostData::remove_wrapperreclaim semantics. Adding a future ObjectIdfield to either struct without updating the trace will fail at least
one of these tests (compile-enforced for ObjectKind via the existing
exhaustive match).
Test plan
mise run ci— lint + test-all + deny greencargo test -p elidex-js --features engine— 1753 passedcargo test -p elidex-js(default) — 1697 passed (unchanged)function(e){e.preventDefault()}registered, dispatched,
event.flags.default_prevented == truePromise.reject(new Error('boom'))→unhandledrejectionlistener observes.reason.message === 'boom'stopPropagation / once / passive / mouse / keyboard payloads)
iterate to convergence per project policy)
🤖 Generated with Claude Code