Closed Bug 1333024 Opened 4 years ago Closed 4 years ago

Scalars record even when not allowed in the process when using the C++ API

Categories

(Toolkit :: Telemetry, defect, P1)

defect
Points:
2

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox53 --- affected
firefox54 --- fixed

People

(Reporter: Dexter, Assigned: Dexter)

References

Details

(Whiteboard: [measurement:client])

Attachments

(1 file)

When using the C++ API to change scalars from child processes, we don't check if we're expected to record from the process [1].

This is happening for all the scalars operations, but it's not happening for the parent process, which looks ok as internal_GetRecordableScalar checks for that too [2].

[1] - http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/toolkit/components/telemetry/TelemetryScalar.cpp#1397
[2] - http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/toolkit/components/telemetry/TelemetryScalar.cpp#1031
Blocks: 1278556
Points: --- → 2
Priority: -- → P1
Whiteboard: [measurement:client]
Assignee: nobody → alessio.placitelli
Status: NEW → ASSIGNED
Attached patch bug1333024.patchSplinter Review
Chris, any chance you could review this? No rush though :)

The background here is that the C++ API paths were not checking if we were allowed to record scalar data in the current process. The JS API was. Part of the problem was that the function checking for the "permissions" were different: I've refactored the code a bit to use the same code in both places.

I've also filed bug 1333026 to extend the C++ API test coverage to content scalars as well.
Attachment #8829453 - Flags: review?(chutten)
Comment on attachment 8829453 [details] [diff] [review]
bug1333024.patch

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

I will be even happier when this is tested :)
Attachment #8829453 - Flags: review?(chutten) → review+
Pushed by alessio.placitelli@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ab1592a1034
Don't record scalars with the C++ API if not allowed in the current process. r=chutten
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ab1592a10342f9ba513e0ffb3a04d1df67bd3aa
Bug 1333024 - Don't record scalars with the C++ API if not allowed in the current process. r=chutten
Whiteboard: [measurement:client] → [measurement:client][measurement:client:uplift]
https://hg.mozilla.org/mozilla-central/rev/4ab1592a1034
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Whiteboard: [measurement:client][measurement:client:uplift] → [measurement:client]
Blocks: 1325536
You need to log in before you can comment on or make changes to this bug.