Odd warning in telemetry IPC code

RESOLVED FIXED in Firefox 58

Status

()

enhancement
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Nika, Assigned: paul.bignier, Mentored)

Tracking

unspecified
mozilla58
Points:
---

Firefox Tracking Flags

(firefox58 fixed)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

While reading some telemetry ipc code, I noticed this code where we check whether ipcActor is null, and issue a warning if it is, keep running, and then proceed to dereference the actor pointer anyway.

It seems to me that this should be changed to a MOZ_ASSERT.

http://searchfox.org/mozilla-central/rev/aa1343961fca52e8c7dab16788530da31aab7c8d/toolkit/components/telemetry/ipc/TelemetryIPCAccumulator.cpp#293
It would be odd to not have valid values for these, they should be the singleton for the specific child process the code runs in.
MOZ_ASSERT() sounds good to me.

Chris, sounds good? Want to set it up as a mentored bug?
Flags: needinfo?(chutten)
Priority: -- → P2
Sure.

To fix this bug, do as Nika says in comment#0: Replace the NS_WARN_IF line in SendAccumulatedData with a MOZ_ASSERT with a helpful message.
Mentor: chutten
Flags: needinfo?(chutten)
Whiteboard: [good first bug][lang=c++]
Assignee

Comment 3

2 years ago
Assignee

Updated

2 years ago
Attachment #8926576 - Flags: review?(nika)
Comment on attachment 8926576 [details] [diff] [review]
Telemetry: replace NS_WARN_IF with MOZ_ASSERT

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

::: toolkit/components/telemetry/ipc/TelemetryIPCAccumulator.cpp
@@ +294,2 @@
>    if (histogramsToSend.Length()) {
> +    MOZ_ASSERT(ipcActor->SendAccumulateChildHistograms(histogramsToSend));

Please don't change these lines to MOZ_ASSERT. We only want to change line 293 to a MOZ_ASSERT, the other lines can still fail :-).
Attachment #8926576 - Flags: review?(nika)
Assignee

Updated

2 years ago
Attachment #8926774 - Flags: review?(nika)
Assignee

Updated

2 years ago
Attachment #8926576 - Attachment is obsolete: true
Comment on attachment 8926774 [details] [diff] [review]
Telemetry: replace NS_WARN_IF with MOZ_ASSERT

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

Thanks :-)
Attachment #8926774 - Flags: review?(nika) → review+
Reporter

Updated

2 years ago
Assignee: nobody → paul.bignier
Assignee

Updated

2 years ago
Keywords: checkin-needed

Comment 7

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ade6d760d37
Telemetry: replace NS_WARN_IF with MOZ_ASSERT. r=mystor
Keywords: checkin-needed

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3ade6d760d37
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.