[browser] dev tools profiler - method name resolution - #115726
#115726[browser] dev tools profiler - method name resolution#115726
Conversation
|
Tagging subscribers to 'arch-wasm': @lewing |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an uninitialized buffer dereference and ensures overly long method names are handled correctly by zero-terminating snprintf. It also adds a stress test for long class names in the browser profiler and updates test configuration and scenario listings.
- Introduce a long-named class in
BrowserProfilerTestto validate profiler support for long symbols - Patch
main.jsto trackperformance.measure("TestMeaning")and adjust exit logic - Harden
mono_wasm_method_get_name_exindriver.cagainst null names and enforce zero-termination - Update diagnostics test setup and register it in the build scenarios list
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/mono/wasm/testassets/.../main.js | Override performance.measure, track invocation, update exit code |
| src/mono/wasm/testassets/.../BrowserProfilerTest.cs | Add ClassWithVeryVeryLongName… and instantiate in TestMeaning |
| src/mono/wasm/Wasm.Build.Tests/DiagnosticsTests.cs | Switch to extraProperties, remove direct output assertion |
| src/mono/browser/runtime/driver.c | Handle null method_name, zero-terminate snprintf buffer |
| eng/testing/scenarios/BuildWasmAppsJobsList.txt | Add DiagnosticsTests to scenario list |
Comments suppressed due to low confidence (4)
src/mono/wasm/testassets/WasmBasicTestApp/App/wwwroot/main.js:316
- The override of
performance.measureno longer calls the originalorigMeasure, which may break other performance measurements. Consider invokingorigMeasure(method, options)within the override after trackingfoundB.
globalThis.performance.measure = (method, options) => {
src/mono/browser/runtime/driver.c:526
- [nitpick] The size
128is a magic number. Define a named constant (e.g.MAX_METHOD_NAME_LEN) to clarify its purpose and make future adjustments easier.
res = (char *) malloc (128);
src/mono/wasm/Wasm.Build.Tests/DiagnosticsTests.cs:55
- The test no longer asserts that
performance.measure: TestMeaningwas called, potentially missing regressions in profiler activation. Reintroduce an assertion on the browser output or exit code to ensure the profiler hook ran.
await RunForBuildWithDotnetRun(new BrowserRunOptions(Configuration: config, TestScenario: "BrowserProfilerTest"));
src/mono/wasm/testassets/WasmBasicTestApp/App/wwwroot/main.js:315
- [nitpick] The variable
foundBis not descriptive. Consider renaming it tofoundTestMeaningor similar to clarify its purpose.
let foundB = false;
| } else if (method_name && mono_method_get_flags (method, NULL) & 0x0800 /* METHOD_ATTRIBUTE_SPECIAL_NAME */ && strlen (method_name) < 7) { | ||
| res = (char *) malloc (128); | ||
| snprintf (res, 128,"%s.%s", mono_class_get_name (mono_method_get_class (method)), method_name); | ||
| res[127] = '\0'; |
There was a problem hiding this comment.
this is harmless but snprintf should null terminate even at max length, is it really not?
There was a problem hiding this comment.
MS Doc
The _snprintf family of functions only appends a terminating NULL character if the formatted string length is strictly less than count characters.
Maybe emscripten libc is different, but I wanted to be sure.
snprintfdoesn't zero-terminate too long stringsFixes #115551