[PATCH v4 0/8] MR2942: jscript: Misc fixes and improvements.
Some of these fixes are subtle (like the first patch) and very annoying to debug. Although the first patch looks like a hack, surprisingly, it's how the spec itself says it is! It's not even an IE quirk, but a special case in the spec. For example, the variable name (which holds the builtin eval func) **does** matter: if it's called something other than 'eval', it gets treated differently (as if indirect), and this is verified by the tests + the spec's wording (so Microsoft's implementation follows it). Most of the patches other than the first 2 are pretty small so they're in same MR. -- v4: jscript: Store ref to the function code instead of the function instance jscript: Start from the last argument when adding them to named locals. jscript: Always store the passed arguments into the argument obj, if not jscript: Don't use iface_to_jsdisp where it's not necessary to grab it. jscript: Get rid of jsobj in scope_chain_t. jscript: Move arguments_obj from the frame to the base scope. jscript: Fix addressing invalid memory if ref is an argument. jscript: Correctly implement context for indirect eval calls in ES5+ modes. https://gitlab.winehq.org/wine/wine/-/merge_requests/2942
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/jscript/compile.c | 14 ++++++--- dlls/jscript/engine.c | 34 +++++++++++++++++++++ dlls/jscript/engine.h | 1 + dlls/jscript/function.c | 6 ++++ dlls/jscript/global.c | 9 ++++-- dlls/jscript/jscript.h | 2 ++ dlls/jscript/tests/api.js | 43 ++++++++++++++++++++++++++ dlls/mshtml/tests/documentmode.js | 51 +++++++++++++++++++++++++++++++ 8 files changed, 153 insertions(+), 7 deletions(-) diff --git a/dlls/jscript/compile.c b/dlls/jscript/compile.c index 9d7944fcf72..e2690935212 100644 --- a/dlls/jscript/compile.c +++ b/dlls/jscript/compile.c @@ -720,16 +720,20 @@ static HRESULT compile_new_expression(compiler_ctx_t *ctx, call_expression_t *ex static HRESULT compile_call_expression(compiler_ctx_t *ctx, call_expression_t *expr, BOOL emit_ret) { - unsigned arg_cnt = 0, extra_args; + unsigned arg_cnt = 0, extra_args = 0; + HRESULT hres = S_OK; argument_t *arg; unsigned instr; jsop_t op; - HRESULT hres; if(is_memberid_expr(expr->expression->type)) { - op = OP_call_member; - extra_args = 2; - hres = compile_memberid_expression(ctx, expr->expression, 0); + if(expr->expression->type == EXPR_IDENT && !wcscmp(((identifier_expression_t*)expr->expression)->identifier, L"eval")) + op = OP_call_eval; + else { + op = OP_call_member; + extra_args = 2; + hres = compile_memberid_expression(ctx, expr->expression, 0); + } }else { op = OP_call; extra_args = 1; diff --git a/dlls/jscript/engine.c b/dlls/jscript/engine.c index b0557065e69..9e375294cb5 100644 --- a/dlls/jscript/engine.c +++ b/dlls/jscript/engine.c @@ -1459,6 +1459,40 @@ static HRESULT interp_call_member(script_ctx_t *ctx) argn, stack_args(ctx, argn), do_ret ? &ctx->acc : NULL); } +/* ECMA-262 5th Edition 15.1.2.1.1 */ +static HRESULT interp_call_eval(script_ctx_t *ctx) +{ + const unsigned argn = get_op_uint(ctx, 0); + const int do_ret = get_op_int(ctx, 1); + HRESULT hres = S_OK; + exprval_t exprval; + jsdisp_t *jsdisp; + BSTR identifier; + jsval_t v; + + TRACE("%d %d\n", argn, do_ret); + + identifier = SysAllocString(L"eval"); + hres = identifier_eval(ctx, identifier, &exprval); + SysFreeString(identifier); + if(FAILED(hres)) + return hres; + + clear_acc(ctx); + hres = exprval_propget(ctx, &exprval, &v); + if(SUCCEEDED(hres)) { + if(is_object_instance(v) && (jsdisp = to_jsdisp(get_object(v))) && jsdisp->ctx == ctx && is_builtin_eval_func(jsdisp)) + hres = builtin_eval(ctx, ctx->call_ctx, DISPATCH_METHOD | DISPATCH_JSCRIPT_CALLEREXECSSOURCE, + argn, stack_args(ctx, argn), do_ret ? &ctx->acc : NULL); + else + hres = exprval_call(ctx, &exprval, DISPATCH_METHOD | DISPATCH_JSCRIPT_CALLEREXECSSOURCE, + argn, stack_args(ctx, argn), do_ret ? &ctx->acc : NULL); + jsval_release(v); + } + exprval_release(&exprval); + return hres; +} + /* ECMA-262 3rd Edition 11.1.1 */ static HRESULT interp_this(script_ctx_t *ctx) { diff --git a/dlls/jscript/engine.h b/dlls/jscript/engine.h index f8046cbaafb..987e1cfa52d 100644 --- a/dlls/jscript/engine.h +++ b/dlls/jscript/engine.h @@ -25,6 +25,7 @@ X(bool, 1, ARG_INT, 0) \ X(bneg, 1, 0,0) \ X(call, 1, ARG_UINT, ARG_UINT) \ + X(call_eval, 1, ARG_UINT, ARG_UINT) \ X(call_member,1, ARG_UINT, ARG_UINT) \ X(carray, 1, ARG_UINT, 0) \ X(carray_set, 1, ARG_UINT, 0) \ diff --git a/dlls/jscript/function.c b/dlls/jscript/function.c index 5f210a04050..018fd3955db 100644 --- a/dlls/jscript/function.c +++ b/dlls/jscript/function.c @@ -1140,6 +1140,12 @@ static HRESULT FunctionProt_value(script_ctx_t *ctx, jsval_t vthis, WORD flags, return E_NOTIMPL; } +BOOL is_builtin_eval_func(jsdisp_t *jsdisp) +{ + return is_class(jsdisp, JSCLASS_FUNCTION) && function_from_jsdisp(jsdisp)->vtbl == &NativeFunctionVtbl && + ((NativeFunction*)function_from_jsdisp(jsdisp))->proc == JSGlobal_eval; +} + HRESULT init_function_constr(script_ctx_t *ctx, jsdisp_t *object_prototype) { NativeFunction *prot, *constr; diff --git a/dlls/jscript/global.c b/dlls/jscript/global.c index 9dd969aa334..997b2542a9e 100644 --- a/dlls/jscript/global.c +++ b/dlls/jscript/global.c @@ -130,10 +130,9 @@ static HRESULT JSGlobal_escape(script_ctx_t *ctx, jsval_t vthis, WORD flags, uns } /* ECMA-262 3rd Edition 15.1.2.1 */ -HRESULT JSGlobal_eval(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigned argc, jsval_t *argv, +HRESULT builtin_eval(script_ctx_t *ctx, call_frame_t *frame, WORD flags, unsigned argc, jsval_t *argv, jsval_t *r) { - call_frame_t *frame = ctx->call_ctx; DWORD exec_flags = EXEC_EVAL; bytecode_t *code; const WCHAR *src; @@ -174,6 +173,12 @@ HRESULT JSGlobal_eval(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigned arg return hres; } +HRESULT JSGlobal_eval(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigned argc, jsval_t *argv, + jsval_t *r) +{ + return builtin_eval(ctx, ctx->version < SCRIPTLANGUAGEVERSION_ES5 ? ctx->call_ctx : NULL, flags, argc, argv, r); +} + static HRESULT JSGlobal_isNaN(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigned argc, jsval_t *argv, jsval_t *r) { diff --git a/dlls/jscript/jscript.h b/dlls/jscript/jscript.h index 06c446b0c76..e91aa6dfabd 100644 --- a/dlls/jscript/jscript.h +++ b/dlls/jscript/jscript.h @@ -464,6 +464,8 @@ BOOL bool_obj_value(jsdisp_t*) DECLSPEC_HIDDEN; unsigned array_get_length(jsdisp_t*) DECLSPEC_HIDDEN; HRESULT localize_number(script_ctx_t*,DOUBLE,BOOL,jsstr_t**) DECLSPEC_HIDDEN; +BOOL is_builtin_eval_func(jsdisp_t*) DECLSPEC_HIDDEN; +HRESULT builtin_eval(script_ctx_t*,struct _call_frame_t*,WORD,unsigned,jsval_t*,jsval_t*) DECLSPEC_HIDDEN; HRESULT JSGlobal_eval(script_ctx_t*,jsval_t,WORD,unsigned,jsval_t*,jsval_t*) DECLSPEC_HIDDEN; HRESULT Object_get_proto_(script_ctx_t*,jsval_t,WORD,unsigned,jsval_t*,jsval_t*) DECLSPEC_HIDDEN; HRESULT Object_set_proto_(script_ctx_t*,jsval_t,WORD,unsigned,jsval_t*,jsval_t*) DECLSPEC_HIDDEN; diff --git a/dlls/jscript/tests/api.js b/dlls/jscript/tests/api.js index a3b2bbcbba5..481e0612be5 100644 --- a/dlls/jscript/tests/api.js +++ b/dlls/jscript/tests/api.js @@ -2446,6 +2446,49 @@ ok(bool.toLocaleString() === bool.toString(), "bool.toLocaleString() = " + bool. tmp = Object.prototype.valueOf.call(nullDisp); ok(tmp === nullDisp, "nullDisp.valueOf != nullDisp"); +(function(global) { + var i, context, code = "this.foobar = 1234"; + + var direct = [ + function() { eval(code); }, + function() { (eval)(code); }, + function() { (function(eval) { eval(code); }).call(this, eval); }, + function() { eval("eval(" + code + ")"); } + ]; + + for(i = 0; i < direct.length; i++) { + context = {}; + direct[i].call(context); + ok(context.foobar === 1234, "direct[" + i + "] context foobar = " + context.foobar); + } + + var indirect = [ + function() { (true, eval)(code); }, + function() { (eval, eval)(code); }, + function() { (true ? eval : false)(code); }, + function() { [eval][0](code); }, + function() { eval.call(this, code); }, + function() { var f; (f = eval)(code); }, + function() { var f = eval; f(code); }, + function() { (function(f) { f(code); }).call(this, eval); }, + function() { (function(f) { return f; }).call(this, eval)(code); }, + function() { (function() { arguments[0](code) }).call(this, eval); }, + function() { eval("eval")(code); } + ]; + + for(i = 0; i < indirect.length; i++) { + context = {}; + ok(!("foobar" in global), "indirect[" + i + "] has global foobar before call"); + indirect[i].call(context); + ok(context.foobar === 1234, "indirect[" + i + "] context foobar = " + context.foobar); + ok(!("foobar" in global), "indirect[" + i + "] has global foobar"); + } + + context = {}; + (function(eval) { eval(code); })(function() { context.barfoo = 4321; }); + ok(context.barfoo === 4321, "context.barfoo = " + context.barfoo); +})(this); + ok(ActiveXObject instanceof Function, "ActiveXObject is not instance of Function"); ok(ActiveXObject.prototype instanceof Object, "ActiveXObject.prototype is not instance of Object"); diff --git a/dlls/mshtml/tests/documentmode.js b/dlls/mshtml/tests/documentmode.js index 1a9f36927d0..9007d451bdb 100644 --- a/dlls/mshtml/tests/documentmode.js +++ b/dlls/mshtml/tests/documentmode.js @@ -756,6 +756,57 @@ sync_test("JS objs", function() { test_parses("if(false) { o.if; }", v >= 9); }); +sync_test("eval", function() { + var i, context, code = "this.foobar = 1234", v = document.documentMode; + + var direct = [ + function() { eval(code); }, + function() { (eval)(code); }, + function() { (function(eval) { eval(code); }).call(this, eval); }, + function() { eval("eval(" + code + ")"); } + ]; + + for(i = 0; i < direct.length; i++) { + context = {}; + direct[i].call(context); + ok(context.foobar === 1234, "direct[" + i + "] context foobar = " + context.foobar); + } + + var indirect = [ + function() { (true, eval)(code); }, + function() { (eval, eval)(code); }, + function() { (true ? eval : false)(code); }, + function() { [eval][0](code); }, + function() { eval.call(this, code); }, + function() { var f; (f = eval)(code); }, + function() { var f = eval; f(code); }, + function() { (function(f) { f(code); }).call(this, eval); }, + function() { (function(f) { return f; }).call(this, eval)(code); }, + function() { (function() { arguments[0](code) }).call(this, eval); }, + function() { window.eval(code); }, + function() { window["eval"](code); }, + function() { eval("eval")(code); } + ]; + + for(i = 0; i < indirect.length; i++) { + context = {}; + ok(!("foobar" in window), "indirect[" + i + "] has global foobar before call"); + indirect[i].call(context); + if(v < 9) { + ok(context.foobar === 1234, "indirect[" + i + "] context foobar = " + context.foobar); + ok(!("foobar" in window), "indirect[" + i + "] has global foobar"); + }else { + ok(!("foobar" in context), "indirect[" + i + "] has foobar"); + ok(window.foobar === 1234, "indirect[" + i + "] global foobar = " + context.foobar); + delete window.foobar; + } + } + + context = {}; + (function(eval) { eval(code); })(function() { context.barfoo = 4321; }); + ok(context.barfoo === 4321, "context.barfoo = " + context.barfoo); +}); + sync_test("for..in", function() { var v = document.documentMode, found = 0, r; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2942
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> `ref` can be negative in case it refers to an argument. Even though scope != frame->base_scope would rule this out (because only base scopes have args), it was checked *after* the memory access, which would read out of bounds memory first. This didn't appear as an issue in practice since it's using the heap pool, so there's probably valid memory before it, but it's still wrong. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/jscript/engine.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/jscript/engine.c b/dlls/jscript/engine.c index 9e375294cb5..a4b416ba8ed 100644 --- a/dlls/jscript/engine.c +++ b/dlls/jscript/engine.c @@ -657,7 +657,7 @@ static HRESULT detach_scope(script_ctx_t *ctx, call_frame_t *frame, scope_chain_ if (FAILED(hres = jsdisp_propput_name(scope->jsobj, name, ctx->stack[local_off(frame, ref)]))) return hres; - if (frame->function->variables[ref].func_id != -1 && scope != frame->base_scope + if (scope != frame->base_scope && frame->function->variables[ref].func_id != -1 && FAILED(hres = jsdisp_propput_name(frame->variable_obj, name, ctx->stack[local_off(frame, ref)]))) return hres; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2942
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> We will need it when looking up arguments by name. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/jscript/engine.c | 32 ++++++++++++++++++++------------ dlls/jscript/engine.h | 4 ++-- dlls/jscript/function.c | 14 +++++++++----- 3 files changed, 31 insertions(+), 19 deletions(-) diff --git a/dlls/jscript/engine.c b/dlls/jscript/engine.c index a4b416ba8ed..393e5429d3a 100644 --- a/dlls/jscript/engine.c +++ b/dlls/jscript/engine.c @@ -428,7 +428,8 @@ static void scope_destructor(jsdisp_t *dispex) if(scope->next) scope_release(scope->next); - + if(scope->arguments_obj) + jsdisp_release(scope->arguments_obj); if(scope->obj) IDispatch_Release(scope->obj); free(scope); @@ -445,6 +446,12 @@ static HRESULT scope_gc_traverse(struct gc_ctx *gc_ctx, enum gc_traverse_op op, return hres; } + if(scope->arguments_obj) { + hres = gc_process_linked_obj(gc_ctx, op, dispex, scope->arguments_obj, (void**)&scope->arguments_obj); + if(FAILED(hres)) + return hres; + } + if(op == GC_TRAVERSE_UNLINK) { IDispatch *obj = scope->obj; if(obj) { @@ -689,7 +696,7 @@ static HRESULT detach_variable_object(script_ctx_t *ctx, call_frame_t *frame, BO assert(frame == frame->base_scope->frame); assert(frame->variable_obj == frame->base_scope->jsobj); - if(!from_release && !frame->arguments_obj) { + if(!from_release && !frame->base_scope->arguments_obj) { hres = setup_arguments_object(ctx, frame); if(FAILED(hres)) return hres; @@ -2987,23 +2994,24 @@ OP_LIST static void pop_call_frame(script_ctx_t *ctx) { call_frame_t *frame = ctx->call_ctx; + scope_chain_t *scope = frame->scope; frame->stack_base -= frame->pop_locals + frame->pop_variables; 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->dispex.ref > 1) { - HRESULT hres = detach_variable_object(ctx, frame, TRUE); - if(FAILED(hres)) - ERR("Failed to detach variable object: %08lx\n", hres); + if(scope) { + /* If current scope will be kept alive, we need to transfer local variables to its variable object. */ + if(scope->dispex.ref > 1) { + HRESULT hres = detach_variable_object(ctx, frame, TRUE); + if(FAILED(hres)) + ERR("Failed to detach variable object: %08lx\n", hres); + } + if(scope->arguments_obj) + detach_arguments_object(scope->arguments_obj, frame); + scope_release(scope); } - if(frame->arguments_obj) - detach_arguments_object(frame->arguments_obj); - if(frame->scope) - scope_release(frame->scope); - if(frame->pop_variables) stack_popn(ctx, frame->pop_variables); stack_popn(ctx, frame->pop_locals); diff --git a/dlls/jscript/engine.h b/dlls/jscript/engine.h index 987e1cfa52d..6a7a16d2923 100644 --- a/dlls/jscript/engine.h +++ b/dlls/jscript/engine.h @@ -226,6 +226,7 @@ typedef struct _scope_chain_t { jsdisp_t dispex; /* FIXME: don't wrap it in a jsdisp (it holds ref and traverse for the garbage collector) */ jsdisp_t *jsobj; IDispatch *obj; + jsdisp_t *arguments_obj; unsigned int scope_index; struct _call_frame_t *frame; struct _scope_chain_t *next; @@ -279,7 +280,6 @@ typedef struct _call_frame_t { IDispatch *this_obj; jsdisp_t *function_instance; jsdisp_t *variable_obj; - jsdisp_t *arguments_obj; DWORD flags; unsigned argc; @@ -304,4 +304,4 @@ HRESULT exec_source(script_ctx_t*,DWORD,bytecode_t*,function_code_t*,scope_chain HRESULT create_source_function(script_ctx_t*,bytecode_t*,function_code_t*,scope_chain_t*,jsdisp_t**) DECLSPEC_HIDDEN; HRESULT setup_arguments_object(script_ctx_t*,call_frame_t*) DECLSPEC_HIDDEN; -void detach_arguments_object(jsdisp_t*) DECLSPEC_HIDDEN; +void detach_arguments_object(jsdisp_t*,call_frame_t*) DECLSPEC_HIDDEN; diff --git a/dlls/jscript/function.c b/dlls/jscript/function.c index 018fd3955db..f0677948093 100644 --- a/dlls/jscript/function.c +++ b/dlls/jscript/function.c @@ -233,20 +233,24 @@ HRESULT setup_arguments_object(script_ctx_t *ctx, call_frame_t *frame) return hres; } - frame->arguments_obj = &args->jsdisp; + frame->base_scope->arguments_obj = &args->jsdisp; return S_OK; } -void detach_arguments_object(jsdisp_t *args_disp) +void detach_arguments_object(jsdisp_t *args_disp, call_frame_t *popped_frame) { ArgumentsInstance *arguments = arguments_from_jsdisp(args_disp); call_frame_t *frame = arguments->frame; const BOOL on_stack = frame->base_scope->frame == frame; HRESULT hres; + if(frame != popped_frame) + return; + /* Reset arguments value to cut the reference cycle. Note that since all activation contexts have * their own arguments property, it's impossible to use prototype's one during name lookup */ jsdisp_propput_name(frame->base_scope->jsobj, L"arguments", jsval_undefined()); + frame->base_scope->arguments_obj = NULL; arguments->frame = NULL; /* Don't bother coppying arguments if call frame holds the last reference. */ @@ -269,7 +273,7 @@ void detach_arguments_object(jsdisp_t *args_disp) } } - jsdisp_release(frame->arguments_obj); + jsdisp_release(&arguments->jsdisp); } HRESULT Function_invoke(jsdisp_t *func_this, jsval_t vthis, WORD flags, unsigned argc, jsval_t *argv, jsval_t *r) @@ -542,12 +546,12 @@ static HRESULT Function_get_arguments(script_ctx_t *ctx, jsdisp_t *jsthis, jsval for(frame = ctx->call_ctx; frame; frame = frame->prev_frame) { if(frame->function_instance == &function->dispex) { - if(!frame->arguments_obj) { + if(!frame->base_scope->arguments_obj) { hres = setup_arguments_object(ctx, frame); if(FAILED(hres)) return hres; } - *r = jsval_obj(jsdisp_addref(frame->arguments_obj)); + *r = jsval_obj(jsdisp_addref(frame->base_scope->arguments_obj)); return S_OK; } } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2942
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> It was confusing and aliased to obj when it was a jsdisp (and shared ref), but we can obtain that already with helpers as needed (as_jsdisp and to_jsdisp), no reason to keep it so confusing and a separate field. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/jscript/engine.c | 57 +++++++++++++++++++++-------------------- dlls/jscript/engine.h | 1 - dlls/jscript/function.c | 12 +++++---- 3 files changed, 36 insertions(+), 34 deletions(-) diff --git a/dlls/jscript/engine.c b/dlls/jscript/engine.c index 393e5429d3a..3becd1fc042 100644 --- a/dlls/jscript/engine.c +++ b/dlls/jscript/engine.c @@ -207,6 +207,7 @@ static BOOL stack_topn_exprval(script_ctx_t *ctx, unsigned n, exprval_t *r) unsigned off = get_number(v); if(!frame->base_scope->frame && off >= frame->arguments_off) { + jsdisp_t *jsobj; DISPID id; BSTR name; HRESULT hres = E_FAIL; @@ -227,7 +228,7 @@ static BOOL stack_topn_exprval(script_ctx_t *ctx, unsigned n, exprval_t *r) while (1) { - if (scope->jsobj && SUCCEEDED(hres = jsdisp_get_id(scope->jsobj, name, 0, &id))) + if ((jsobj = to_jsdisp(scope->obj)) && SUCCEEDED(hres = jsdisp_get_id(jsobj, name, 0, &id))) break; if (scope == frame->base_scope) { @@ -238,10 +239,10 @@ static BOOL stack_topn_exprval(script_ctx_t *ctx, unsigned n, exprval_t *r) scope = scope->next; } - *stack_top_ref(ctx, n+1) = jsval_obj(jsdisp_addref(scope->jsobj)); + *stack_top_ref(ctx, n+1) = jsval_obj(jsdisp_addref(jsobj)); *stack_top_ref(ctx, n) = jsval_number(id); r->type = EXPRVAL_IDREF; - r->u.idref.disp = scope->obj; + r->u.idref.disp = to_disp(jsobj); r->u.idref.id = id; return TRUE; } @@ -438,6 +439,7 @@ static void scope_destructor(jsdisp_t *dispex) 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); + jsdisp_t *jsobj; HRESULT hres; if(scope->next) { @@ -461,7 +463,7 @@ static HRESULT scope_gc_traverse(struct gc_ctx *gc_ctx, enum gc_traverse_op op, return S_OK; } - return scope->jsobj ? gc_process_linked_obj(gc_ctx, op, dispex, scope->jsobj, (void**)&scope->obj) : S_OK; + return scope->obj && (jsobj = to_jsdisp(scope->obj)) ? gc_process_linked_obj(gc_ctx, op, dispex, jsobj, (void**)&scope->obj) : S_OK; } static const builtin_info_t scope_info = { @@ -477,7 +479,7 @@ static const builtin_info_t scope_info = { scope_gc_traverse }; -static HRESULT scope_push(script_ctx_t *ctx, scope_chain_t *scope, jsdisp_t *jsobj, IDispatch *obj, scope_chain_t **ret) +static HRESULT scope_push(script_ctx_t *ctx, scope_chain_t *scope, IDispatch *obj, scope_chain_t **ret) { scope_chain_t *new_scope; HRESULT hres; @@ -494,7 +496,6 @@ static HRESULT scope_push(script_ctx_t *ctx, scope_chain_t *scope, jsdisp_t *jso if (obj) IDispatch_AddRef(obj); - new_scope->jsobj = jsobj; new_scope->obj = obj; new_scope->frame = NULL; new_scope->next = scope ? scope_addref(scope) : NULL; @@ -635,6 +636,7 @@ static HRESULT detach_scope(script_ctx_t *ctx, call_frame_t *frame, scope_chain_ { function_code_t *func = frame->function; unsigned int i, index; + jsdisp_t *jsobj; HRESULT hres; if (!scope->frame) @@ -643,18 +645,18 @@ static HRESULT detach_scope(script_ctx_t *ctx, call_frame_t *frame, scope_chain_ assert(scope->frame == frame); scope->frame = NULL; - if (!scope->jsobj) + if (!scope->obj) { - assert(!scope->obj); - - if (FAILED(hres = create_object(ctx, NULL, &scope->jsobj))) + if (FAILED(hres = create_object(ctx, NULL, &jsobj))) return hres; - scope->obj = to_disp(scope->jsobj); + scope->obj = to_disp(jsobj); } + else + jsobj = as_jsdisp(scope->obj); if (scope == frame->base_scope && func->name && func->local_ref == INVALID_LOCAL_REF && ctx->version >= SCRIPTLANGUAGEVERSION_ES5) - jsdisp_propput_name(scope->jsobj, func->name, jsval_obj(jsdisp_addref(frame->function_instance))); + jsdisp_propput_name(jsobj, func->name, jsval_obj(jsdisp_addref(frame->function_instance))); index = scope->scope_index; for(i = 0; i < frame->function->local_scopes[index].locals_cnt; i++) @@ -662,7 +664,7 @@ static HRESULT detach_scope(script_ctx_t *ctx, call_frame_t *frame, scope_chain_ WCHAR *name = frame->function->local_scopes[index].locals[i].name; int ref = frame->function->local_scopes[index].locals[i].ref; - if (FAILED(hres = jsdisp_propput_name(scope->jsobj, name, ctx->stack[local_off(frame, ref)]))) + if (FAILED(hres = jsdisp_propput_name(jsobj, name, ctx->stack[local_off(frame, ref)]))) return hres; if (scope != frame->base_scope && frame->function->variables[ref].func_id != -1 && FAILED(hres = jsdisp_propput_name(frame->variable_obj, name, ctx->stack[local_off(frame, ref)]))) @@ -694,7 +696,7 @@ static HRESULT detach_variable_object(script_ctx_t *ctx, call_frame_t *frame, BO TRACE("detaching %p\n", frame); assert(frame == frame->base_scope->frame); - assert(frame->variable_obj == frame->base_scope->jsobj); + assert(to_disp(frame->variable_obj) == frame->base_scope->obj); if(!from_release && !frame->base_scope->arguments_obj) { hres = setup_arguments_object(ctx, frame); @@ -791,13 +793,10 @@ static HRESULT identifier_eval(script_ctx_t *ctx, BSTR identifier, exprval_t *re } } - if (!scope->jsobj && !scope->obj) + if (!scope->obj) continue; - if(scope->jsobj) - hres = jsdisp_get_id(scope->jsobj, identifier, fdexNameImplicit, &id); - else - hres = disp_get_id(ctx, scope->obj, identifier, identifier, fdexNameImplicit, &id); + hres = disp_get_id(ctx, scope->obj, identifier, identifier, fdexNameImplicit, &id); if(SUCCEEDED(hres)) { exprval_set_disp_ref(ret, scope->obj, id); return S_OK; @@ -950,6 +949,7 @@ static HRESULT scope_init_locals(script_ctx_t *ctx) unsigned int i, off, index; scope_chain_t *scope; BOOL detached_vars; + jsdisp_t *jsobj; HRESULT hres; scope = frame->scope; @@ -961,13 +961,14 @@ static HRESULT scope_init_locals(script_ctx_t *ctx) assert(frame->base_scope->frame == frame); frame->scope->frame = ctx->call_ctx; } - else if (!scope->jsobj) + else if (!scope->obj) { - assert(!scope->obj); - if (FAILED(hres = create_object(ctx, NULL, &scope->jsobj))) + if (FAILED(hres = create_object(ctx, NULL, &jsobj))) return hres; - scope->obj = to_disp(scope->jsobj); + scope->obj = to_disp(jsobj); } + else + jsobj = as_jsdisp(scope->obj); for(i = 0; i < frame->function->local_scopes[index].locals_cnt; i++) { @@ -994,7 +995,7 @@ static HRESULT scope_init_locals(script_ctx_t *ctx) if (detached_vars) { - if (FAILED(hres = jsdisp_propput_name(scope->jsobj, name, val))) + if (FAILED(hres = jsdisp_propput_name(jsobj, name, val))) return hres; } else @@ -1022,7 +1023,7 @@ static HRESULT interp_push_with_scope(script_ctx_t *ctx) if(FAILED(hres)) return hres; - hres = scope_push(ctx, ctx->call_ctx->scope, to_jsdisp(disp), disp, &ctx->call_ctx->scope); + hres = scope_push(ctx, ctx->call_ctx->scope, disp, &ctx->call_ctx->scope); IDispatch_Release(disp); return hres; } @@ -1036,7 +1037,7 @@ static HRESULT interp_push_block_scope(script_ctx_t *ctx) TRACE("scope_index %u.\n", scope_index); - hres = scope_push(ctx, ctx->call_ctx->scope, NULL, NULL, &frame->scope); + hres = scope_push(ctx, ctx->call_ctx->scope, NULL, &frame->scope); if (FAILED(hres) || !scope_index) return hres; @@ -1248,7 +1249,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, ctx->call_ctx->scope, scope_obj, to_disp(scope_obj), &ctx->call_ctx->scope); + hres = scope_push(ctx, ctx->call_ctx->scope, to_disp(scope_obj), &ctx->call_ctx->scope); jsdisp_release(scope_obj); return hres; } @@ -3273,7 +3274,7 @@ static HRESULT setup_scope(script_ctx_t *ctx, call_frame_t *frame, scope_chain_t frame->pop_variables = i; - hres = scope_push(ctx, scope_chain, variable_object, to_disp(variable_object), &scope); + hres = scope_push(ctx, scope_chain, 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 6a7a16d2923..3688972f29f 100644 --- a/dlls/jscript/engine.h +++ b/dlls/jscript/engine.h @@ -224,7 +224,6 @@ static inline bytecode_t *bytecode_addref(bytecode_t *code) typedef struct _scope_chain_t { jsdisp_t dispex; /* FIXME: don't wrap it in a jsdisp (it holds ref and traverse for the garbage collector) */ - jsdisp_t *jsobj; IDispatch *obj; jsdisp_t *arguments_obj; unsigned int scope_index; diff --git a/dlls/jscript/function.c b/dlls/jscript/function.c index f0677948093..b1d7e249923 100644 --- a/dlls/jscript/function.c +++ b/dlls/jscript/function.c @@ -145,7 +145,7 @@ static HRESULT Arguments_idx_get(jsdisp_t *jsdisp, unsigned idx, jsval_t *r) return jsval_copy(*ref, r); /* FIXME: Accessing by name won't work for duplicated argument names */ - return jsdisp_propget_name(arguments->frame->base_scope->jsobj, + return jsdisp_propget_name(as_jsdisp(arguments->frame->base_scope->obj), arguments->function->func_code->params[idx], r); } @@ -169,7 +169,7 @@ static HRESULT Arguments_idx_put(jsdisp_t *jsdisp, unsigned idx, jsval_t val) } /* FIXME: Accessing by name won't work for duplicated argument names */ - return jsdisp_propput_name(arguments->frame->base_scope->jsobj, + return jsdisp_propput_name(as_jsdisp(arguments->frame->base_scope->obj), arguments->function->func_code->params[idx], val); } @@ -227,7 +227,7 @@ HRESULT setup_arguments_object(script_ctx_t *ctx, call_frame_t *frame) hres = jsdisp_define_data_property(&args->jsdisp, L"callee", PROPF_WRITABLE | PROPF_CONFIGURABLE, jsval_obj(&args->function->function.dispex)); if(SUCCEEDED(hres)) - hres = jsdisp_propput(frame->base_scope->jsobj, L"arguments", PROPF_WRITABLE, TRUE, jsval_obj(&args->jsdisp)); + hres = jsdisp_propput(as_jsdisp(frame->base_scope->obj), L"arguments", PROPF_WRITABLE, TRUE, jsval_obj(&args->jsdisp)); if(FAILED(hres)) { jsdisp_release(&args->jsdisp); return hres; @@ -242,14 +242,16 @@ void detach_arguments_object(jsdisp_t *args_disp, call_frame_t *popped_frame) ArgumentsInstance *arguments = arguments_from_jsdisp(args_disp); call_frame_t *frame = arguments->frame; const BOOL on_stack = frame->base_scope->frame == frame; + jsdisp_t *jsobj; HRESULT hres; if(frame != popped_frame) return; + jsobj = as_jsdisp(frame->base_scope->obj); /* Reset arguments value to cut the reference cycle. Note that since all activation contexts have * their own arguments property, it's impossible to use prototype's one during name lookup */ - jsdisp_propput_name(frame->base_scope->jsobj, L"arguments", jsval_undefined()); + jsdisp_propput_name(jsobj, L"arguments", jsval_undefined()); frame->base_scope->arguments_obj = NULL; arguments->frame = NULL; @@ -263,7 +265,7 @@ void detach_arguments_object(jsdisp_t *args_disp, call_frame_t *popped_frame) if(on_stack || i >= frame->function->param_cnt) hres = jsval_copy(arguments->jsdisp.ctx->stack[frame->arguments_off + i], arguments->buf+i); else - hres = jsdisp_propget_name(frame->base_scope->jsobj, frame->function->params[i], arguments->buf+i); + hres = jsdisp_propget_name(jsobj, frame->function->params[i], arguments->buf+i); if(FAILED(hres)) arguments->buf[i] = jsval_undefined(); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2942
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/jscript/array.c | 3 +-- dlls/jscript/dispex.c | 3 +-- dlls/jscript/engine.c | 20 ++++++-------------- dlls/jscript/json.c | 3 +-- dlls/jscript/jsregexp.c | 16 ++++------------ 5 files changed, 13 insertions(+), 32 deletions(-) diff --git a/dlls/jscript/array.c b/dlls/jscript/array.c index 341505a335c..4e1af864a8e 100644 --- a/dlls/jscript/array.c +++ b/dlls/jscript/array.c @@ -1663,9 +1663,8 @@ static HRESULT ArrayConstr_isArray(script_ctx_t *ctx, jsval_t vthis, WORD flags, return S_OK; } - obj = iface_to_jsdisp(get_object(argv[0])); + obj = to_jsdisp(get_object(argv[0])); if(r) *r = jsval_bool(obj && is_class(obj, JSCLASS_ARRAY)); - if(obj) jsdisp_release(obj); return S_OK; } diff --git a/dlls/jscript/dispex.c b/dlls/jscript/dispex.c index c3cc41ce782..e8f7d599463 100644 --- a/dlls/jscript/dispex.c +++ b/dlls/jscript/dispex.c @@ -101,10 +101,9 @@ static inline BOOL is_function_prop(dispex_prop_t *prop) if (is_object_instance(prop->u.val)) { - jsdisp_t *jsdisp = iface_to_jsdisp(get_object(prop->u.val)); + jsdisp_t *jsdisp = to_jsdisp(get_object(prop->u.val)); if (jsdisp) ret = is_class(jsdisp, JSCLASS_FUNCTION); - jsdisp_release(jsdisp); } return ret; } diff --git a/dlls/jscript/engine.c b/dlls/jscript/engine.c index 3becd1fc042..d24b54ec800 100644 --- a/dlls/jscript/engine.c +++ b/dlls/jscript/engine.c @@ -521,12 +521,9 @@ static HRESULT disp_get_id(script_ctx_t *ctx, IDispatch *disp, const WCHAR *name BSTR bstr; HRESULT hres; - jsdisp = iface_to_jsdisp(disp); - if(jsdisp) { - hres = jsdisp_get_id(jsdisp, name, flags, id); - jsdisp_release(jsdisp); - return hres; - } + jsdisp = to_jsdisp(disp); + if(jsdisp) + return jsdisp_get_id(jsdisp, name, flags, id); if(name_bstr) { bstr = name_bstr; @@ -1773,7 +1770,7 @@ static HRESULT interp_obj_prop(script_ctx_t *ctx) jsdisp_t *func; assert(is_object_instance(val)); - func = iface_to_jsdisp(get_object(val)); + func = to_jsdisp(get_object(val)); desc.mask = desc.flags; if(type == PROPERTY_DEFINITION_GETTER) { @@ -1785,7 +1782,6 @@ static HRESULT interp_obj_prop(script_ctx_t *ctx) } hres = jsdisp_define_property(obj, name, &desc); - jsdisp_release(func); } jsval_release(val); @@ -1929,15 +1925,12 @@ static HRESULT interp_instanceof(script_ctx_t *ctx) hres = JS_E_OBJECT_EXPECTED; else if(is_object_instance(prot)) { if(is_object_instance(v)) - tmp = iface_to_jsdisp(get_object(v)); + tmp = to_jsdisp(get_object(v)); for(iter = tmp; !ret && iter; iter = iter->prototype) { hres = disp_cmp(get_object(prot), to_disp(iter), &ret); if(FAILED(hres)) break; } - - if(tmp) - jsdisp_release(tmp); }else { FIXME("prototype is not an object\n"); hres = E_FAIL; @@ -2223,9 +2216,8 @@ static HRESULT typeof_string(jsval_t v, const WCHAR **ret) case JSV_OBJECT: { jsdisp_t *dispex; - if((dispex = iface_to_jsdisp(get_object(v)))) { + if((dispex = to_jsdisp(get_object(v)))) { *ret = is_class(dispex, JSCLASS_FUNCTION) ? L"function" : L"object"; - jsdisp_release(dispex); }else { *ret = L"object"; } diff --git a/dlls/jscript/json.c b/dlls/jscript/json.c index e36c4973ef2..7826000f87d 100644 --- a/dlls/jscript/json.c +++ b/dlls/jscript/json.c @@ -754,14 +754,13 @@ static HRESULT stringify(stringify_ctx_t *ctx, jsdisp_t *object, const WCHAR *na jsdisp_t *obj; DISPID id; - obj = iface_to_jsdisp(get_object(value)); + obj = to_jsdisp(get_object(value)); if(!obj) { jsval_release(value); return S_FALSE; } hres = jsdisp_get_id(obj, L"toJSON", 0, &id); - jsdisp_release(obj); if(hres == S_OK) FIXME("Use toJSON.\n"); } diff --git a/dlls/jscript/jsregexp.c b/dlls/jscript/jsregexp.c index 7b97a2951df..378dee9bc53 100644 --- a/dlls/jscript/jsregexp.c +++ b/dlls/jscript/jsregexp.c @@ -666,17 +666,14 @@ HRESULT create_regexp_var(script_ctx_t *ctx, jsval_t src_arg, jsval_t *flags_arg if(is_object_instance(src_arg)) { jsdisp_t *obj; - obj = iface_to_jsdisp(get_object(src_arg)); + obj = to_jsdisp(get_object(src_arg)); if(obj) { if(is_class(obj, JSCLASS_REGEXP)) { RegExpInstance *regexp = regexp_from_jsdisp(obj); hres = create_regexp(ctx, regexp->str, regexp->jsregexp->flags, ret); - jsdisp_release(obj); return hres; } - - jsdisp_release(obj); } } @@ -913,21 +910,16 @@ static HRESULT RegExpConstr_value(script_ctx_t *ctx, jsval_t vthis, WORD flags, case DISPATCH_METHOD: if(argc) { if(is_object_instance(argv[0])) { - jsdisp_t *jsdisp = iface_to_jsdisp(get_object(argv[0])); + jsdisp_t *jsdisp = to_jsdisp(get_object(argv[0])); if(jsdisp) { if(is_class(jsdisp, JSCLASS_REGEXP)) { - if(argc > 1 && !is_undefined(argv[1])) { - jsdisp_release(jsdisp); + if(argc > 1 && !is_undefined(argv[1])) return JS_E_REGEXP_SYNTAX; - } if(r) - *r = jsval_obj(jsdisp); - else - jsdisp_release(jsdisp); + *r = jsval_obj(jsdisp_addref(jsdisp)); return S_OK; } - jsdisp_release(jsdisp); } } } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2942
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Rather than using the variable obj for them. And look them up from the argument object, if available. The variable obj will still contain extra arguments that were not passed, or even the passed args if the argument obj is detached from the call frame (at that point, both need to have their own distinct copies). Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/jscript/engine.c | 65 +++++++++++++++++------ dlls/jscript/engine.h | 2 + dlls/jscript/function.c | 102 ++++++++++++++++++++++--------------- dlls/jscript/tests/lang.js | 32 ++++++++++++ 4 files changed, 144 insertions(+), 57 deletions(-) diff --git a/dlls/jscript/engine.c b/dlls/jscript/engine.c index d24b54ec800..fba0e5bddef 100644 --- a/dlls/jscript/engine.c +++ b/dlls/jscript/engine.c @@ -215,28 +215,44 @@ static BOOL stack_topn_exprval(script_ctx_t *ctx, unsigned n, exprval_t *r) /* Got stack reference in deoptimized code. Need to convert it back to variable object reference. */ assert(off < frame->variables_off + frame->function->var_cnt); - if (off >= frame->variables_off) + if (off < frame->variables_off && off - frame->arguments_off < frame->argc && frame->base_scope->arguments_obj) { - name = frame->function->variables[off - frame->variables_off].name; - scope = frame->scope; - } - else - { - name = frame->function->params[off - frame->arguments_off]; - scope = frame->base_scope; - } + WCHAR name_idx[11]; + jsobj = frame->base_scope->arguments_obj; + swprintf(name_idx, ARRAY_SIZE(name_idx), L"%u", off - frame->arguments_off); - while (1) - { - if ((jsobj = to_jsdisp(scope->obj)) && SUCCEEDED(hres = jsdisp_get_id(jsobj, name, 0, &id))) - break; - if (scope == frame->base_scope) + if (FAILED(hres = jsdisp_get_id(jsobj, name_idx, 0, &id))) { r->type = EXPRVAL_INVALID; r->u.hres = hres; return FALSE; } - scope = scope->next; + } + else + { + if (off >= frame->variables_off) + { + name = frame->function->variables[off - frame->variables_off].name; + scope = frame->scope; + } + else + { + name = frame->function->params[off - frame->arguments_off]; + scope = frame->base_scope; + } + + while (1) + { + if ((jsobj = to_jsdisp(scope->obj)) && SUCCEEDED(hres = jsdisp_get_id(jsobj, name, 0, &id))) + break; + if (scope == frame->base_scope) + { + r->type = EXPRVAL_INVALID; + r->u.hres = hres; + return FALSE; + } + scope = scope->next; + } } *stack_top_ref(ctx, n+1) = jsval_obj(jsdisp_addref(jsobj)); @@ -632,7 +648,7 @@ HRESULT jsval_strict_equal(jsval_t lval, jsval_t rval, BOOL *ret) static HRESULT detach_scope(script_ctx_t *ctx, call_frame_t *frame, scope_chain_t *scope) { function_code_t *func = frame->function; - unsigned int i, index; + unsigned int i, index, args_len = 0; jsdisp_t *jsobj; HRESULT hres; @@ -651,6 +667,13 @@ static HRESULT detach_scope(script_ctx_t *ctx, call_frame_t *frame, scope_chain_ else jsobj = as_jsdisp(scope->obj); + if (scope->arguments_obj) + { + args_len = frame->argc; + if (FAILED(hres = copy_arguments_from_stack(scope->arguments_obj, frame))) + return hres; + } + if (scope == frame->base_scope && func->name && func->local_ref == INVALID_LOCAL_REF && ctx->version >= SCRIPTLANGUAGEVERSION_ES5) jsdisp_propput_name(jsobj, func->name, jsval_obj(jsdisp_addref(frame->function_instance))); @@ -661,6 +684,8 @@ static HRESULT detach_scope(script_ctx_t *ctx, call_frame_t *frame, scope_chain_ WCHAR *name = frame->function->local_scopes[index].locals[i].name; int ref = frame->function->local_scopes[index].locals[i].ref; + if(ref < 0 && -ref - 1 < args_len) + continue; if (FAILED(hres = jsdisp_propput_name(jsobj, name, ctx->stack[local_off(frame, ref)]))) return hres; if (scope != frame->base_scope && frame->function->variables[ref].func_id != -1 @@ -788,6 +813,14 @@ static HRESULT identifier_eval(script_ctx_t *ctx, BSTR identifier, exprval_t *re ret->u.val = jsval_obj(jsdisp_addref(scope->frame->function_instance)); return S_OK; } + }else if(scope->arguments_obj) { + assert(scope->scope_index == 0); + hres = get_arguments_identifier_id(scope->arguments_obj, identifier, &id); + if(hres != DISP_E_MEMBERNOTFOUND) { + if(SUCCEEDED(hres)) + exprval_set_disp_ref(ret, to_disp(scope->arguments_obj), id); + return hres; + } } if (!scope->obj) diff --git a/dlls/jscript/engine.h b/dlls/jscript/engine.h index 3688972f29f..13e5f7c4f12 100644 --- a/dlls/jscript/engine.h +++ b/dlls/jscript/engine.h @@ -303,4 +303,6 @@ HRESULT exec_source(script_ctx_t*,DWORD,bytecode_t*,function_code_t*,scope_chain HRESULT create_source_function(script_ctx_t*,bytecode_t*,function_code_t*,scope_chain_t*,jsdisp_t**) DECLSPEC_HIDDEN; HRESULT setup_arguments_object(script_ctx_t*,call_frame_t*) DECLSPEC_HIDDEN; +HRESULT copy_arguments_from_stack(jsdisp_t*,call_frame_t*) DECLSPEC_HIDDEN; void detach_arguments_object(jsdisp_t*,call_frame_t*) DECLSPEC_HIDDEN; +HRESULT get_arguments_identifier_id(jsdisp_t*,const WCHAR*,DISPID*) DECLSPEC_HIDDEN; diff --git a/dlls/jscript/function.c b/dlls/jscript/function.c index b1d7e249923..d4314fa560f 100644 --- a/dlls/jscript/function.c +++ b/dlls/jscript/function.c @@ -129,48 +129,34 @@ static jsval_t *get_argument_ref(ArgumentsInstance *arguments, unsigned idx) { if(arguments->buf) return arguments->buf + idx; - if(arguments->frame->base_scope->frame || idx >= arguments->frame->function->param_cnt) - return arguments->jsdisp.ctx->stack + arguments->frame->arguments_off + idx; - return NULL; + return arguments->jsdisp.ctx->stack + arguments->frame->arguments_off + idx; } static HRESULT Arguments_idx_get(jsdisp_t *jsdisp, unsigned idx, jsval_t *r) { ArgumentsInstance *arguments = arguments_from_jsdisp(jsdisp); - jsval_t *ref; TRACE("%p[%u]\n", arguments, idx); - if((ref = get_argument_ref(arguments, idx))) - return jsval_copy(*ref, r); - - /* FIXME: Accessing by name won't work for duplicated argument names */ - return jsdisp_propget_name(as_jsdisp(arguments->frame->base_scope->obj), - arguments->function->func_code->params[idx], r); + return jsval_copy(*get_argument_ref(arguments, idx), r); } static HRESULT Arguments_idx_put(jsdisp_t *jsdisp, unsigned idx, jsval_t val) { ArgumentsInstance *arguments = arguments_from_jsdisp(jsdisp); - jsval_t *ref; + jsval_t copy, *ref; HRESULT hres; TRACE("%p[%u] = %s\n", arguments, idx, debugstr_jsval(val)); - if((ref = get_argument_ref(arguments, idx))) { - jsval_t copy; - hres = jsval_copy(val, ©); - if(FAILED(hres)) - return hres; - - jsval_release(*ref); - *ref = copy; - return S_OK; - } + hres = jsval_copy(val, ©); + if(FAILED(hres)) + return hres; - /* FIXME: Accessing by name won't work for duplicated argument names */ - return jsdisp_propput_name(as_jsdisp(arguments->frame->base_scope->obj), - arguments->function->func_code->params[idx], val); + ref = get_argument_ref(arguments, idx); + jsval_release(*ref); + *ref = copy; + return S_OK; } static HRESULT Arguments_gc_traverse(struct gc_ctx *gc_ctx, enum gc_traverse_op op, jsdisp_t *jsdisp) @@ -237,13 +223,34 @@ HRESULT setup_arguments_object(script_ctx_t *ctx, call_frame_t *frame) return S_OK; } +HRESULT copy_arguments_from_stack(jsdisp_t *args_disp, call_frame_t *frame) +{ + ArgumentsInstance *arguments = arguments_from_jsdisp(args_disp); + const jsval_t *args = arguments->jsdisp.ctx->stack + frame->arguments_off; + HRESULT hres; + unsigned i; + + arguments->buf = malloc(arguments->argc * sizeof(*arguments->buf)); + if(!arguments->buf) + return E_OUTOFMEMORY; + + for(i = 0; i < arguments->argc; i++) { + hres = jsval_copy(args[i], &arguments->buf[i]); + if(FAILED(hres)) { + do arguments->buf[i++] = jsval_undefined(); while(i < arguments->argc); + return hres; + } + } + + return S_OK; +} + void detach_arguments_object(jsdisp_t *args_disp, call_frame_t *popped_frame) { ArgumentsInstance *arguments = arguments_from_jsdisp(args_disp); call_frame_t *frame = arguments->frame; const BOOL on_stack = frame->base_scope->frame == frame; jsdisp_t *jsobj; - HRESULT hres; if(frame != popped_frame) return; @@ -255,29 +262,42 @@ void detach_arguments_object(jsdisp_t *args_disp, call_frame_t *popped_frame) frame->base_scope->arguments_obj = NULL; arguments->frame = NULL; - /* Don't bother coppying arguments if call frame holds the last reference. */ - if(arguments->jsdisp.ref > 1) { - arguments->buf = malloc(arguments->argc * sizeof(*arguments->buf)); - if(arguments->buf) { - int i; + /* Don't bother copying arguments if scope holds the last reference */ + if(arguments->jsdisp.ref > 1 && on_stack) + copy_arguments_from_stack(&arguments->jsdisp, frame); - for(i = 0; i < arguments->argc ; i++) { - if(on_stack || i >= frame->function->param_cnt) - hres = jsval_copy(arguments->jsdisp.ctx->stack[frame->arguments_off + i], arguments->buf+i); - else - hres = jsdisp_propget_name(jsobj, frame->function->params[i], arguments->buf+i); - if(FAILED(hres)) - arguments->buf[i] = jsval_undefined(); - } - }else { - ERR("out of memory\n"); - arguments->argc = 0; + /* If scope stays alive, copy the args to the variable obj, as they'll be separate from the arguments obj now */ + if(frame->base_scope->dispex.ref > 1) { + const local_ref_scopes_t *local_scope = &frame->function->local_scopes[frame->base_scope->scope_index]; + const jsval_t *args = on_stack ? arguments->jsdisp.ctx->stack + frame->arguments_off : arguments->buf; + unsigned i; + + for(i = 0; i < local_scope->locals_cnt; i++) { + int ref = local_scope->locals[i].ref; + + if(ref < 0 && -ref - 1 < arguments->argc) + jsdisp_propput_name(jsobj, local_scope->locals[i].name, args[-ref - 1]); } } jsdisp_release(&arguments->jsdisp); } +HRESULT get_arguments_identifier_id(jsdisp_t *args_disp, const WCHAR *identifier, DISPID *id) +{ + ArgumentsInstance *arguments = arguments_from_jsdisp(args_disp); + local_ref_t *ref; + WCHAR buf[11]; + + ref = lookup_local(arguments->function->func_code, identifier, 0); + + if(!ref || ref->ref >= 0 || -ref->ref - 1 >= arguments->argc) + return DISP_E_MEMBERNOTFOUND; + + swprintf(buf, ARRAY_SIZE(buf), L"%u", -ref->ref - 1); + return jsdisp_get_id(&arguments->jsdisp, buf, fdexNameImplicit, id); +} + HRESULT Function_invoke(jsdisp_t *func_this, jsval_t vthis, WORD flags, unsigned argc, jsval_t *argv, jsval_t *r) { FunctionInstance *function; diff --git a/dlls/jscript/tests/lang.js b/dlls/jscript/tests/lang.js index 5c201d5bf26..36afcc82417 100644 --- a/dlls/jscript/tests/lang.js +++ b/dlls/jscript/tests/lang.js @@ -306,6 +306,38 @@ argumentsTest(); ok(arguments === 1, "arguments = " + arguments); })(); +// duplicated argument names are shadowed by the last argument with the same name +(function() { + var args, get_a, set_a; + + (function(a, a, b, c) { + get_a = function() { return a; } + set_a = function(v) { a = v; } + todo_wine_ok(get_a() === 2, "function(a, a, b, c) get_a() = " + get_a()); + todo_wine_ok(a === 2, "function(a, a, b, c) a = " + a); + ok(b === 3, "function(a, a, b, c) b = " + b); + ok(c === 4, "function(a, a, b, c) c = " + c); + a = 42; + todo_wine_ok(arguments[0] === 1, "function(a, a, b, c) arguments[0] = " + arguments[0]); + todo_wine_ok(arguments[1] === 42, "function(a, a, b, c) arguments[1] = " + arguments[1]); + ok(get_a() === 42, "function(a, a, b, c) get_a() = " + get_a() + " expected 42"); + args = arguments; + })(1, 2, 3, 4); + + ok(get_a() === 42, "function(a, a, b, c) get_a() after detach = " + get_a()); + set_a(100); + ok(get_a() === 100, "function(a, a, b, c) get_a() = " + get_a() + " expected 100"); + todo_wine_ok(args[0] === 1, "function(a, a, b, c) detached args[0] = " + args[0]); + todo_wine_ok(args[1] === 42, "function(a, a, b, c) detached args[1] = " + args[1]); + + (function(a, a) { + eval("var a = 7;"); + ok(a === 7, "function(a, a) a = " + a); + todo_wine_ok(arguments[0] === 5, "function(a, a) arguments[0] = " + arguments[0]); + todo_wine_ok(arguments[1] === 7, "function(a, a) arguments[1] = " + arguments[1]); + })(5, 6); +})(); + (function callAsExprTest() { ok(callAsExprTest.arguments === null, "callAsExprTest.arguments = " + callAsExprTest.arguments); })(1,2); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2942
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> This is needed for duplicated argument names, as the last arg will shadow all the prior ones when it comes to name lookup. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/jscript/compile.c | 2 +- dlls/jscript/tests/lang.js | 16 ++++++++-------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/dlls/jscript/compile.c b/dlls/jscript/compile.c index e2690935212..2d2c2c9f40a 100644 --- a/dlls/jscript/compile.c +++ b/dlls/jscript/compile.c @@ -2567,7 +2567,7 @@ static HRESULT compile_function(compiler_ctx_t *ctx, statement_t *source, functi } } - for(i = 0; i < func->param_cnt; i++) { + for(i = func->param_cnt; i--;) { if(!find_local(ctx, func->params[i], 0) && !alloc_local(ctx, func->params[i], -i-1, 0)) return E_OUTOFMEMORY; } diff --git a/dlls/jscript/tests/lang.js b/dlls/jscript/tests/lang.js index 36afcc82417..30d8a338fd3 100644 --- a/dlls/jscript/tests/lang.js +++ b/dlls/jscript/tests/lang.js @@ -313,13 +313,13 @@ argumentsTest(); (function(a, a, b, c) { get_a = function() { return a; } set_a = function(v) { a = v; } - todo_wine_ok(get_a() === 2, "function(a, a, b, c) get_a() = " + get_a()); - todo_wine_ok(a === 2, "function(a, a, b, c) a = " + a); + ok(get_a() === 2, "function(a, a, b, c) get_a() = " + get_a()); + ok(a === 2, "function(a, a, b, c) a = " + a); ok(b === 3, "function(a, a, b, c) b = " + b); ok(c === 4, "function(a, a, b, c) c = " + c); a = 42; - todo_wine_ok(arguments[0] === 1, "function(a, a, b, c) arguments[0] = " + arguments[0]); - todo_wine_ok(arguments[1] === 42, "function(a, a, b, c) arguments[1] = " + arguments[1]); + ok(arguments[0] === 1, "function(a, a, b, c) arguments[0] = " + arguments[0]); + ok(arguments[1] === 42, "function(a, a, b, c) arguments[1] = " + arguments[1]); ok(get_a() === 42, "function(a, a, b, c) get_a() = " + get_a() + " expected 42"); args = arguments; })(1, 2, 3, 4); @@ -327,14 +327,14 @@ argumentsTest(); ok(get_a() === 42, "function(a, a, b, c) get_a() after detach = " + get_a()); set_a(100); ok(get_a() === 100, "function(a, a, b, c) get_a() = " + get_a() + " expected 100"); - todo_wine_ok(args[0] === 1, "function(a, a, b, c) detached args[0] = " + args[0]); - todo_wine_ok(args[1] === 42, "function(a, a, b, c) detached args[1] = " + args[1]); + ok(args[0] === 1, "function(a, a, b, c) detached args[0] = " + args[0]); + ok(args[1] === 42, "function(a, a, b, c) detached args[1] = " + args[1]); (function(a, a) { eval("var a = 7;"); ok(a === 7, "function(a, a) a = " + a); - todo_wine_ok(arguments[0] === 5, "function(a, a) arguments[0] = " + arguments[0]); - todo_wine_ok(arguments[1] === 7, "function(a, a) arguments[1] = " + arguments[1]); + ok(arguments[0] === 5, "function(a, a) arguments[0] = " + arguments[0]); + ok(arguments[1] === 7, "function(a, a) arguments[1] = " + arguments[1]); })(5, 6); })(); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2942
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> We don't need the function instance anymore, and this simplifies the traversal and removes possible cyclic refs with it. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/jscript/function.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/dlls/jscript/function.c b/dlls/jscript/function.c index d4314fa560f..d26bb1fa436 100644 --- a/dlls/jscript/function.c +++ b/dlls/jscript/function.c @@ -65,7 +65,7 @@ typedef struct { typedef struct { jsdisp_t jsdisp; - InterpretedFunction *function; + function_code_t *func_code; jsval_t *buf; call_frame_t *frame; unsigned argc; @@ -114,8 +114,7 @@ static void Arguments_destructor(jsdisp_t *jsdisp) free(arguments->buf); } - if(arguments->function) - jsdisp_release(&arguments->function->function.dispex); + release_bytecode(arguments->func_code->bytecode); free(arguments); } @@ -173,7 +172,7 @@ static HRESULT Arguments_gc_traverse(struct gc_ctx *gc_ctx, enum gc_traverse_op } } - return gc_process_linked_obj(gc_ctx, op, jsdisp, &arguments->function->function.dispex, (void**)&arguments->function); + return S_OK; } static const builtin_info_t Arguments_info = { @@ -203,7 +202,8 @@ HRESULT setup_arguments_object(script_ctx_t *ctx, call_frame_t *frame) return hres; } - args->function = (InterpretedFunction*)function_from_jsdisp(jsdisp_addref(frame->function_instance)); + args->func_code = ((InterpretedFunction*)function_from_jsdisp(frame->function_instance))->func_code; + bytecode_addref(args->func_code->bytecode); args->argc = frame->argc; args->frame = frame; @@ -211,7 +211,7 @@ HRESULT setup_arguments_object(script_ctx_t *ctx, call_frame_t *frame) jsval_number(args->argc)); if(SUCCEEDED(hres)) hres = jsdisp_define_data_property(&args->jsdisp, L"callee", PROPF_WRITABLE | PROPF_CONFIGURABLE, - jsval_obj(&args->function->function.dispex)); + jsval_obj(frame->function_instance)); if(SUCCEEDED(hres)) hres = jsdisp_propput(as_jsdisp(frame->base_scope->obj), L"arguments", PROPF_WRITABLE, TRUE, jsval_obj(&args->jsdisp)); if(FAILED(hres)) { @@ -289,7 +289,7 @@ HRESULT get_arguments_identifier_id(jsdisp_t *args_disp, const WCHAR *identifier local_ref_t *ref; WCHAR buf[11]; - ref = lookup_local(arguments->function->func_code, identifier, 0); + ref = lookup_local(arguments->func_code, identifier, 0); if(!ref || ref->ref >= 0 || -ref->ref - 1 >= arguments->argc) return DISP_E_MEMBERNOTFOUND; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2942
Jacek Caban (@jacek) commented about dlls/jscript/engine.h:
jsdisp_t dispex; /* FIXME: don't wrap it in a jsdisp (it holds ref and traverse for the garbage collector) */ jsdisp_t *jsobj; IDispatch *obj; + jsdisp_t *arguments_obj;
It doesn't really belong here, it's a per-call property and I think that it fits call frame. The whole solution still suffers from over-allocating arguments object. Another solution would be to store `function_code_t` (we already store scope index) as well as detached locals buffer here. Arguments object could then just store a reference to scope chain. That would need other solutions for addressing arguments. Maybe we have a reason to treat scope as an object in more aspects after all. You could use that to expose locals using `idx_put()`/`idx_get()`, for example. Since applications will never use this as an object directly, names associated with dispids don't matter so the same mechanism could be used in the future for local variables as well. Potentially, variable object is needed only for `with` statement and extendable scopes. Eventually, having a static view of the whole scope chain with easy to identify bail outs could give nice performance opportunities. For example, we could move more name lookups to the compiler and entirely skip them in runtime. It's obviously not related to your bug. It's fine to implement only arguments part, but I'd prefer a solution that does not make future changes harder. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2942#note_35457
On Mon Jun 12 14:33:40 2023 +0000, Jacek Caban wrote:
It doesn't really belong here, it's a per-call property and I think that it fits call frame. The whole solution still suffers from over-allocating arguments object. Another solution would be to store `function_code_t` (we already store scope index) as well as detached locals buffer here. Arguments object could then just store a reference to scope chain. That would need other solutions for addressing arguments. Maybe we have a reason to treat scope as an object in more aspects after all. You could use that to expose locals using `idx_put()`/`idx_get()`, for example. Since applications will never use this as an object directly, names associated with dispids don't matter so the same mechanism could be used in the future for local variables as well. Potentially, variable object is needed only for `with` statement and extendable scopes. Eventually, having a static view of the whole scope chain with easy to identify bail outs could give nice performance opportunities. For example, we could move more name lookups to the compiler and entirely skip them in runtime. It's obviously not related to your bug. It's fine to implement only arguments part, but I'd prefer a solution that does not make future changes harder. So to summarize, I move all detached scope locals to the scope's jsdisp, accessed by index (from lookup_local). If so, what would the point of "detached locals buffer" be? Or was that just an alternative?
I mean I could store the local's value itself directly into the scope's props, no need to keep a separate buffer, I think. I'll only have to map indexed args into props with idx_get/put, and for arguments with "no name" (shadowed by duplicated names, or extra args passed to a func) maybe just store them with prop name = index, since locals can't have names starting with a digit… I think it should work, unless I'm missing something. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2942#note_35473
On Mon Jun 12 15:46:39 2023 +0000, Gabriel Ivăncescu wrote:
So to summarize, I move all detached scope locals to the scope's jsdisp, accessed by index (from lookup_local). If so, what would the point of "detached locals buffer" be? Or was that just an alternative? I mean I could store the local's value itself directly into the scope's props, no need to keep a separate buffer, I think. I'll only have to map indexed args into props with idx_get/put, and for arguments with "no name" (shadowed by duplicated names, or extra args passed to a func) maybe just store them with prop name = index, since locals can't have names starting with a digit… I think it should work, unless I'm missing something. That would again special case shadowed arguments instead of making them a non-issue by design. That also wouldn't make values easily available to arguments object, so you'd need some unpleasant lookups there.
Also, performance. Copying an array is much cheaper than allocating props and gives further possible optimizations. Although we currently alloc props when addressing indexed properties, we could probably avoid that at least for objects with constant length. Interpreter could optimize the access by using the same code as we use for attached locals by accessing the buffer directly. That combined with compiler optimization that I mentioned could probably get rid of vast majority of lookups. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2942#note_35477
participants (2)
-
Gabriel Ivăncescu -
Jacek Caban (＠jacek)