Closed
Bug 661573
Opened 13 years ago
Closed 13 years ago
Telemetry: Do not record/send data in private mode
Categories
(Toolkit :: Telemetry, defect)
Tracking
()
RESOLVED
FIXED
mozilla7
People
(Reporter: taras.mozilla, Unassigned)
References
Details
(Keywords: dev-doc-complete, privacy)
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
Since histograms can be viewed via about:telemetry, it would be nice to wipe them when leaving private mode.
Comment 1•13 years ago
|
||
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•13 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•13 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
Comment 4•13 years ago
|
||
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 | ||
Updated•13 years ago
|
Component: General → Telemetry
QA Contact: general → telemetry
Comment 6•13 years ago
|
||
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•13 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•13 years ago
|
||
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•13 years ago
|
||
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)
Comment 10•13 years ago
|
||
(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•13 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.
Comment 12•13 years ago
|
||
(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•13 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•13 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 15•13 years ago
|
||
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•13 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/48f9773dd8a0
Whiteboard: [inbound]
Target Milestone: --- → mozilla7
Comment 17•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/48f9773dd8a0
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Reporter | ||
Updated•13 years ago
|
Keywords: dev-doc-needed
Whiteboard: [inbound]
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•