From 6598449246d7c0fdade5cf23fb6d2ff4dcbaf498 Mon Sep 17 00:00:00 2001 From: Ankit Jain Date: Mon, 6 Jul 2020 19:41:06 -0400 Subject: [PATCH] [wasm][debugger] Small checks on inputs, and some negative tests - These tests don't actually depend on the error message, and we don't have another to way to differentiate why a command might have failed with an exception. So, right now, they will pass as long as the commands fail as expected. - Future TODO: return `error`, instead of exception details for issues in `mono.js`, like incorrect input, invalid id etc, and update these tests accordingly. --- src/mono/mono/mini/mini-wasm-debugger.c | 8 + .../debugger/DebuggerTestSuite/ArrayTests.cs | 63 ++++++++ .../DebuggerTestSuite/CallFunctionOnTests.cs | 151 ++++++++++++++++++ .../debugger/DebuggerTestSuite/Support.cs | 4 +- .../wasm/debugger/DebuggerTestSuite/Tests.cs | 39 ++++- .../wasm/debugger/tests/debugger-cfo-test.cs | 7 + src/mono/wasm/debugger/tests/other.js | 14 ++ src/mono/wasm/runtime/library_mono.js | 29 ++-- 8 files changed, 301 insertions(+), 14 deletions(-) diff --git a/src/mono/mono/mini/mini-wasm-debugger.c b/src/mono/mono/mini/mini-wasm-debugger.c index b179eb9434c14..d19d1a4cc55e5 100644 --- a/src/mono/mono/mini/mini-wasm-debugger.c +++ b/src/mono/mono/mini/mini-wasm-debugger.c @@ -1345,6 +1345,14 @@ EMSCRIPTEN_KEEPALIVE gboolean mono_wasm_invoke_getter_on_value (void *value, MonoClass *klass, const char *name) { DEBUG_PRINTF (2, "mono_wasm_invoke_getter_on_value: v: %p klass: %p, name: %s\n", value, klass, name); + if (!klass || !value) + return FALSE; + + if (!m_class_is_valuetype (klass)) { + DEBUG_PRINTF (2, "mono_wasm_invoke_getter_on_value: klass is not a valuetype. name: %s\n", mono_class_full_name (klass)); + return FALSE; + } + return invoke_getter (value, klass, name); } diff --git a/src/mono/wasm/debugger/DebuggerTestSuite/ArrayTests.cs b/src/mono/wasm/debugger/DebuggerTestSuite/ArrayTests.cs index f24c9372475af..f9319a7b491fd 100644 --- a/src/mono/wasm/debugger/DebuggerTestSuite/ArrayTests.cs +++ b/src/mono/wasm/debugger/DebuggerTestSuite/ArrayTests.cs @@ -628,5 +628,68 @@ public async Task InvalidArrayId() => await CheckInspectLocalsAtBreakpointSite( Assert.True(false, "Expected a numeric value part of the object id: {c_obj_id}"); await GetProperties($"dotnet:array:{{\"arrayId\":{idNum}}}", expect_ok : false); }); + + [Fact] + public async Task InvalidValueTypeArrayIndex() => await CheckInspectLocalsAtBreakpointSite( + "DebuggerTests.Container", "PlaceholderMethod", 1, "PlaceholderMethod", + "window.setTimeout(function() { invoke_static_method ('[debugger-test] DebuggerTests.ArrayTestsClass:ObjectArrayMembers'); }, 1);", + locals_fn : async(locals) => + { + var this_obj = GetAndAssertObjectWithName(locals, "this"); + var c_obj = GetAndAssertObjectWithName(await GetProperties(this_obj["value"]["objectId"].Value()), "c"); + var c_obj_id = c_obj["value"] ? ["objectId"]?.Value(); + Assert.NotNull(c_obj_id); + + var c_props = await GetProperties(c_obj_id); + + var pf_arr = GetAndAssertObjectWithName(c_props, "PointsField"); + var pf_arr_elems = await GetProperties(pf_arr["value"]["objectId"].Value()); + + if (!DotnetObjectId.TryParse(pf_arr_elems[0]["value"] ? ["objectId"]?.Value(), out var id)) + Assert.True(false, "Couldn't parse objectId for PointsFields' elements"); + + AssertEqual("valuetype", id.Scheme, "Expected a valuetype id"); + var id_args = id.ValueAsJson; + Assert.True(id_args["arrayId"] != null, "ObjectId format for array seems to have changed. Expected to find 'arrayId' in the value. Update this test"); + Assert.True(id_args != null, "Expected to get a json as the value part of {id}"); + + // Try one valid query, to confirm that the id format hasn't changed! + id_args["arrayIdx"] = 0; + await GetProperties($"dotnet:valuetype:{id_args.ToString (Newtonsoft.Json.Formatting.None)}", expect_ok : true); + + id_args["arrayIdx"] = 12399; + await GetProperties($"dotnet:valuetype:{id_args.ToString (Newtonsoft.Json.Formatting.None)}", expect_ok : false); + + id_args["arrayIdx"] = -1; + await GetProperties($"dotnet:valuetype:{id_args.ToString (Newtonsoft.Json.Formatting.None)}", expect_ok : false); + + id_args["arrayIdx"] = "qwe"; + await GetProperties($"dotnet:valuetype:{id_args.ToString (Newtonsoft.Json.Formatting.None)}", expect_ok : false); + }); + + [Fact] + public async Task InvalidAccessors() => await CheckInspectLocalsAtBreakpointSite( + "DebuggerTests.Container", "PlaceholderMethod", 1, "PlaceholderMethod", + "window.setTimeout(function() { invoke_static_method ('[debugger-test] DebuggerTests.ArrayTestsClass:ObjectArrayMembers'); }, 1);", + locals_fn : async(locals) => + { + var this_obj = GetAndAssertObjectWithName(locals, "this"); + var c_obj = GetAndAssertObjectWithName(await GetProperties(this_obj["value"]["objectId"].Value()), "c"); + var c_obj_id = c_obj["value"] ? ["objectId"]?.Value(); + Assert.NotNull(c_obj_id); + + var c_props = await GetProperties(c_obj_id); + + var pf_arr = GetAndAssertObjectWithName(c_props, "PointsField"); + + var invalid_accessors = new object[] { "NonExistant", "10000", "-2", 10000, -2, null, String.Empty }; + foreach (var invalid_accessor in invalid_accessors) + { + // var res = await InvokeGetter (JObject.FromObject (new { value = new { objectId = obj_id } }), invalid_accessor, expect_ok: true); + var res = await InvokeGetter(pf_arr, invalid_accessor, expect_ok : true); + AssertEqual("undefined", res.Value["result"] ? ["type"]?.ToString(), "Expected to get undefined result for non-existant accessor"); + } + }); + } } diff --git a/src/mono/wasm/debugger/DebuggerTestSuite/CallFunctionOnTests.cs b/src/mono/wasm/debugger/DebuggerTestSuite/CallFunctionOnTests.cs index f44c6a1242258..1afa398c1a28b 100644 --- a/src/mono/wasm/debugger/DebuggerTestSuite/CallFunctionOnTests.cs +++ b/src/mono/wasm/debugger/DebuggerTestSuite/CallFunctionOnTests.cs @@ -481,6 +481,36 @@ await insp.Ready(async(cli, token) => // callFunctionOn result = await ctx.cli.SendCommand("Runtime.callFunctionOn", cfo_args, ctx.token); await CheckValue(result.Value["result"], TNumber(5), "cfo-res"); + + cfo_args = JObject.FromObject(new + { + functionDeclaration = "function () { return 'test value'; }", + objectId = obj_id + }); + + // value of @returnByValue doesn't matter, as the returned value + // is a primitive + if (return_by_val) + cfo_args["returnByValue"] = return_by_val; + + // callFunctionOn + result = await ctx.cli.SendCommand("Runtime.callFunctionOn", cfo_args, ctx.token); + await CheckValue(result.Value["result"], JObject.FromObject(new { type = "string", value = "test value" }), "cfo-res"); + + cfo_args = JObject.FromObject(new + { + functionDeclaration = "function () { return null; }", + objectId = obj_id + }); + + // value of @returnByValue doesn't matter, as the returned value + // is a primitive + if (return_by_val) + cfo_args["returnByValue"] = return_by_val; + + // callFunctionOn + result = await ctx.cli.SendCommand("Runtime.callFunctionOn", cfo_args, ctx.token); + await CheckValue(result.Value["result"], JObject.Parse("{ type: 'object', subtype: 'null', value: null }"), "cfo-res"); }); } @@ -736,6 +766,127 @@ async Task GetPropertiesAndCheckAccessors(JObject get_prop_req, int num_ } } + public static TheoryData NegativeTestsData(bool use_cfo = false) => new TheoryData + { { "invoke_static_method ('[debugger-test] DebuggerTests.CallFunctionOnTest:MethodForNegativeTests', null);", "dotnet://debugger-test.dll/debugger-cfo-test.cs", 45, 12, use_cfo }, + { "negative_cfo_test ();", "/other.js", 41, 1, use_cfo } + }; + + [Theory] + [MemberData(nameof(NegativeTestsData), false)] + public async Task RunOnInvalidCfoId(string eval_fn, string bp_loc, int line, int col, bool use_cfo) => await RunCallFunctionOn( + eval_fn, "function() { return this; }", "ptd", + bp_loc, line, col, + test_fn : async(cfo_result) => + { + var ptd_id = cfo_result.Value?["result"] ? ["objectId"]?.Value(); + + var cfo_args = JObject.FromObject(new + { + functionDeclaration = "function () { return 0; }", + objectId = ptd_id + "_invalid" + }); + + var res = await ctx.cli.SendCommand("Runtime.callFunctionOn", cfo_args, ctx.token); + Assert.True(res.IsErr); + }); + + [Theory] + [MemberData(nameof(NegativeTestsData), false)] + public async Task RunOnInvalidThirdSegmentOfObjectId(string eval_fn, string bp_loc, int line, int col, bool use_cfo) + { + var insp = new Inspector(); + //Collect events + var scripts = SubscribeToScripts(insp); + + await Ready(); + await insp.Ready(async(cli, token) => + { + ctx = new DebugTestContext(cli, insp, token, scripts); + ctx.UseCallFunctionOnBeforeGetProperties = use_cfo; + await SetBreakpoint(bp_loc, line, col); + + // callFunctionOn + var eval_expr = $"window.setTimeout(function() {{ {eval_fn} }}, 1);"; + var result = await ctx.cli.SendCommand("Runtime.evaluate", JObject.FromObject(new { expression = eval_expr }), ctx.token); + var pause_location = await ctx.insp.WaitFor(Inspector.PAUSE); + + var frame_locals = await GetProperties(pause_location["callFrames"][0]["scopeChain"][0]["object"]["objectId"].Value()); + var ptd = GetAndAssertObjectWithName(frame_locals, "ptd"); + var ptd_id = ptd["value"]["objectId"].Value(); + + var cfo_args = JObject.FromObject(new + { + functionDeclaration = "function () { return 0; }", + objectId = ptd_id + "_invalid" + }); + + var res = await ctx.cli.SendCommand("Runtime.callFunctionOn", cfo_args, ctx.token); + Assert.True(res.IsErr); + }); + } + + [Theory] + [MemberData(nameof(NegativeTestsData), false)] + [MemberData(nameof(NegativeTestsData), true)] + public async Task InvalidPropertyGetters(string eval_fn, string bp_loc, int line, int col, bool use_cfo) + { + var insp = new Inspector(); + //Collect events + var scripts = SubscribeToScripts(insp); + + await Ready(); + await insp.Ready(async(cli, token) => + { + ctx = new DebugTestContext(cli, insp, token, scripts); + await SetBreakpoint(bp_loc, line, col); + ctx.UseCallFunctionOnBeforeGetProperties = use_cfo; + + // callFunctionOn + var eval_expr = $"window.setTimeout(function() {{ {eval_fn} }}, 1);"; + await SendCommand("Runtime.evaluate", JObject.FromObject(new { expression = eval_expr })); + var pause_location = await ctx.insp.WaitFor(Inspector.PAUSE); + + var frame_locals = await GetProperties(pause_location["callFrames"][0]["scopeChain"][0]["object"]["objectId"].Value()); + var ptd = GetAndAssertObjectWithName(frame_locals, "ptd"); + var ptd_id = ptd["value"]["objectId"].Value(); + + var invalid_args = new object[] { "NonExistant", String.Empty, null, 12310 }; + foreach (var invalid_arg in invalid_args) + { + var getter_res = await InvokeGetter(JObject.FromObject(new { value = new { objectId = ptd_id } }), invalid_arg); + AssertEqual("undefined", getter_res.Value["result"] ? ["type"]?.ToString(), $"Expected to get undefined result for non-existant accessor - {invalid_arg}"); + } + }); + } + + [Theory] + [MemberData(nameof(NegativeTestsData), false)] + public async Task ReturnNullFromCFO(string eval_fn, string bp_loc, int line, int col, bool use_cfo) => await RunCallFunctionOn( + eval_fn, "function() { return this; }", "ptd", + bp_loc, line, col, + test_fn : async(result) => + { + var is_js = bp_loc.EndsWith(".js"); + var ptd = JObject.FromObject(new { value = new { objectId = result.Value?["result"] ? ["objectId"]?.Value() } }); + + var null_value_json = JObject.Parse("{ 'type': 'object', 'subtype': 'null', 'value': null }"); + foreach (var returnByValue in new bool?[] { null, false, true }) + { + var res = await InvokeGetter(ptd, "StringField", returnByValue : returnByValue); + if (is_js) + { + // In js case, it doesn't know the className, so the result looks slightly different + Assert.True( + JObject.DeepEquals(res.Value["result"], null_value_json), + $"[StringField#returnByValue = {returnByValue}] Json didn't match. Actual: {res.Value ["result"]} vs {null_value_json}"); + } + else + { + await CheckValue(res.Value["result"], TString(null), "StringField"); + } + } + }); + /* * 1. runs `Runtime.callFunctionOn` on the objectId, * if @roundtrip == false, then diff --git a/src/mono/wasm/debugger/DebuggerTestSuite/Support.cs b/src/mono/wasm/debugger/DebuggerTestSuite/Support.cs index 3ff49682b5087..bce41ac4da7f2 100644 --- a/src/mono/wasm/debugger/DebuggerTestSuite/Support.cs +++ b/src/mono/wasm/debugger/DebuggerTestSuite/Support.cs @@ -443,7 +443,7 @@ internal async Task RunUntil(string methodName) return wait_res; } - internal async Task InvokeGetter(JToken obj, object arguments, string fn = "function(e){return this[e]}", bool expect_ok = true) + internal async Task InvokeGetter(JToken obj, object arguments, string fn = "function(e){return this[e]}", bool expect_ok = true, bool? returnByValue = null) { var req = JObject.FromObject(new { @@ -451,6 +451,8 @@ internal async Task InvokeGetter(JToken obj, object arguments, string fn objectId = obj["value"]?["objectId"]?.Value(), arguments = new[] { new { value = arguments } } }); + if (returnByValue != null) + req["returnByValue"] = returnByValue.Value; var res = await ctx.cli.SendCommand("Runtime.callFunctionOn", req, ctx.token); Assert.True(expect_ok == res.IsOk, $"InvokeGetter failed for {req} with {res}"); diff --git a/src/mono/wasm/debugger/DebuggerTestSuite/Tests.cs b/src/mono/wasm/debugger/DebuggerTestSuite/Tests.cs index ec7e43a4e3625..5ba31d8c37013 100644 --- a/src/mono/wasm/debugger/DebuggerTestSuite/Tests.cs +++ b/src/mono/wasm/debugger/DebuggerTestSuite/Tests.cs @@ -1485,6 +1485,43 @@ await insp.Ready(async(cli, token) => }); } + [Fact] + public async Task InvalidValueTypeData() + { + await CheckInspectLocalsAtBreakpointSite( + "dotnet://debugger-test.dll/debugger-test.cs", 85, 8, + "OuterMethod", + "window.setTimeout(function() { invoke_static_method ('[debugger-test] Math:OuterMethod'); })", + wait_for_event_fn : async(pause_location) => + { + var new_id = await CreateNewId(@"MONO._new_or_add_id_props ({ scheme: 'valuetype', idArgs: { containerId: 1 }, props: { klass: 3, value64: 4 }});"); + await _invoke_getter(new_id, "NonExistant", expect_ok : false); + + new_id = await CreateNewId(@"MONO._new_or_add_id_props ({ scheme: 'valuetype', idArgs: { containerId: 1 }, props: { klass: 3 }});"); + await _invoke_getter(new_id, "NonExistant", expect_ok : false); + + new_id = await CreateNewId(@"MONO._new_or_add_id_props ({ scheme: 'valuetype', idArgs: { containerId: 1 }, props: { klass: 3, value64: 'AA' }});"); + await _invoke_getter(new_id, "NonExistant", expect_ok : false); + }); + + async Task CreateNewId(string expr) + { + var res = await ctx.cli.SendCommand("Runtime.evaluate", JObject.FromObject(new { expression = expr }), ctx.token); + Assert.True(res.IsOk, "Expected Runtime.evaluate to succeed"); + AssertEqual("string", res.Value["result"] ? ["type"]?.Value(), "Expected Runtime.evaluate to return a string type result"); + return res.Value["result"] ? ["value"]?.Value(); + } + + async Task _invoke_getter(string obj_id, string property_name, bool expect_ok) + { + var expr = $"MONO._invoke_getter ('{obj_id}', '{property_name}')"; + var res = await ctx.cli.SendCommand("Runtime.evaluate", JObject.FromObject(new { expression = expr }), ctx.token); + AssertEqual(expect_ok, res.IsOk, "Runtime.evaluate result not as expected for {expr}"); + + return res; + } + } + //TODO add tests covering basic stepping behavior as step in/out/over } -} \ No newline at end of file +} diff --git a/src/mono/wasm/debugger/tests/debugger-cfo-test.cs b/src/mono/wasm/debugger/tests/debugger-cfo-test.cs index 077f4b12724ee..6e885646bf001 100644 --- a/src/mono/wasm/debugger/tests/debugger-cfo-test.cs +++ b/src/mono/wasm/debugger/tests/debugger-cfo-test.cs @@ -38,6 +38,13 @@ public static async System.Threading.Tasks.Task PropertyGettersTestAsync() System.Console.WriteLine("break here"); await System.Threading.Tasks.Task.CompletedTask; } + + public static void MethodForNegativeTests (string value = null) + { + var ptd = new ClassWithProperties { StringField = value }; + var swp = new StructWithProperties { StringField = value }; + Console.WriteLine("break here"); + } } class ClassWithProperties diff --git a/src/mono/wasm/debugger/tests/other.js b/src/mono/wasm/debugger/tests/other.js index 7ba8e66c8fa54..083614fc0ea05 100644 --- a/src/mono/wasm/debugger/tests/other.js +++ b/src/mono/wasm/debugger/tests/other.js @@ -31,3 +31,17 @@ function getters_js_test () { console.log (`break here`); return ptd; } + +function negative_cfo_test (str_value = null) { + var ptd = { + get Int () { return 5; }, + get String () { return "foobar"; }, + get DT () { return "dt"; }, + get IntArray () { return [1,2,3]; }, + get DTArray () { return ["dt0", "dt1"]; }, + DTAutoProperty: "dt", + StringField: str_value + }; + console.log (`break here`); + return ptd; +} \ No newline at end of file diff --git a/src/mono/wasm/runtime/library_mono.js b/src/mono/wasm/runtime/library_mono.js index 70d507d9c5dec..a285a7ce5f24b 100644 --- a/src/mono/wasm/runtime/library_mono.js +++ b/src/mono/wasm/runtime/library_mono.js @@ -202,7 +202,7 @@ var MonoSupportLib = { if (!isNaN (id.o.containerId)) this._get_object_properties (id.o.containerId, true); else if (!isNaN (id.o.arrayId)) - this._get_array_values (id, id.o.arrayIdx, 1, true); + this._get_array_values (id, Number (id.o.arrayIdx), 1, true); else throw new Error (`Invalid valuetype id (${id.idStr}). Can't get properties for it.`); @@ -309,7 +309,7 @@ var MonoSupportLib = { * @returns {object[]} */ _get_array_values: function (id, startIdx = 0, count = -1, expandValueTypes = false) { - if (isNaN (id.o.arrayId)) + if (isNaN (id.o.arrayId) || isNaN (startIdx)) throw new Error (`Invalid array id: ${id.idStr}`); let { res_ok, res } = this.mono_wasm_get_array_values_info (id.o.arrayId, startIdx, count, expandValueTypes); @@ -529,11 +529,10 @@ var MonoSupportLib = { switch (id.scheme) { case "object": { - const id_num = Number (id.value); - if (id_num === undefined) + if (isNaN (id.value)) throw new Error (`Invalid objectId: ${objectId}. Expected a numeric id.`); - return this._get_object_properties(id_num, false); + return this._get_object_properties(id.value, false); } case "array": @@ -590,8 +589,8 @@ var MonoSupportLib = { if (id_props === undefined) throw new Error (`Unknown valuetype id: ${objectIdStr}`); - if (id_props.value64 === undefined || id_props.klass === undefined) - throw new Error (`Bug: Cannot invoke getter on ${objectIdStr}, because of missing klass/value64 fields`); + if (typeof id_props.value64 !== 'string' || isNaN (id_props.klass)) + throw new Error (`Bug: Cannot invoke getter on ${objectIdStr}, because of missing or invalid klass/value64 fields. idProps: ${JSON.stringify (id_props)}`); const dataPtr = Module._malloc (id_props.value64.length); const dataHeap = new Uint8Array (Module.HEAPU8.buffer, dataPtr, id_props.value64.length); @@ -644,9 +643,12 @@ var MonoSupportLib = { const objId = request.objectId; let proxy; - if (objId in this._call_function_res_cache) { - proxy = this._call_function_res_cache [objId]; - } else if (!objId.startsWith ('dotnet:cfo_res:')) { + if (objId.startsWith ('dotnet:cfo_res:')) { + if (objId in this._call_function_res_cache) + proxy = this._call_function_res_cache [objId]; + else + throw new Error (`Unknown object id ${objId}`); + } else { proxy = this._create_proxy_from_object_id (objId); } @@ -654,8 +656,11 @@ var MonoSupportLib = { const fn_eval_str = `var fn = ${request.functionDeclaration}; fn.call (proxy, ...[${fn_args}]);`; const fn_res = eval (fn_eval_str); - if (fn_res == undefined) // should we just return undefined? - throw Error ('Function returned undefined result'); + if (fn_res === undefined) + return { type: "undefined" }; + + if (fn_res === null || (fn_res.subtype === 'null' && fn_res.value === undefined)) + return fn_res; // primitive type if (Object (fn_res) !== fn_res)