Skip to content

fix(android): convert values assigned to primitive array elements like the v8 runtime - #133

Open
ammarahm-ed wants to merge 2 commits into
mainfrom
fix/android-array-element-coercion
Open

ammarahm-ed wants to merge 2 commits into
mainfrom
fix/android-array-element-coercion

Conversation

@ammarahm-ed

@ammarahm-ed ammarahm-ed commented Nov 9, 2026 •

Copy link
Copy Markdown
Collaborator

Assigning a value that is not already the element type to a primitive Java array element logged this and left the element unwritten:

TNS.Native: Node-API returned error: 6
TNS.Native:     napi_get_value_double(env, value, &d)
TNS.Native:     at NativeScript/ffi/jni/napi/conversion/ArrayElementAccessor.cpp:207

error: 6 is napi_number_expected. SetArrayElement called the strict napi_get_value_* getters, which reject anything that is not exactly a number (or boolean, or string), and returned. The classic V8 runtime converts the value with the JS rules (NumberValue, Int32Value, BooleanValue, ToString), so floatArray[0] = undefined stores NaN, null stores 0, "3.5" stores 3.5. The direct runtime did not log anything, but its js_util::get_number returned 0 for anything that was not a number, so "3.5" stored 0 and undefined stored 0 instead of NaN.

Both runtimes now convert like the V8 runtime. Every site where a user-supplied value reaches a strict getter is covered:

  • Array element assignment (SetArrayElement): boolean, byte, char, short, int, long, float and double arrays. A long[] given an object still goes through ConvertToJavaLong, as in the V8 runtime.
  • Array getValueAtIndex / setValueAtIndex index.
  • float(x), double(x) and long(x) casts passed to a Java method that takes Object (JsArgToArrayConverter).
  • onerror handler return value (Node-API only; the flag was also read uninitialised when the conversion failed).

How it avoids slowing the fast path

Each conversion tries the existing strict getter first, so a value of the right type does exactly what it did. Only when that fails does it coerce (napi_coerce_to_number / _to_bool / _to_string, or the global Number / Boolean in the direct runtime). If coercion fails too, the same Node-API error is logged as before and nothing is thrown; any exception the coercion raised (for example from a Symbol) is cleared. The strict-then-coerce helpers are in a new Coercion.h in each tree.

Results

Debug build on a Sony SO-01M (Android 11, arm64), V8, both runtimes, the Jasmine suite. The first commit adds five specs to testArrays.js; the second is the fix.

specs failures
Node-API, specs only 528 5 (all the new specs)
Node-API, with the fix 528 0
Direct, specs only 456 5 (all the new specs)
Direct, with the fix 456 0

The fast path, from the existing benchmark rows, release build, 5 launches, median, ns per assignment:

Node-API before after Direct before after
int[0] = n 1,973 2,042 (+3.5%) 386 400 (+3.7%)
double[0] = n 2,037 2,100 (+3.0%) 392 408 (+4.2%)
boolean[0] = b 2,013 2,046 (+1.6%) 392 388 (-1.1%)
String[0] = s 5,609 5,580 (-0.5%) 1,280 1,313 (+2.5%)
int[0] read 1,622 1,637 (+0.9%) 251 248 (-1.5%)

The median over all benchmark rows is +0.6% on Node-API and -0.1% on the direct runtime. The writes that now do one extra range check are 2–4% slower, which is inside this device's noise of about ±5% per row; I would not call it a regression, but I did not measure it below that.

Notes

  • Field setters (obj.floatField = x) were not touched: they already check the type and store 0 for anything else, so they never raised this error, though they do not produce NaN like the V8 runtime would.
  • Only V8 was built and run. The Node-API coercion relies on napi_coerce_to_number, _to_bool and _to_string from each engine shim; I have not built or checked the other engines.
  • main does not link V8 on Android until fix(android/v8): skip the ES module cleanup the Android build does not link #119 merges, so the builds above carried that one fix underneath this change. It is not part of this PR.

@coderabbitai

coderabbitai Bot commented Nov 9, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 56098c4a-9b48-4245-aaeb-bf1424b9e63a

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • Trigger review
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant