1556025 - Add a comment for why we're storing the scalar name in KeyedScalars

Status: RESOLVED FIXED

(Toolkit :: Telemetry, task, P3)

Description

[Chris H-C :chutten]

In bug 1555798 Alessio noted that we should add a comment to KeyedScalar to explain why we're going through the dance of storing the scalar and then getting the info based on the name when we want to allocate a new scalar.

In short it's because references to BaseScalarInfo aren't guaranteed to be safe if they're DynamicScalarInfos (because their underlying storage is an nsTArray that can reshuffle its data).

Comment 1

[Chris H-C :chutten]

To help Mozilla out with this bug, here's the steps:

1. Comment here on the bug that you want to volunteer to help. I (or someone else) will assign it to you.
2. Download and build the Firefox source code
   • If you have any problems, please ask on IRC in the #introduction channel. They're there to help you get started.
   • You can also read the Developer Guide, which has answers to most development questions:
3. Start working on this bug. In this bug you'll be adding a code comment to the KeyedScalar constructor here. The comment should read something like // We store the name instead of a reference to the BaseScalarInfo because the BaseScalarInfo can move if it's from a dynamic scalar..
   • If you have any problems with this bug, please comment on this bug and set the needinfo flag for me. Also, you can find me and my teammates on the #telemetry channel on IRC most hours of most days.
4. Build your change with mach build and test your change with mach test toolkit/components/telemetry/tests/unit/. Also check your changes for adherence to our style guidelines by using mach lint
5. Submit the patch (including an automated test, if applicable) for review. Mark me as a reviewer so I'll get an email to come look at your code.
   • How to Submit a Patch
6. After a series of reviews and changes to your patch, I'll mark it for checkin or push it to autoland. Your code will soon be shipping to Firefox users worldwide!
7. ...now you get to think about what kind of bug you'd like to work on next. Let me know what you're interested in and I can help you find your next contribution.

Comment 2

[danielvictoriadbugzilla]

Hello Chris:

I would like to volunteer to help add this comment to the code.

Thanks,
Daniel

Comment 3

[Chris H-C :chutten]

Great! I have assigned this bug to you. Let me know if you have any questions.

Comment 4

[danielvictoriadbugzilla]

Attachment: Bug 1556025 - Added comment to explain that we store the scalar name in KeyedScalars because references to BaseScalarInfo aren't guaranteed to be safe if they're DynamicScalarInfos. r=[e-mail]

Comment 5

[danielvictoriadbugzilla]

Hey Chris I have posted the patch on the Phabricator, let me know if everything is Ok with it.

Comment 6

[danielvictoriadbugzilla]

Hi Chris, thanks for the review, I would like to contribute C++ code.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/95e8c5139082
Added comment to explain that we store the scalar name in KeyedScalars because references to BaseScalarInfo aren't guaranteed to be safe if they're DynamicScalarInfos. r=[e-mail]

Comment 8

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/95e8c5139082