Last Comment Bug 661573 - Telemetry: Do not record/send data in private mode
: Telemetry: Do not record/send data in private mode
Status: RESOLVED FIXED
: dev-doc-complete, privacy
Product: Toolkit
Classification: Components
Component: Telemetry (show other bugs)
: unspecified
: x86 Linux
: -- normal (vote)
: mozilla7
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on: 661574
Blocks: 659396
  Show dependency treegraph
 
Reported: 2011-06-02 10:24 PDT by (dormant account)
Modified: 2011-07-03 01:00 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Turn off telemetry in private mode (7.32 KB, patch)
2011-06-14 17:23 PDT, (dormant account)
dtownsend: review+
Details | Diff | Review
Turn off telemetry in private mode (8.47 KB, patch)
2011-06-15 12:48 PDT, (dormant account)
taras.mozilla: review+
Details | Diff | Review
Turn off telemetry recording in private mode (8.63 KB, patch)
2011-06-21 13:56 PDT, (dormant account)
dtownsend: review+
Details | Diff | Review

Description (dormant account) 2011-06-02 10:24:59 PDT
Since histograms can be viewed via about:telemetry, it would be nice to wipe them when leaving private mode.
Comment 1 Justin Dolske [:Dolske] 2011-06-02 13:46:30 PDT
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) 2011-06-02 13:58:34 PDT
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) 2011-06-13 15:03:08 PDT
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] 2011-06-13 15:06:18 PDT
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) 2011-06-14 17:23:38 PDT
Created attachment 539381 [details] [diff] [review]
Turn off telemetry in private mode
Comment 6 Dave Townsend [:mossop] 2011-06-15 09:46:40 PDT
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) 2011-06-15 09:50:14 PDT
(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) 2011-06-15 12:48:50 PDT
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.
Comment 9 (dormant account) 2011-06-21 13:56:09 PDT
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.
Comment 10 Dave Townsend [:mossop] 2011-06-22 11:48:22 PDT
(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) 2011-06-22 11:55:00 PDT
(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] 2011-06-22 12:01:10 PDT
(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) 2011-06-22 12:09:12 PDT
(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) 2011-06-28 13:18:40 PDT
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] 2011-06-28 13:23:27 PDT
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 17 Marco Bonardo [::mak] 2011-06-29 02:23:56 PDT
http://hg.mozilla.org/mozilla-central/rev/48f9773dd8a0

Note You need to log in before you can comment on or make changes to this bug.