661573 - Telemetry: Do not record/send data in private mode

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[(dormant account)]

Since histograms can be viewed via about:telemetry, it would be nice to wipe them when leaving private mode.

Comment 1

[Justin Dolske [:Dolske]]

Can you expand on this?

We shouldn't be recording anything sensitive when in PB mode, ergo there shouldn't be anything to wipe when leaving it. (Conversely, if the telemetry bits are not sensitive, which if what I thought, there shouldn't be any need to stop recording or wipe at all.)

Comment 2

[(dormant account)]

Sid & Security Review People were concerned that one would be able to figure out where someone went in private mode based on stats recorded(ie memory ballooning). I think that's a pretty low risk.

Idea here is to clear stats on privacy exit(and not to send pings during private mode) to mitigate this.

Comment 3

[(dormant account)]

On further reflection, it doesn't make sense to wipe pre-private mode data. Instead I'll just add a flag to make data recording a nop during private mode

Comment 4

[Sid Stamm [:geekboy or :sstamm]]

Yeah, it makes sense to just "pause" operations while in private mode, and resume them after leaving private mode.  It should appear (to the data and other users of the computer) like I didn't use Firefox while I was in private mode.

Comment 5

[(dormant account)]

Attachment: Turn off telemetry in private mode

Comment 6

[Dave Townsend [:mossop]]

Comment on attachment 539381 [details] [diff] [review]
Turn off telemetry in private mode

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

::: toolkit/components/telemetry/Telemetry.cpp
@@ +90,5 @@
>  private:
>    // This is used to cache JS string->TelemetryImpl::ID conversions
>    typedef map<const char*, Telemetry::ID, ltstr> NameHistogramMap;
>    NameHistogramMap mHistogramMap;
> +  bool mPrivateMode;

Just use PRBool (or PRPackedBool) for this rather than needing the casts.

Comment 7

[(dormant account)]

(In reply to comment #6)
> Just use PRBool (or PRPackedBool) for this rather than needing the casts.
We are moving away from prbool for safety reasons.

Comment 8

[(dormant account)]

Attachment: Turn off telemetry in private mode

Updated to match changes in bug 661574. Carrying over Mossop's review.

Mike, I would like you to review the GetInstance/GetSingleton dance. The idea is to offer a fast C++ey GetSingleton and keep GetInstance for XPCOMy stuff. Since you know xpcom better than me, perhaps you can suggest a cleaner way.

Comment 9

[(dormant account)]

Attachment: Turn off telemetry recording in private mode

The previous patch had some issues with nsITelemetry lifecycle. This time I changed my approach. Now I observe privacy notifications in only js and then disable telemetry when needed.
This patch also turns off local telemetry recording if telemetry is disabled. This may change once about:telemetry lands.

Comment 10

[Dave Townsend [:mossop]]

(In reply to comment #7)
> (In reply to comment #6)
> > Just use PRBool (or PRPackedBool) for this rather than needing the casts.
> We are moving away from prbool for safety reasons.

What reasons?

Comment 11

[(dormant account)]

(In reply to comment #10)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > Just use PRBool (or PRPackedBool) for this rather than needing the casts.
> > We are moving away from prbool for safety reasons.
> 
> What reasons?

see bug 266048 for details
Basically prbool is bad because it often does not limit self to 0/1 and accidentally leaks to/from other int types such as nsresult.

Comment 12

[Dave Townsend [:mossop]]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #7)
> > > (In reply to comment #6)
> > > > Just use PRBool (or PRPackedBool) for this rather than needing the casts.
> > > We are moving away from prbool for safety reasons.
> > 
> > What reasons?
> 
> see bug 266048 for details
> Basically prbool is bad because it often does not limit self to 0/1 and
> accidentally leaks to/from other int types such as nsresult.

Unless there is some global decision to stop using PRBool I think you should continue to use it for the public parts, like CanRecord.

Comment 13

[(dormant account)]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > (In reply to comment #7)
> > > > (In reply to comment #6)
> > > > > Just use PRBool (or PRPackedBool) for this rather than needing the casts.
> > > > We are moving away from prbool for safety reasons.
> > > 
> > > What reasons?
> > 
> > see bug 266048 for details
> > Basically prbool is bad because it often does not limit self to 0/1 and
> > accidentally leaks to/from other int types such as nsresult.
> 
> Unless there is some global decision to stop using PRBool I think you should
> continue to use it for the public parts, like CanRecord.

Well, Jonas has a plan to switch C++ boolean for the C++ side by the end of the year. We've been moving in the look-more-like-real-C++ direction for a while, so I don't see the benefit of using prbool. It saves a single !! conversion and introduces more suck.

Comment 14

[(dormant account)]

Comment on attachment 540871 [details] [diff] [review]
Turn off telemetry recording in private mode

Dave, can you r+/- this so we can move on?

Comment 15

[Dave Townsend [:mossop]]

Comment on attachment 540871 [details] [diff] [review]
Turn off telemetry recording in private mode

Jonas tells me we aren't doing the move to bool anytime soon but that it's fine to start using it now anyway

Comment 16

[(dormant account)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/48f9773dd8a0

Comment 17

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/48f9773dd8a0