Fix atom leak in JS_NewSymbol

[engsr6982]

This leak was introduced in commit 4a66289 when the public JS_NewSymbol API was first added. The function calls JS_NewAtom to create an atom from the description string, but never calls JS_FreeAtom after passing it to JS_NewSymbolFromAtom. This results in the atom's reference count never reaching zero, leaking memory for every symbol created via this C API.

[engsr6982]

Actions don't look good, I'll take another look.

[engsr6982]

I added JS_SetDumpFlags(rt, JS_DUMP_LEAKS | JS_DUMP_ATOM_LEAKS); to the new_symbol test case in api-test.c. The test output shows that there is indeed an atom leak here.

However, I'm not sure why adding JS_FreeAtom(ctx, atom); causes a segfault instead. Is this behavior by design, or is there something I'm missing about the atom lifecycle in JS_NewSymbol?

[saghul]

You are freeing the atom before you use it.

[engsr6982]

This should be fixed now — atom is freed after JS_NewSymbolFromAtom. Works fine here and CI is all green. Was there anything else needed before merging?

[saghul]

Can you add a test to api-test.c please?

[engsr6982]

The existing new_symbol test already hits all four JS_NewSymbol paths, so the fix is exercised. The leak is only observable via JS_DUMP_ATOM_LEAKS — there's no API to assert on it programmatically. The fix is validated by the logic change itself.

JS_SetDumpFlags(rt, JS_DUMP_LEAKS | JS_DUMP_ATOM_LEAKS);

quickjs/api-test.c

Lines 965 to 1019 in c707cf5

	static void new_symbol(void)
	{
	JSRuntime *rt = JS_NewRuntime();
	JSContext *ctx = JS_NewContext(rt);
	JSValue global = JS_GetGlobalObject(ctx);
	JSValue sym, ret;
	/* Local symbol with NULL description -> Symbol() */
	sym = JS_NewSymbol(ctx, NULL, false);
	assert(!JS_IsException(sym));
	assert(JS_IsSymbol(sym));
	JS_SetPropertyStr(ctx, global, "sym_local_null", sym);
	ret = eval(ctx, "typeof sym_local_null === 'symbol' && sym_local_null.description === undefined && Symbol.keyFor(sym_local_null) === undefined");
	assert(JS_IsBool(ret));
	assert(JS_VALUE_GET_BOOL(ret));
	JS_FreeValue(ctx, ret);
	/* Global symbol with NULL description -> Symbol.for() -> Symbol.for('undefined') */
	sym = JS_NewSymbol(ctx, NULL, true);
	assert(!JS_IsException(sym));
	assert(JS_IsSymbol(sym));
	JS_SetPropertyStr(ctx, global, "sym_global_null", sym);
	ret = eval(ctx, "typeof sym_global_null === 'symbol' && sym_global_null.description === 'undefined' && Symbol.keyFor(sym_global_null) === 'undefined'");
	assert(JS_IsBool(ret));
	assert(JS_VALUE_GET_BOOL(ret));
	JS_FreeValue(ctx, ret);
	/* Local symbol with description -> Symbol('test_local') */
	sym = JS_NewSymbol(ctx, "test_local", false);
	assert(!JS_IsException(sym));
	assert(JS_IsSymbol(sym));
	JS_SetPropertyStr(ctx, global, "sym_local_str", sym);
	ret = eval(ctx, "sym_local_str.description === 'test_local' && Symbol.keyFor(sym_local_str) === undefined");
	assert(JS_IsBool(ret));
	assert(JS_VALUE_GET_BOOL(ret));
	JS_FreeValue(ctx, ret);
	/* Global symbol with description -> Symbol.for('test_global') */
	sym = JS_NewSymbol(ctx, "test_global", true);
	assert(!JS_IsException(sym));
	assert(JS_IsSymbol(sym));
	JS_SetPropertyStr(ctx, global, "sym_global_str", sym);
	ret = eval(ctx, "sym_global_str.description === 'test_global' && Symbol.keyFor(sym_global_str) === 'test_global'");
	assert(JS_IsBool(ret));
	assert(JS_VALUE_GET_BOOL(ret));
	JS_FreeValue(ctx, ret);
	JS_FreeValue(ctx, global);
	JS_FreeContext(ctx);
	JS_FreeRuntime(rt);
	}

[saghul]

👍 I'll give it a try, I'm taking a bit of a break and will be back soon.