1413621 - Odd warning in telemetry IPC code

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P2)

Description

[Nika Layzell [:nika] (ni? for response)]

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

Comment 1

[Georg Fritzsche [:gfritzsche]]

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?

Comment 2

[Chris H-C :chutten]

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.

Comment 3

[Paul Bignier]

Attachment: Telemetry: replace NS_WARN_IF with MOZ_ASSERT

Comment 4

[Nika Layzell [:nika] (ni? for response)]

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

Comment 5

[Paul Bignier]

Attachment: Telemetry: replace NS_WARN_IF with MOZ_ASSERT

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8926774 [details] [diff] [review]
Telemetry: replace NS_WARN_IF with MOZ_ASSERT

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

Thanks :-)

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ade6d760d37
Telemetry: replace NS_WARN_IF with MOZ_ASSERT. r=mystor

Comment 8

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/3ade6d760d37