Closed
Bug 1341023
Opened 7 years ago
Closed 7 years ago
CreateJSStackObject is not taking the encoding of the module name into account
Categories
(Toolkit :: Telemetry, defect, P3)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: marco, Assigned: marco, Mentored)
Details
(Whiteboard: [measurement:client] [lang=c++])
Attachments
(1 file, 1 obsolete file)
9.17 KB,
patch
|
marco
:
review+
|
Details | Diff | Splinter Review |
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
Updated•7 years ago
|
Mentor: gfritzsche, alessio.placitelli
Priority: -- → P3
Whiteboard: [measurement:client] [lang=c++]
Assignee | ||
Comment 1•7 years ago
|
||
Assignee: nobody → mcastelluccio
Status: NEW → ASSIGNED
Attachment #8839250 -
Flags: review?(alessio.placitelli)
Comment 2•7 years ago
|
||
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?
Attachment #8839250 -
Flags: review?(alessio.placitelli) → review+
Comment 3•7 years ago
|
||
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
Assignee | ||
Comment 4•7 years ago
|
||
Carrying r+. The failures look unrelated: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bd98c2ca321d81b4db58a44f0145c245827a34a&selectedJob=79105934.
Attachment #8839250 -
Attachment is obsolete: true
Attachment #8839712 -
Flags: review+
Pushed by mcastelluccio@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/fbd5216f6a49 Take module name encoding into account for late writes and hangs. r=Dexter
Pushed by mcastelluccio@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f52fb0ffd880 Add missing nsString.h include in ProcessedStack.h. r=me CLOSED TREE
Comment 7•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fbd5216f6a49 https://hg.mozilla.org/mozilla-central/rev/f52fb0ffd880
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•