Allow symbols as WeakMap and WeakSet keys

[bnoordhuis]

This is not a proper fix but it shows why #56 crashes.

map_add_record() treat the key as a JSObject when it can be a JSAtomStruct/JSString. As a quick hack I added the necessary struct JSMapRecord to JSString.

I'll think about this some more tomorrow, it's late now. :-)

[bnoordhuis]

Is github having trouble? I'm pretty sure there aren't conflicts. The CI doesn't start either. - it picked the wrong merge base somehow

[saghul]

Ah nice! I think you accidentally added support for putting them in a weakest too!

[saghul]

The PR LGTM as is. A thought: looking at implementing WeakRef and FinalizationRegistry we'd likely need to move away from the current model to accomodate that I think.

[bnoordhuis]

Yes, I think so too. What I don't like about this PR is that it makes JSString bigger by an otherwise unused pointer.

Branching on the tag to get the field offset is kind of yuck too although that could be fixed by unifying the JSObject and JSString header, but that puts a mostly unused field in precious small offset-from-base territory.

[saghul]

Sweet!

[bnoordhuis]

I don't think the asan bot is wrong to complain. I'll investigate more tomorrow.

[bnoordhuis]

Diagnosis: JS_FreeRuntime calls free_object, which then calls JS_FreeAtomStruct before js_map_finalizer, resulting in a UAF when js_map_finalizer calls delete_weak_ref.

Atoms probably need to add themselves to rt->gc_zero_ref_count_list first now, just like JSObjects do.

[saghul]

That does sound like it would fix the dump free issue too maybe?

[bnoordhuis]

Nah, turns out the bug was mine, I missed a delete_weak_ref call somewhere. The !atom_is_free(p) bug is still at large but symbols-in-weak{maps,sets} seem to be working now.

[saghul]

Awesome stuff!