1341023 - CreateJSStackObject is not taking the encoding of the module name into account

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P3)

Description

[Marco Castelluccio [:marco]]

https://hg.mozilla.org/mozilla-central/file/16effd5b21ab03629feca04b5b83911bb757394c/toolkit/components/telemetry/Telemetry.cpp#l1592

It should actually use JS_NewUCStringCopyZ (like bug 1330833), as the module name might contain unicode characters:
https://hg.mozilla.org/integration/mozilla-inbound/file/8fc28f2d52d9/toolkit/components/telemetry/Telemetry.cpp#l1726

Comment 1

[Marco Castelluccio [:marco]]

Attachment: Patch

Comment 2

[Alessio Placitelli [:Dexter]]

Comment on attachment 8839250 [details] [diff] [review]
Patch

Review of attachment 8839250 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good Marco, thanks for the patch! There are just two tiny nits.

Could you also mention in the docs (both sections + late writes) that module names can contain unicode characters?

It's r+ with the fixes above.

::: toolkit/components/telemetry/ProcessedStack.h
@@ +31,5 @@
>      uint16_t mModIndex;
>    };
>    struct Module
>    {
>      // The file name, /foo/bar/libxul.so for example.

nit: can you also comment why we're switching to nsString? (i.e. module names can contain unicode characters).

::: toolkit/components/telemetry/tests/unit/test_TelemetryLateWrites.js
@@ +20,5 @@
>  const LOADED_MODULES = {
>    "4759A7E6993548C89CAF716A67EC242D00": "libtest.so",
>    "F77AF15BB8D6419FA875954B4A3506CA00": "libxul.so",
> +  "1E2F7FB590424E8F93D60BB88D66B8C500": "libc.so",
> +  "1E2F7FB590424E8F93D60BB88D66B8C500": "libmodμles.so",

nit: doesn't matter much, but can we use a different hash for "libmodμles.so", so that differs from the one from the previous line?

Comment 3

[Alessio Placitelli [:Dexter]]

The sections of the docs that need to be updated: http://searchfox.org/mozilla-central/rev/12cf11303392edac9f1da0c02e3d9ad2ecc8f4d3/toolkit/components/telemetry/docs/data/main-ping.rst#292,332,564

Comment 4

[Marco Castelluccio [:marco]]

Attachment: Patch

Carrying r+.

The failures look unrelated: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bd98c2ca321d81b4db58a44f0145c245827a34a&selectedJob=79105934.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbd5216f6a49
Take module name encoding into account for late writes and hangs. r=Dexter

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f52fb0ffd880
Add missing nsString.h include in ProcessedStack.h. r=me CLOSED TREE

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fbd5216f6a49
https://hg.mozilla.org/mozilla-central/rev/f52fb0ffd880