1333578 - Telemetry::ScalarAdd call causes MOZ_CRASH(nsVariant not thread-safe) in debug build

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Comment 1

[Michael Froman [:mjf]]

After switching from using histograms to Scalars, I'm getting asserts on (at least) linux debug builds.  These calls are happening on the STS thread during teardown of PeerConnectionMedia but the original 

Hit MOZ_CRASH(nsVariant not thread-safe) at /home/mfroman/mozilla/moz-central/xpcom/ds/nsVariant.cpp:2199
#01: nsVariant::Release() (/home/mfroman/mozilla/moz-central/xpcom/ds/nsVariant.cpp:2199 (discriminator 3))
#02: nsCOMPtr<nsIVariant>::~nsCOMPtr() (/home/mfroman/mozilla/obj/deb/dist/include/nsCOMPtr.h:406)
#03: mozilla::Telemetry::ScalarAction::~ScalarAction() (/home/mfroman/mozilla/obj/deb/dist/include/mozilla/TelemetryComms.h:41)
#04: nsTArrayElementTraits<mozilla::Telemetry::ScalarAction>::Destruct(mozilla::Telemetry::ScalarAction*) (/home/mfroman/mozilla/obj/deb/dist/include/nsTArray.h:677)
#05: nsTArray_Impl<mozilla::Telemetry::ScalarAction, nsTArrayInfallibleAllocator>::DestructRange(unsigned long, unsigned long) (/home/mfroman/mozilla/obj/deb/dist/include/nsTArray.h:2041 (discriminator 1))
#06: nsTArray_Impl<mozilla::Telemetry::ScalarAction, nsTArrayInfallibleAllocator>::RemoveElementsAt(unsigned long, unsigned long) (/home/mfroman/mozilla/obj/deb/dist/include/nsTArray.h:2087)
[Child 19928] WARNING: Audio Buffer is not full by the end of the callback.: 'Available() == 0 || mSampleWriteOffset == 0', file /home/mfroman/mozilla/moz-central/dom/media/AudioBufferUtils.h, line 88
#07: nsTArray_Impl<mozilla::Telemetry::ScalarAction, nsTArrayInfallibleAllocator>::Clear() (/home/mfroman/mozilla/obj/deb/dist/include/nsTArray.h:1768)
#08: nsTArray_Impl<mozilla::Telemetry::ScalarAction, nsTArrayInfallibleAllocator>::~nsTArray_Impl() (/home/mfroman/mozilla/obj/deb/dist/include/nsTArray.h:999)
#09: nsTArray<mozilla::Telemetry::ScalarAction>::~nsTArray() (/home/mfroman/mozilla/obj/deb/dist/include/nsTArray.h:2252)
#10: TelemetryIPCAccumulator::IPCTimerFired(nsITimer*, void*) (/home/mfroman/mozilla/moz-central/toolkit/components/telemetry/TelemetryIPCAccumulator.cpp:181)
#11: nsTimerImpl::Fire() (/home/mfroman/mozilla/moz-central/xpcom/threads/nsTimerImpl.cpp:476)
#12: nsTimerEvent::Run() (/home/mfroman/mozilla/moz-central/xpcom/threads/TimerThread.cpp:305)
#13: nsThread::ProcessNextEvent(bool, bool*) (/home/mfroman/mozilla/moz-central/xpcom/threads/nsThread.cpp:1240)
#14: NS_ProcessNextEvent(nsIThread*, bool) (/home/mfroman/mozilla/moz-central/xpcom/glue/nsThreadUtils.cpp:390)
#15: nsThread::Shutdown() (/home/mfroman/mozilla/moz-central/xpcom/threads/nsThread.cpp:1009)
#16: nsThreadPool::Shutdown() (/home/mfroman/mozilla/moz-central/xpcom/threads/nsThreadPool.cpp:326 (discriminator 2))
#17: decltype (((*{parm#1}).*{parm#2})()) mozilla::detail::RunnableMethodArguments<>::applyImpl<nsIThreadPool, nsresult (nsIThreadPool::*)()>(nsIThreadPool*, nsresult (nsIThreadPool::*)(), mozilla::Tuple<>&, mozilla::IndexSequence<>) (/home/mfroman/mozilla/obj/deb/dist/include/nsThreadUtils.h:826 (discriminator 4))
#18: decltype (applyImpl({parm#1}, {parm#2}, (*this).mArguments, (mozilla::IndexSequence<>)())) mozilla::detail::RunnableMethodArguments<>::apply<nsIThreadPool, nsresult (nsIThreadPool::*)()>(nsIThreadPool*, nsresult (nsIThreadPool::*)()) (/home/mfroman/mozilla/obj/deb/dist/include/nsThreadUtils.h:832)
#19: mozilla::detail::RunnableMethodImpl<nsCOMPtr<nsIThreadPool>, nsresult (nsIThreadPool::*)(), true, false>::Run() (/home/mfroman/mozilla/obj/deb/dist/include/nsThreadUtils.h:862)
#20: nsThread::ProcessNextEvent(bool, bool*) (/home/mfroman/mozilla/moz-central/xpcom/threads/nsThread.cpp:1240)
#21: NS_ProcessNextEvent(nsIThread*, bool) (/home/mfroman/mozilla/moz-central/xpcom/glue/nsThreadUtils.cpp:390)
#22: nsThread::Shutdown() (/home/mfroman/mozilla/moz-central/xpcom/threads/nsThread.cpp:1009)
#23: nsThreadPool::Shutdown() (/home/mfroman/mozilla/moz-central/xpcom/threads/nsThreadPool.cpp:326 (discriminator 2))
#24: decltype (((*{parm#1}).*{parm#2})()) mozilla::detail::RunnableMethodArguments<>::applyImpl<nsIThreadPool, nsresult (nsIThreadPool::*)()>(nsIThreadPool*, nsresult (nsIThreadPool::*)(), mozilla::Tuple<>&, mozilla::IndexSequence<>) (/home/mfroman/mozilla/obj/deb/dist/include/nsThreadUtils.h:826 (discriminator 4))
#25: decltype (applyImpl({parm#1}, {parm#2}, (*this).mArguments, (mozilla::IndexSequence<>)())) mozilla::detail::RunnableMethodArguments<>::apply<nsIThreadPool, nsresult (nsIThreadPool::*)()>(nsIThreadPool*, nsresult (nsIThreadPool::*)()) (/home/mfroman/mozilla/obj/deb/dist/include/nsThreadUtils.h:832)
#26: mozilla::detail::RunnableMethodImpl<nsCOMPtr<nsIThreadPool>, nsresult (nsIThreadPool::*)(), true, false>::Run() (/home/mfroman/mozilla/obj/deb/dist/include/nsThreadUtils.h:862)
#27: nsThread::ProcessNextEvent(bool, bool*) (/home/mfroman/mozilla/moz-central/xpcom/threads/nsThread.cpp:1240)
#28: NS_ProcessNextEvent(nsIThread*, bool) (/home/mfroman/mozilla/moz-central/xpcom/glue/nsThreadUtils.cpp:390)
#29: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (/home/mfroman/mozilla/moz-central/ipc/glue/MessagePump.cpp:96)
#30: mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) (/home/mfroman/mozilla/moz-central/ipc/glue/MessagePump.cpp:302)
#31: MessageLoop::RunInternal() (/home/mfroman/mozilla/moz-central/ipc/chromium/src/base/message_loop.cc:239)
#32: MessageLoop::RunHandler() (/home/mfroman/mozilla/moz-central/ipc/chromium/src/base/message_loop.cc:232)
#33: MessageLoop::Run() (/home/mfroman/mozilla/moz-central/ipc/chromium/src/base/message_loop.cc:210)
#34: nsBaseAppShell::Run() (/home/mfroman/mozilla/moz-central/widget/nsBaseAppShell.cpp:158)
#35: XRE_RunAppShell() (/home/mfroman/mozilla/moz-central/toolkit/xre/nsEmbedFunctions.cpp:927)
#36: mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) (/home/mfroman/mozilla/moz-central/ipc/glue/MessagePump.cpp:269)
#37: MessageLoop::RunInternal() (/home/mfroman/mozilla/moz-central/ipc/chromium/src/base/message_loop.cc:239)
#38: MessageLoop::RunHandler() (/home/mfroman/mozilla/moz-central/ipc/chromium/src/base/message_loop.cc:232)
#39: MessageLoop::Run() (/home/mfroman/mozilla/moz-central/ipc/chromium/src/base/message_loop.cc:210)
#40: XRE_InitChildProcess(int, char**, XREChildData const*) (/home/mfroman/mozilla/moz-central/toolkit/xre/nsEmbedFunctions.cpp:763)
#41: mozilla::BootstrapImpl::XRE_InitChildProcess(int, char**, XREChildData const*) (/home/mfroman/mozilla/moz-central/toolkit/xre/Bootstrap.cpp:66)
#42: content_process_main(mozilla::Bootstrap*, int, char**) (/home/mfroman/mozilla/moz-central/browser/app/../../ipc/contentproc/plugin-container.cpp:115)
#43: main (/home/mfroman/mozilla/moz-central/browser/app/nsBrowserApp.cpp:326)
#44: __libc_start_main (/build/glibc-t3gR2i/glibc-2.23/csu/../csu/libc-start.c:325)
#45: _start (/home/mfroman/mozilla/obj/deb/dist/bin/firefox)
#46: ??? (???:???)

Comment 2

[Michael Froman [:mjf]]

(sorry, continuing description) but the original calls to Telemetry::Accumulate were happening on the same thread and same location.

The following should show the assert when running with the latest code from https://hg.mozilla.org/integration/autoland/rev/95d09880ddf6
 
./mach mochitest dom/media/tests/mochitest/test_peerConnection_basicAudioVideo.html

Comment 3

[Alessio Placitelli [:Dexter]]

(In reply to Michael Froman [:mjf] from comment #2)
> (sorry, continuing description) but the original calls to
> Telemetry::Accumulate were happening on the same thread and same location.
> 
> The following should show the assert when running with the latest code from
> https://hg.mozilla.org/integration/autoland/rev/95d09880ddf6
>  
> ./mach mochitest
> dom/media/tests/mochitest/test_peerConnection_basicAudioVideo.html

Thanks for reporting this! A couple of notes: the code from comment 2 is only recording data for the main process, but the calls happen on a different process. I left a comment on bug 1325536 for that. This wasn't a problem due to bug 1333024, which was fixed & landed yesterday.

Moreover, the probes you added are 'opt-in', but the test doesn't make sure extended telemetry is enabled, so no collection takes place when running the test with:

> ./mach mochitest dom/media/tests/mochitest/test_peerConnection_basicAudioVideo.html

No data gets collected (that would be true for opt-in histograms as well).

Comment 4

[Alessio Placitelli [:Dexter]]

Ok, I was able to reproduce by temporarily setting the scalars to opt-out and allowing the collection in child processes.

Comment 5

[Alessio Placitelli [:Dexter]]

The problem seems to happen due to [1] being called off the main thread.

[1] - http://searchfox.org/mozilla-central/rev/bf98cd4315b5efa1b28831001ad27d54df7bbb68/toolkit/components/telemetry/TelemetryIPCAccumulator.cpp#146,162

Comment 6

[Michael Froman [:mjf]]

(In reply to Alessio Placitelli [:Dexter] from comment #4)
> Ok, I was able to reproduce by temporarily setting the scalars to opt-out
> and allowing the collection in child processes.

For telemetry with local builds, I have to add the following lines to my .mozconfig file:
export MOZ_TELEMETRY_REPORTING=1
export MOZILLA_OFFICIAL=1

I'm not sure I did anything special to opt-in, but I was seeing the asserts in debug builds even if the data wasn't getting collected.  I had to look in {profile}/datareporting/archived/yyyy-mm/*.jsonlz4 to see data collected when running the non-debug local build and doing webrtc calls.  It was accumulating the Scalars based on the content of those files.

Comment 7

[Alessio Placitelli [:Dexter]]

(In reply to Michael Froman [:mjf] from comment #6)
> (In reply to Alessio Placitelli [:Dexter] from comment #4)
> > Ok, I was able to reproduce by temporarily setting the scalars to opt-out
> > and allowing the collection in child processes.
> 
> For telemetry with local builds, I have to add the following lines to my
> .mozconfig file:
> export MOZ_TELEMETRY_REPORTING=1
> export MOZILLA_OFFICIAL=1
> 
> I'm not sure I did anything special to opt-in, but I was seeing the asserts
> in debug builds even if the data wasn't getting collected.  I had to look in
> {profile}/datareporting/archived/yyyy-mm/*.jsonlz4 to see data collected
> when running the non-debug local build and doing webrtc calls.  It was
> accumulating the Scalars based on the content of those files.

Yes, that was due to bug 1333024. The fix for that bug landed today, which means data won't be collected if you update your local copy.

Comment 8

[Andrew McCreight [:mccr8]]

Do you have any ideas here, Nathan? nsPropertyBag is threadsafe, but somehow uses nsVariant. Maybe networking code is just good about only doing stuff with the variants on a single thread. I don't think it would be too hard to make a threadsafe version of a variant, assuming that makes sense for the underlying data.

Comment 9

[Andrew McCreight [:mccr8]]

Looking for nsIVariant, I do see Variant_base, which uses threadsafe refcounting. I'm not sure how usable that is outside of storage stuff.

http://searchfox.org/mozilla-central/source/storage/Variant.h#40

Comment 10

[Alessio Placitelli [:Dexter]]

Attachment: bug1333578.patch

Comment 11

[Alessio Placitelli [:Dexter]]

Comment on attachment 8830649 [details] [diff] [review]
bug1333578.patch

The crash from the previous comment is only happening when accumulating scalars from a child process and on a non-main thread. That's because when accumulating on the main process, scalars are stored as basic uints [1], bools and strings: Variants are only used to propagate the values from JS land down to the final storage.

When accumulating in a child process, each change action is stored as a Variant, which is then sent to the parent process to build up the final value. This works fine when calling accumulation functions from the main thread, but crashes when the Telemetry::Scalar* functions are called from a non-main thread. And that's because nsVariant doesn't implement thread-safe ref counting.

This patch insures that |TelemetryIPCAccumulator::RecordChild*| calls happen on the main thread when called from the C++ API.

I have a few doubts/open questions about this approach:

a) Can we have Scalar JS API calls off the main thread? Is that possible at all?
b) Does dispatching the scalar accumulation function to the main thread create a performance bottle neck?

With that said, the other options I could think of are:

1. Implement a thread-safely refcounted Variant.
2. Completely get rid of Variants (that would be painful, especially for the IPC side of things, since we would need to create a message for each supported type).

Georg, what do you think of the suggested approach? Do you have any other idea/suggestion?

Ah, this patch also adds a new test to TelemetryScalars.cpp that tests the off-the-main thread calls in the parent process. Just to be safe.

[1] - http://searchfox.org/mozilla-central/rev/bf98cd4315b5efa1b28831001ad27d54df7bbb68/toolkit/components/telemetry/TelemetryScalar.cpp#228
[2] - http://searchfox.org/mozilla-central/rev/bf98cd4315b5efa1b28831001ad27d54df7bbb68/toolkit/components/telemetry/TelemetryComms.h#46

Comment 12

[Georg Fritzsche [:gfritzsche]]

(In reply to Alessio Placitelli [:Dexter] from comment #11)
> a) Can we have Scalar JS API calls off the main thread? Is that possible at
> all?

Yes, we need to support that. Gecko is heavily multi-threaded and users of the Telemetry API should not need to worry about where the call comes from.

> b) Does dispatching the scalar accumulation function to the main thread
> create a performance bottle neck?

I don't think i can answer that well, but this sounds like dispatch overhead just to work around a limitation of the nsVariant.

> 1. Implement a thread-safely refcounted Variant.

... or check if nsVariant could be "fixed", or if there is an existing threadsafe variant.

> 2. Completely get rid of Variants (that would be painful, especially for the
> IPC side of things, since we would need to create a message for each
> supported type).

That's just 3 types * 2 (keyed/unkeyed) right now?
Where key could be an optional property, reducing it to three?
I like the easy to maintain approach we have now, but if its too much effort to keep lets just implement the messages.

Comment 13

[Nathan Froyd [:froydnj]]

(In reply to Andrew McCreight [:mccr8] from comment #8)
> Do you have any ideas here, Nathan? nsPropertyBag is threadsafe, but somehow
> uses nsVariant.

IRC discussion suggested looking into mozilla::Variant to handle things here, which has the benefit of avoiding refcounting entirely and might even make the code more pleasant.

Comment 14

[Alessio Placitelli [:Dexter]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #12)
> (In reply to Alessio Placitelli [:Dexter] from comment #11)
> > a) Can we have Scalar JS API calls off the main thread? Is that possible at
> > all?
> 
> Yes, we need to support that. Gecko is heavily multi-threaded and users of
> the Telemetry API should not need to worry about where the call comes from.
> 
> > b) Does dispatching the scalar accumulation function to the main thread
> > create a performance bottle neck?
> 
> I don't think i can answer that well, but this sounds like dispatch overhead
> just to work around a limitation of the nsVariant.

Good point!

> > 1. Implement a thread-safely refcounted Variant.
> 
> ... or check if nsVariant could be "fixed", or if there is an existing
> threadsafe variant.

bz/froydnj suggested to have a look at mozilla::Variant [1] (docs [2]). At the very least we should not be using nsVariant in this case, as "nsVariant is definitely not threadsafe and can't really be. It's worse, in fact; I expect that trying to refcount it on the STS thread or some such would crash even if all the refcounting were limited to that thread. Because it's cycle collected, but the STS thread doesn't have cycle collection, I expect" (quoting :bz).

> > 2. Completely get rid of Variants (that would be painful, especially for the
> > IPC side of things, since we would need to create a message for each
> > supported type).
> 
> That's just 3 types * 2 (keyed/unkeyed) right now?
> Where key could be an optional property, reducing it to three?
> I like the easy to maintain approach we have now, but if its too much effort
> to keep lets just implement the messages.

Yeah, I guess we'll get to that if the mozilla::Variant thing doesn't help

[1] - http://searchfox.org/mozilla-central/source/mfbt/Variant.h
[2] - http://searchfox.org/mozilla-central/source/mfbt/Variant.h#334

Comment 15

[Andrew McCreight [:mccr8]]

This is just a nitpick, but nsVariant is not cycle collected, so using it entirely on one thread is okay. Networking code does this. nsCCVariant is the variant (no pun intended) that is cycle collected, and so must be used only on a thread that is cycle collected.

Comment 16

[Alessio Placitelli [:Dexter]]

Attachment: bug1333578.patch

Georg, this patch uses mozilla::Variant in place of nsIVariants to power the IPC communications. The crash from comment 0 seems to be fixed.

This also adds a gtest that tests the C++ API from a different thread on the main process.

I left 2 TODO comments in the code, in case you have better ideas about how to handle the two ugly blocks there.

I'll flag Nathan to cross-check TelemetryComms.h once it looks ok to you.

Please note that I wasn't able to use nsAutoString with mozilla::Variant, probably due to the fact that the string lived on the stack. Their value evaluated to garbage in TelemetryIPCAccumulation::IPCTimer, so I switched back to nsString. Maybe Nathan will have some pointers on this.

Comment 17

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8831078 [details] [diff] [review]
bug1333578.patch

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

::: toolkit/components/telemetry/TelemetryComms.h
@@ +37,5 @@
>    eAdd = 1,
>    eSetMaximum = 2
>  };
>  
> +typedef mozilla::Variant<uint32_t, bool, nsString> ScalarDataType;

Nit: This could use a better name - ScalarVariant?

@@ +46,5 @@
>    uint32_t mScalarType;
>    ScalarActionType mActionType;
> +  // We need to wrap mData in a Maybe otherwise the IPC system
> +  // is unable to instantiate a ScalarAction.
> +  Maybe<ScalarDataType> mData;

This sounds like you actually want to use a pointer - `Maybe` is for optional data and `mData` is not optional.

Are you sure though you can't just instantiate this though using a ctor or other means?
This introduces multiple potential paths for failure/checking/... in other parts of the code.

@@ +138,3 @@
>      switch(aParam.mScalarType) {
>        case nsITelemetry::SCALAR_COUNT:
> +        WriteParam(aMsg, aParam.mData->as<uint32_t>());

So, i assume we don't have any existing IPC (de)serialization code for variants?
Or are there other reasons to do this manually?

::: toolkit/components/telemetry/TelemetryScalar.cpp
@@ +115,5 @@
>  }
>  
>  /**
> + * Convert a nsIVariant to a mozilla::Variant, which is used for
> + * accumulating child process scalars.

Is it possible to just use mozilla::Variant everywhere instead?

@@ +2136,5 @@
>      }
>  
> +    if (upd.mData.isNothing()) {
> +      NS_WARNING("There is no data in the ScalarActionType.");
> +      continue;

This should never happen, MOZ_ASSERT() it.

@@ +2217,5 @@
>      }
>  
> +    if (upd.mData.isNothing()) {
> +      NS_WARNING("There is no data in the KeyedScalarAction.");
> +      continue;

MOZ_ASSERT().

Comment 18

[Alessio Placitelli [:Dexter]]

Attachment: bug1333578.patch

> @@ +46,5 @@
> >    uint32_t mScalarType;
> >    ScalarActionType mActionType;
> > +  // We need to wrap mData in a Maybe otherwise the IPC system
> > +  // is unable to instantiate a ScalarAction.
> > +  Maybe<ScalarDataType> mData;
> 
> This sounds like you actually want to use a pointer - `Maybe` is for
> optional data and `mData` is not optional.
> 
> Are you sure though you can't just instantiate this though using a ctor or
> other means?
> This introduces multiple potential paths for failure/checking/... in other
> parts of the code.

After discussing with Georg over IRC, I'm leaving this as it is.

The background is that I had a conversation with Nathan yesterday, because I had a problem with using the Variant as a member of ScalarActionType. The background is that making mozilla::Variant a member of the ScalarActionType would break the IPC mechanism as mozilla::Variant does not have a default () constructor and the IPC mumbo jumbo doesn't like that.

There were 2 options:

- add a default constructor to ScalarDataType who inits mData to a dummy value
- use Maybe (Nathan suggested using this one)

As discussed, I'm leaving the TelemetryComms.h bits for Nathan to review, along with the two portions of code with the TODO bits.

I'm flagging you again for feedback.

> @@ +138,3 @@
> >      switch(aParam.mScalarType) {
> >        case nsITelemetry::SCALAR_COUNT:
> > +        WriteParam(aMsg, aParam.mData->as<uint32_t>());
> 
> So, i assume we don't have any existing IPC (de)serialization code for
> variants?
> Or are there other reasons to do this manually?

I don't think we do or, at least, I couldn't find any in IPCMEssageUtils.h.

> ::: toolkit/components/telemetry/TelemetryScalar.cpp
> @@ +115,5 @@
> >  }
> >  
> >  /**
> > + * Convert a nsIVariant to a mozilla::Variant, which is used for
> > + * accumulating child process scalars.
> 
> Is it possible to just use mozilla::Variant everywhere instead?

I think so. Let me try to address that part in this bug as a separate patch. If it gets too time consuming,
I'll file a new bug and move on from there.

Comment 19

[Alessio Placitelli [:Dexter]]

Comment on attachment 8831138 [details] [diff] [review]
bug1333578.patch

Nathan, would you mind reviewing the usages of mozilla::Variant in this patch? It's spread to many different files, but it's mostly the same use.

I left 2 TODOs inside the patch, in case you have ideas on how to handle those parts better.

Comment 20

[Alessio Placitelli [:Dexter]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #17)
> ::: toolkit/components/telemetry/TelemetryScalar.cpp
> @@ +115,5 @@
> >  }
> >  
> >  /**
> > + * Convert a nsIVariant to a mozilla::Variant, which is used for
> > + * accumulating child process scalars.
> 
> Is it possible to just use mozilla::Variant everywhere instead?

Filed bug 1335009 for this.

Comment 21

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8831138 [details] [diff] [review]
bug1333578.patch

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

Overall i'm fine with the approach here after our discussion with the follow-up filed.
I'll leave the details to the review.

Comment 22

[Nathan Froyd [:froydnj]]

Comment on attachment 8831138 [details] [diff] [review]
bug1333578.patch

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

r-'ing for the IPC issues only.

::: toolkit/components/telemetry/TelemetryComms.h
@@ +130,5 @@
>      WriteParam(aMsg, aParam.mScalarType);
>      WriteParam(aMsg, static_cast<uint32_t>(aParam.mActionType));
>  
> +    if (aParam.mData.isNothing()) {
> +      MOZ_ASSERT(false, "There is no data in the ScalarActionType.");

This is not going to work well if mData.isNothing ever happened to be true in non-debug builds: |Write()| wouldn't write anything, and then the |Read()| on the other end would be very confused.  MOZ_CRASH()'ing here is probably the best choice.

@@ +148,2 @@
>        default:
>          MOZ_ASSERT(false, "Unknown scalar type.");

The same logic applies here as well.

::: toolkit/components/telemetry/TelemetryScalar.cpp
@@ +2140,5 @@
> +      continue;
> +    }
> +
> +    // Extract the data from the mozilla::Variant.
> +    // TODO: Is there a better way to do that?

There's probably not for these cases, except perhaps for non-keyed ScalarActionType::eSet, where you could more easily get away with Variant::match.

(As an aside, consider removing mScalarType and letting the variant reflect both the type and the data it contains, so you're not maintaining redundant data.)

Comment 23

[Nathan Froyd [:froydnj]]

Comment on attachment 8831138 [details] [diff] [review]
bug1333578.patch

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

::: toolkit/components/telemetry/TelemetryScalar.cpp
@@ +130,5 @@
> +        if (NS_FAILED(rv)) {
> +          return ScalarResult::CannotUnpackVariant;
> +        }
> +        aOutput = mozilla::Some(mozilla::AsVariant(val));
> +      } break;

Oh, and please don't hide the |break;| after a block like this.  Please put it on its own line.  (Several instances throughout the patch.)

Comment 24

[Alessio Placitelli [:Dexter]]

Attachment: bug1333578.patch

Thanks Georg!

Nathan, I think I've addressed all your concerns in this patch. 

Just two observations:

- I kept the nested switches for the non-keyed scalars too, as using a matcher there resulted into the wrong value being recorded (didn't dig too much though, as it didn't make a big change in terms of LOCs).
- The |mScalarType| is only really needed for the IPC Read functions as, otherwise, I wouldn't know what data type to read there (unless I'm missing something, which could be the case).

Thank you for your time!

Comment 25

[Nathan Froyd [:froydnj]]

(In reply to Alessio Placitelli [:Dexter] from comment #24)
> - The |mScalarType| is only really needed for the IPC Read functions as,
> otherwise, I wouldn't know what data type to read there (unless I'm missing
> something, which could be the case).

You can call mData.as<nsString>(), which will return true is it's currently storing an `nsString`, and false otherwise.

Comment 26

[Alessio Placitelli [:Dexter]]

Attachment: bug1333578.patch

After discussing over IRC, here's the new patch that gets rid of the mScalarType field in the Scalar/KeyedScalarAction

Comment 27

[Nathan Froyd [:froydnj]]

Comment on attachment 8831788 [details] [diff] [review]
bug1333578.patch

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

Thanks!

Comment 28

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e841ec102f71

Comment 29

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3e3bd2d9ea2d0dc0b99e18753da9b364f2bedcb
Bug 1333578 - Use mozilla::Variant instead of nsIVariant for child processes scalars. r=froydnj, f=gfritzsche

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e3e3bd2d9ea2

Comment 31

[Georg Fritzsche [:gfritzsche]]

Bug 1278556 landed on 53.
Do we have any content scalar users of this on 53 or can we set 53 to wontfix?

Comment 32

[Alessio Placitelli [:Dexter]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #31)
> Bug 1278556 landed on 53.
> Do we have any content scalar users of this on 53 or can we set 53 to
> wontfix?

We don't, right now. If bug 1325536 gets uplifted, we will. Setting to wontfix for now.