wasm: Add revamped tracked JavaScript values API - #2359
Conversation
6bbb881 to
54f63f7
Compare
bettio
changed the title
|
This PR supersedes #2013. Provenance and history shape. The five original commits are Jakub's I rebased them onto Review comments from #2013. All of them are addressed:
New findings fixed on top of the review. A full pass over the whole
Compatibility with popcorn. Popcorn consumes this API in production, so Note for Software Mansion: FissionVM Testing. The cypress suite ( Thanks @jgonet for the original design and implementation, and @pguyot for |
| set(JIT_LINK_FLAGS | ||
| -sALLOW_TABLE_GROWTH | ||
| "-sEXPORTED_RUNTIME_METHODS=['ccall','addFunction','ENV']" | ||
| "-sEXPORTED_RUNTIME_METHODS=['ccall','addFunction','ENV','FS']" |
There was a problem hiding this comment.
I don't think FS is needed within this PR.
| ) | ||
| else() | ||
| set(JIT_LINK_FLAGS "-sEXPORTED_RUNTIME_METHODS=['ccall','ENV']") | ||
| set(JIT_LINK_FLAGS "-sEXPORTED_RUNTIME_METHODS=['ccall','ENV','FS']") |
There was a problem hiding this comment.
I don't think FS is needed within this PR.
| } | ||
|
|
||
| Heap heap; | ||
| size_t heap_size = LIST_SIZE(objects_n, TUPLE_SIZE(2)) + LIST_SIZE(strings_n, BINARY_HEADER_SIZE) + term_binary_data_size_in_terms(all_byte_size); |
There was a problem hiding this comment.
This allocation looks wrong and I wonder if we should use TERM_BINARY_DATA_SIZE_IN_TERMS macro. It is too complex for the CodeQL queries, but the following additional tests crash:
test_many_large_values_round_trip() ->
Strings = [lists:duplicate(50, C) || C <- lists:seq($a, $h)],
{ok, Refs} = emscripten:run_script_tracked(tracked_script(Strings)),
Expected = [{ok, list_to_binary(S)} || S <- Strings],
Expected = emscripten:get_tracked(Refs, value),
ok.
test_many_small_values_round_trip() ->
Strings = [lists:duplicate(10, C) || C <- lists:seq($a, $j)],
{ok, Refs} = emscripten:run_script_tracked(tracked_script(Strings)),
Expected = [{ok, list_to_binary(S)} || S <- Strings],
Expected = emscripten:get_tracked(Refs, value),
ok.
tracked_script(Strings) ->
Quoted = [[$', S, $'] || S <- Strings],
["[", lists:join($,, Quoted), "]"].
| Heap heap; | ||
| if (UNLIKELY(memory_init_heap(&heap, TUPLE_SIZE(2) + LIST_SIZE(keys_n, TERM_BOXED_REFERENCE_RESOURCE_SIZE)) != MEMORY_GC_OK)) { | ||
| free(keys); | ||
| send_tracked_trap_oom(global, process_id); |
There was a problem hiding this comment.
We need to clean up what js_tracked_eval inserted.
| -type html5_target() :: window | document | screen | iodata(). | ||
| -opaque listener_handle() :: binary(). | ||
| -opaque tracked_object() :: reference(). | ||
| -type get_tracked_result() :: {ok, binary()} | {error, badkey} | {error, badvalue}. |
There was a problem hiding this comment.
Not a fan of defining get_tracked_result() here, but if we do, we should export it.
There was a problem hiding this comment.
Fixed. I believe it is more readable this way.
| } | ||
| HEAPU32[size / HEAPU32.BYTES_PER_ELEMENT] = keys.length; | ||
| HEAPU32.set(keys, ptr / HEAPU32.BYTES_PER_ELEMENT); | ||
| return ptr; |
There was a problem hiding this comment.
malloc(0) could return NULL. Tests test_empty_array_yields_empty_list/0 and test_null_yields_empty_list/0 don't fail because emscripten's malloc(0) doesn't return NULL.
|
AMP, usual caveats: PR review: tracked JavaScript values APIRange reviewed: The final state of the 21 commits was reviewed as one change set; bugs fixed by later commits in the range were not reported. The review covered the JavaScript/C boundary, resource ownership, trap delivery, heap sizing, key identity, public Erlang contracts, and browser tests. Oracle was also asked to independently stress-test the final diff; its findings were verified against the cited implementation before inclusion below. Findings1. [Critical] Size each returned binary separately or
|
Fixed all the reported issues. |
|
by now I'm just the messsenger;-) Follow-up review: tracked JavaScript values fixesFollow-up range: The follow-up diff and the resulting final PR state were reviewed. Oracle independently reviewed the same range and the five findings in Disposition of the original findings
Remaining findings1. [High] Roll back invalid keys too, or exhaustion and malformed hooks still retain values
dropKeys(keys.filter(isKey));That deletes only valid siblings. Any value stored under the invalid key remains owned by neither JavaScript nor an Erlang resource. This also breaks the new exhaustion path. The default The new invalid-key test does not catch this: Every key returned by the hook is an ownership claim for this attempted call. If validation fails before resources exist, cleanup should invoke the deletion hook for every returned key, including malformed values. Exceptions are already caught by Smallest fix: diff --git a/src/platforms/emscripten/src/lib/platform_nifs.c b/src/platforms/emscripten/src/lib/platform_nifs.c
--- a/src/platforms/emscripten/src/lib/platform_nifs.c
+++ b/src/platforms/emscripten/src/lib/platform_nifs.c
@@ -263,7 +263,7 @@ EM_JS(uint32_t *, js_tracked_eval, (const char *code, uint32_t *size, uint8_t *s
const isKey = (key) => Number.isInteger(key) && key >= 0 && key <= 0xfffffffe;
if (!keys.every(isKey)) {
console.error("onRunTrackedJs returned invalid keys", keys);
- dropKeys(keys.filter(isKey));
+ dropKeys(keys);
setOutcome(ERROR, 0);
return 0;
}Update the test hook to store values under both the valid sibling and the actual invalid key, then assert both are absent. Add an exhaustion regression by temporarily overriding only 2. [High] Copy get-hook output inside the
|
Second follow-up review: tracked JavaScript values fixesRemote range reviewed: The contributor's remote branch was fetched and reviewed without changing the local Disposition of
|
| Finding | Status | Verification |
|---|---|---|
| 1. Invalid/exhausted keys omitted from rollback | Resolved | Validation now passes every returned key to dropKeys. Tests store under both valid and malformed keys, and the exhaustion test uses the default run hook and verifies that the sentinel entry is absent. |
| 2. Throwing getter in get-hook output traps the caller | Resolved | The hook result is copied with Array.from inside the exception guard. The new getter test expects the whole-call badvalue, restores the hook, and successfully fetches the original handle afterward. |
| 3. Duplicate/reused key ownership is not enforced or documented | Partially resolved | Duplicate keys within one result are rejected, and cross-call exclusive ownership is documented. The validation itself can still throw outside the guard, and rollback calls deletion once per occurrence rather than once per identity; see findings 1 and 2. |
4. Get-side allocation OOM is reported as badvalue |
Resolved in implementation; untested | Every explicit js_get_tracked_objects failure initializes the new call status. Allocation failures route to out_of_memory; malformed hooks retain the existing badvalue path. No allocator fault-injection test exercises the distinction. |
Findings
1. [High] Guard duplicate validation or evaluated JavaScript can trap the caller and leak its values
After onRunTrackedJs returns, duplicate validation executes outside the surrounding try/catch:
const hasDuplicates =
keys.length > 1 && new Set(keys).size !== keys.length;Set is a mutable JavaScript global and construction consumes the array iterator. Either operation can throw. This is reachable through the default hook, not only a deliberately broken override: the evaluated script runs before this line and can replace globalThis.Set, then return ordinary values:
globalThis.Set = class {
constructor() {
throw new Error("boom");
}
};
["owned"];The default hook stores "owned" and returns its key. The replacement Set constructor then throws outside the guard, so no trap answer is sent and no rollback runs. The Erlang caller waits indefinitely while the JavaScript value remains retained without a resource handle. A catchable allocation failure during Set construction has the same control flow. keys.every(isKey) is also outside the guard and resolves a mutable prototype method.
Suggested fix: perform validation inside an exception boundary, and make rollback independent of array iterators. The following combines that with the distinct-key cleanup required by finding 2:
diff --git a/src/platforms/emscripten/src/lib/platform_nifs.c b/src/platforms/emscripten/src/lib/platform_nifs.c
--- a/src/platforms/emscripten/src/lib/platform_nifs.c
+++ b/src/platforms/emscripten/src/lib/platform_nifs.c
@@ -230,14 +230,29 @@ EM_JS(uint32_t *, js_tracked_eval, (const char *code, uint32_t *size, uint8_t *s
// the hook stored these values but no resource owns them yet: drop them
// the way the resource destructor does, or they stay tracked forever
const dropKeys = (keys) => {
- for (const key of keys) {
+ for (let i = 0; i < keys.length; ++i) {
+ const key = keys[i];
+ let seen = false;
+ for (let j = 0; j < i; ++j) {
+ const previous = keys[j];
+ // SameValueZero, matching JavaScript Map key identity.
+ if (previous === key
+ || (previous !== previous && key !== key)) {
+ seen = true;
+ break;
+ }
+ }
+ if (seen) {
+ continue;
+ }
try {
Module['onTrackedObjectDelete'](key);
} catch (e) {
- console.error("onTrackedObjectDelete threw", e);
+ try {
+ console.error("onTrackedObjectDelete threw", e);
+ } catch (_) {
+ // Cleanup must not escape the dispatched callback.
+ }
}
}
};
@@ -262,11 +277,21 @@ EM_JS(uint32_t *, js_tracked_eval, (const char *code, uint32_t *size, uint8_t *s
// would coerce anything else silently (-1 to 4294967295, 2 ** 32 to 0,
// a fraction to its truncation) and alias an unrelated entry
const isKey = (key) => Number.isInteger(key) && key >= 0 && key <= 0xfffffffe;
- // a repeated key would get one handle each, and the first of them
- // collected deletes the value the others still address; only an array of
- // two or more keys can repeat one, so a single value result builds no set
- const hasDuplicates = keys.length > 1 && new Set(keys).size !== keys.length;
- if (!keys.every(isKey) || hasDuplicates) {
+ let hasInvalidKeys;
+ let hasDuplicates;
+ try {
+ hasInvalidKeys = !keys.every(isKey);
+ hasDuplicates
+ = keys.length > 1 && new Set(keys).size !== keys.length;
+ } catch (e) {
+ dropKeys(keys);
+ setOutcome(ERROR, 0);
+ return 0;
+ }
+ if (hasInvalidKeys || hasDuplicates) {
console.error("onRunTrackedJs returned invalid keys", keys);
// every returned key is an ownership claim, malformed ones included:
// the hook may have stored a value under them tooThis is the smallest control-flow correction. A stronger implementation can avoid mutable Set/Array.prototype.every entirely by validating and checking duplicates with indexed loops; that trades the temporary set allocation for quadratic duplicate checking.
Add a browser regression that replaces Set during the evaluated script, verifies that run_script_tracked/1 returns promptly, and verifies that the newly tracked value was deleted. Restore Set before making the assertion call.
2. [Medium] Roll back each distinct key once, not once per duplicate occurrence
When a hook returns [Key, Key], duplicate validation correctly rejects the result, but dropKeys(keys) invokes Module.onTrackedObjectDelete(Key) twice. The documented model transfers ownership of a key identity, not ownership of every occurrence in a malformed array. Since no resources were created, rollback should release that identity exactly once.
The default Map.delete hook is idempotent, so the current duplicate test passes. A valid custom deletion hook need not be idempotent: it may decrement a reference count, release an external object, or perform application-side cleanup. Calling it twice can therefore double-release state even if a second thrown exception is caught.
The indexed, SameValueZero-aware dropKeys diff in finding 1 fixes this without relying on Set, a mutable iterator, or native-key coercion. Update the internals wording from “every returned key is deleted” to “each distinct returned key is deleted once.”
Add a browser test that installs a counting onTrackedObjectDelete, returns [Key, Key], and asserts exactly one deletion callback before restoring the hook.
Verification performed
git diff --check 1078b72cd18ede37e46f2ae6b451c44d36958391..449ff9301c7c69ded84e00e2c8e4d29ed9eba321passes.- A clean release web build of exact remote tip
449ff9301completed with Emscripten 5.0.7 andAVM_DISABLE_JIT=ON. AtomVM.mjslinked successfully.- The
emscripten_erlang_test_modulestarget compiled all Emscripten test modules, including the revisedtest_run_script_tracked.erl. - The explicit
js_get_tracked_objectsstatus/cleanup paths were audited: malformed results setCALL_ERROR; all three allocation failures and the unrepresentable aggregate size setCALL_OOM; success setsCALL_OKafter publishing outputs. - Browser/Cypress tests and allocation fault injection were not run.
Merge recommendation
Do not merge remote tip 449ff9301 yet. Guard duplicate validation and make rollback operate once per distinct key. Both changes are localized; focused browser coverage should be added for the escaped-validation exception and deletion callback count.
|
btw I believe FS is used by popcorn - so maybe keep it around, seems unrelated to PR either way. |
I will make a PR just for it |
|
Fresh AMP, I mostly worry about the first finding, the rest are nits, and edge cases where a user replaces JS functions.. PR review: tracked JavaScript values for EmscriptenScope and recommendationReviewed the aggregate diff for the last 34 commits, Recommendation: request changes. The normal ownership and rollback paths Findings[High] Completion runs scheduler-only heap and mailbox operations on the browser main threadThe tracked eval/fetch callbacks are dispatched to Emscripten's browser main This becomes a destructive operation, rather than a harmless decrement, when:
The same foreign-thread completion path posts directly with Impact: ready-queue races, off-scheduler resource destruction, deadlock, or Required fix: move tracked completion onto an established task-safe path. This should not be fixed by mechanically replacing only [High] Evaluated code can replace post-hook marshalling helpers and leave the caller trappedAfter running arbitrary JavaScript, both EM_JS functions use mutable public or
For example: emscripten:run_script_tracked(
<<"Module._malloc = () => { throw new Error('clobbered') }; ['value']">>
).The default hook tracks Suggested fix: capture non-overridable/internal Emscripten helpers and heap - const ptr = Module['_malloc'](keys.length * HEAPU32.BYTES_PER_ELEMENT);
+ const ptr = _malloc(keys.length * HEAPU32.BYTES_PER_ELEMENT);
if (ptr === 0) {
dropKeys(keys);
setOutcome(OOM, 0);
return 0;
}
- HEAPU32.set(keys, ptr / HEAPU32.BYTES_PER_ELEMENT);
+ const offset = ptr / HEAPU32.BYTES_PER_ELEMENT;
+ for (let i = 0; i < keys.length; ++i) {
+ HEAPU32[offset + i] = keys[i];
+ }Apply the same internal-helper and final-guard treatment to [High] Error logging can throw out of the guards that are meant to contain hook failuresThe hook catches and invalid-key rollback path call A script or custom hook can set Suggested small fix: diagnostics must be best-effort and incapable of + const logError = (...args) => {
+ try {
+ console.error(...args);
+ } catch (_) {
+ // Diagnostics must never affect cleanup or trap completion.
+ }
+ };
// ...
- console.error("onRunTrackedJs threw", e);
+ logError("onRunTrackedJs threw", e);The invalid-key path must set an outcome and run [Medium] The default run hook leaks values when array iteration throws after yielding an elementThe default const values = [];
values[Symbol.iterator] = function* () {
yield "leaked";
throw new Error("iterator failed");
};
values;
Suggested small fix: record keys as they are allocated and roll back every Module["onRunTrackedJs"] = (scriptString, isDebug) => {
+ const trackedKeys = [];
const trackValue = (value) => {
const key = Module["nextTrackedObjectKey"]();
Module["trackedObjectsMap"].set(key, value);
+ trackedKeys[trackedKeys.length] = key;
return key;
};
// ...
- return Array.from(result, trackValue);
+ try {
+ return Array.from(result, trackValue);
+ } catch (e) {
+ for (let i = 0; i < trackedKeys.length; ++i) {
+ try {
+ Module["onTrackedObjectDelete"](trackedKeys[i]);
+ } catch (_) {
+ // Continue rolling back the remaining keys.
+ }
+ }
+ return null;
+ }
};Also add the custom-iterator case to [Low]
|
7a05893 to
d0d20cc
Compare
d0d20cc to
2971066
Compare
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
Signed-off-by: Jakub Gonet <jakub.gonet@swmansion.com>
Signed-off-by: Jakub Gonet <jakub.gonet@swmansion.com>
It's a field in emscripten platform struct. It could be a callback entirely in JS but we use threads. Threads are emulated via webworkers which have their own contexts and variables aren't shared with each other. This means that we would lose uniqueness of the keys. Signed-off-by: Jakub Gonet <jakub.gonet@swmansion.com>
Signed-off-by: Jakub Gonet <jakub.gonet@swmansion.com>
Signed-off-by: Jakub Gonet <jakub.gonet@swmansion.com>
The lib was extracted from eavmlib without updating the doc pipeline, so emscripten/websocket edoc was never rendered. Also fix the three links still pointing at the module's old eavmlib location. Signed-off-by: Davide Bettio <davide@uninstall.it>
EMULATE_FUNCTION_POINTER_CASTS makes the JIT trap at runtime, and nothing here uses cwrap, stringToNewUTF8 or FS. Signed-off-by: Davide Bettio <davide@uninstall.it>
- the result validation read an out of scope variable, so a script evaluating to an array threw and left the caller waiting forever - sparse array holes were tracked as key 0 - keys came back from ccall signed, while the VM read them unsigned - a throw while mapping the result left values owned by nobody - the hook stored a value under the sentinel key that means the key space is exhausted, so the VM had to undo it Signed-off-by: Davide Bettio <davide@uninstall.it>
- the answer was built on the caller's heap from the browser main thread, which is not a scheduler thread, and posted from there too - the answer heap was sized for a single binary, so fetching several values overflowed it - the strings buffer was one byte too small for its last NUL - ref_keys and the fetched buffers leaked - allocation failures wrote through address zero, aborted the VM or dereferenced NULL, instead of raising out_of_memory - a throwing hook escaped the proxied call, leaving the caller trapped forever - keys coming from a hook were narrowed to uint32_t unchecked - keys that never became a handle stayed in the map - the key counter wrapped and handed out keys that live handles own - the exported counter dereferenced the null global context when called before main - keys and statuses had a different type in every layer Signed-off-by: Davide Bettio <davide@uninstall.it>
Add the Erlang stubs with their specs and edoc, and an internals section on the flow, the value lifetime and the hook contract. Signed-off-by: Davide Bettio <davide@uninstall.it>
Add an extensive test suite for the tracked values API. It is web-only like the other emscripten tests: these NIFs dispatch to the browser main thread, which is blocked in main() under node. Signed-off-by: Davide Bettio <davide@uninstall.it>
A fetch hook that threw, or returned the wrong number of entries, was
answered with the bare atom badvalue. Callers map over the result, so
that atom reaches them as a term their list cannot hold, and popcorn
gets there with its own hook: it serializes with JSON.stringify, which
throws on a circular value or a BigInt.
Tell them the same thing in the shape they asked for. The hook said
nothing about any single key, so every element is {error, badvalue},
an outcome callers already handle for a value that is not a string.
Signed-off-by: Davide Bettio <davide@uninstall.it>
run_script_tracked/1 answers {ok, []} for a script that tracked
nothing, and get_tracked/2 then refused that very list, so callers had
to special case it before passing it on. Answer with an empty list
instead. It costs the value arm nothing, since there is no key to ask
the main thread about.
Signed-off-by: Davide Bettio <davide@uninstall.it>
Extend the suite to handles crossing processes, a caller killed mid-call, concurrency, edge inputs and scale, plus a module that drives the whole API through overridden hooks. Signed-off-by: Davide Bettio <davide@uninstall.it>
The workflow built the JIT flavor and threw it away. Keep both builds and run the tracked specs against each. A JIT build compiles every module at startup, so the pages take their argument list from a query parameter and load the bundle carrying the wasm32 backend. The JIT leg cannot run the example specs, whose runnables are packed without a backend. Signed-off-by: Davide Bettio <davide@uninstall.it>
The example tracks a DOM node, reaches it again through its key, reads a property back, and lets a dropped handle take its JavaScript value with it. Signed-off-by: Davide Bettio <davide@uninstall.it>
Signed-off-by: Davide Bettio <davide@uninstall.it>
2971066 to
0895eb0
Compare
5 participants
The existing JavaScript bridge (cast, call and the promise NIFs) only
passes integers and strings, so values such as DOM nodes or callback
functions could not be handled from Erlang at all. This API lets
Erlang hold opaque handles to arbitrary JavaScript values and ties
the JavaScript value lifetime to the Erlang handle lifetime.
emscripten:run_script_tracked/1 evaluates a script on the browser's
main thread; each element of the array it evaluates to is stored in a
JavaScript-side map under a fresh integer key, and the caller gets
one opaque handle (a NIF resource wrapping the key) per element. When
the VM garbage collects a handle, the resource destructor dispatches
a call to the main thread that drops the value from the map.
emscripten:get_tracked/2 maps handles back to their integer keys, or
fetches the current values as UTF-8 binaries (values must be
JavaScript strings; anything else is typically serialized to JSON
first).
The JavaScript side is a set of hooks on the emscripten module object
that embedders may override to customize evaluation, fetching and
cleanup without patching AtomVM; the defaults are the reference
implementation of the hook contract. On the VM side the trap answers
are built in a private heap and copied into a signal, so no foreign
thread ever touches a process heap.
The API was designed and first implemented by Jakub Gonet (Software
Mansion) for Popcorn, where it powers Elixir-to-JavaScript interop in
production. This PR revamps that work for upstream: rebased onto
release-0.7, review findings fixed, hardened against throwing hooks
and allocation failures, and completed with typespecs, edoc, prose
documentation and a browser test suite.
Supersedes: #2013
Closes: #2013
These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).
SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later