1422035 - TelemetryEvent should assert if registerEvent is used in child processes

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P1)

Description

[Alessio Placitelli [:Dexter]]

registerScalars [1] can only be used in the parent process, however registerEvents [2] doesn't seem to enforce the same behaviour and, when used outside of the parent process, event recording won't work.

We should add an assertion to make this explicit.

[1] - https://searchfox.org/mozilla-central/rev/be78e6ea9b10b1f5b2b3b013f01d86e1062abb2b/toolkit/components/telemetry/TelemetryScalar.cpp#2338
[2] - https://searchfox.org/mozilla-central/rev/be78e6ea9b10b1f5b2b3b013f01d86e1062abb2b/toolkit/components/telemetry/TelemetryEvent.cpp#947

Comment 1

[Alessio Placitelli [:Dexter]]

Rohan, would you be interested in taking this one?

Comment 2

[Rohan Rajpal]

Yeah sure,i'll take this

Comment 3

[Rohan Rajpal]

So what i can understand is to add an assert statement to explicitly tell that registerevents also has to be used in parent process only,right?

Comment 4

[Alessio Placitelli [:Dexter]]

(In reply to Rohan Rajpal from comment #3)
> So what i can understand is to add an assert statement to explicitly tell
> that registerevents also has to be used in parent process only,right?

Yes, you can find an example of that in the code for the scalars in comment 0.

Comment 5

[Rohan Rajpal]

Means i have to add the statement :
MOZ_ASSERT(XRE_IsParentProcess(),
             "Dynamic scalars should only be created in the parent process.");

To registerevents as well,right?

Comment 6

[Rohan Rajpal]

(In reply to Rohan Rajpal from comment #5)
> Means i have to add the statement :
> MOZ_ASSERT(XRE_IsParentProcess(),
>              "Dynamic scalars should only be created in the parent
> process.");
> 
> To registerevents as well,right?

Did the same,building now.Do let me know how to test the build to check for this issue.
Thanks

Comment 7

[Alessio Placitelli [:Dexter]]

(In reply to Rohan Rajpal from comment #5)
> Means i have to add the statement :
> MOZ_ASSERT(XRE_IsParentProcess(),
>              "Dynamic scalars should only be created in the parent
> process.");
> 
> To registerevents as well,right?

Yes. You can test that with:

> ./mach gtest Telemetry*
> ./mach test toolkit/components/telemetry

Comment 8

[Rohan Rajpal]

A successfull build 
here are the logs for the two commands you gave to test with:
> ./mach gtest Telemetry*
https://pastebin.com/EaTRj7tV

> ./mach test toolkit/components/telemetry
https://pastebin.com/DAw8zrS9 (got an error here)

I've given the last few lines of the second command as it's way too big
Here is the part before that as well
https://pastebin.com/tatAXXbc

Sorry for the late reply ran into a few things

Comment 9

[Alessio Placitelli [:Dexter]]

(In reply to Rohan Rajpal from comment #8)
> A successfull build 
> here are the logs for the two commands you gave to test with:
> > ./mach gtest Telemetry*
> https://pastebin.com/EaTRj7tV
> 
> > ./mach test toolkit/components/telemetry
> https://pastebin.com/DAw8zrS9 (got an error here)
> 
> I've given the last few lines of the second command as it's way too big
> Here is the part before that as well
> https://pastebin.com/tatAXXbc
> 
> Sorry for the late reply ran into a few things

Looks like all the tests are passing: there's a weird Python error that looks unrelated though, so I think we can move on. Would you mind attaching the patch and flag me for review on it?

Comment 10

[Rohan Rajpal]

Alright i will,on it.

Comment 11

[Rohan Rajpal]

Attachment: This was the file that had to be edited to add an assert statement.

File is located at
mozilla-central/toolkit/components/telemetry/TelemetryEvent.cpp

Comment 12

[Alessio Placitelli [:Dexter]]

(In reply to Rohan Rajpal from comment #11)
> Created attachment 8939588 [details] [diff] [review]
> This was the file that had to be edited to add an assert statement.
> 
> File is located at
> mozilla-central/toolkit/components/telemetry/TelemetryEvent.cpp

Hi Rohan!

Your changes need to be submitted as a patch or as a MozReview request. You can have a look at this page [1] to understand how to generate a patch and attach it to this bug! Once the patch is attached, please flag me for review on it ;)

[1] - https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Creating_a_patch

Comment 13

[Rohan Rajpal]

Ohh my bad,on it.

Comment 14

[Rohan Rajpal]

Attachment: Bug 1422035 - Make RegisterEvents assert on children processes.

r?Dexter

Review commit: https://reviewboard.mozilla.org/r/210572/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/210572/

Comment 15

[Rohan Rajpal]

If the bug is fixed, can you assign me another one?

Comment 16

[Alessio Placitelli [:Dexter]]

Comment on attachment 8940275 [details]
Bug 1422035 - Make RegisterEvents assert on children processes.

https://reviewboard.mozilla.org/r/210572/#review216570

Thank you for your patch! In order to land this, we need to make some changes to the code. I've highlighted them below.

::: commit-message-e3048:1
(Diff revision 1)
> +Fixing bug 1422035

Please fix the commit message: it should describe what was done in the patch. See [this guide](https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions) for the commit message conventions.

::: my-change.patch:1
(Diff revision 1)
> +# HG changeset patch

I think this "my-change.patch" file was mistakenly added to the review request. Let's drop it.

::: toolkit/components/telemetry/TelemetryEvent.cpp:948
(Diff revision 1)
>  TelemetryEvent::RegisterEvents(const nsACString& aCategory,
>                                 JS::Handle<JS::Value> aEventData,
>                                 JSContext* cx)
>  {
> +  MOZ_ASSERT(XRE_IsParentProcess(),
> +             "Dynamic scalars should only be created in the parent process.");

Can you change the assertion message to something like: "Events can only be registered in the parent process"?

Comment 17

[Alessio Placitelli [:Dexter]]

Comment on attachment 8939588 [details] [diff] [review]
This was the file that had to be edited to add an assert statement.

(In reply to Rohan Rajpal from comment #15)
> If the bug is fixed, can you assign me another one?

I'm afraid the bug is not fixed yet :) The state of the bug will be changed to "RESOLVED FIXED" once it lands on mozilla-central. Before moving on to another bug, we should at least make sure it's landing without problems after the review is complete.

Comment 18

[Rohan Rajpal]

Attachment: Bug 1422035 - Added assert statement to TelemetryEvent.cpp by addiding asseertions on line 947.

Review commit: https://reviewboard.mozilla.org/r/211404/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/211404/

Comment 19

[Alessio Placitelli [:Dexter]]

Comment on attachment 8941129 [details]
Bug 1422035 - Added assert statement to TelemetryEvent.cpp by addiding asseertions on line 947.

https://reviewboard.mozilla.org/r/211404/#review217426

::: commit-message-1f87d:1
(Diff revision 1)
> +Bug 1422035 - Added assert statement to TelemetryEvent.cpp by addiding asseertions on line 947.r=Dexter

Let's not get too detailed about the implementation here. In general, the first line of a commit message should briefly explain what was done, high level. A good commit message in this case would be "Bug 1422035 - Make RegisterEvents assert on children processes. r?dexter"

::: toolkit/components/telemetry/TelemetryEvent.cpp:948
(Diff revision 1)
>  TelemetryEvent::RegisterEvents(const nsACString& aCategory,
>                                 JS::Handle<JS::Value> aEventData,
>                                 JSContext* cx)
>  {
>    MOZ_ASSERT(XRE_IsParentProcess(),
> -             "Dynamic scalars should only be created in the parent process.");
> +             "Events can only be registered in the parent process");

This changeset doesn't apply cleanly to mozilla-central: it requires your other patch. Please change this so that it applies cleanly to mozilla-central.

To do so you should probably edit your local commit history with `hg histedit`, drop this change and change your previous commit to address the requested feedback.

Comment 20

[Rohan Rajpal]

Comment on attachment 8940275 [details]
Bug 1422035 - Make RegisterEvents assert on children processes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/210572/diff/1-2/

Comment 21

[Rohan Rajpal]

Done with the necessary, let me know if anything else is wrong.

Comment 22

[Alessio Placitelli [:Dexter]]

Comment on attachment 8940275 [details]
Bug 1422035 - Make RegisterEvents assert on children processes.

https://reviewboard.mozilla.org/r/210572/#review218094

This looks good now, thanks!

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/686aaeae4c92
Make RegisterEvents assert on children processes. r=Dexter

Comment 24

[Rohan Rajpal]

Alright then, new bug suggestion?

Comment 25

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/686aaeae4c92