[PATCH v5 0/5] MR1635: jscript: Implement a Garbage Collector to deal with circular references.
Implement a basic GC based on the mark-and-sweep algorithm, without requiring manually specifying "roots", which vastly simplifies the management. For now, it is triggered every 30 seconds since it last finished, on a new object initialization. Better heuristics could be used in the future. The comments in the code should hopefully understand the high level logic of this approach without boilerplate details. I've tested it on FFXIV launcher (along with other patches from Proton to have it work) and it stops the massive memory leak successfully by itself, so at least it does its job properly. The last patch in the MR is just an optimization for a *very* common case. For artificial testing, one could use something like: ```javascript function leak() { var a = {}, b = {}; a.b = b; b.a = a; } ``` which creates a circular ref and will leak when the function returns. It also introduces and makes use of a "heap_stack", which prevents stack overflows on long chains. -- v5: jscript: Create the source function's 'prototype' prop object on demand. jscript: Run the garbage collector every 30 seconds on a new object jscript: Implement CollectGarbage(). jscript: Implement a Garbage Collector to deal with circular references. jscript: Use a jsdisp to hold refs for scopes. https://gitlab.winehq.org/wine/wine/-/merge_requests/1635
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> So the garbage collector can traverse it. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/jscript/engine.c | 56 ++++++++++++++++++++++++++----------------- dlls/jscript/engine.h | 11 +++++---- 2 files changed, 41 insertions(+), 26 deletions(-) diff --git a/dlls/jscript/engine.c b/dlls/jscript/engine.c index 6c5fe36d29f..bb43df3d4f2 100644 --- a/dlls/jscript/engine.c +++ b/dlls/jscript/engine.c @@ -422,15 +422,40 @@ static inline void clear_acc(script_ctx_t *ctx) ctx->acc = jsval_undefined(); } -static HRESULT scope_push(scope_chain_t *scope, jsdisp_t *jsobj, IDispatch *obj, scope_chain_t **ret) +static void scope_destructor(jsdisp_t *dispex) +{ + scope_chain_t *scope = CONTAINING_RECORD(dispex, scope_chain_t, dispex); + + if(scope->next) + scope_release(scope->next); + + if(scope->obj) + IDispatch_Release(scope->obj); + free(scope); +} + +static const builtin_info_t scope_info = { + JSCLASS_NONE, + NULL, + 0, + NULL, + scope_destructor, +}; + +static HRESULT scope_push(script_ctx_t *ctx, scope_chain_t *scope, jsdisp_t *jsobj, IDispatch *obj, scope_chain_t **ret) { scope_chain_t *new_scope; + HRESULT hres; - new_scope = malloc(sizeof(scope_chain_t)); + new_scope = calloc(1, sizeof(scope_chain_t)); if(!new_scope) return E_OUTOFMEMORY; - new_scope->ref = 1; + hres = init_dispex(&new_scope->dispex, ctx, &scope_info, NULL); + if(FAILED(hres)) { + free(new_scope); + return hres; + } if (obj) IDispatch_AddRef(obj); @@ -453,19 +478,6 @@ static void scope_pop(scope_chain_t **scope) scope_release(tmp); } -void scope_release(scope_chain_t *scope) -{ - if(--scope->ref) - return; - - if(scope->next) - scope_release(scope->next); - - if (scope->obj) - IDispatch_Release(scope->obj); - free(scope); -} - static HRESULT disp_get_id(script_ctx_t *ctx, IDispatch *disp, const WCHAR *name, BSTR name_bstr, DWORD flags, DISPID *id) { IDispatchEx *dispex; @@ -975,7 +987,7 @@ static HRESULT interp_push_with_scope(script_ctx_t *ctx) if(FAILED(hres)) return hres; - hres = scope_push(ctx->call_ctx->scope, to_jsdisp(disp), disp, &ctx->call_ctx->scope); + hres = scope_push(ctx, ctx->call_ctx->scope, to_jsdisp(disp), disp, &ctx->call_ctx->scope); IDispatch_Release(disp); return hres; } @@ -989,7 +1001,7 @@ static HRESULT interp_push_block_scope(script_ctx_t *ctx) TRACE("scope_index %u.\n", scope_index); - hres = scope_push(ctx->call_ctx->scope, NULL, NULL, &frame->scope); + hres = scope_push(ctx, ctx->call_ctx->scope, NULL, NULL, &frame->scope); if (FAILED(hres) || !scope_index) return hres; @@ -1004,7 +1016,7 @@ static HRESULT interp_pop_scope(script_ctx_t *ctx) { TRACE("\n"); - if(ctx->call_ctx->scope->ref > 1) { + if(ctx->call_ctx->scope->dispex.ref > 1) { HRESULT hres = detach_variable_object(ctx, ctx->call_ctx, FALSE); if(FAILED(hres)) ERR("Failed to detach variable object: %08lx\n", hres); @@ -1201,7 +1213,7 @@ static HRESULT interp_enter_catch(script_ctx_t *ctx) hres = jsdisp_propput_name(scope_obj, ident, v); jsval_release(v); if(SUCCEEDED(hres)) - hres = scope_push(ctx->call_ctx->scope, scope_obj, to_disp(scope_obj), &ctx->call_ctx->scope); + hres = scope_push(ctx, ctx->call_ctx->scope, scope_obj, to_disp(scope_obj), &ctx->call_ctx->scope); jsdisp_release(scope_obj); return hres; } @@ -2919,7 +2931,7 @@ static void pop_call_frame(script_ctx_t *ctx) assert(frame->scope == frame->base_scope); /* If current scope will be kept alive, we need to transfer local variables to its variable object. */ - if(frame->scope && frame->scope->ref > 1) { + if(frame->scope && frame->scope->dispex.ref > 1) { HRESULT hres = detach_variable_object(ctx, frame, TRUE); if(FAILED(hres)) ERR("Failed to detach variable object: %08lx\n", hres); @@ -3191,7 +3203,7 @@ static HRESULT setup_scope(script_ctx_t *ctx, call_frame_t *frame, scope_chain_t frame->pop_variables = i; - hres = scope_push(scope_chain, variable_object, to_disp(variable_object), &scope); + hres = scope_push(ctx, scope_chain, variable_object, to_disp(variable_object), &scope); if(FAILED(hres)) { stack_popn(ctx, ctx->stack_top - orig_stack); return hres; diff --git a/dlls/jscript/engine.h b/dlls/jscript/engine.h index 194e5244f12..71915658517 100644 --- a/dlls/jscript/engine.h +++ b/dlls/jscript/engine.h @@ -222,7 +222,7 @@ static inline bytecode_t *bytecode_addref(bytecode_t *code) } typedef struct _scope_chain_t { - LONG ref; + jsdisp_t dispex; /* FIXME: don't wrap it in an jsdisp (it holds ref and traverse for the garbage collector) */ jsdisp_t *jsobj; IDispatch *obj; unsigned int scope_index; @@ -230,14 +230,17 @@ typedef struct _scope_chain_t { struct _scope_chain_t *next; } scope_chain_t; -void scope_release(scope_chain_t*) DECLSPEC_HIDDEN; - static inline scope_chain_t *scope_addref(scope_chain_t *scope) { - scope->ref++; + jsdisp_addref(&scope->dispex); return scope; } +static inline void scope_release(scope_chain_t *scope) +{ + jsdisp_release(&scope->dispex); +} + struct _jsexcept_t { HRESULT error; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1635
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Implement a basic GC based on the mark-and-sweep algorithm, without requiring manually specifying "roots", which vastly simplifies the code. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/jscript/dispex.c | 317 ++++++++++++++++++++++++++++++++++++++ dlls/jscript/engine.c | 28 ++++ dlls/jscript/enumerator.c | 11 +- dlls/jscript/function.c | 88 ++++++++++- dlls/jscript/jscript.c | 3 + dlls/jscript/jscript.h | 18 ++- dlls/jscript/jsregexp.c | 17 +- dlls/jscript/set.c | 37 ++++- dlls/jscript/tests/run.c | 18 +++ 9 files changed, 523 insertions(+), 14 deletions(-) diff --git a/dlls/jscript/dispex.c b/dlls/jscript/dispex.c index 1a7a75875df..875130f7e66 100644 --- a/dlls/jscript/dispex.c +++ b/dlls/jscript/dispex.c @@ -666,6 +666,320 @@ static HRESULT fill_protrefs(jsdisp_t *This) return S_OK; } +static void unlink_props(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); + break; + case PROP_ACCESSOR: + if(prop->u.accessor.getter) + jsdisp_release(prop->u.accessor.getter); + if(prop->u.accessor.setter) + jsdisp_release(prop->u.accessor.setter); + break; + default: + break; + } + prop->type = PROP_DELETED; + } +} + + + +/* + * To deal with circular refcounts, a basic Garbage Collector is used with a variant of the + * mark-and-sweep algorithm that doesn't require knowing or traversing any specific "roots". + * This works based on the assumption that circular references can only happen when objects + * end up pointing to each other, and each other alone, without any external refs. + * + * An "external ref" is a ref to the object that's not from any other object. Example of such + * refs can be local variables, the script ctx (which keeps a ref to the global object), etc. + * + * At a high level, there are 3 logical passes done on the entire list of objects: + * + * 1. Speculatively decrease refcounts of each linked-to-object from each object. This ensures + * 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. + * + * 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. + * + * 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. + * + * During unlinking (GC_TRAVERSE_UNLINK), it is important that we unlink *all* linked objects + * from the object, to be certain that releasing the object later will not delete any other + * objects. Otherwise calculating the "next" object in the list becomes impossible. + * + * This collection process has to be done periodically, but can be pretty expensive so there + * has to be a balance between reclaiming dangling objects and performance. + * + */ +struct gc_stack_chunk { + jsdisp_t *objects[1020]; + struct gc_stack_chunk *prev; +}; + +struct gc_ctx { + struct gc_stack_chunk *chunk; + struct gc_stack_chunk *next; + unsigned idx; +}; + +static HRESULT gc_stack_push(struct gc_ctx *gc_ctx, jsdisp_t *obj) +{ + if(!gc_ctx->idx) { + if(gc_ctx->next) + gc_ctx->chunk = gc_ctx->next; + else { + struct gc_stack_chunk *prev, *tmp = malloc(sizeof(*tmp)); + if(!tmp) + return E_OUTOFMEMORY; + prev = gc_ctx->chunk; + gc_ctx->chunk = tmp; + gc_ctx->chunk->prev = prev; + } + gc_ctx->idx = ARRAY_SIZE(gc_ctx->chunk->objects); + gc_ctx->next = NULL; + } + gc_ctx->chunk->objects[--gc_ctx->idx] = obj; + return S_OK; +} + +static jsdisp_t *gc_stack_pop(struct gc_ctx *gc_ctx) +{ + jsdisp_t *obj = gc_ctx->chunk->objects[gc_ctx->idx]; + + if(++gc_ctx->idx == ARRAY_SIZE(gc_ctx->chunk->objects)) { + free(gc_ctx->next); + gc_ctx->next = gc_ctx->chunk; + gc_ctx->chunk = gc_ctx->chunk->prev; + gc_ctx->idx = 0; + } + return obj; +} + +HRESULT gc_run(script_ctx_t *ctx) +{ + /* Save original refcounts in a linked list of chunks */ + struct chunk + { + 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; + HRESULT hres = S_OK; + struct list *iter; + + if(!(head = malloc(sizeof(*head)))) + return E_OUTOFMEMORY; + head->next = NULL; + chunk = head; + + /* 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)) { + if(!(chunk->next = malloc(sizeof(*chunk)))) { + do { + chunk = head->next; + free(head); + head = chunk; + } while(head); + return E_OUTOFMEMORY; + } + chunk = chunk->next, chunk_idx = 0; + chunk->next = NULL; + } + chunk->ref[chunk_idx++] = obj->ref; + } + LIST_FOR_EACH_ENTRY(obj, &ctx->objects, jsdisp_t, entry) { + 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) && (link = to_jsdisp(get_object(prop->u.val))) && link->ctx == ctx) + link->ref--; + break; + case PROP_ACCESSOR: + if(prop->u.accessor.getter && prop->u.accessor.getter->ctx == ctx) + prop->u.accessor.getter->ref--; + if(prop->u.accessor.setter && prop->u.accessor.setter->ctx == ctx) + prop->u.accessor.setter->ref--; + break; + default: + break; + } + } + + 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); + obj->gc_marked = TRUE; + } + + /* 2. Clear mark on objects with non-zero "external refcount" and all objects accessible from them */ + LIST_FOR_EACH_ENTRY(obj, &ctx->objects, jsdisp_t, entry) { + if(!obj->ref || !obj->gc_marked) + continue; + + hres = gc_stack_push(&gc_ctx, NULL); + 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); + + /* Restore */ + chunk = head, chunk_idx = 0; + LIST_FOR_EACH_ENTRY(obj, &ctx->objects, jsdisp_t, entry) { + obj->ref = chunk->ref[chunk_idx++]; + if(chunk_idx == ARRAY_SIZE(chunk->ref)) { + struct chunk *next = chunk->next; + free(chunk); + chunk = next, chunk_idx = 0; + } + } + free(chunk); + + if(FAILED(hres)) + return hres; + + /* 3. Remove all the links from the marked objects, since they are dangling */ + iter = list_head(&ctx->objects); + while(iter) { + obj = LIST_ENTRY(iter, jsdisp_t, entry); + if(!obj->gc_marked) { + iter = list_next(&ctx->objects, iter); + continue; + } + + /* 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); + + /* Releasing unlinked object should not delete any other object, + so we can safely obtain the next pointer now */ + iter = list_next(&ctx->objects, iter); + jsdisp_release(obj); + } + + 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) { + *unlink_ref = NULL; + jsdisp_release(link); + return S_OK; + } + + if(link->ctx != obj->ctx) + return S_OK; + if(op == GC_TRAVERSE_SPECULATIVELY) + link->ref--; + else if(link->gc_marked) + return gc_stack_push(gc_ctx, link); + return S_OK; +} + +HRESULT gc_process_linked_val(struct gc_ctx *gc_ctx, enum gc_traverse_op op, jsdisp_t *obj, jsval_t *link) +{ + jsdisp_t *jsdisp; + + if(op == GC_TRAVERSE_UNLINK) { + jsval_t val = *link; + *link = jsval_undefined(); + jsval_release(val); + return S_OK; + } + + if(!is_object_instance(*link) || !(jsdisp = to_jsdisp(get_object(*link))) || jsdisp->ctx != obj->ctx) + return S_OK; + if(op == GC_TRAVERSE_SPECULATIVELY) + jsdisp->ref--; + else if(jsdisp->gc_marked) + return gc_stack_push(gc_ctx, jsdisp); + return S_OK; +} + + + struct typeinfo_func { dispex_prop_t *prop; function_code_t *code; @@ -1848,6 +2162,7 @@ HRESULT init_dispex(jsdisp_t *dispex, script_ctx_t *ctx, const builtin_info_t *b script_addref(ctx); dispex->ctx = ctx; + list_add_tail(&ctx->objects, &dispex->entry); return S_OK; } @@ -1882,6 +2197,8 @@ void jsdisp_free(jsdisp_t *obj) { dispex_prop_t *prop; + list_remove(&obj->entry); + TRACE("(%p)\n", obj); for(prop = obj->props; prop < obj->props+obj->prop_cnt; prop++) { diff --git a/dlls/jscript/engine.c b/dlls/jscript/engine.c index bb43df3d4f2..28ada7e53bd 100644 --- a/dlls/jscript/engine.c +++ b/dlls/jscript/engine.c @@ -434,12 +434,40 @@ static void scope_destructor(jsdisp_t *dispex) free(scope); } +static HRESULT scope_gc_traverse(struct gc_ctx *gc_ctx, enum gc_traverse_op op, jsdisp_t *dispex) +{ + scope_chain_t *scope = CONTAINING_RECORD(dispex, scope_chain_t, dispex); + HRESULT hres; + + if(scope->next) { + hres = gc_process_linked_obj(gc_ctx, op, dispex, &scope->next->dispex, (void**)&scope->next); + if(FAILED(hres)) + return hres; + } + + if(op == GC_TRAVERSE_UNLINK) { + IDispatch *obj = scope->obj; + if(obj) { + scope->obj = NULL; + IDispatch_Release(obj); + } + return S_OK; + } + + return scope->jsobj ? gc_process_linked_obj(gc_ctx, op, dispex, scope->jsobj, (void**)&scope->obj) : S_OK; +} + static const builtin_info_t scope_info = { JSCLASS_NONE, NULL, 0, NULL, scope_destructor, + NULL, + NULL, + NULL, + NULL, + scope_gc_traverse }; static HRESULT scope_push(script_ctx_t *ctx, scope_chain_t *scope, jsdisp_t *jsobj, IDispatch *obj, scope_chain_t **ret) diff --git a/dlls/jscript/enumerator.c b/dlls/jscript/enumerator.c index cce2f2d8ec0..d724d0685c9 100644 --- a/dlls/jscript/enumerator.c +++ b/dlls/jscript/enumerator.c @@ -88,6 +88,11 @@ static void Enumerator_destructor(jsdisp_t *dispex) free(dispex); } +static HRESULT Enumerator_gc_traverse(struct gc_ctx *gc_ctx, enum gc_traverse_op op, jsdisp_t *dispex) +{ + return gc_process_linked_val(gc_ctx, op, dispex, &enumerator_from_jsdisp(dispex)->item); +} + static HRESULT Enumerator_atEnd(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigned argc, jsval_t *argv, jsval_t *r) { @@ -189,7 +194,11 @@ static const builtin_info_t EnumeratorInst_info = { 0, NULL, Enumerator_destructor, - NULL + NULL, + NULL, + NULL, + NULL, + Enumerator_gc_traverse }; static HRESULT alloc_enumerator(script_ctx_t *ctx, jsdisp_t *object_prototype, EnumeratorInstance **ret) diff --git a/dlls/jscript/function.c b/dlls/jscript/function.c index 8d2b3ed9f4f..8aa20b44876 100644 --- a/dlls/jscript/function.c +++ b/dlls/jscript/function.c @@ -39,6 +39,7 @@ struct _function_vtbl_t { HRESULT (*toString)(FunctionInstance*,jsstr_t**); function_code_t* (*get_code)(FunctionInstance*); void (*destructor)(FunctionInstance*); + HRESULT (*gc_traverse)(struct gc_ctx*,enum gc_traverse_op,FunctionInstance*); }; typedef struct { @@ -72,6 +73,11 @@ typedef struct { static HRESULT create_bind_function(script_ctx_t*,FunctionInstance*,jsval_t,unsigned,jsval_t*,jsdisp_t**r); +static HRESULT no_gc_traverse(struct gc_ctx *gc_ctx, enum gc_traverse_op op, FunctionInstance *function) +{ + return S_OK; +} + static inline FunctionInstance *function_from_jsdisp(jsdisp_t *jsdisp) { return CONTAINING_RECORD(jsdisp, FunctionInstance, dispex); @@ -108,7 +114,8 @@ static void Arguments_destructor(jsdisp_t *jsdisp) free(arguments->buf); } - jsdisp_release(&arguments->function->function.dispex); + if(arguments->function) + jsdisp_release(&arguments->function->function.dispex); free(arguments); } @@ -166,6 +173,23 @@ static HRESULT Arguments_idx_put(jsdisp_t *jsdisp, unsigned idx, jsval_t val) arguments->function->func_code->params[idx], val); } +static HRESULT Arguments_gc_traverse(struct gc_ctx *gc_ctx, enum gc_traverse_op op, jsdisp_t *jsdisp) +{ + ArgumentsInstance *arguments = arguments_from_jsdisp(jsdisp); + HRESULT hres; + unsigned i; + + if(arguments->buf) { + for(i = 0; i < arguments->argc; i++) { + hres = gc_process_linked_val(gc_ctx, op, jsdisp, &arguments->buf[i]); + if(FAILED(hres)) + return hres; + } + } + + return gc_process_linked_obj(gc_ctx, op, jsdisp, &arguments->function->function.dispex, (void**)&arguments->function); +} + static const builtin_info_t Arguments_info = { JSCLASS_ARGUMENTS, Arguments_value, @@ -174,7 +198,8 @@ static const builtin_info_t Arguments_info = { NULL, Arguments_idx_length, Arguments_idx_get, - Arguments_idx_put + Arguments_idx_put, + Arguments_gc_traverse }; HRESULT setup_arguments_object(script_ctx_t *ctx, call_frame_t *frame) @@ -548,6 +573,12 @@ static void Function_destructor(jsdisp_t *dispex) free(function); } +static HRESULT Function_gc_traverse(struct gc_ctx *gc_ctx, enum gc_traverse_op op, jsdisp_t *dispex) +{ + FunctionInstance *function = function_from_jsdisp(dispex); + return function->vtbl->gc_traverse(gc_ctx, op, function); +} + static const builtin_prop_t Function_props[] = { {L"apply", Function_apply, PROPF_METHOD|2}, {L"arguments", NULL, 0, Function_get_arguments}, @@ -563,7 +594,11 @@ static const builtin_info_t Function_info = { ARRAY_SIZE(Function_props), Function_props, Function_destructor, - NULL + NULL, + NULL, + NULL, + NULL, + Function_gc_traverse }; static const builtin_prop_t FunctionInst_props[] = { @@ -577,7 +612,11 @@ static const builtin_info_t FunctionInst_info = { ARRAY_SIZE(FunctionInst_props), FunctionInst_props, Function_destructor, - NULL + NULL, + NULL, + NULL, + NULL, + Function_gc_traverse }; static HRESULT create_function(script_ctx_t *ctx, const builtin_info_t *builtin_info, const function_vtbl_t *vtbl, size_t size, @@ -657,7 +696,8 @@ static const function_vtbl_t NativeFunctionVtbl = { NativeFunction_call, NativeFunction_toString, NativeFunction_get_code, - NativeFunction_destructor + NativeFunction_destructor, + no_gc_traverse }; HRESULT create_builtin_function(script_ctx_t *ctx, builtin_invoke_t value_proc, const WCHAR *name, @@ -776,11 +816,22 @@ static void InterpretedFunction_destructor(FunctionInstance *func) scope_release(function->scope_chain); } +static HRESULT InterpretedFunction_gc_traverse(struct gc_ctx *gc_ctx, enum gc_traverse_op op, FunctionInstance *func) +{ + InterpretedFunction *function = (InterpretedFunction*)func; + + if(!function->scope_chain) + return S_OK; + return gc_process_linked_obj(gc_ctx, op, &function->function.dispex, &function->scope_chain->dispex, + (void**)&function->scope_chain); +} + static const function_vtbl_t InterpretedFunctionVtbl = { InterpretedFunction_call, InterpretedFunction_toString, InterpretedFunction_get_code, - InterpretedFunction_destructor + InterpretedFunction_destructor, + InterpretedFunction_gc_traverse }; HRESULT create_source_function(script_ctx_t *ctx, bytecode_t *code, function_code_t *func_code, @@ -870,15 +921,36 @@ static void BindFunction_destructor(FunctionInstance *func) for(i = 0; i < function->argc; i++) jsval_release(function->args[i]); - jsdisp_release(&function->target->dispex); + if(function->target) + jsdisp_release(&function->target->dispex); jsval_release(function->this); } +static HRESULT BindFunction_gc_traverse(struct gc_ctx *gc_ctx, enum gc_traverse_op op, FunctionInstance *func) +{ + BindFunction *function = (BindFunction*)func; + HRESULT hres; + unsigned i; + + for(i = 0; i < function->argc; i++) { + hres = gc_process_linked_val(gc_ctx, op, &function->function.dispex, &function->args[i]); + if(FAILED(hres)) + return hres; + } + + hres = gc_process_linked_obj(gc_ctx, op, &function->function.dispex, &function->target->dispex, (void**)&function->target); + if(FAILED(hres)) + return hres; + + return gc_process_linked_val(gc_ctx, op, &function->function.dispex, &function->this); +} + static const function_vtbl_t BindFunctionVtbl = { BindFunction_call, BindFunction_toString, BindFunction_get_code, - BindFunction_destructor + BindFunction_destructor, + BindFunction_gc_traverse }; static HRESULT create_bind_function(script_ctx_t *ctx, FunctionInstance *target, jsval_t bound_this, unsigned argc, diff --git a/dlls/jscript/jscript.c b/dlls/jscript/jscript.c index b020f24beb3..f5331a13b96 100644 --- a/dlls/jscript/jscript.c +++ b/dlls/jscript/jscript.c @@ -495,6 +495,8 @@ static void decrease_state(JScript *This, SCRIPTSTATE state) } script_globals_release(This->ctx); + gc_run(This->ctx); + /* FALLTHROUGH */ case SCRIPTSTATE_UNINITIALIZED: change_state(This, state); @@ -734,6 +736,7 @@ static HRESULT WINAPI JScript_SetScriptSite(IActiveScript *iface, ctx->html_mode = This->html_mode; ctx->acc = jsval_undefined(); list_init(&ctx->named_items); + list_init(&ctx->objects); heap_pool_init(&ctx->tmp_heap); hres = create_jscaller(ctx); diff --git a/dlls/jscript/jscript.h b/dlls/jscript/jscript.h index e1a3da04097..928a7f1ee7e 100644 --- a/dlls/jscript/jscript.h +++ b/dlls/jscript/jscript.h @@ -137,9 +137,20 @@ typedef struct named_item_t { struct list entry; } named_item_t; +struct gc_ctx; + +enum gc_traverse_op { + GC_TRAVERSE_UNLINK, + GC_TRAVERSE_SPECULATIVELY, + GC_TRAVERSE +}; + HRESULT create_named_item_script_obj(script_ctx_t*,named_item_t*) DECLSPEC_HIDDEN; named_item_t *lookup_named_item(script_ctx_t*,const WCHAR*,unsigned) DECLSPEC_HIDDEN; void release_named_item(named_item_t*) DECLSPEC_HIDDEN; +HRESULT gc_run(script_ctx_t*) DECLSPEC_HIDDEN; +HRESULT gc_process_linked_obj(struct gc_ctx*,enum gc_traverse_op,jsdisp_t*,jsdisp_t*,void**) DECLSPEC_HIDDEN; +HRESULT gc_process_linked_val(struct gc_ctx*,enum gc_traverse_op,jsdisp_t*,jsval_t*) DECLSPEC_HIDDEN; typedef struct { const WCHAR *name; @@ -159,6 +170,7 @@ typedef struct { unsigned (*idx_length)(jsdisp_t*); 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*); } builtin_info_t; struct jsdisp_t { @@ -166,15 +178,18 @@ struct jsdisp_t { LONG ref; + BOOLEAN extensible; + BOOLEAN gc_marked; + DWORD buf_size; DWORD prop_cnt; dispex_prop_t *props; script_ctx_t *ctx; - BOOL extensible; jsdisp_t *prototype; const builtin_info_t *builtin_info; + struct list entry; }; static inline IDispatch *to_disp(jsdisp_t *jsdisp) @@ -350,6 +365,7 @@ struct _script_ctx_t { struct _call_frame_t *call_ctx; struct list named_items; + struct list objects; IActiveScriptSite *site; IInternetHostSecurityManager *secmgr; DWORD safeopt; diff --git a/dlls/jscript/jsregexp.c b/dlls/jscript/jsregexp.c index 1cbc828ce09..3775df823c3 100644 --- a/dlls/jscript/jsregexp.c +++ b/dlls/jscript/jsregexp.c @@ -548,6 +548,11 @@ static void RegExp_destructor(jsdisp_t *dispex) free(This); } +static HRESULT RegExp_gc_traverse(struct gc_ctx *gc_ctx, enum gc_traverse_op op, jsdisp_t *dispex) +{ + return gc_process_linked_val(gc_ctx, op, dispex, ®exp_from_jsdisp(dispex)->last_index_val); +} + static const builtin_prop_t RegExp_props[] = { {L"exec", RegExp_exec, PROPF_METHOD|1}, {L"global", NULL,0, RegExp_get_global}, @@ -565,7 +570,11 @@ static const builtin_info_t RegExp_info = { ARRAY_SIZE(RegExp_props), RegExp_props, RegExp_destructor, - NULL + NULL, + NULL, + NULL, + NULL, + RegExp_gc_traverse }; static const builtin_prop_t RegExpInst_props[] = { @@ -582,7 +591,11 @@ static const builtin_info_t RegExpInst_info = { ARRAY_SIZE(RegExpInst_props), RegExpInst_props, RegExp_destructor, - NULL + NULL, + NULL, + NULL, + NULL, + RegExp_gc_traverse }; static HRESULT alloc_regexp(script_ctx_t *ctx, jsstr_t *str, jsdisp_t *object_prototype, RegExpInstance **ret) diff --git a/dlls/jscript/set.c b/dlls/jscript/set.c index 535ecdc49a4..eca26a890f7 100644 --- a/dlls/jscript/set.c +++ b/dlls/jscript/set.c @@ -362,6 +362,31 @@ static void Map_destructor(jsdisp_t *dispex) free(map); } + +static HRESULT Map_gc_traverse(struct gc_ctx *gc_ctx, enum gc_traverse_op op, jsdisp_t *dispex) +{ + MapInstance *map = (MapInstance*)dispex; + struct jsval_map_entry *entry, *entry2; + HRESULT hres; + + if(op == GC_TRAVERSE_UNLINK) { + LIST_FOR_EACH_ENTRY_SAFE(entry, entry2, &map->entries, struct jsval_map_entry, list_entry) + release_map_entry(entry); + wine_rb_destroy(&map->map, NULL, NULL); + return S_OK; + } + + LIST_FOR_EACH_ENTRY(entry, &map->entries, struct jsval_map_entry, list_entry) { + hres = gc_process_linked_val(gc_ctx, op, dispex, &entry->key); + if(FAILED(hres)) + return hres; + hres = gc_process_linked_val(gc_ctx, op, dispex, &entry->value); + if(FAILED(hres)) + return hres; + } + return S_OK; +} + static const builtin_prop_t Map_prototype_props[] = { {L"clear", Map_clear, PROPF_METHOD}, {L"delete" , Map_delete, PROPF_METHOD|1}, @@ -390,7 +415,11 @@ static const builtin_info_t Map_info = { ARRAY_SIZE(Map_props), Map_props, Map_destructor, - NULL + NULL, + NULL, + NULL, + NULL, + Map_gc_traverse }; static HRESULT Map_constructor(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigned argc, jsval_t *argv, @@ -545,7 +574,11 @@ static const builtin_info_t Set_info = { ARRAY_SIZE(Map_props), Map_props, Map_destructor, - NULL + NULL, + NULL, + NULL, + NULL, + Map_gc_traverse }; static HRESULT Set_constructor(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigned argc, jsval_t *argv, diff --git a/dlls/jscript/tests/run.c b/dlls/jscript/tests/run.c index f40ffb3a84e..7955f345281 100644 --- a/dlls/jscript/tests/run.c +++ b/dlls/jscript/tests/run.c @@ -3446,6 +3446,12 @@ static void test_invokeex(void) static void test_destructors(void) { + static const WCHAR cyclic_refs[] = L"(function() {\n" + "var a = function() {}, c = { 'a': a, 'ref': Math }, b = { 'a': a, 'c': c };\n" + "Math.ref = { 'obj': testDestrObj, 'ref': Math, 'a': a, 'b': b };\n" + "a.ref = { 'ref': Math, 'a': a }; b.ref = Math.ref;\n" + "a.self = a; b.self = b; c.self = c;\n" + "})(), true"; IActiveScript *script; VARIANT v; HRESULT hres; @@ -3461,6 +3467,18 @@ static void test_destructors(void) CHECK_CALLED(testdestrobj); IActiveScript_Release(script); + + V_VT(&v) = VT_EMPTY; + hres = parse_script_expr(cyclic_refs, &v, &script); + ok(hres == S_OK, "parse_script_expr failed: %08lx\n", hres); + ok(V_VT(&v) == VT_BOOL, "V_VT(v) = %d\n", V_VT(&v)); + + SET_EXPECT(testdestrobj); + hres = IActiveScript_SetScriptState(script, SCRIPTSTATE_UNINITIALIZED); + ok(hres == S_OK, "SetScriptState(SCRIPTSTATE_UNINITIALIZED) failed: %08lx\n", hres); + CHECK_CALLED(testdestrobj); + + IActiveScript_Release(script); } static void test_eval(void) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1635
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/jscript/global.c | 5 +---- dlls/jscript/tests/run.c | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/dlls/jscript/global.c b/dlls/jscript/global.c index c0ed954230b..9dd969aa334 100644 --- a/dlls/jscript/global.c +++ b/dlls/jscript/global.c @@ -549,10 +549,7 @@ static HRESULT JSGlobal_ScriptEngineBuildVersion(script_ctx_t *ctx, jsval_t vthi static HRESULT JSGlobal_CollectGarbage(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigned argc, jsval_t *argv, jsval_t *r) { - static int once = 0; - if (!once++) - FIXME(": stub\n"); - return S_OK; + return gc_run(ctx); } static HRESULT JSGlobal_encodeURI(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigned argc, jsval_t *argv, diff --git a/dlls/jscript/tests/run.c b/dlls/jscript/tests/run.c index 7955f345281..21f09474fee 100644 --- a/dlls/jscript/tests/run.c +++ b/dlls/jscript/tests/run.c @@ -3452,6 +3452,7 @@ static void test_destructors(void) "a.ref = { 'ref': Math, 'a': a }; b.ref = Math.ref;\n" "a.self = a; b.self = b; c.self = c;\n" "})(), true"; + IActiveScriptParse *parser; IActiveScript *script; VARIANT v; HRESULT hres; @@ -3479,6 +3480,25 @@ static void test_destructors(void) CHECK_CALLED(testdestrobj); IActiveScript_Release(script); + + V_VT(&v) = VT_EMPTY; + hres = parse_script_expr(cyclic_refs, &v, &script); + ok(hres == S_OK, "parse_script_expr failed: %08lx\n", hres); + ok(V_VT(&v) == VT_BOOL, "V_VT(v) = %d\n", V_VT(&v)); + + hres = IActiveScript_QueryInterface(script, &IID_IActiveScriptParse, (void**)&parser); + ok(hres == S_OK, "Could not get IActiveScriptParse: %08lx\n", hres); + + SET_EXPECT(testdestrobj); + V_VT(&v) = VT_EMPTY; + hres = IActiveScriptParse_ParseScriptText(parser, L"Math.ref = undefined, CollectGarbage(), true", + NULL, NULL, NULL, 0, 0, SCRIPTTEXT_ISEXPRESSION, &v, NULL); + ok(hres == S_OK, "ParseScriptText failed: %08lx\n", hres); + ok(V_VT(&v) == VT_BOOL, "V_VT(v) = %d\n", V_VT(&v)); + IActiveScriptParse_Release(parser); + CHECK_CALLED(testdestrobj); + + IActiveScript_Release(script); } static void test_eval(void) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1635
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Better heuristics can be used in the future. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/jscript/dispex.c | 5 +++++ dlls/jscript/jscript.h | 1 + 2 files changed, 6 insertions(+) diff --git a/dlls/jscript/dispex.c b/dlls/jscript/dispex.c index 875130f7e66..d37f048343b 100644 --- a/dlls/jscript/dispex.c +++ b/dlls/jscript/dispex.c @@ -938,6 +938,7 @@ HRESULT gc_run(script_ctx_t *ctx) jsdisp_release(obj); } + ctx->gc_last_tick = GetTickCount(); return S_OK; } @@ -2138,6 +2139,10 @@ HRESULT init_dispex(jsdisp_t *dispex, script_ctx_t *ctx, const builtin_info_t *b { unsigned i; + /* FIXME: Use better heuristics to decide when to run the GC */ + if(GetTickCount() - ctx->gc_last_tick > 30000) + gc_run(ctx); + TRACE("%p (%p)\n", dispex, prototype); dispex->IDispatchEx_iface.lpVtbl = &DispatchExVtbl; diff --git a/dlls/jscript/jscript.h b/dlls/jscript/jscript.h index 928a7f1ee7e..266928b9b1c 100644 --- a/dlls/jscript/jscript.h +++ b/dlls/jscript/jscript.h @@ -380,6 +380,7 @@ struct _script_ctx_t { jsval_t *stack; unsigned stack_top; + DWORD gc_last_tick; jsval_t acc; jsstr_t *last_match; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1635
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> The 'prototype' prop of a source function is, by default, an empty object with a 'constructor' prop pointing back to the function. Currently, every source function is created in this fashion, which makes it a circular reference and thus prevents it from being freed until the Garbage Collector kicks in. The performance impact comes from the function keeping a ref to the enclosing scope, and since the scope is being held by it, the engine will detach the scope, believing it to be used for the time being (until the GC cleans it). This can cause substantial performance issues for such a common case. The FFXIV Launcher, for example, leaks a large amount of such short-lived functions and the enclosing scopes. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/jscript/function.c | 67 +++++++++++++++++++++++++++++++---------- 1 file changed, 51 insertions(+), 16 deletions(-) diff --git a/dlls/jscript/function.c b/dlls/jscript/function.c index 8aa20b44876..5f210a04050 100644 --- a/dlls/jscript/function.c +++ b/dlls/jscript/function.c @@ -756,6 +756,55 @@ HRESULT create_builtin_constructor(script_ctx_t *ctx, builtin_invoke_t value_pro return S_OK; } +/* + * Create the actual prototype on demand, since it is a circular ref, which prevents the vast + * majority of functions from being released quickly, leading to unnecessary scope detach. + */ +static HRESULT InterpretedFunction_get_prototype(script_ctx_t *ctx, jsdisp_t *jsthis, jsval_t *r) +{ + jsdisp_t *prototype; + HRESULT hres; + + hres = create_object(ctx, NULL, &prototype); + if(FAILED(hres)) + return hres; + + hres = jsdisp_define_data_property(jsthis, L"prototype", PROPF_WRITABLE, jsval_obj(prototype)); + if(SUCCEEDED(hres)) + hres = set_constructor_prop(ctx, jsthis, prototype); + if(FAILED(hres)) { + jsdisp_release(prototype); + return hres; + } + + *r = jsval_obj(prototype); + return S_OK; +} + +static HRESULT InterpretedFunction_set_prototype(script_ctx_t *ctx, jsdisp_t *jsthis, jsval_t value) +{ + return jsdisp_define_data_property(jsthis, L"prototype", PROPF_WRITABLE, value); +} + +static const builtin_prop_t InterpretedFunction_props[] = { + {L"arguments", NULL, 0, Function_get_arguments}, + {L"length", NULL, 0, Function_get_length}, + {L"prototype", NULL, 0, InterpretedFunction_get_prototype, InterpretedFunction_set_prototype} +}; + +static const builtin_info_t InterpretedFunction_info = { + JSCLASS_FUNCTION, + Function_value, + ARRAY_SIZE(InterpretedFunction_props), + InterpretedFunction_props, + Function_destructor, + NULL, + NULL, + NULL, + NULL, + Function_gc_traverse +}; + static HRESULT InterpretedFunction_call(script_ctx_t *ctx, FunctionInstance *func, jsval_t vthis, unsigned flags, unsigned argc, jsval_t *argv, jsval_t *r) { @@ -838,24 +887,10 @@ HRESULT create_source_function(script_ctx_t *ctx, bytecode_t *code, function_cod scope_chain_t *scope_chain, jsdisp_t **ret) { InterpretedFunction *function; - jsdisp_t *prototype; HRESULT hres; - hres = create_object(ctx, NULL, &prototype); - if(FAILED(hres)) - return hres; - - hres = create_function(ctx, NULL, &InterpretedFunctionVtbl, sizeof(InterpretedFunction), PROPF_CONSTR, - FALSE, NULL, (void**)&function); - if(SUCCEEDED(hres)) { - hres = jsdisp_define_data_property(&function->function.dispex, L"prototype", PROPF_WRITABLE, - jsval_obj(prototype)); - if(SUCCEEDED(hres)) - hres = set_constructor_prop(ctx, &function->function.dispex, prototype); - if(FAILED(hres)) - jsdisp_release(&function->function.dispex); - } - jsdisp_release(prototype); + hres = create_function(ctx, &InterpretedFunction_info, &InterpretedFunctionVtbl, sizeof(InterpretedFunction), + PROPF_CONSTR, FALSE, NULL, (void**)&function); if(FAILED(hres)) return hres; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1635
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=127369 Your paranoid android. === debian11 (32 bit report) === ws2_32: afd.c:2045: Test failed: got flags 0x44
CollectGarbage (can't get called by unlinking)
Why? Unlinking calls potentially external `Release()`, which may have any side effect, including `CollectGarbage()` call. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1635#note_19074
On Wed Dec 7 20:46:32 2022 +0000, Gabriel Ivăncescu wrote:
Okay, I skimmed a bit through the GC in gecko (`xpcom/base/nsCycleCollector.cpp`). I don't know if I got it right, but at a high level, it seems to collect them by allocating `PtrInfo`s for each participant ("object") when collecting. It does this for a long time, lifetime of object even (if it's colored "black"), so not just during the GC traversal itself. It seems to use 2 pointers, two 32-bit values, and a mFirstChild iterator (I assume it's pointer-sized?) for each PtrInfo, so that's quite large overhead. With my approach I store a list of objects in the script ctx, so only 2 pointer sized extra per object (for the list entry). The gc_marked flag is free due to padding (and anyway if it wasn't I could make it a bit field). Considering objects are way more common than scopes, this seems like the overhead from the hack of having scopes be objects isn't so bad, since it's offset by the GC storing way less information per object. Well for now I go with my approach since it's far simpler to reason about, but I'm curious about this, since I'm still not entirely sure if the way gecko does it is necessarily an improvement, at least for wine's jscript. Gecko's does work with arbitrary objects, though. So the potential advantage I can see from gecko side: it works for all participants. But I wonder if that's really useful for us and the jscript in particular we use. Since we take care of not holding cyclic refs already in mshtml builtins (non-JS objects), and only arbitrary jscript code can create them that's out of our control, I don't really see how they can end up being created otherwise? So it doesn't seem like much of an advantage at all. What I mean is, we can't mix a mshtml object's refs with jscript one to create cyclic refs between them (well, we can with my JS proxies patches but, then they become JS objects, so it still works because the GC can deal with them), so the potential advantage is not really existent. Only issue I can see, is different script ctx, which can create cyclic refs now due to arbitrary js code doing it between two different script ctx. I think it's not likely in practice, but this could be handled specially though. It's rare enough (maybe doesn't exist at all in the wild) that there's no need to include it in first attempt IMO. Now towards my understanding, and an idea how to get rid of the hack: From what I can see, to avoid an "extra vtbl for each participant", it abuses QueryInterface so it returns a static vtbl (i.e. one in which you can't QI back to original object anymore). I mean `IID_nsXPCOMCycleCollectionParticipant`. Am I right? But setting aside whether that's acceptable in wine or not, the thing is we don't have an interface for `scope_chain_t` at all, so it would still need one. If it was, do you think I can do something like that "abused" QI (or extend the object's IDispatchEx itself with an extra internal method) to get rid of the jsdisp thing with `scope_chain_t`? We'd still need a dummy interface (for the QI/extra method) for `scope_chain_t`, though. But at least, the jsdisp would re-use its own. Sorry for the long winded reply. :) I meant something like cycle collector participant. Note that only one per object type is needed, not one per object instance. In practice, when annotating a reference, when we know the type of referenced thing, we may pass that information to CC/GC. It doesn't even need to be accessible from object itself (although it could be convenient and having IUnknown for scope chain doesn't sound too bad).
Since we take care of not holding cyclic refs already in mshtml builtins (non-JS objects)
We have some cycles, that's how nsIDOMNode <-> HTMLNode works, for example. We depend on cycle collector to handle it.
What I mean is, we can't mix a mshtml object's refs with jscript one to create cyclic refs between them
Of course we can, here is an example: ``` var div = document.createElement("div"), div2 = document.createElement("div"); div.appendChild(div2); div2.prop = { d: div }; ``` The whole point of integrating it is to handle such cases. We already have CC integration, so something like following example will be collected (and potentially broken by your proxies; I didn't look at recent version, but I recall pointing it out in the very early version of your patches): ``` var div = document.createElement("div"), div2 = document.createElement("div"); div.appendChild(div2); div2.prop = div; ``` But to fix the first example, GC/CC needs to be able to handle all kinds of objects: JS, MSHTML and Gecko. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1635#note_19084
On Wed Dec 7 21:30:36 2022 +0000, Jacek Caban wrote:
CollectGarbage (can't get called by unlinking) Why? Unlinking calls potentially external `Release()`, which may have any side effect, including `CollectGarbage()` call. Oops, I really hadn't thought of that. :D
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1635#note_19179
participants (3)
-
Gabriel Ivăncescu -
Jacek Caban (＠jacek)
-
Marvin