The default bug view has changed. See this FAQ.

Telemetry: Do not record/send data in private mode

RESOLVED FIXED in mozilla7

Status

()

Toolkit
Telemetry
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: (dormant account), Unassigned)

Tracking

({dev-doc-complete, privacy})

unspecified
mozilla7
x86
Linux
dev-doc-complete, privacy
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
Since histograms can be viewed via about:telemetry, it would be nice to wipe them when leaving private mode.
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.)
(Reporter)

Comment 2

6 years ago
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.
(Reporter)

Comment 3

6 years ago
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
Depends on: 661574
Summary: Telemetry: Reset histograms when leaving private mode → Telemetry: Do not record/send data in private mode
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.
(Reporter)

Comment 5

6 years ago
Created attachment 539381 [details] [diff] [review]
Turn off telemetry in private mode
Attachment #539381 - Flags: review?(dtownsend)
(Reporter)

Updated

6 years ago
Component: General → Telemetry
QA Contact: general → telemetry
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.
Attachment #539381 - Flags: review?(dtownsend) → review+
(Reporter)

Comment 7

6 years ago
(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.
(Reporter)

Comment 8

6 years ago
Created attachment 539625 [details] [diff] [review]
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.
Attachment #539381 - Attachment is obsolete: true
Attachment #539625 - Flags: superreview?(mh+mozilla)
Attachment #539625 - Flags: review+
(Reporter)

Comment 9

6 years ago
Created attachment 540871 [details] [diff] [review]
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.
Attachment #539625 - Attachment is obsolete: true
Attachment #539625 - Flags: superreview?(mh+mozilla)
Attachment #540871 - Flags: review?(dtownsend)
(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?
(Reporter)

Comment 11

6 years ago
(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.
(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.
(Reporter)

Comment 13

6 years ago
(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.
(Reporter)

Comment 14

6 years ago
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 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
Attachment #540871 - Flags: review?(dtownsend) → review+
(Reporter)

Comment 16

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/48f9773dd8a0
Whiteboard: [inbound]
Target Milestone: --- → mozilla7
http://hg.mozilla.org/mozilla-central/rev/48f9773dd8a0
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Reporter)

Updated

6 years ago
Keywords: dev-doc-needed
Whiteboard: [inbound]

Updated

6 years ago
Keywords: dev-doc-needed → dev-doc-complete

Updated

6 years ago
Keywords: privacy
You need to log in before you can comment on or make changes to this bug.