[PATCH v2 0/10] MR3300: jscript: WeakMap implementation.
-- v2: jscript: Properly set JS_E_WRONG_THIS as a TypeError. mshtml/tests: Add tests for WeakMap. jscript: Implement WeakMap.has(). jscript: Implement WeakMap.clear(). jscript: Implement WeakMap.delete(). jscript: Implement WeakMap.get(). jscript: Implement WeakMap.set(). jscript: Implement WeakMap instance stub and constructor. jscript: Move the GC traversal for unmarking alive objects into a helper jscript: Convert unlink_props to a helper that unlinks the entire object. https://gitlab.winehq.org/wine/wine/-/merge_requests/3300
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> It will be useful for other cases, and we don't need the gc_ctx for unlinking. Also set the PROP_PROTREFs to PROP_DELETED since we're unliking the prototype. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/jscript/dispex.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/dlls/jscript/dispex.c b/dlls/jscript/dispex.c index b5fc3699212..9e87c38004f 100644 --- a/dlls/jscript/dispex.c +++ b/dlls/jscript/dispex.c @@ -665,14 +665,13 @@ static HRESULT fill_protrefs(jsdisp_t *This) return S_OK; } -static void unlink_props(jsdisp_t *jsdisp) +static void unlink_jsdisp(jsdisp_t *jsdisp) { dispex_prop_t *prop = jsdisp->props, *end; for(end = prop + jsdisp->prop_cnt; prop < end; prop++) { switch(prop->type) { case PROP_DELETED: - case PROP_PROTREF: continue; case PROP_JSVAL: jsval_release(prop->u.val); @@ -688,6 +687,14 @@ static void unlink_props(jsdisp_t *jsdisp) } prop->type = PROP_DELETED; } + + if(jsdisp->prototype) { + jsdisp_release(jsdisp->prototype); + jsdisp->prototype = NULL; + } + + if(jsdisp->builtin_info->gc_traverse) + jsdisp->builtin_info->gc_traverse(NULL, GC_TRAVERSE_UNLINK, jsdisp); } @@ -927,15 +934,7 @@ HRESULT gc_run(script_ctx_t *ctx) /* Grab it since it gets removed when unlinked */ jsdisp_addref(obj); - unlink_props(obj); - - if(obj->prototype) { - jsdisp_release(obj->prototype); - obj->prototype = NULL; - } - - if(obj->builtin_info->gc_traverse) - obj->builtin_info->gc_traverse(&gc_ctx, GC_TRAVERSE_UNLINK, obj); + unlink_jsdisp(obj); /* Releasing unlinked object should not delete any other object, so we can safely obtain the next pointer now */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3300
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/jscript/dispex.c | 125 ++++++++++++++++++++++------------------- dlls/jscript/jscript.h | 1 + 2 files changed, 69 insertions(+), 57 deletions(-) diff --git a/dlls/jscript/dispex.c b/dlls/jscript/dispex.c index 9e87c38004f..f2e5109352e 100644 --- a/dlls/jscript/dispex.c +++ b/dlls/jscript/dispex.c @@ -775,6 +775,72 @@ static jsdisp_t *gc_stack_pop(struct gc_ctx *gc_ctx) return obj; } +HRESULT gc_unmark(struct gc_ctx *gc_ctx, jsdisp_t *obj) +{ + dispex_prop_t *prop, *props_end; + script_ctx_t *ctx = obj->ctx; + jsdisp_t *link, *link2; + HRESULT hres; + + hres = gc_stack_push(gc_ctx, NULL); + if(FAILED(hres)) + return hres; + + do + { + obj->gc_marked = FALSE; + + for(prop = obj->props, props_end = prop + obj->prop_cnt; prop < props_end; prop++) { + switch(prop->type) { + case PROP_JSVAL: + if(!is_object_instance(prop->u.val)) + continue; + link = to_jsdisp(get_object(prop->u.val)); + link2 = NULL; + break; + case PROP_ACCESSOR: + link = prop->u.accessor.getter; + link2 = prop->u.accessor.setter; + break; + default: + continue; + } + if(link && link->gc_marked && link->ctx == ctx) { + hres = gc_stack_push(gc_ctx, link); + if(FAILED(hres)) + break; + } + if(link2 && link2->gc_marked && link2->ctx == ctx) { + hres = gc_stack_push(gc_ctx, link2); + if(FAILED(hres)) + break; + } + } + + if(FAILED(hres)) + break; + + if(obj->prototype && obj->prototype->gc_marked && obj->prototype->ctx == ctx) { + hres = gc_stack_push(gc_ctx, obj->prototype); + if(FAILED(hres)) + break; + } + + if(obj->builtin_info->gc_traverse) { + hres = obj->builtin_info->gc_traverse(gc_ctx, GC_TRAVERSE, obj); + if(FAILED(hres)) + break; + } + + do obj = gc_stack_pop(gc_ctx); while(obj && !obj->gc_marked); + } while(obj); + + if(FAILED(hres)) + do obj = gc_stack_pop(gc_ctx); while(obj); + + return hres; +} + HRESULT gc_run(script_ctx_t *ctx) { /* Save original refcounts in a linked list of chunks */ @@ -783,10 +849,10 @@ HRESULT gc_run(script_ctx_t *ctx) struct chunk *next; LONG ref[1020]; } *head, *chunk; - jsdisp_t *obj, *obj2, *link, *link2; dispex_prop_t *prop, *props_end; struct gc_ctx gc_ctx = { 0 }; unsigned chunk_idx = 0; + jsdisp_t *obj, *link; HRESULT hres = S_OK; struct list *iter; @@ -845,64 +911,9 @@ HRESULT gc_run(script_ctx_t *ctx) if(!obj->ref || !obj->gc_marked) continue; - hres = gc_stack_push(&gc_ctx, NULL); + hres = gc_unmark(&gc_ctx, obj); if(FAILED(hres)) break; - - obj2 = obj; - do - { - obj2->gc_marked = FALSE; - - for(prop = obj2->props, props_end = prop + obj2->prop_cnt; prop < props_end; prop++) { - switch(prop->type) { - case PROP_JSVAL: - if(!is_object_instance(prop->u.val)) - continue; - link = to_jsdisp(get_object(prop->u.val)); - link2 = NULL; - break; - case PROP_ACCESSOR: - link = prop->u.accessor.getter; - link2 = prop->u.accessor.setter; - break; - default: - continue; - } - if(link && link->gc_marked && link->ctx == ctx) { - hres = gc_stack_push(&gc_ctx, link); - if(FAILED(hres)) - break; - } - if(link2 && link2->gc_marked && link2->ctx == ctx) { - hres = gc_stack_push(&gc_ctx, link2); - if(FAILED(hres)) - break; - } - } - - if(FAILED(hres)) - break; - - if(obj2->prototype && obj2->prototype->gc_marked && obj2->prototype->ctx == ctx) { - hres = gc_stack_push(&gc_ctx, obj2->prototype); - if(FAILED(hres)) - break; - } - - if(obj2->builtin_info->gc_traverse) { - hres = obj2->builtin_info->gc_traverse(&gc_ctx, GC_TRAVERSE, obj2); - if(FAILED(hres)) - break; - } - - do obj2 = gc_stack_pop(&gc_ctx); while(obj2 && !obj2->gc_marked); - } while(obj2); - - if(FAILED(hres)) { - do obj2 = gc_stack_pop(&gc_ctx); while(obj2); - break; - } } free(gc_ctx.next); diff --git a/dlls/jscript/jscript.h b/dlls/jscript/jscript.h index 46644b8247b..7be8e3bc24c 100644 --- a/dlls/jscript/jscript.h +++ b/dlls/jscript/jscript.h @@ -149,6 +149,7 @@ HRESULT create_named_item_script_obj(script_ctx_t*,named_item_t*); named_item_t *lookup_named_item(script_ctx_t*,const WCHAR*,unsigned); void release_named_item(named_item_t*); HRESULT gc_run(script_ctx_t*); +HRESULT gc_unmark(struct gc_ctx*,jsdisp_t*); HRESULT gc_process_linked_obj(struct gc_ctx*,enum gc_traverse_op,jsdisp_t*,jsdisp_t*,void**); HRESULT gc_process_linked_val(struct gc_ctx*,enum gc_traverse_op,jsdisp_t*,jsval_t*); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3300
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/jscript/jscript.h | 6 +- dlls/jscript/object.c | 1 + dlls/jscript/set.c | 174 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 179 insertions(+), 2 deletions(-) diff --git a/dlls/jscript/jscript.h b/dlls/jscript/jscript.h index 7be8e3bc24c..71f2e2f38bd 100644 --- a/dlls/jscript/jscript.h +++ b/dlls/jscript/jscript.h @@ -117,6 +117,7 @@ typedef enum { JSCLASS_JSON, JSCLASS_MAP, JSCLASS_SET, + JSCLASS_WEAKMAP, } jsclass_t; jsdisp_t *iface_to_jsdisp(IDispatch*); @@ -419,11 +420,12 @@ struct _script_ctx_t { jsdisp_t *vbarray_constr; jsdisp_t *map_prototype; jsdisp_t *set_prototype; + jsdisp_t *weakmap_prototype; }; - jsdisp_t *global_objects[22]; + jsdisp_t *global_objects[23]; }; }; -C_ASSERT(RTL_SIZEOF_THROUGH_FIELD(script_ctx_t, set_prototype) == RTL_SIZEOF_THROUGH_FIELD(script_ctx_t, global_objects)); +C_ASSERT(RTL_SIZEOF_THROUGH_FIELD(script_ctx_t, weakmap_prototype) == RTL_SIZEOF_THROUGH_FIELD(script_ctx_t, global_objects)); void script_release(script_ctx_t*); diff --git a/dlls/jscript/object.c b/dlls/jscript/object.c index 4f6acacbc95..65b25cab239 100644 --- a/dlls/jscript/object.c +++ b/dlls/jscript/object.c @@ -51,6 +51,7 @@ static HRESULT Object_toString(script_ctx_t *ctx, jsval_t vthis, WORD flags, uns L"[object Object]", L"[object Object]", L"[object Object]", + L"[object Object]", L"[object Object]" }; diff --git a/dlls/jscript/set.c b/dlls/jscript/set.c index eca26a890f7..f2de6924468 100644 --- a/dlls/jscript/set.c +++ b/dlls/jscript/set.c @@ -614,6 +614,165 @@ static HRESULT Set_constructor(script_ctx_t *ctx, jsval_t vthis, WORD flags, uns } } +typedef struct { + jsdisp_t dispex; + struct rb_tree map; +} WeakMapInstance; + +struct weakmap_entry { + struct rb_entry entry; + jsdisp_t *key; + jsval_t value; +}; + +static int weakmap_compare(const void *k, const struct rb_entry *e) +{ + ULONG_PTR a = (ULONG_PTR)k, b = (ULONG_PTR)WINE_RB_ENTRY_VALUE(e, const struct weakmap_entry, entry)->key; + return (a > b) - (a < b); +} + +static void release_weakmap_entry(struct weakmap_entry *entry) +{ + jsval_release(entry->value); + free(entry); +} + +static HRESULT WeakMap_clear(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigned argc, jsval_t *argv, + jsval_t *r) +{ + FIXME("\n"); + return E_NOTIMPL; +} + +static HRESULT WeakMap_delete(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigned argc, jsval_t *argv, + jsval_t *r) +{ + FIXME("\n"); + return E_NOTIMPL; +} + +static HRESULT WeakMap_get(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigned argc, jsval_t *argv, + jsval_t *r) +{ + FIXME("\n"); + return E_NOTIMPL; +} + +static HRESULT WeakMap_set(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigned argc, jsval_t *argv, + jsval_t *r) +{ + FIXME("\n"); + return E_NOTIMPL; +} + +static HRESULT WeakMap_has(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigned argc, jsval_t *argv, + jsval_t *r) +{ + FIXME("\n"); + return E_NOTIMPL; +} + +static HRESULT WeakMap_value(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigned argc, jsval_t *argv, + jsval_t *r) +{ + FIXME("\n"); + return E_NOTIMPL; +} + +static void WeakMap_destructor(jsdisp_t *dispex) +{ + WeakMapInstance *weakmap = (WeakMapInstance*)dispex; + struct weakmap_entry *entry, *entry2; + + RB_FOR_EACH_ENTRY_DESTRUCTOR(entry, entry2, &weakmap->map, struct weakmap_entry, entry) + release_weakmap_entry(entry); + + free(weakmap); +} + +static HRESULT WeakMap_gc_traverse(struct gc_ctx *gc_ctx, enum gc_traverse_op op, jsdisp_t *dispex) +{ + WeakMapInstance *weakmap = (WeakMapInstance*)dispex; + struct weakmap_entry *entry, *entry2; + HRESULT hres; + + if(op == GC_TRAVERSE_UNLINK) { + RB_FOR_EACH_ENTRY_DESTRUCTOR(entry, entry2, &weakmap->map, struct weakmap_entry, entry) + release_weakmap_entry(entry); + rb_destroy(&weakmap->map, NULL, NULL); + return S_OK; + } + + RB_FOR_EACH_ENTRY(entry, &weakmap->map, struct weakmap_entry, entry) { + hres = gc_process_linked_val(gc_ctx, op, dispex, &entry->value); + if(FAILED(hres)) + return hres; + } + return S_OK; +} + +static const builtin_prop_t WeakMap_prototype_props[] = { + {L"clear", WeakMap_clear, PROPF_METHOD}, + {L"delete", WeakMap_delete, PROPF_METHOD|1}, + {L"get", WeakMap_get, PROPF_METHOD|1}, + {L"has", WeakMap_has, PROPF_METHOD|1}, + {L"set", WeakMap_set, PROPF_METHOD|2}, +}; + +static const builtin_info_t WeakMap_prototype_info = { + JSCLASS_OBJECT, + WeakMap_value, + ARRAY_SIZE(WeakMap_prototype_props), + WeakMap_prototype_props, + NULL, + NULL +}; + +static const builtin_info_t WeakMap_info = { + JSCLASS_WEAKMAP, + WeakMap_value, + 0, + NULL, + WeakMap_destructor, + NULL, + NULL, + NULL, + NULL, + WeakMap_gc_traverse, +}; + +static HRESULT WeakMap_constructor(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigned argc, jsval_t *argv, + jsval_t *r) +{ + WeakMapInstance *weakmap; + HRESULT hres; + + switch(flags) { + case DISPATCH_CONSTRUCT: + TRACE("\n"); + + if(!r) + return S_OK; + if(!(weakmap = calloc(1, sizeof(*weakmap)))) + return E_OUTOFMEMORY; + + hres = init_dispex(&weakmap->dispex, ctx, &WeakMap_info, ctx->weakmap_prototype); + if(FAILED(hres)) + return hres; + + rb_init(&weakmap->map, weakmap_compare); + *r = jsval_obj(&weakmap->dispex); + return S_OK; + + case DISPATCH_METHOD: + return throw_error(ctx, JS_E_WRONG_THIS, L"WeakMap"); + + default: + FIXME("unimplemented flags %x\n", flags); + return E_NOTIMPL; + } +} + HRESULT init_set_constructor(script_ctx_t *ctx) { jsdisp_t *constructor; @@ -649,5 +808,20 @@ HRESULT init_set_constructor(script_ctx_t *ctx) hres = jsdisp_define_data_property(ctx->global, L"Map", PROPF_WRITABLE, jsval_obj(constructor)); jsdisp_release(constructor); + if(FAILED(hres)) + return hres; + + hres = create_dispex(ctx, &WeakMap_prototype_info, ctx->object_prototype, &ctx->weakmap_prototype); + if(FAILED(hres)) + return hres; + + hres = create_builtin_constructor(ctx, WeakMap_constructor, L"WeakMap", NULL, + PROPF_CONSTR, ctx->weakmap_prototype, &constructor); + if(FAILED(hres)) + return hres; + + hres = jsdisp_define_data_property(ctx->global, L"WeakMap", PROPF_WRITABLE, + jsval_obj(constructor)); + jsdisp_release(constructor); return hres; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3300
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Objects that have had a weakref will become "zombies" with zero refcount and gc_marked (to identify them as garbage) but not part of the objects list, until we clean them up during a garbage collector run. It also doesn't hurt performance like scanning the WeakMaps on every such object's release would, and the object is otherwise completely unlinked anyway, so it doesn't keep unnecessary refs in the meantime. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/jscript/dispex.c | 127 +++++++++++++++++++++++++++++++++++++++- dlls/jscript/error.c | 1 + dlls/jscript/jscript.c | 1 + dlls/jscript/jscript.h | 6 ++ dlls/jscript/jscript.rc | 1 + dlls/jscript/resource.h | 1 + dlls/jscript/set.c | 115 +++++++++++++++++++++++++++++++++++- 7 files changed, 246 insertions(+), 6 deletions(-) diff --git a/dlls/jscript/dispex.c b/dlls/jscript/dispex.c index f2e5109352e..8300e2ccec0 100644 --- a/dlls/jscript/dispex.c +++ b/dlls/jscript/dispex.c @@ -714,12 +714,20 @@ static void unlink_jsdisp(jsdisp_t *jsdisp) * that the only remaining refcount on each object is the number of "external refs" to it. * At the same time, mark all of the objects so that they can be potentially collected. * + * We also build a list of WeakMaps during this iteration for the next step. + * * 2. For each object with a non-zero "external refcount", clear the mark from step 1, and * recursively traverse all linked objects from it, clearing their marks as well (regardless * of their refcount), stopping a given path when the object is unmarked (and then going back * up the GC stack). This basically unmarks all of the objects with "external refcounts" * and those accessible from them, and only the leaked dangling objects will still be marked. * + * Note that we do *not* traverse WeakMaps when traversing the other objects. We need to do at + * least one additional pass over WeakMaps next, unmarking all objects that have live keys and + * are part of a live WeakMap, since we need both the key and the WeakMap to actually be able + * to access the value. But we have to repeat this iteration over WeakMaps as long as we have + * unmarked at least one object in the process, since it can make new paths alive for them. + * * 3. For each object that is marked, unlink all of the objects linked from it, because they * are dangling in a circular refcount and not accessible. This should release them. * @@ -731,15 +739,21 @@ static void unlink_jsdisp(jsdisp_t *jsdisp) * has to be a balance between reclaiming dangling objects and performance. * */ +struct gc_weakmap_list { + jsdisp_t *weakmaps[252]; + struct gc_weakmap_list *prev; +}; + struct gc_stack_chunk { jsdisp_t *objects[1020]; struct gc_stack_chunk *prev; }; struct gc_ctx { + struct gc_weakmap_list *weakmaps; struct gc_stack_chunk *chunk; struct gc_stack_chunk *next; - unsigned idx; + unsigned idx, weakmap_idx; }; static HRESULT gc_stack_push(struct gc_ctx *gc_ctx, jsdisp_t *obj) @@ -841,6 +855,45 @@ HRESULT gc_unmark(struct gc_ctx *gc_ctx, jsdisp_t *obj) return hres; } +static void gc_free_dead_weak_refs(struct gc_ctx *gc_ctx, script_ctx_t *ctx) +{ + jsdisp_t *obj, *obj2; + + /* Only bother if we actually had any weak maps in the first place, but note that + we can't rely on the weakmaps list in the gc_ctx, even if it was still available, + because some of them might have been destroyed already during unlinking. */ + if(gc_ctx->weakmaps) { + struct list *iter = list_head(&ctx->objects); + while(iter) { + obj = LIST_ENTRY(iter, jsdisp_t, entry); + if(!obj->builtin_info->free_dead_weak_refs) { + iter = list_next(&ctx->objects, iter); + continue; + } + + /* Grab it since it could get released when dead entries get removed */ + jsdisp_addref(obj); + obj->builtin_info->free_dead_weak_refs(obj); + + /* If releasing it now causes the object to be destroyed, unlink it + first so we can safely obtain the next pointer before releasing it. */ + if(obj->ref == 1) + unlink_jsdisp(obj); + + iter = list_next(&ctx->objects, iter); + jsdisp_release(obj); + } + } + + LIST_FOR_EACH_ENTRY_SAFE(obj, obj2, &ctx->zombie_objects, jsdisp_t, entry) { + if(obj->builtin_info->destructor) + obj->builtin_info->destructor(obj); + else + free(obj); + } + list_init(&ctx->zombie_objects); +} + HRESULT gc_run(script_ctx_t *ctx) { /* Save original refcounts in a linked list of chunks */ @@ -849,9 +902,11 @@ HRESULT gc_run(script_ctx_t *ctx) struct chunk *next; LONG ref[1020]; } *head, *chunk; + struct gc_weakmap_list *weakmaps; dispex_prop_t *prop, *props_end; struct gc_ctx gc_ctx = { 0 }; unsigned chunk_idx = 0; + BOOL objects_unmarked; jsdisp_t *obj, *link; HRESULT hres = S_OK; struct list *iter; @@ -865,6 +920,9 @@ HRESULT gc_run(script_ctx_t *ctx) head->next = NULL; chunk = head; + /* Init it so it allocates the first chunk properly, when needed */ + gc_ctx.weakmap_idx = ARRAY_SIZE(gc_ctx.weakmaps->weakmaps); + /* 1. Save actual refcounts and decrease them speculatively as-if we unlinked the objects */ LIST_FOR_EACH_ENTRY(obj, &ctx->objects, jsdisp_t, entry) { if(chunk_idx == ARRAY_SIZE(chunk->ref)) { @@ -901,8 +959,11 @@ HRESULT gc_run(script_ctx_t *ctx) if(obj->prototype && obj->prototype->ctx == ctx) obj->prototype->ref--; - if(obj->builtin_info->gc_traverse) - obj->builtin_info->gc_traverse(&gc_ctx, GC_TRAVERSE_SPECULATIVELY, obj); + if(obj->builtin_info->gc_traverse) { + hres = obj->builtin_info->gc_traverse(&gc_ctx, GC_TRAVERSE_SPECULATIVELY, obj); + if(FAILED(hres)) + goto restore; + } obj->gc_marked = TRUE; } @@ -915,6 +976,31 @@ HRESULT gc_run(script_ctx_t *ctx) if(FAILED(hres)) break; } + + do { + unsigned idx = gc_ctx.weakmap_idx; + + weakmaps = gc_ctx.weakmaps; + objects_unmarked = FALSE; + + while(weakmaps) { + do { + jsdisp_t *weakmap = weakmaps->weakmaps[--idx]; + + if(weakmap->gc_marked) + continue; + hres = gc_traverse_weakmap(&gc_ctx, weakmap); + if(FAILED(hres)) + goto restore; + objects_unmarked |= (hres == S_OK); + } while(idx); + + idx = ARRAY_SIZE(weakmaps->weakmaps); + weakmaps = weakmaps->prev; + } + } while(objects_unmarked); + +restore: free(gc_ctx.next); /* Restore */ @@ -929,6 +1015,13 @@ HRESULT gc_run(script_ctx_t *ctx) } free(chunk); + weakmaps = gc_ctx.weakmaps; + while(weakmaps) { + struct gc_weakmap_list *tmp = weakmaps; + weakmaps = tmp->prev; + free(tmp); + } + if(FAILED(hres)) return hres; @@ -953,11 +1046,28 @@ HRESULT gc_run(script_ctx_t *ctx) jsdisp_release(obj); } + gc_free_dead_weak_refs(&gc_ctx, ctx); ctx->gc_is_unlinking = FALSE; ctx->gc_last_tick = GetTickCount(); return S_OK; } +HRESULT gc_note_weakmap(struct gc_ctx *gc_ctx, jsdisp_t *weakmap) +{ + if(gc_ctx->weakmap_idx == ARRAY_SIZE(gc_ctx->weakmaps->weakmaps)) { + struct gc_weakmap_list *new_chunk = malloc(sizeof(*new_chunk)); + + if(!new_chunk) + return E_OUTOFMEMORY; + + new_chunk->prev = gc_ctx->weakmaps; + gc_ctx->weakmaps = new_chunk; + gc_ctx->weakmap_idx = 0; + } + gc_ctx->weakmaps->weakmaps[gc_ctx->weakmap_idx++] = weakmap; + return S_OK; +} + HRESULT gc_process_linked_obj(struct gc_ctx *gc_ctx, enum gc_traverse_op op, jsdisp_t *obj, jsdisp_t *link, void **unlink_ref) { if(op == GC_TRAVERSE_UNLINK) { @@ -2254,6 +2364,17 @@ void jsdisp_free(jsdisp_t *obj) if(obj->prototype) jsdisp_release(obj->prototype); + /* If we possibly have weak refs to this object, keep it around as a zombie, + so that it only gets actually freed after the weak refs are removed. + Unlink it first so it doesn't hold pointless refs to other objects. */ + if(obj->has_weak_refs) { + if(obj->builtin_info->gc_traverse) + obj->builtin_info->gc_traverse(NULL, GC_TRAVERSE_UNLINK, obj); + list_add_tail(&obj->ctx->zombie_objects, &obj->entry); + obj->gc_marked = TRUE; + return; + } + if(obj->builtin_info->destructor) obj->builtin_info->destructor(obj); else diff --git a/dlls/jscript/error.c b/dlls/jscript/error.c index c3768668178..cf0ece42f15 100644 --- a/dlls/jscript/error.c +++ b/dlls/jscript/error.c @@ -483,6 +483,7 @@ jsdisp_t *create_builtin_error(script_ctx_t *ctx) case JS_E_OBJECT_NONEXTENSIBLE: case JS_E_NONCONFIGURABLE_REDEFINED: case JS_E_NONWRITABLE_MODIFIED: + case JS_E_KEY_NOT_OBJECT: case JS_E_PROP_DESC_MISMATCH: case JS_E_INVALID_WRITABLE_PROP_DESC: constr = ctx->type_error_constr; diff --git a/dlls/jscript/jscript.c b/dlls/jscript/jscript.c index 401f6ca85b0..8896fff720c 100644 --- a/dlls/jscript/jscript.c +++ b/dlls/jscript/jscript.c @@ -748,6 +748,7 @@ static HRESULT WINAPI JScript_SetScriptSite(IActiveScript *iface, ctx->acc = jsval_undefined(); list_init(&ctx->named_items); list_init(&ctx->objects); + list_init(&ctx->zombie_objects); heap_pool_init(&ctx->tmp_heap); hres = create_jscaller(ctx); diff --git a/dlls/jscript/jscript.h b/dlls/jscript/jscript.h index 71f2e2f38bd..d32528aa090 100644 --- a/dlls/jscript/jscript.h +++ b/dlls/jscript/jscript.h @@ -151,6 +151,8 @@ named_item_t *lookup_named_item(script_ctx_t*,const WCHAR*,unsigned); void release_named_item(named_item_t*); HRESULT gc_run(script_ctx_t*); HRESULT gc_unmark(struct gc_ctx*,jsdisp_t*); +HRESULT gc_note_weakmap(struct gc_ctx*,jsdisp_t*); +HRESULT gc_traverse_weakmap(struct gc_ctx*,jsdisp_t*); HRESULT gc_process_linked_obj(struct gc_ctx*,enum gc_traverse_op,jsdisp_t*,jsdisp_t*,void**); HRESULT gc_process_linked_val(struct gc_ctx*,enum gc_traverse_op,jsdisp_t*,jsval_t*); @@ -173,6 +175,7 @@ typedef struct { HRESULT (*idx_get)(jsdisp_t*,unsigned,jsval_t*); HRESULT (*idx_put)(jsdisp_t*,unsigned,jsval_t); HRESULT (*gc_traverse)(struct gc_ctx*,enum gc_traverse_op,jsdisp_t*); + void (*free_dead_weak_refs)(jsdisp_t*); } builtin_info_t; struct jsdisp_t { @@ -180,6 +183,7 @@ struct jsdisp_t { LONG ref; + BOOLEAN has_weak_refs; BOOLEAN extensible; BOOLEAN gc_marked; @@ -372,6 +376,7 @@ struct _script_ctx_t { struct _call_frame_t *call_ctx; struct list named_items; struct list objects; + struct list zombie_objects; IActiveScriptSite *site; IInternetHostSecurityManager *secmgr; DWORD safeopt; @@ -551,6 +556,7 @@ static inline HRESULT disp_call_value(script_ctx_t *ctx, IDispatch *disp, jsval_ #define JS_E_NONCONFIGURABLE_REDEFINED MAKE_JSERROR(IDS_NONCONFIGURABLE_REDEFINED) #define JS_E_NONWRITABLE_MODIFIED MAKE_JSERROR(IDS_NONWRITABLE_MODIFIED) #define JS_E_WRONG_THIS MAKE_JSERROR(IDS_WRONG_THIS) +#define JS_E_KEY_NOT_OBJECT MAKE_JSERROR(IDS_KEY_NOT_OBJECT) #define JS_E_PROP_DESC_MISMATCH MAKE_JSERROR(IDS_PROP_DESC_MISMATCH) #define JS_E_INVALID_WRITABLE_PROP_DESC MAKE_JSERROR(IDS_INVALID_WRITABLE_PROP_DESC) diff --git a/dlls/jscript/jscript.rc b/dlls/jscript/jscript.rc index fbf965fb7e8..e5289a550a3 100644 --- a/dlls/jscript/jscript.rc +++ b/dlls/jscript/jscript.rc @@ -77,6 +77,7 @@ STRINGTABLE IDS_NONCONFIGURABLE_REDEFINED "Cannot redefine non-configurable property '|'" IDS_NONWRITABLE_MODIFIED "Cannot modify non-writable property '|'" IDS_WRONG_THIS "'this' is not a | object" + IDS_KEY_NOT_OBJECT "'key' is not an object" IDS_PROP_DESC_MISMATCH "Property cannot have both accessors and a value" IDS_COMPILATION_ERROR "Microsoft JScript compilation error" diff --git a/dlls/jscript/resource.h b/dlls/jscript/resource.h index 0ac457d740d..ad771b67475 100644 --- a/dlls/jscript/resource.h +++ b/dlls/jscript/resource.h @@ -75,6 +75,7 @@ #define IDS_NONCONFIGURABLE_REDEFINED 0x13D6 #define IDS_NONWRITABLE_MODIFIED 0x13D7 #define IDS_WRONG_THIS 0x13FC +#define IDS_KEY_NOT_OBJECT 0x13FD /* FIXME: This is not compatible with native, but we would * conflict with IDS_UNSUPPORTED_ACTION otherwise */ #define IDS_PROP_DESC_MISMATCH 0x1F00 diff --git a/dlls/jscript/set.c b/dlls/jscript/set.c index f2de6924468..cdd7b50c7a9 100644 --- a/dlls/jscript/set.c +++ b/dlls/jscript/set.c @@ -631,8 +631,31 @@ static int weakmap_compare(const void *k, const struct rb_entry *e) return (a > b) - (a < b); } +static HRESULT get_weakmap_this(script_ctx_t *ctx, jsval_t vthis, WeakMapInstance **ret) +{ + jsdisp_t *jsdisp; + + if(!is_object_instance(vthis)) + return JS_E_OBJECT_EXPECTED; + if(!(jsdisp = to_jsdisp(get_object(vthis))) || !is_class(jsdisp, JSCLASS_WEAKMAP)) { + WARN("not a WeakMap object passed as 'this'\n"); + return throw_error(ctx, JS_E_WRONG_THIS, L"WeakMap"); + } + + *ret = CONTAINING_RECORD(jsdisp, WeakMapInstance, dispex); + return S_OK; +} + +static struct weakmap_entry *get_weakmap_entry(WeakMapInstance *weakmap, jsdisp_t *key) +{ + struct rb_entry *entry; + if(!(entry = rb_get(&weakmap->map, key))) return NULL; + return CONTAINING_RECORD(entry, struct weakmap_entry, entry); +} + static void release_weakmap_entry(struct weakmap_entry *entry) { + /* We can't clear the has_weak_refs flag on the key, because there might be other weak refs */ jsval_release(entry->value); free(entry); } @@ -661,8 +684,50 @@ static HRESULT WeakMap_get(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigne static HRESULT WeakMap_set(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigned argc, jsval_t *argv, jsval_t *r) { - FIXME("\n"); - return E_NOTIMPL; + jsdisp_t *key = (argc >= 1 && is_object_instance(argv[0])) ? to_jsdisp(get_object(argv[0])) : NULL; + jsval_t value = argc >= 2 ? argv[1] : jsval_undefined(); + struct weakmap_entry *entry; + WeakMapInstance *weakmap; + HRESULT hres; + + hres = get_weakmap_this(ctx, vthis, &weakmap); + if(FAILED(hres)) + return hres; + + TRACE("%p (%p %s)\n", weakmap, key, debugstr_jsval(value)); + + if(!key) + return JS_E_KEY_NOT_OBJECT; + + if(key->ctx != ctx) { + FIXME("different ctx not supported\n"); + return JS_E_KEY_NOT_OBJECT; + } + + if((entry = get_weakmap_entry(weakmap, key))) { + jsval_t val; + hres = jsval_copy(value, &val); + if(FAILED(hres)) + return hres; + + jsval_release(entry->value); + entry->value = val; + }else { + if(!(entry = malloc(sizeof(*entry)))) + return E_OUTOFMEMORY; + + entry->key = key; + hres = jsval_copy(value, &entry->value); + if(FAILED(hres)) { + free(entry); + return hres; + } + key->has_weak_refs = TRUE; + rb_put(&weakmap->map, key, &entry->entry); + } + + if(r) *r = jsval_undefined(); + return S_OK; } static HRESULT WeakMap_has(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigned argc, jsval_t *argv, @@ -703,14 +768,57 @@ static HRESULT WeakMap_gc_traverse(struct gc_ctx *gc_ctx, enum gc_traverse_op op return S_OK; } + /* Don't traverse the values here, that's done in a separate iteration in the GC. */ + if(op == GC_TRAVERSE) + return S_OK; + + /* For speculative traversal, the behavior is identical so it is OK to process them normally. + However, we do have to add us to the list of WeakMaps for the GC to traverse later. */ + assert(op == GC_TRAVERSE_SPECULATIVELY); RB_FOR_EACH_ENTRY(entry, &weakmap->map, struct weakmap_entry, entry) { - hres = gc_process_linked_val(gc_ctx, op, dispex, &entry->value); + gc_process_linked_val(gc_ctx, op, dispex, &entry->value); + hres = gc_note_weakmap(gc_ctx, &weakmap->dispex); if(FAILED(hres)) return hres; } return S_OK; } +HRESULT gc_traverse_weakmap(struct gc_ctx *gc_ctx, jsdisp_t *dispex) +{ + WeakMapInstance *weakmap = (WeakMapInstance*)dispex; + script_ctx_t *ctx = weakmap->dispex.ctx; + struct weakmap_entry *entry; + HRESULT hres = S_FALSE; + jsdisp_t *jsdisp; + + /* Here we need to traverse the WeakMap and unmark all objects found from its live keys */ + RB_FOR_EACH_ENTRY(entry, &weakmap->map, struct weakmap_entry, entry) { + if(!is_object_instance(entry->value) || !(jsdisp = to_jsdisp(get_object(entry->value)))) + continue; + + if(jsdisp->gc_marked && !entry->key->gc_marked && jsdisp->ctx == ctx) { + hres = gc_unmark(gc_ctx, jsdisp); + if(FAILED(hres)) + return hres; + } + } + return hres; +} + +static void WeakMap_free_dead_weak_refs(jsdisp_t *dispex) +{ + WeakMapInstance *weakmap = (WeakMapInstance*)dispex; + struct weakmap_entry *entry, *entry2; + + RB_FOR_EACH_ENTRY_DESTRUCTOR(entry, entry2, &weakmap->map, struct weakmap_entry, entry) { + if(entry->key->gc_marked) { + rb_remove(&weakmap->map, &entry->entry); + release_weakmap_entry(entry); + } + } +} + static const builtin_prop_t WeakMap_prototype_props[] = { {L"clear", WeakMap_clear, PROPF_METHOD}, {L"delete", WeakMap_delete, PROPF_METHOD|1}, @@ -739,6 +847,7 @@ static const builtin_info_t WeakMap_info = { NULL, NULL, WeakMap_gc_traverse, + WeakMap_free_dead_weak_refs }; static HRESULT WeakMap_constructor(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigned argc, jsval_t *argv, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3300
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/jscript/set.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/dlls/jscript/set.c b/dlls/jscript/set.c index cdd7b50c7a9..acefe535a34 100644 --- a/dlls/jscript/set.c +++ b/dlls/jscript/set.c @@ -677,8 +677,23 @@ static HRESULT WeakMap_delete(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsi static HRESULT WeakMap_get(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigned argc, jsval_t *argv, jsval_t *r) { - FIXME("\n"); - return E_NOTIMPL; + jsdisp_t *key = (argc >= 1 && is_object_instance(argv[0])) ? to_jsdisp(get_object(argv[0])) : NULL; + struct weakmap_entry *entry; + WeakMapInstance *weakmap; + HRESULT hres; + + hres = get_weakmap_this(ctx, vthis, &weakmap); + if(FAILED(hres)) + return hres; + + TRACE("%p (%p)\n", weakmap, key); + + if(!(entry = get_weakmap_entry(weakmap, key))) { + if(r) *r = jsval_undefined(); + return S_OK; + } + + return r ? jsval_copy(entry->value, r) : S_OK; } static HRESULT WeakMap_set(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigned argc, jsval_t *argv, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3300
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/jscript/set.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/dlls/jscript/set.c b/dlls/jscript/set.c index acefe535a34..2acc81e3b9a 100644 --- a/dlls/jscript/set.c +++ b/dlls/jscript/set.c @@ -670,8 +670,23 @@ static HRESULT WeakMap_clear(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsig static HRESULT WeakMap_delete(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigned argc, jsval_t *argv, jsval_t *r) { - FIXME("\n"); - return E_NOTIMPL; + jsdisp_t *key = (argc >= 1 && is_object_instance(argv[0])) ? to_jsdisp(get_object(argv[0])) : NULL; + struct weakmap_entry *entry; + WeakMapInstance *weakmap; + HRESULT hres; + + hres = get_weakmap_this(ctx, vthis, &weakmap); + if(FAILED(hres)) + return hres; + + TRACE("%p (%p)\n", weakmap, key); + + if((entry = get_weakmap_entry(weakmap, key))) { + rb_remove(&weakmap->map, &entry->entry); + release_weakmap_entry(entry); + } + if(r) *r = jsval_bool(!!entry); + return S_OK; } static HRESULT WeakMap_get(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigned argc, jsval_t *argv, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3300
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/jscript/set.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/dlls/jscript/set.c b/dlls/jscript/set.c index 2acc81e3b9a..c251d675cc8 100644 --- a/dlls/jscript/set.c +++ b/dlls/jscript/set.c @@ -663,8 +663,22 @@ static void release_weakmap_entry(struct weakmap_entry *entry) static HRESULT WeakMap_clear(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigned argc, jsval_t *argv, jsval_t *r) { - FIXME("\n"); - return E_NOTIMPL; + struct weakmap_entry *entry, *entry2; + WeakMapInstance *weakmap; + HRESULT hres; + + hres = get_weakmap_this(ctx, vthis, &weakmap); + if(FAILED(hres)) + return hres; + + TRACE("%p\n", weakmap); + + RB_FOR_EACH_ENTRY_DESTRUCTOR(entry, entry2, &weakmap->map, struct weakmap_entry, entry) + release_weakmap_entry(entry); + rb_destroy(&weakmap->map, NULL, NULL); + + if(r) *r = jsval_undefined(); + return S_OK; } static HRESULT WeakMap_delete(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigned argc, jsval_t *argv, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3300
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/jscript/set.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/dlls/jscript/set.c b/dlls/jscript/set.c index c251d675cc8..bab7c605569 100644 --- a/dlls/jscript/set.c +++ b/dlls/jscript/set.c @@ -777,8 +777,18 @@ static HRESULT WeakMap_set(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigne static HRESULT WeakMap_has(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigned argc, jsval_t *argv, jsval_t *r) { - FIXME("\n"); - return E_NOTIMPL; + jsdisp_t *key = (argc >= 1 && is_object_instance(argv[0])) ? to_jsdisp(get_object(argv[0])) : NULL; + WeakMapInstance *weakmap; + HRESULT hres; + + hres = get_weakmap_this(ctx, vthis, &weakmap); + if(FAILED(hres)) + return hres; + + TRACE("%p (%p)\n", weakmap, key); + + if(r) *r = jsval_bool(!!get_weakmap_entry(weakmap, key)); + return S_OK; } static HRESULT WeakMap_value(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigned argc, jsval_t *argv, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3300
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/tests/documentmode.js | 130 ++++++++++++++++++ dlls/mshtml/tests/script.c | 210 ++++++++++++++++++++++++++++++ 2 files changed, 340 insertions(+) diff --git a/dlls/mshtml/tests/documentmode.js b/dlls/mshtml/tests/documentmode.js index 3533d721960..00a6eded3fe 100644 --- a/dlls/mshtml/tests/documentmode.js +++ b/dlls/mshtml/tests/documentmode.js @@ -470,6 +470,8 @@ sync_test("window_props", function() { test_exposed("requestAnimationFrame", v >= 10); test_exposed("Map", v >= 11); test_exposed("Set", v >= 11); + test_exposed("WeakMap", v >= 11); + test_exposed("WeakSet", false); test_exposed("performance", true); test_exposed("console", v >= 10); test_exposed("matchMedia", v >= 10); @@ -1608,6 +1610,134 @@ sync_test("map_obj", function() { ok(r === 1, "r = " + r); }); +async_test("weakmap_obj", function() { + if(!("WeakMap" in window)) { next_test(); return; } + + try { + var s = WeakMap(); + ok(false, "expected exception calling constructor as method"); + }catch(e) { + ok(e.number === 0xa13fc - 0x80000000, "calling constructor as method threw " + e.number); + } + + var s = new WeakMap, r, o, o2; + ok(Object.getPrototypeOf(s) === WeakMap.prototype, "unexpected WeakMap prototype"); + + function test_length(name, len) { + ok(WeakMap.prototype[name].length === len, "WeakMap.prototype." + name + " = " + WeakMap.prototype[name].length); + } + test_length("clear", 0); + test_length("delete", 1); + test_length("get", 1); + test_length("has", 1); + test_length("set", 2); + ok(!("entries" in s), "entries is in WeakMap"); + ok(!("forEach" in s), "forEach is in WeakMap"); + ok(!("keys" in s), "keys is in WeakMap"); + ok(!("size" in s), "size is in WeakMap"); + ok(!("values" in s), "values is in WeakMap"); + + r = Object.prototype.toString.call(s); + ok(r === "[object Object]", "toString returned " + r); + + r = s.get("test"); + ok(r === undefined, "get('test') returned " + r); + r = s.has("test"); + ok(r === false, "has('test') returned " + r); + + try { + r = s.set("test", 1); + ok(false, "set('test') did not throw"); + }catch(e) { + ok(e.number === 0xa13fd - 0x80000000, "set('test') threw " + e.number); + } + try { + r = s.set(external.testHostContext(true), 1); + ok(false, "set(host_obj) did not throw"); + }catch(e) { + ok(e.number === 0xa13fd - 0x80000000, "set(host_obj) threw " + e.number); + } + + r = s.set({}, 1); + ok(r === undefined, "set({}, 1) returned " + r); + + o = {}, o2 = {}; + r = s.get({}); + ok(r === undefined, "get({}) returned " + r); + r = s.has({}); + ok(r === false, "has({}) returned " + r); + + r = s.set(o, 2); + ok(r === undefined, "set(o, 2) returned " + r); + r = s.get(o); + ok(r === 2, "get(o) returned " + r); + r = s.has(o); + ok(r === true, "has(o) returned " + r); + r = s.get(o2); + ok(r === undefined, "get(o2) before set returned " + r); + r = s.has(o2); + ok(r === false, "has(o2) before set returned " + r); + r = s.set(o2, "test"); + ok(r === undefined, "set(o2, 'test') returned " + r); + r = s.get(o2); + ok(r === "test", "get(o2) returned " + r); + r = s.has(o2); + ok(r === true, "has(o2) returned " + r); + + r = s["delete"]("test"); /* using s.delete() would break parsing in quirks mode */ + ok(r === false, "delete('test') returned " + r); + r = s["delete"]({}); + ok(r === false, "delete({}) returned " + r); + r = s["delete"](o); + ok(r === true, "delete(o) returned " + r); + + r = s.get(o); + ok(r === undefined, "get(o) after delete returned " + r); + r = s.has(o); + ok(r === false, "has(o) after delete returned " + r); + r = s.get(o2); + ok(r === "test", "get(o2) after delete returned " + r); + r = s.has(o2); + ok(r === true, "has(o2) after delete returned " + r); + + r = s.set(o, undefined); + ok(r === undefined, "set(o, undefined) returned " + r); + r = s.get(o); + ok(r === undefined, "get(o) after re-set returned " + r); + r = s.has(o); + ok(r === true, "has(o) after re-set returned " + r); + + r = s.clear(); + ok(r === undefined, "clear() returned " + r); + r = s.get(o); + ok(r === undefined, "get(o) after clear returned " + r); + r = s.has(o); + ok(r === false, "has(o) after clear returned " + r); + r = s.get(o2); + ok(r === undefined, "get(o2) after clear returned " + r); + r = s.has(o2); + ok(r === false, "has(o2) after clear returned " + r); + + r = external.newRefTest(); + ok(r.ref === 1, "wrong ref after newRefTest: " + r.ref); + o = { val: r.get(), map: s }; + s.set(o, o); + ok(r.ref > 1, "map entry released"); + + o = Date.now(); + CollectGarbage(); + function retry() { + if(r.ref > 1 && Date.now() - o < 5000) { + CollectGarbage(); + window.setTimeout(retry); + return; + } + ok(r.ref === 1, "map entry not released"); + next_test(); + } + window.setTimeout(retry); +}); + sync_test("storage", function() { var v = document.documentMode, i, r, list; diff --git a/dlls/mshtml/tests/script.c b/dlls/mshtml/tests/script.c index 2dc68f1456e..60f8b07e88a 100644 --- a/dlls/mshtml/tests/script.c +++ b/dlls/mshtml/tests/script.c @@ -155,6 +155,8 @@ DEFINE_EXPECT(GetTypeInfo); #define DISPID_SCRIPT_TESTPROP 0x100000 #define DISPID_SCRIPT_TESTPROP2 0x100001 +#define DISPID_REFTEST_GET 0x100000 +#define DISPID_REFTEST_REF 0x100001 #define DISPID_EXTERNAL_OK 0x300000 #define DISPID_EXTERNAL_TRACE 0x300001 @@ -171,6 +173,7 @@ DEFINE_EXPECT(GetTypeInfo); #define DISPID_EXTERNAL_TESTHOSTCTX 0x30000C #define DISPID_EXTERNAL_GETMIMETYPE 0x30000D #define DISPID_EXTERNAL_SETVIEWSIZE 0x30000E +#define DISPID_EXTERNAL_NEWREFTEST 0x30000F static const GUID CLSID_TestScript[] = { {0x178fc163,0xf585,0x4e24,{0x9c,0x13,0x4b,0xb7,0xfa,0xf8,0x07,0x46}}, @@ -641,6 +644,13 @@ static HRESULT WINAPI DispatchEx_GetDispID(IDispatchEx *iface, BSTR bstrName, DW return E_NOTIMPL; } +static HRESULT WINAPI DispatchEx_InvokeEx(IDispatchEx *iface, DISPID id, LCID lcid, WORD wFlags, DISPPARAMS *pdp, + VARIANT *pvarRes, EXCEPINFO *pei, IServiceProvider *pspCaller) +{ + ok(0, "unexpected call\n"); + return E_NOTIMPL; +} + static HRESULT WINAPI funcDisp_InvokeEx(IDispatchEx *iface, DISPID id, LCID lcid, WORD wFlags, DISPPARAMS *pdp, VARIANT *pvarRes, EXCEPINFO *pei, IServiceProvider *pspCaller) { @@ -840,6 +850,181 @@ static IDispatchExVtbl testHostContextDisp_no_this_vtbl = { static IDispatchEx testHostContextDisp_no_this = { &testHostContextDisp_no_this_vtbl }; +struct refTestObj +{ + IDispatchEx IDispatchEx_iface; + LONG ref; +}; + +struct refTest +{ + IDispatchEx IDispatchEx_iface; + LONG ref; + struct refTestObj *obj; +}; + +static inline struct refTestObj *refTestObj_from_IDispatchEx(IDispatchEx *iface) +{ + return CONTAINING_RECORD(iface, struct refTestObj, IDispatchEx_iface); +} + +static inline struct refTest *refTest_from_IDispatchEx(IDispatchEx *iface) +{ + return CONTAINING_RECORD(iface, struct refTest, IDispatchEx_iface); +} + +static HRESULT WINAPI refTestObj_QueryInterface(IDispatchEx *iface, REFIID riid, void **ppv) +{ + struct refTestObj *This = refTestObj_from_IDispatchEx(iface); + + if(IsEqualGUID(riid, &IID_IUnknown) || IsEqualGUID(riid, &IID_IDispatch) || IsEqualGUID(riid, &IID_IDispatchEx)) + *ppv = &This->IDispatchEx_iface; + else { + *ppv = NULL; + return E_NOINTERFACE; + } + + IDispatchEx_AddRef(&This->IDispatchEx_iface); + return S_OK; +} + +static ULONG WINAPI refTestObj_AddRef(IDispatchEx *iface) +{ + struct refTestObj *This = refTestObj_from_IDispatchEx(iface); + + return InterlockedIncrement(&This->ref); +} + +static ULONG WINAPI refTestObj_Release(IDispatchEx *iface) +{ + struct refTestObj *This = refTestObj_from_IDispatchEx(iface); + LONG ref = InterlockedDecrement(&This->ref); + + if(!ref) + free(This); + + return ref; +} + +static IDispatchExVtbl refTestObj_vtbl = { + refTestObj_QueryInterface, + refTestObj_AddRef, + refTestObj_Release, + DispatchEx_GetTypeInfoCount, + DispatchEx_GetTypeInfo, + DispatchEx_GetIDsOfNames, + DispatchEx_Invoke, + DispatchEx_GetDispID, + DispatchEx_InvokeEx, + DispatchEx_DeleteMemberByName, + DispatchEx_DeleteMemberByDispID, + DispatchEx_GetMemberProperties, + DispatchEx_GetMemberName, + DispatchEx_GetNextDispID, + DispatchEx_GetNameSpaceParent +}; + +static HRESULT WINAPI refTest_QueryInterface(IDispatchEx *iface, REFIID riid, void **ppv) +{ + struct refTest *This = refTest_from_IDispatchEx(iface); + + if(IsEqualGUID(riid, &IID_IUnknown) || IsEqualGUID(riid, &IID_IDispatch) || IsEqualGUID(riid, &IID_IDispatchEx)) + *ppv = &This->IDispatchEx_iface; + else { + *ppv = NULL; + return E_NOINTERFACE; + } + + IDispatchEx_AddRef(&This->IDispatchEx_iface); + return S_OK; +} + +static ULONG WINAPI refTest_AddRef(IDispatchEx *iface) +{ + struct refTest *This = refTest_from_IDispatchEx(iface); + + return InterlockedIncrement(&This->ref); +} + +static ULONG WINAPI refTest_Release(IDispatchEx *iface) +{ + struct refTest *This = refTest_from_IDispatchEx(iface); + LONG ref = InterlockedDecrement(&This->ref); + + if(!ref) { + IDispatchEx_Release(&This->obj->IDispatchEx_iface); + free(This); + } + + return ref; +} + +static HRESULT WINAPI refTest_GetDispID(IDispatchEx *iface, BSTR bstrName, DWORD grfdex, DISPID *pid) +{ + if(!wcscmp(bstrName, L"get")) { + *pid = DISPID_REFTEST_GET; + return S_OK; + } + if(!wcscmp(bstrName, L"ref")) { + *pid = DISPID_REFTEST_REF; + return S_OK; + } + ok(0, "unexpected call %s\n", wine_dbgstr_w(bstrName)); + return E_NOTIMPL; +} + +static HRESULT WINAPI refTest_InvokeEx(IDispatchEx *iface, DISPID id, LCID lcid, WORD wFlags, DISPPARAMS *pdp, + VARIANT *pvarRes, EXCEPINFO *pei, IServiceProvider *pspCaller) +{ + struct refTest *This = refTest_from_IDispatchEx(iface); + + ok(pdp != NULL, "pdp == NULL\n"); + ok(!pdp->cArgs, "pdp->cArgs = %d\n", pdp->cArgs); + ok(pvarRes != NULL, "pvarRes == NULL\n"); + ok(pei != NULL, "pei == NULL\n"); + ok(pspCaller != NULL, "pspCaller == NULL\n"); + + switch(id) { + case DISPID_REFTEST_GET: { + ok(wFlags == DISPATCH_METHOD, "DISPID_REFTEST_GET wFlags = %x\n", wFlags); + V_VT(pvarRes) = VT_DISPATCH; + V_DISPATCH(pvarRes) = (IDispatch*)&This->obj->IDispatchEx_iface; + IDispatchEx_AddRef(&This->obj->IDispatchEx_iface); + break; + } + case DISPID_REFTEST_REF: + ok(wFlags == DISPATCH_PROPERTYGET, "DISPID_REFTEST_REF wFlags = %x\n", wFlags); + V_VT(pvarRes) = VT_I4; + V_I4(pvarRes) = This->obj->ref; + break; + + default: + ok(0, "id = %ld", id); + V_VT(pvarRes) = VT_EMPTY; + break; + } + + return S_OK; +} + +static IDispatchExVtbl refTest_vtbl = { + refTest_QueryInterface, + refTest_AddRef, + refTest_Release, + DispatchEx_GetTypeInfoCount, + DispatchEx_GetTypeInfo, + DispatchEx_GetIDsOfNames, + DispatchEx_Invoke, + refTest_GetDispID, + refTest_InvokeEx, + DispatchEx_DeleteMemberByName, + DispatchEx_DeleteMemberByDispID, + DispatchEx_GetMemberProperties, + DispatchEx_GetMemberName, + DispatchEx_GetNextDispID, + DispatchEx_GetNameSpaceParent +}; + static HRESULT WINAPI externalDisp_GetDispID(IDispatchEx *iface, BSTR bstrName, DWORD grfdex, DISPID *pid) { if(!lstrcmpW(bstrName, L"ok")) { @@ -902,6 +1087,10 @@ static HRESULT WINAPI externalDisp_GetDispID(IDispatchEx *iface, BSTR bstrName, *pid = DISPID_EXTERNAL_SETVIEWSIZE; return S_OK; } + if(!lstrcmpW(bstrName, L"newRefTest")) { + *pid = DISPID_EXTERNAL_NEWREFTEST; + return S_OK; + } ok(0, "unexpected name %s\n", wine_dbgstr_w(bstrName)); return DISP_E_UNKNOWNNAME; @@ -1180,6 +1369,27 @@ static HRESULT WINAPI externalDisp_InvokeEx(IDispatchEx *iface, DISPID id, LCID return IOleDocumentView_SetRect(view, &rect); } + case DISPID_EXTERNAL_NEWREFTEST: { + struct refTest *obj = malloc(sizeof(*obj)); + + ok(pdp != NULL, "pdp == NULL\n"); + ok(pdp->rgvarg != NULL, "rgvarg == NULL\n"); + ok(!pdp->rgdispidNamedArgs, "rgdispidNamedArgs != NULL\n"); + ok(!pdp->cArgs, "cArgs = %d\n", pdp->cArgs); + ok(!pdp->cNamedArgs, "cNamedArgs = %d\n", pdp->cNamedArgs); + ok(pei != NULL, "pei == NULL\n"); + + obj->IDispatchEx_iface.lpVtbl = &refTest_vtbl; + obj->ref = 1; + obj->obj = malloc(sizeof(*obj->obj)); + obj->obj->IDispatchEx_iface.lpVtbl = &refTestObj_vtbl; + obj->obj->ref = 1; + + V_VT(pvarRes) = VT_DISPATCH; + V_DISPATCH(pvarRes) = (IDispatch*)&obj->IDispatchEx_iface; + return S_OK; + } + default: ok(0, "unexpected call\n"); return E_NOTIMPL; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3300
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/jscript/error.c | 1 + 1 file changed, 1 insertion(+) diff --git a/dlls/jscript/error.c b/dlls/jscript/error.c index cf0ece42f15..f6eda62b88d 100644 --- a/dlls/jscript/error.c +++ b/dlls/jscript/error.c @@ -483,6 +483,7 @@ jsdisp_t *create_builtin_error(script_ctx_t *ctx) case JS_E_OBJECT_NONEXTENSIBLE: case JS_E_NONCONFIGURABLE_REDEFINED: case JS_E_NONWRITABLE_MODIFIED: + case JS_E_WRONG_THIS: case JS_E_KEY_NOT_OBJECT: case JS_E_PROP_DESC_MISMATCH: case JS_E_INVALID_WRITABLE_PROP_DESC: -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3300
Ok, I think this should cover all cases, with updated tests included. For reference, I also took a brief look at how wine-gecko handles weak maps, and it seems to be pretty similar (in concept), so I think it's enough. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3300#note_38993
On Mon Jul 17 11:40:25 2023 +0000, Gabriel Ivăncescu wrote:
Ok, I think this should cover all cases, with updated tests included. For reference, I also took a brief look at how wine-gecko handles weak maps, and it seems to be pretty similar (in concept), so I think it's enough. This extra complication to GC does not seem needed for me. As I mentioned earlier, if you track weak references, you could just use the existing traversal mechanism. To summarize: "zombie" objects are not great in the first place, the solution depends on GC to a degree that it doesn't need to, delays reclaiming memory even in simple cases and it makes GC more complicated than it needs to be. I'd expect that avoiding all this would make the code simpler.
For reference, I also took a brief look at how wine-gecko handles weak maps, and it seems to be pretty similar (in concept), so I think it's enough.
Aside from weak refs, as a side note, some (most?) of JS engine depend much more on GC for object handling than we do. They often don't track object ref count at all, so objects can't be freed any other way than by GC. In such implementations, essentially all unused objects are "zombies". This is not how our jscript works (while changing that may be theoretically possible, it does not really align with how jscript exposes its internals with IDispatch). We pay the price of tracking ref count, but benefit from the fact that we can reclaim memory immediately in simple cases, allowing GC to run less often and on smaller graphs. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3300#note_39127
On Mon Jul 17 11:40:25 2023 +0000, Jacek Caban wrote:
This extra complication to GC does not seem needed for me. As I mentioned earlier, if you track weak references, you could just use the existing traversal mechanism. To summarize: "zombie" objects are not great in the first place, the solution depends on GC to a degree that it doesn't need to, delays reclaiming memory even in simple cases and it makes GC more complicated than it needs to be. I'd expect that avoiding all this would make the code simpler.
For reference, I also took a brief look at how wine-gecko handles weak maps, and it seems to be pretty similar (in concept), so I think it's enough. Aside from weak refs, as a side note, some (most?) of JS engine depend much more on GC for object handling than we do. They often don't track object ref count at all, so objects can't be freed any other way than by GC. In such implementations, essentially all unused objects are "zombies". This is not how our jscript works (while changing that may be theoretically possible, it does not really align with how jscript exposes its internals with IDispatch). We pay the price of tracking ref count, but benefit from the fact that we can reclaim memory immediately in simple cases, allowing GC to run less often and on smaller graphs. Do you mean just the "free_dead_weak_refs" part? That's the only thing I could conceivably get rid of by getting rid of the zombie objects, but I did it this way because it's actually better in terms of performance, and not much worse in terms of memory.
Tracking weak refs will add some memory overhead as well, while zombies only exist for those refs that are released until the next GC, but they don't keep any refs so they don't add any complications to the graph or extend lives of other objects at all. Here's the problem though. The other new part of the GC, traversing weak maps, has nothing to do with zombies. While it's true that I could traverse the weak map entry values as part of the key's traversal if we had weak ref tracking, it does *not* work in all cases (it will have some leaks). This was my original idea, but it does not handle the cases where the WeakMap itself is "dead", since it will still traverse them in such case from the key, even though they're not accessible (because you need **both** the WeakMap *and* the key to access it, not just the key). I can't just check the WeakMap's liveness, though, because it could change *during* the traversal and become alive. In such case, we would end up unlinking a live object, which is even worse than a leak. It does with this current method as well which is why I have to keep re-iterating over the weak maps until no more changes are done. So, are you sure it's worth it? Do you have some ideas to tackle this problem? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3300#note_39151
Do you have some ideas to tackle this problem?
What's wrong with my earlier suggestion? During GC_TRAVERSE, you could traverse weak ref entry both when traversing the object itself and the weak map. On the first visit, just mark the entry as visited. On the second visit, continue visiting the value. It means that if any of them are missing, value will not be marked as alive. All of that may happen during regular GC steps, so no extra passes are needed. And again, jsdisp_t memory overhead is avoidable. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3300#note_39154
On Mon Jul 17 14:30:34 2023 +0000, Jacek Caban wrote:
Do you have some ideas to tackle this problem? What's wrong with my earlier suggestion? During GC_TRAVERSE, you could traverse weak ref entry both when traversing the object itself and the weak map. On the first visit, just mark the entry as visited. On the second visit, continue visiting the value. It means that if any of them are missing, value will not be marked as alive. All of that may happen during regular GC steps, so no extra passes are needed. And again, jsdisp_t memory overhead is avoidable. That creates a "either key OR weakmap keeps it alive", but we want "both key AND weakmap keep it alive" situation — i.e. it will leak in some cases when the map is alive.
After all, such an entry would only be alive if both the key and the weakmap are alive, else it cannot be accessed. Perhaps the following would work (let me know if you can figure out a wrong case): * When traversing objects, if they have weak refs, traverse the values associated with them from the weakmap **only if** the WeakMap is alive at this point. Note that WeakMaps can become alive later down the line. * When traversing a WeakMap, check if the key is alive before traversing the associated value. If it is, traverse the value, because it means the key did not traverse it due to the WeakMap not being alive earlier, even though it actually is. Scenarios: If the WeakMap is traversed first, and key is not alive, the value will not get traversed, but the WeakMap will be marked alive. If the key later turns out to be alive, it will traverse the value as part of step 1, so no wrong unlinks. If the key is traversed first, and WeakMap is not alive, the value will not get traversed during the key's traversal. If the WeakMap is later traversed and turns out to be alive, it will see the key is alive (because it got traversed), and thus traverse the value. I think it should handle all cases? BTW, what's the best way to track weak refs here? Let's say we have a rb_tree that maps an object to such tracking. We'd need a list of WeakMaps and possibly the entry values (associated with the key) for the "value" of the rb_tree. An array, like props, that doubles in size everytime it exceeds its max? Or something else? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3300#note_39156
That creates a "either key OR weakmap keeps it alive", but we want "both key AND weakmap keep it alive" situation — i.e. it will leak in some cases when the map is alive.
Why would it? With tracked weak refs, we can unlink map entry both from map and key's destructor, so if any of them is collected, the entry will be collected as well. What we need to make sure is that GC correctly recognizes value's reference relations, so that GC has a chance to unlink relevant objects. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3300#note_39160
BTW, what's the best way to track weak refs here? Let's say we have a rb_tree that maps an object to such tracking. We'd need a list of WeakMaps and possibly the entry values (associated with the key) for the "value" of the rb_tree. An array, like props, that doubles in size everytime it exceeds its max? Or something else?
It could probably be just a list of map entries. Eg. instead of `weakmap_entry`, you could have: ``` struct weak_reference { struct rb_entry map_entry; struct list key_entry; jsdisp_t *key; jsval_t value; int gc_mark; }; ``` It would be both part of mentioned list associated with jsdisp_t and WeakMap. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3300#note_39162
On Mon Jul 17 15:18:53 2023 +0000, Jacek Caban wrote:
BTW, what's the best way to track weak refs here? Let's say we have a rb_tree that maps an object to such tracking. We'd need a list of WeakMaps and possibly the entry values (associated with the key) for the "value" of the rb_tree. An array, like props, that doubles in size everytime it exceeds its max? Or something else? It could probably be just a list of map entries. Eg. instead of `weakmap_entry`, you could have:
struct weak_reference { struct rb_entry map_entry; struct list key_entry; jsdisp_t *key; jsval_t value; int gc_mark; };It would be both part of mentioned list associated with jsdisp_t and WeakMap. There could be cyclic refs in the entry value's path. If we unconditionally visit them from both the key and the WeakMap, they will be marked alive when either of them is alive, not when *both* of them are. For example if we keep the WeakMap alive (local variable for example), the entries will also remain alive, even if the key is released.
Traversing basically unmarks the entire path from garbage collection; only one traversal is enough to do this, it's an "OR" operation, not an "AND". That's why we need to add those conditions I mentioned. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3300#note_39169
On Mon Jul 17 16:06:18 2023 +0000, Gabriel Ivăncescu wrote:
There could be cyclic refs in the entry value's path. If we unconditionally visit them from both the key and the WeakMap, they will be marked alive when either of them is alive, not when *both* of them are. For example if we keep the WeakMap alive (local variable for example), the entries will also remain alive, even if the key is released. Traversing basically unmarks the entire path from garbage collection; only one traversal is enough to do this, it's an "OR" operation, not an "AND". That's why we need to add those conditions I mentioned. We wouldn't unconditionally visit value:
During GC_TRAVERSE, you could traverse weak ref entry both when traversing the object itself and the weak map. On the first visit, just mark the entry as visited. On the second visit, continue visiting the value.
Note that the marking the entry is not the same as "mark alive" in GC. It's purely a marker to know if we should visit the value or leave it for the potential second traversal. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3300#note_39173
On Mon Jul 17 16:24:10 2023 +0000, Jacek Caban wrote:
We wouldn't unconditionally visit value:
During GC_TRAVERSE, you could traverse weak ref entry both when traversing the object itself and the weak map. On the first visit, just mark the entry as visited. On the second visit, continue visiting the value. Note that the marking the entry is not the same as "mark alive" in GC. It's purely a marker to know if we should visit the value or leave it for the potential second traversal. Oh so you meant an additional flag.
That works too, yes, but is there something wrong with my approach? Not that it matters, but there's 1 less flag to worry about, and same amount of checks (check liveness of key or map, instead of the new flag). Well I will try my version and see what you think first, unless I run into some troubles with it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3300#note_39174
That works too, yes, but is there something wrong with my approach? Not that it matters, but there's 1 less flag to worry about, and same amount of checks (check liveness of key or map, instead of the new flag).
It should possible yes, but that would need a pointer to WeakMap instead of the flag. Anyway, the part I don't like is the extra pass (which should be possible to avoid with reference tracking). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3300#note_39176
On Mon Jul 17 16:44:37 2023 +0000, Jacek Caban wrote:
That works too, yes, but is there something wrong with my approach? Not that it matters, but there's 1 less flag to worry about, and same amount of checks (check liveness of key or map, instead of the new flag). It should possible yes, but that would need a pointer to WeakMap instead of the flag. Anyway, the part I don't like is the extra pass (which should be possible to avoid with reference tracking). I tried with the flag, but it looks like we will need pointer to WeakMap anyway, for jsdisp_free to be able to remove the entry (which needs access to the WeakMap's rb_tree).
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3300#note_39193
participants (2)
-
Gabriel Ivăncescu -
Jacek Caban (＠jacek)