Closed Bug 1413621 Opened 3 years ago Closed 3 years ago

Odd warning in telemetry IPC code


(Toolkit :: Telemetry, enhancement, P2)




Tracking Status
firefox58 --- fixed


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


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


(1 file, 1 obsolete file)

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.
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

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++]
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)
Attachment #8926774 - Flags: review?(nika)
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+
Assignee: nobody → paul.bignier
Keywords: checkin-needed
Pushed by
Telemetry: replace NS_WARN_IF with MOZ_ASSERT. r=mystor
Keywords: checkin-needed
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.