Skip to content

Add native JavaScript class bindings and optimized utilities#122

Open
scorsin-oai wants to merge 3 commits into
Snapchat:mainfrom
scorsin-oai:simon/260723-native-js-class-bindings
Open

Add native JavaScript class bindings and optimized utilities#122
scorsin-oai wants to merge 3 commits into
Snapchat:mainfrom
scorsin-oai:simon/260723-native-js-class-bindings

Conversation

@scorsin-oai

Copy link
Copy Markdown
Collaborator

Description

This change adds complete support for bridging native classes to JS, which was something that has been missing for some time in Valdi.

Before this change, we only supported bridging native functions to JS. These functions can have attached states, as such we were able to go quite far without supporting actual classes. There were a few downsides
with that approach:

  • Every instance needs to have a complete copy of all its methods and properties. If you have a bunch of instance with methods on them, the methods technically don't change, only the instance changes. The existing model made it impossible to re-use the methods, we had to generate lambdas for them on the fly every time.
  • There were no ways to create an actual class that can be used like new MyNativeClass() where MyNativeClass was a real native class. We always had to make a TS class wrapper that then calls native with a call like this.nativeInstance = nativeCreate().
  • Because there were no actual classes, we couldn't use the instanceof operator to identify the class of a specific instance.

This change also introduces some optimized utilities like a Base64 module backed by C++ code so that we don't need to do base64 in pure JS.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • Documentation improvement
  • Performance optimization
  • Test improvement
  • Other (please describe)

Testing

  • Tests pass locally (bazel test //...)
  • Added/updated tests for changes (if applicable)
  • Tested on multiple platforms (iOS/Android/Web/macOS as applicable)
  • Manual testing performed (describe below)

Testing Details

Checklist

  • Code follows project style guidelines
  • Documentation updated (if needed)
  • No breaking changes (or documented in description)
  • Commit messages follow conventional format
  • No secrets, API keys, or internal URLs included

Related Issues

Additional Context

@github-actions

Copy link
Copy Markdown

Sensitive Files Detected

⚠️ Core runtime — App-wide blast radius — touches the rendering core. Must be gated.

This is an automated notice. A maintainer will review after import.

@github-actions github-actions Bot added area/runtime Valdi runtime (C++/native) area/build-system Bazel build rules and config area/docs Documentation labels Jul 23, 2026

@clholgat clholgat left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of issues on the Base64 migration flagged during review (both would ship undetected given the current test matrix does not exercise web, and the TS specs aren't run in CI):

}
return output.join('');
}
import { decodeFromBase64, encodeToBase64 } from './Base64Native';

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This swaps the pure-JS Base64 implementation for the C++-backed Base64Native module, but there's no web polyglot fallback for it — unlike the Unicode migration in this same PR.

coreutils/web/ only contains UnicodeNative.ts, and web_register_native_module_id_overrides in coreutils/BUILD.bazel only maps UnicodeNative.js; there's no Base64Native entry. On web, ./Base64Native won't resolve at runtime, so encodeToBase64/decodeFromBase64 will be undefined and Base64.fromByteArray / toByteArray / byteLength will throw.

Could you add a coreutils/web/Base64Native.ts mirroring UnicodeNative.ts, plus a matching entry in the override map?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I was hoping that would all be part of my web PR which has this change, but I understand in the mean time this is breaking things on web.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Realistically, it'll be at least a week before the web stuff lands at the earliest so we need a stop gap until then.

Comment on lines +21 to 26
if (validLen % 4 === 1) {
throw new Error('Invalid string. Length must be a valid Base64 length');
}

const placeHoldersLen = validLen === b64.length ? (4 - (validLen % 4)) % 4 : 4 - (validLen % 4);
return ((validLen + placeHoldersLen) * 3) / 4 - placeHoldersLen;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

byteLength no longer rejects malformed strings that contain = at a non-multiple-of-4 boundary, which the previous implementation threw on. The has-= branch uses 4 - (validLen % 4) without the % 4 wrap the no-padding branch has, and the only guard is validLen % 4 === 1, so:

  • byteLength('=')-1 (validLen=0 → placeHoldersLen=4 → (0+4)*3/4 - 4 = -1)
  • byteLength('Zm9v=')2 (validLen=4 → placeHoldersLen=4 → 6 - 4 = 2)

Both threw before this change. A decoded-size helper returning a negative/incorrect value is risky for any caller that uses it to size an allocation. Suggest rejecting = at a non-%4 boundary and adding tests for these cases — the suite currently only covers byteLength('Z').

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't remember the exact specifics but I know I did that on purpose, byteLength() was more strict that our decoding code for no valid reason

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prior to this change: Base64 decode was lenient, it added missing padding before decoding, byteLength() was stricter, it did not accept some inputs that decode would accept. Now they accept unpadded inputs

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed the unpadded leniency itself is good — 'Zg' → 1 and 'Zm9v' → 3 now is right, and matching decode there makes sense.

The thing I was flagging is narrower: for a few malformed inputs byteLength now returns a value that can't be a real byte length —

  • byteLength('=')-1
  • byteLength('==')-1
  • byteLength('Zm9v=')2 (would expect 3, or a reject)

The -1 is the one I'd want to avoid: a caller using byteLength to size a new Uint8Array(n) / buffer gets a negative length. Root cause is the =-present branch using 4 - (validLen % 4) without the % 4 wrap the no-= branch has, so a leading/misplaced = (where validLen % 4 === 0) gives placeHoldersLen = 4 and the formula goes negative.

Not a blocker and fine to defer — but might be worth clamping to 0, or rejecting = at a non-multiple-of-4 offset. Your call.

@github-actions

github-actions Bot commented Jul 23, 2026

Copy link
Copy Markdown

⚠️ Bazel & CI Test Results

Test Suite Result
Valdi Smoke Tests ❌ cancelled
API Surface Check ✅ success
Snapshot Tests ❌ cancelled
Linux: C++ Tests ❌ cancelled
Linux: Build Compiler ✅ success
Linux: Build & Export ❌ cancelled
macOS: C++ & Platform Tests ❌ failure

Some tests failed. Please check the workflow logs for details.

🚀 Bazel remote cache is now enabled - future builds will be faster!

Workflow: Valdi CI

JSValueRef V8JavaScriptContext::newNativeClass(const Ref<RefCountable>& /*classOpaque*/,
const JSClassDefinition& /*classDefinition*/,
JSExceptionTracker& exceptionTracker) {
exceptionTracker.onError("Native classes are not supported by V8");

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a blocker for this PR — flagging for the record since V8 is experimental and isn't wired into any shipping config yet.

This PR routes the Base64/Unicode module factories through newNativeClass, which throws here on V8. So on V8 those modules will now fail to load, where the previous pure-JS impls worked. It's invisible today because V8 is excluded from the test matrix and unused.

No action needed now — just noting it so that whenever V8 does get picked up, native-class support (or a V8 fallback for these factories) is a known prerequisite rather than a surprise. Might be worth a // TODO here pointing at that.

@clholgat

Copy link
Copy Markdown
Collaborator

One V8 nit, but I'm pulling this in to start testing

@clholgat

Copy link
Copy Markdown
Collaborator

Native-class binder: required reference params crash on null/undefined from JS

Following up on the review with one issue I think is worth fixing at the source before this lands.

In JSNativeClassBinder's parseArgument, required primitive params correctly reject null:

if constexpr (kIsRequiredPrimitive<T>) {
    if (isParameterNullOrUndefined(callContext, index)) {
        callContext.getExceptionTracker().onError("Native class primitive argument cannot be null or undefined");
        return T();
    }
    ...
}

But required reference params (Ref<StaticString>, JSTypedArray) fall into the generic branch, which returns a default-constructed value and does not set the exception tracker:

} else {
    if (isParameterNullOrUndefined(callContext, index)) {
        return T();   // null Ref<StaticString> / zero-initialized JSTypedArray, no exception set
    }
    ...
}

parseArguments then sees no exception and invokes the native method with the null value. Concrete crashes reachable from ordinary app JS:

  • Base64ModuleFactory::decodeFromBase64base64->utf8Storage() (null Ref deref)
  • UnicodeModuleFactory::strToCodepointsstr->utf32Storage() (null Ref deref)
  • UnicodeModuleFactory::encodeStringstr->utf8Storage() (null Ref deref)
  • UnicodeModuleFactory::decodeIntoStringbuffer.data == nullptr, then std::string_view(nullptr, 0) / utf32ToUtf8 (UB)

So e.g. Base64.decodeFromBase64(null) takes down the native runtime instead of throwing a JS error.

I'd suggest fixing this once in the binder rather than adding per-method null checks: have required reference types reject null the same way primitives already do. Genuinely optional params already go through the kIsSupportedOptional branch and return std::nullopt, so only they should accept null. A single change in parseArgument covers every current and future native-class method.

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

Labels

area/build-system Bazel build rules and config area/docs Documentation area/runtime Valdi runtime (C++/native)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants