TelemetryEvent should assert if registerEvent is used in child processes

RESOLVED FIXED in Firefox 59

Status

()

enhancement
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Dexter, Assigned: rohan17089, Mentored)

Tracking

Trunk
mozilla59
Points:
---

Firefox Tracking Flags

(firefox59 fixed)

Details

(Whiteboard: [lang=C++][good-first-bug])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

a year ago
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
(Reporter)

Updated

a year ago
Blocks: 1302663
Priority: -- → P2
(Reporter)

Updated

a year ago
Mentor: alessio.placitelli
Whiteboard: [lang=C++][good-first-bug]
(Reporter)

Comment 1

a year ago
Rohan, would you be interested in taking this one?
Flags: needinfo?(rohan17089)
(Assignee)

Comment 2

a year ago
Yeah sure,i'll take this
Flags: needinfo?(rohan17089)
(Reporter)

Updated

a year ago
Assignee: nobody → rohan17089
(Assignee)

Comment 3

a year ago
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?
(Reporter)

Comment 4

a year ago
(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.
(Assignee)

Comment 5

a year ago
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?
(Assignee)

Comment 6

a year ago
(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
(Reporter)

Comment 7

a year ago
(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
(Assignee)

Comment 8

a year ago
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
(Reporter)

Comment 9

a year ago
(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?
Flags: needinfo?(alessio.placitelli)
(Reporter)

Updated

a year ago
Flags: needinfo?(alessio.placitelli) → needinfo?(rohan17089)
(Assignee)

Comment 10

a year ago
Alright i will,on it.
Flags: needinfo?(rohan17089)
(Assignee)

Comment 11

a year ago
File is located at
mozilla-central/toolkit/components/telemetry/TelemetryEvent.cpp
Attachment #8939588 - Flags: review?(alessio.placitelli)
(Reporter)

Comment 12

a year ago
(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
Flags: needinfo?(rohan17089)
(Reporter)

Updated

a year ago
Attachment #8939588 - Flags: review?(alessio.placitelli)
(Assignee)

Comment 13

a year ago
Ohh my bad,on it.
Flags: needinfo?(rohan17089)
Priority: P2 → P1
Comment hidden (mozreview-request)
(Assignee)

Comment 15

a year ago
If the bug is fixed, can you assign me another one?
Flags: needinfo?(alessio.placitelli)
(Reporter)

Comment 16

a year ago
mozreview-review
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"?
Attachment #8940275 - Flags: review?(alessio.placitelli) → review-
(Reporter)

Comment 17

a year ago
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.
Attachment #8939588 - Attachment is obsolete: true
Flags: needinfo?(alessio.placitelli)
Comment hidden (mozreview-request)
(Reporter)

Updated

a year ago
Attachment #8940275 - Attachment is obsolete: true
(Reporter)

Updated

a year ago
Attachment #8940275 - Attachment is obsolete: false
(Reporter)

Comment 19

a year ago
mozreview-review
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.
Attachment #8941129 - Flags: review?(alessio.placitelli) → review-
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8941129 - Attachment is obsolete: true
(Assignee)

Comment 21

a year ago
Done with the necessary, let me know if anything else is wrong.
(Reporter)

Comment 22

a year ago
mozreview-review
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!
Attachment #8940275 - Flags: review?(alessio.placitelli) → review+

Comment 23

a year ago
Pushed by alessio.placitelli@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/686aaeae4c92
Make RegisterEvents assert on children processes. r=Dexter
(Assignee)

Comment 24

a year ago
Alright then, new bug suggestion?

Comment 25

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/686aaeae4c92
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.