Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for func_id rollover #94

Merged
merged 11 commits into from Jan 3, 2023
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions c/interface.c
Expand Up @@ -552,7 +552,7 @@ int QTS_BuildIsAsyncify() {
// -------------------
// function: C -> Host
#ifdef __EMSCRIPTEN__
EM_JS(MaybeAsync(JSValue *), qts_host_call_function, (JSContext * ctx, JSValueConst *this_ptr, int argc, JSValueConst *argv, int magic_func_id), {
EM_JS(MaybeAsync(JSValue *), qts_host_call_function, (JSContext * ctx, JSValueConst *this_ptr, int argc, JSValueConst *argv, uint32_t magic_func_id), {
#ifdef QTS_ASYNCIFY
const asyncify = {['handleSleep'] : Asyncify.handleSleep};
#else
Expand All @@ -563,7 +563,7 @@ EM_JS(MaybeAsync(JSValue *), qts_host_call_function, (JSContext * ctx, JSValueCo
#endif

// Function: QuickJS -> C
JSValue qts_call_function(JSContext *ctx, JSValueConst this_val, int argc, JSValueConst *argv, int magic) {
JSValue qts_call_function(JSContext *ctx, JSValueConst this_val, int argc, JSValueConst *argv, uint32_t magic) {
JSValue *result_ptr = qts_host_call_function(ctx, &this_val, argc, argv, magic);
if (result_ptr == NULL) {
return JS_UNDEFINED;
Expand All @@ -574,7 +574,7 @@ JSValue qts_call_function(JSContext *ctx, JSValueConst this_val, int argc, JSVal
}

// Function: Host -> QuickJS
JSValue *QTS_NewFunction(JSContext *ctx, int func_id, const char *name) {
JSValue *QTS_NewFunction(JSContext *ctx, uint32_t func_id, const char *name) {
#ifdef QTS_DEBUG_MODE
char msg[500];
sprintf(msg, "new_function(name: %s, magic: %d)", name, func_id);
Expand Down
8 changes: 4 additions & 4 deletions generate.ts
Expand Up @@ -135,7 +135,7 @@ function cTypeToTypescriptType(ctype: string): ParsedType {
if (type === "void") {
ffi = null
}
if (type === "double" || type === "int" || type === "size_t") {
if (type === "double" || type === "int" || type === "size_t" || type === "uint16_t" || type === "uint32_t") {
ffi = "number"
typescript = "number"
}
Expand Down Expand Up @@ -237,8 +237,8 @@ function buildSyncSymbols(matches: RegExpMatchArray[]) {
return filtered.map((fn) => "_" + fn.functionName)
}

// Input: EM_JS(MaybeAsync(JSValue *), qts_host_call_function, (JSContext * ctx, JSValueConst *this_ptr, int argc, JSValueConst *argv, int magic_func_id), {
// Match: MaybeAsync(JSValue *), qts_host_call_function, (JSContext * ctx, JSValueConst *this_ptr, int argc, JSValueConst *argv, int magic_func_id)
// Input: EM_JS(MaybeAsync(JSValue *), qts_host_call_function, (JSContext * ctx, JSValueConst *this_ptr, int argc, JSValueConst *argv, uint32_t magic_func_id), {
// Match: MaybeAsync(JSValue *), qts_host_call_function, (JSContext * ctx, JSValueConst *this_ptr, int argc, JSValueConst *argv, uint32_t magic_func_id)
function buildAsyncifySymbols(matches: RegExpMatchArray[]) {
const parsed = matches.map((match) => {
const [, contents] = match
Expand Down Expand Up @@ -312,7 +312,7 @@ export function matchAll(regexp: RegExp, text: string) {
// We're using .exec, which mutates the regexp by setting the .lastIndex
const initialLastIndex = regexp.lastIndex
const result: RegExpExecArray[] = []
let match = null
let match: RegExpExecArray | null = null
while ((match = regexp.exec(text)) !== null) {
result.push(match)
}
Expand Down
30 changes: 26 additions & 4 deletions ts/context.ts
Expand Up @@ -412,7 +412,7 @@ export class QuickJSContext implements LowLevelJavascriptVm<QuickJSHandle>, Disp
*/
newFunction(name: string, fn: VmFunctionImplementation<QuickJSHandle>): QuickJSHandle {
const fnId = ++this.fnNextId
this.fnMap.set(fnId, fn)
this.setFunction(fnId, fn)
return this.memory.heapValueHandle(this.ffi.QTS_NewFunction(this.ctx.value, fnId, name))
}

Expand Down Expand Up @@ -784,9 +784,30 @@ export class QuickJSContext implements LowLevelJavascriptVm<QuickJSHandle>, Disp
}

/** @private */
protected fnNextId = 0
protected fnNextId = -32768 // min value of signed 16bit int used by Quickjs
/** @private */
protected fnMap = new Map<number, VmFunctionImplementation<QuickJSHandle>>()
protected fnMaps = new Map<number, Map<number, VmFunctionImplementation<QuickJSHandle>>>()

/** @private */
protected getFunction(fn_id: number): VmFunctionImplementation<QuickJSHandle> | undefined {
const map_id = fn_id >> 8
const fnMap = this.fnMaps.get(map_id)
if (!fnMap) {
return undefined
}
return fnMap.get(fn_id)
}

/** @private */
protected setFunction(fn_id: number, handle: VmFunctionImplementation<QuickJSHandle>) {
const map_id = fn_id >> 8
let fnMap = this.fnMaps.get(map_id)
if (!fnMap) {
fnMap = new Map<number, VmFunctionImplementation<QuickJSHandle>>()
this.fnMaps.set(map_id, fnMap)
}
return fnMap.set(fn_id, handle)
}

/**
* @hidden
Expand All @@ -797,8 +818,9 @@ export class QuickJSContext implements LowLevelJavascriptVm<QuickJSHandle>, Disp
throw new Error("QuickJSContext instance received C -> JS call with mismatched ctx")
}

const fn = this.fnMap.get(fn_id)
const fn = this.getFunction(fn_id)
if (!fn) {
// this "throw" is not catch-able from the TS side. could we somehow handle this higher up?
throw new Error(`QuickJSContext had no callback with id ${fn_id}`)
}

Expand Down

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions ts/generated/emscripten-module.WASM_DEBUG_SYNC.js
Expand Up @@ -2359,8 +2359,8 @@ var tempI64;
// === Body ===

var ASM_CONSTS = {
114190: function() {return withBuiltinMalloc(function () { return allocateUTF8(Module['LSAN_OPTIONS'] || 0); });},
114287: function() {var setting = Module['printWithColors']; if (setting != null) { return setting; } else { return ENVIRONMENT_IS_NODE && process.stderr.isTTY; }}
113795: function() {return withBuiltinMalloc(function () { return allocateUTF8(Module['LSAN_OPTIONS'] || 0); });},
113892: function() {var setting = Module['printWithColors']; if (setting != null) { return setting; } else { return ENVIRONMENT_IS_NODE && process.stderr.isTTY; }}
};
function qts_host_call_function(ctx,this_ptr,argc,argv,magic_func_id){ const asyncify = undefined; return Module['callbacks']['callFunction'](asyncify, ctx, this_ptr, argc, argv, magic_func_id); }
function qts_host_interrupt_handler(rt){ const asyncify = undefined; return Module['callbacks']['shouldInterrupt'](asyncify, rt); }
Expand Down
Binary file modified ts/generated/emscripten-module.WASM_DEBUG_SYNC.wasm
Binary file not shown.
2 changes: 1 addition & 1 deletion ts/generated/emscripten-module.WASM_RELEASE_ASYNCIFY.js

Large diffs are not rendered by default.

23 changes: 22 additions & 1 deletion ts/quickjs.test.ts
Expand Up @@ -13,7 +13,7 @@ import {
import { it, describe } from "mocha"
import assert from "assert"
import { isFail, VmCallResult } from "./vm-interface"
import fs from "fs"
import fs, { chmod } from "fs"
import { QuickJSContext } from "./context"
import { QuickJSAsyncContext } from "./context-asyncify"
import { DEBUG_ASYNC, DEBUG_SYNC, memoizePromiseFactory, QuickJSFFI } from "./variants"
Expand Down Expand Up @@ -150,6 +150,27 @@ function contextTests(getContext: () => Promise<QuickJSContext>) {

fnHandle.dispose()
})

it("can handle more than signed int max functions being registered", function (done) {
// test for unsigned func_id impl
this.timeout(30000) // we need more time to register 2^16 functions

for (let i = 0; i < Math.pow(2, 16); i++) {
const funcID = i
const fnHandle = vm.newFunction(`__func-${i}`, () => {
return vm.newNumber(funcID)
})
if (i % 1024 === 0) {
// spot check every 1024 funcs
const res = vm.unwrapResult(vm.callFunction(fnHandle, vm.undefined))
const calledFuncID = vm.dump(res)
assert(calledFuncID === i)
res.dispose()
}
fnHandle.dispose()
}
done()
})
})

describe("properties", () => {
Expand Down