[PATCH 0/3] MR1067: jscript: Bind function 'thisArg' and JSON.parse improvements.
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/jscript/function.c | 40 +++++++++++++++++++++------------------- dlls/mshtml/tests/es5.js | 17 +++++++++++++++++ 2 files changed, 38 insertions(+), 19 deletions(-) diff --git a/dlls/jscript/function.c b/dlls/jscript/function.c index bf152e22868..8009f3a5d8b 100644 --- a/dlls/jscript/function.c +++ b/dlls/jscript/function.c @@ -57,7 +57,7 @@ typedef struct { typedef struct { FunctionInstance function; FunctionInstance *target; - IDispatch *this; + jsval_t this; unsigned argc; jsval_t args[1]; } BindFunction; @@ -70,7 +70,7 @@ typedef struct { unsigned argc; } ArgumentsInstance; -static HRESULT create_bind_function(script_ctx_t*,FunctionInstance*,IDispatch*,unsigned,jsval_t*,jsdisp_t**r); +static HRESULT create_bind_function(script_ctx_t*,FunctionInstance*,jsval_t,unsigned,jsval_t*,jsdisp_t**r); static inline FunctionInstance *function_from_jsdisp(jsdisp_t *jsdisp) { @@ -448,7 +448,7 @@ static HRESULT Function_call(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsig static HRESULT Function_bind(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigned argc, jsval_t *argv, jsval_t *r) { - IDispatch *bound_this = NULL; + jsval_t bound_this = jsval_undefined(); FunctionInstance *function; jsdisp_t *new_function; HRESULT hres; @@ -459,18 +459,19 @@ static HRESULT Function_bind(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsig return JS_E_FUNCTION_EXPECTED; if(argc < 1) { - FIXME("no this argument\n"); - return E_NOTIMPL; - } - - if(is_object_instance(argv[0])) { - bound_this = get_object(argv[0]); - }else if(!is_null(argv[0])) { - FIXME("%s is not an object instance\n", debugstr_jsval(argv[0])); - return E_NOTIMPL; + argc = 1; + }else if(is_null(argv[0])) { + bound_this = argv[0]; + }else if(!is_undefined(argv[0])) { + IDispatch *obj; + hres = to_object(ctx, argv[0], &obj); + if(FAILED(hres)) + return hres; + bound_this = jsval_disp(obj); } hres = create_bind_function(ctx, function, bound_this, argc - 1, argv + 1, &new_function); + jsval_release(bound_this); if(FAILED(hres)) return hres; @@ -849,8 +850,7 @@ static HRESULT BindFunction_call(script_ctx_t *ctx, FunctionInstance *func, jsva memcpy(call_args + function->argc, argv, argc * sizeof(*call_args)); } - hres = function->target->vtbl->call(ctx, function->target, function->this ? jsval_disp(function->this) : jsval_null(), - flags, call_argc, call_args, r); + hres = function->target->vtbl->call(ctx, function->target, function->this, flags, call_argc, call_args, r); heap_free(call_args); return hres; @@ -877,8 +877,7 @@ 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->this) - IDispatch_Release(function->this); + jsval_release(function->this); } static const function_vtbl_t BindFunctionVtbl = { @@ -888,7 +887,7 @@ static const function_vtbl_t BindFunctionVtbl = { BindFunction_destructor }; -static HRESULT create_bind_function(script_ctx_t *ctx, FunctionInstance *target, IDispatch *bound_this, unsigned argc, +static HRESULT create_bind_function(script_ctx_t *ctx, FunctionInstance *target, jsval_t bound_this, unsigned argc, jsval_t *argv, jsdisp_t **ret) { BindFunction *function; @@ -902,8 +901,11 @@ static HRESULT create_bind_function(script_ctx_t *ctx, FunctionInstance *target, jsdisp_addref(&target->dispex); function->target = target; - if(bound_this) - IDispatch_AddRef(function->this = bound_this); + hres = jsval_copy(bound_this, &function->this); + if(FAILED(hres)) { + jsdisp_release(&function->function.dispex); + return hres; + } for(function->argc = 0; function->argc < argc; function->argc++) { hres = jsval_copy(argv[function->argc], function->args + function->argc); diff --git a/dlls/mshtml/tests/es5.js b/dlls/mshtml/tests/es5.js index bfaacd1bd5b..e22f610e197 100644 --- a/dlls/mshtml/tests/es5.js +++ b/dlls/mshtml/tests/es5.js @@ -1188,6 +1188,23 @@ sync_test("bind", function() { ok(t != a, "t == a"); ok(Function.prototype.bind.length === 1, "Function.prototype.bind.length = " + Function.prototype.bind.length); + + ((function() { ok(this === window, "bind() this = " + this); }).bind())(); + ((function() { ok(this === window, "bind(undefined) = " + this); }).bind(undefined))(); + ((function() { ok(this === window, "bind(nullDisp) = " + this); }).bind(external.nullDisp))(); + ((function() { + ok(typeof(this) === "object", "bind(42) typeof(this) = " + typeof(this)); + ok(this.valueOf() === 42, "bind(42) this = " + this); + }).bind(42))(); + + r = (Object.prototype.toString.bind())(); + ok(r === "[object Undefined]", "toString.bind() returned " + r); + r = (Object.prototype.toString.bind(undefined))(); + ok(r === "[object Undefined]", "toString.bind(undefined) returned " + r); + r = (Object.prototype.toString.bind(null))(); + ok(r === "[object Null]", "toString.bind(null) returned " + r); + r = (Object.prototype.toString.bind(external.nullDisp))(); + ok(r === "[object Null]", "toString.bind(nullDisp) returned " + r); }); sync_test("keys", function() { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1067
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/jscript/tests/api.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/jscript/tests/api.js b/dlls/jscript/tests/api.js index 123665cfdde..e47e2759abb 100644 --- a/dlls/jscript/tests/api.js +++ b/dlls/jscript/tests/api.js @@ -2025,7 +2025,7 @@ ok(isNaN(tmp), "Math.tan(-Infinity) is not NaN"); for(var prop in x) { if(!x.hasOwnProperty(prop)) continue; - if(!x.hasOwnProperty(prop)) + if(!y.hasOwnProperty(prop)) return false; if(!json_cmp(x[prop], y[prop])) return false; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1067
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- We store the hres in the ctx to minimize the amount of parameters to the recursive function, since it propagates up the call stack anyway. --- dlls/jscript/array.c | 2 +- dlls/jscript/jscript.h | 1 + dlls/jscript/json.c | 126 ++++++++++++++++++++++++++++++++++++-- dlls/jscript/tests/api.js | 57 ++++++++++++++++- 4 files changed, 178 insertions(+), 8 deletions(-) diff --git a/dlls/jscript/array.c b/dlls/jscript/array.c index 5a8131f092c..d69c9f7edf4 100644 --- a/dlls/jscript/array.c +++ b/dlls/jscript/array.c @@ -93,7 +93,7 @@ static HRESULT set_length(jsdisp_t *obj, DWORD length) return jsdisp_propput_name(obj, L"length", jsval_number(length)); } -static WCHAR *idx_to_str(DWORD idx, WCHAR *ptr) +WCHAR *idx_to_str(DWORD idx, WCHAR *ptr) { if(!idx) { *ptr = '0'; diff --git a/dlls/jscript/jscript.h b/dlls/jscript/jscript.h index 0f8baea0188..757abf8aa6d 100644 --- a/dlls/jscript/jscript.h +++ b/dlls/jscript/jscript.h @@ -318,6 +318,7 @@ HRESULT variant_date_to_string(script_ctx_t*,double,jsstr_t**) DECLSPEC_HIDDEN; HRESULT decode_source(WCHAR*) DECLSPEC_HIDDEN; HRESULT double_to_string(double,jsstr_t**) DECLSPEC_HIDDEN; +WCHAR *idx_to_str(DWORD,WCHAR*) DECLSPEC_HIDDEN; static inline BOOL is_digit(WCHAR c) { diff --git a/dlls/jscript/json.c b/dlls/jscript/json.c index dfcc39daa2a..95a6ff16bc2 100644 --- a/dlls/jscript/json.c +++ b/dlls/jscript/json.c @@ -267,20 +267,107 @@ static HRESULT parse_json_value(json_parse_ctx_t *ctx, jsval_t *r) return E_FAIL; } +struct transform_json_object_ctx +{ + script_ctx_t *ctx; + IDispatch *reviver; + HRESULT hres; +}; + +static jsval_t transform_json_object(struct transform_json_object_ctx *proc_ctx, jsdisp_t *holder, jsstr_t *name) +{ + jsval_t res, args[2]; + const WCHAR *str; + HRESULT hres; + + if(!(str = jsstr_flatten(name))) + hres = E_OUTOFMEMORY; + else + hres = jsdisp_propget_name(holder, str, &args[1]); + if(FAILED(hres)) { + proc_ctx->hres = hres; + return jsval_undefined(); + } + + if(is_object_instance(args[1])) { + jsdisp_t *obj = as_jsdisp(get_object(args[1])); + jsstr_t *jsstr; + DISPID id; + BOOL b; + + if(is_class(obj, JSCLASS_ARRAY)) { + unsigned i, length = array_get_length(obj); + WCHAR buf[14], *buf_end; + + buf_end = buf + ARRAY_SIZE(buf) - 1; + *buf_end-- = 0; + for(i = 0; i < length; i++) { + str = idx_to_str(i, buf_end); + if(!(jsstr = jsstr_alloc(str))) { + hres = E_OUTOFMEMORY; + break; + } + res = transform_json_object(proc_ctx, obj, jsstr); + jsstr_release(jsstr); + if(is_undefined(res)) { + if(proc_ctx->hres != S_OK) + return res; + hres = jsdisp_get_id(obj, str, 0, &id); + if(FAILED(hres)) + continue; + hres = disp_delete((IDispatch*)&obj->IDispatchEx_iface, id, &b); + }else { + hres = jsdisp_define_data_property(obj, str, PROPF_WRITABLE | PROPF_ENUMERABLE | PROPF_CONFIGURABLE, res); + jsval_release(res); + } + if(FAILED(hres)) + break; + } + }else { + id = DISPID_STARTENUM; + do { + hres = jsdisp_next_prop(obj, id, JSDISP_ENUM_OWN_ENUMERABLE, &id); + if(hres != S_OK || FAILED(hres = jsdisp_get_prop_name(obj, id, &jsstr))) + break; + res = transform_json_object(proc_ctx, obj, jsstr); + if(is_undefined(res)) + hres = (proc_ctx->hres != S_OK) ? proc_ctx->hres : disp_delete((IDispatch*)&obj->IDispatchEx_iface, id, &b); + else { + if(!(str = jsstr_flatten(jsstr))) + hres = E_OUTOFMEMORY; + else + hres = jsdisp_define_data_property(obj, str, PROPF_WRITABLE | PROPF_ENUMERABLE | PROPF_CONFIGURABLE, res); + jsval_release(res); + } + jsstr_release(jsstr); + } while(SUCCEEDED(hres)); + } + if(FAILED(hres)) { + proc_ctx->hres = hres; + return jsval_undefined(); + } + } + + args[0] = jsval_string(name); + hres = disp_call_value(proc_ctx->ctx, proc_ctx->reviver, (IDispatch*)&holder->IDispatchEx_iface, + DISPATCH_METHOD, ARRAY_SIZE(args), args, &res); + if(FAILED(hres)) { + proc_ctx->hres = hres; + return jsval_undefined(); + } + return res; +} + /* ECMA-262 5.1 Edition 15.12.2 */ static HRESULT JSON_parse(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigned argc, jsval_t *argv, jsval_t *r) { json_parse_ctx_t parse_ctx; const WCHAR *buf; + jsdisp_t *root; jsstr_t *str; jsval_t ret; HRESULT hres; - if(argc != 1) { - FIXME("Unsupported args\n"); - return E_INVALIDARG; - } - hres = to_flat_string(ctx, argv[0], &str, &buf); if(FAILED(hres)) return hres; @@ -296,8 +383,35 @@ static HRESULT JSON_parse(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigned hres = E_FAIL; } jsstr_release(str); - if(FAILED(hres)) + if(FAILED(hres)) { + jsval_release(ret); return hres; + } + + /* FIXME: check IsCallable */ + if(argc > 1 && is_object_instance(argv[1])) { + hres = create_object(ctx, NULL, &root); + if(FAILED(hres)) { + jsval_release(ret); + return hres; + } + hres = jsdisp_define_data_property(root, L"", PROPF_WRITABLE | PROPF_ENUMERABLE | PROPF_CONFIGURABLE, ret); + jsval_release(ret); + + if(SUCCEEDED(hres)) { + struct transform_json_object_ctx proc_ctx = { ctx, get_object(argv[1]), S_OK }; + if(!(str = jsstr_alloc(L""))) + hres = E_OUTOFMEMORY; + else { + ret = transform_json_object(&proc_ctx, root, str); + jsstr_release(str); + hres = proc_ctx.hres; + } + } + jsdisp_release(root); + if(FAILED(hres)) + return hres; + } if(r) *r = ret; diff --git a/dlls/jscript/tests/api.js b/dlls/jscript/tests/api.js index e47e2759abb..c33791f1fb1 100644 --- a/dlls/jscript/tests/api.js +++ b/dlls/jscript/tests/api.js @@ -1960,7 +1960,7 @@ ok(isNaN(tmp), "Math.tan(-Infinity) is not NaN"); [[[,2,undefined,3,{prop:0},],undefined," "],"[\n null,\n 2,\n null,\n 3,\n {\n \"prop\": 0\n },\n null\n]"] ]; - var i, s, v; + var i, s, v, t; for(i=0; i < stringify_tests.length; i++) { s = JSON.stringify.apply(null, stringify_tests[i][0]); @@ -2043,6 +2043,61 @@ ok(isNaN(tmp), "Math.tan(-Infinity) is not NaN"); v = JSON.parse(parse_tests[i][0]); ok(json_cmp(v, parse_tests[i][1]), "parse[" + i + "] returned " + v + ", expected " + parse_tests[i][1]); } + + v = [ [-1, "b"], {"length": -2, "0": -4, "1": -5}, [{}], [{"x": [null]}] ]; + s = + '{' + + '"foo": true,' + + '"bar": [],' + + '"baz": "remove_me",' + + '"obj": {' + + ' "arr": [ [1, "b"], {"length": 2, "0": 4, "1": 5}, [{}], [{"x": [null]}] ],' + + ' "": "empty"' + + '},' + + '"last": false' + + '}'; + o = JSON.parse(s), t = JSON.parse(s), i = new Object(); + i[""] = t; + delete t.baz; /* baz gets removed */ + t.obj.arr = v; /* has negative values */ + + var walk_expect = [ + [ o, "foo", true ], + [ o, "bar", [] ], + [ o, "baz", "remove_me" ], + [ [1, "b"], "0", 1 ], + [ [-1, "b"], "1", "b" ], + [ [ [-1, "b"], {"length": 2, "0": 4, "1": 5}, [{}], [{"x": [null]}] ], "0", [-1, "b"] ], + [ {"length": 2, "0": 4, "1": 5}, "length", 2 ], + [ {"length": -2, "0": 4, "1": 5}, "0", 4 ], + [ {"length": -2, "0": -4, "1": 5}, "1", 5 ], + [ v, "1", {"length": -2, "0": -4, "1": -5} ], + [ [{}], "0", {} ], + [ v, "2", [{}] ], + [ [null], "0", null ], + [ {"x": [null]}, "x", [null] ], + [ [{"x": [null]}], "0", {"x": [null]} ], + [ v, "3", [{"x": [null]}] ], + [ { "arr": v, "": "empty" }, "arr", v ], + [ { "arr": v, "": "empty" }, "", "empty" ], + [ t, "obj", { "arr": v, "": "empty" } ], + [ t, "last", false ], + [ i, "", t ] + ]; + i = 0; + v = JSON.parse(s, function(prop, value) { + var a = [this, prop, value]; + ok(json_cmp(a, walk_expect[i]), "[walk step " + i + "] got [" + a + "], expected [" + walk_expect[i] + "]"); + i++; + return (typeof value === 'number') ? -value : (value === "remove_me" ? undefined : value); + }); + ok(i === walk_expect.length, "parse with reviver walked " + i + " steps, expected " + walk_expect.length); + ok(json_cmp(v, t), "parse with reviver returned wrong object"); + + v = JSON.parse('true', function(prop, value) { return prop === "" ? undefined : value; }); + ok(v === undefined, "parse with reviver removing last prop returned " + v); + v = JSON.parse('true', function(prop, value) { return prop === "" ? false : value; }); + ok(v === false, "parse with reviver setting last prop to false returned " + v); })(); var func = function (a) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1067
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=124995 Your paranoid android. === debian11 (32 bit report) === quartz: filtergraph.c:524: Test marked flaky: didn't get EOS filtergraph.c:529: Test marked flaky: expected 1243c8, got 0 === debian11 (build log) === Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720, <$LogFile> line 24751. Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720, <$LogFile> line 24751. Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720, <$LogFile> line 24751.
Jacek Caban (@jacek) commented about dlls/jscript/json.c:
+{ + jsval_t res, args[2]; + const WCHAR *str; + HRESULT hres; + + if(!(str = jsstr_flatten(name))) + hres = E_OUTOFMEMORY; + else + hres = jsdisp_propget_name(holder, str, &args[1]); + if(FAILED(hres)) { + proc_ctx->hres = hres; + return jsval_undefined(); + } + + if(is_object_instance(args[1])) { + jsdisp_t *obj = as_jsdisp(get_object(args[1])); It seems that we shouldn't assume that the object is of jsdisp_t type, reviewer may have already replaced it with any object type (it's probably fine to have a FIXME for that, through).
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1067#note_10765
Jacek Caban (@jacek) commented about dlls/jscript/json.c:
hres = E_FAIL; } jsstr_release(str); - if(FAILED(hres)) + if(FAILED(hres)) { + jsval_release(ret);
This may be uninitialized if parsing failed. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1067#note_10766
You store hres in a local variable (which you could avoid, BTW), so you're not really saving anything. More importantly, if stack usage is a real concern, we should implement the algorithm without using recursion instead. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1067#note_10767
Well without that I'd have to add an output parameter, which seems pointless when the hres gets immediately propagated. I'll remove the local variable though, just in case it doesn't get optimized out. Apparently it makes it a bit less code, too. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1067#note_10776
participants (3)
-
Gabriel Ivăncescu -
Jacek Caban (＠jacek)
-
Marvin