Last Comment Bug 783005 - need a system for deleting pings after N failed attempts
: need a system for deleting pings after N failed attempts
Status: RESOLVED FIXED
[qa-]
:
Product: Toolkit
Classification: Components
Component: Telemetry (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla18
Assigned To: Nathan Froyd [:froydnj]
: telemetry
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-15 09:59 PDT by Nathan Froyd [:froydnj]
Modified: 2012-10-18 09:13 PDT (History)
5 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
patch (4.03 KB, patch)
2012-09-27 11:16 PDT, Nathan Froyd [:froydnj]
taras.mozilla: review+
bajaj.bhavana: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Nathan Froyd [:froydnj] 2012-08-15 09:59:09 PDT
As noted in bug 715299 comment 1 and 715299 comment 2, we don't have any way of deleting ping data from disk save by sending those pings in.  And if we don't send those pings in (e.g. short sessions, disconnected from the network for a long time, ping server downtime, etc.), they will live on the disk indefinitely, which is bad from a privacy perspective.

We should have a better way of handling this.
Comment 1 (dormant account) 2012-08-15 10:10:15 PDT
We could use timestamps and delete pings > 1 week old
Comment 2 Nathan Froyd [:froydnj] 2012-09-27 11:16:18 PDT
Created attachment 665552 [details] [diff] [review]
patch
Comment 3 (dormant account) 2012-09-27 15:30:36 PDT
Comment on attachment 665552 [details] [diff] [review]
patch

I think that's ok assuming the other codepath exercises the same expiry code and does not evict saved pings with a more recent timestamp
Comment 4 Nathan Froyd [:froydnj] 2012-10-01 08:59:55 PDT
(In reply to Taras Glek (:taras) from comment #3)
> I think that's ok assuming the other codepath exercises the same expiry code
> and does not evict saved pings with a more recent timestamp

I made this a little more robust by sticking the check in loadHistograms rather than loadSavedPings; that way the tests exercise the codepath too.  (The tests were subtly wrong before because I rebased incorrectly.)

https://hg.mozilla.org/integration/mozilla-inbound/rev/acb61f5a441c
Comment 5 Ryan VanderMeulen [:RyanVM] 2012-10-01 19:00:51 PDT
https://hg.mozilla.org/mozilla-central/rev/acb61f5a441c
Comment 6 Nathan Froyd [:froydnj] 2012-10-05 09:13:37 PDT
Comment on attachment 665552 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): adding persistent telemetry
User impact if declined: Persistent telemetry was approved on the assumption ping files would be short-lived on disk.  This is not necessarily true with the current system.  Users may have private data exposed through these long-lived ping files.
Testing completed (on m-c, etc.): On m-c for ~2 weeks.
Risk to taking this patch (and alternatives if risky): Low risk.
String or UUID changes made by this patch: None.
Comment 7 bhavana bajaj [:bajaj] 2012-10-05 15:25:10 PDT
Comment on attachment 665552 [details] [diff] [review]
patch

approving for aurora . Please land before monday oct 8th merge.
Comment 9 Justin Wood (:Callek) 2012-10-08 01:14:59 PDT
Backed out of aurora for XPCshell bustage

TEST-INFO | (xpcshell/head.js) | exiting test
/Users/cltbld/talos-slave/test/build/xpcshell/tests/toolkit/components/telemetry/tests/unit/test_TelemetryPing.js:87: NS_ERROR_FILE_TARGET_DOES_NOT_EXIST: Component returned failure code: 0x80520006 (NS_ERROR_FILE_TARGET_DOES_NOT_EXIST) [nsIFile.remove]
WARNING: nsExceptionService ignoring thread destruction after shutdown: file ../../../xpcom/base/nsExceptionService.cpp, line 166
Assertion failure: !connections[i]->ConnectionReady(), at ../../../storage/src/mozStorageService.cpp:853
<<<<<<<

e.g. https://tbpl.mozilla.org/php/getParsedLog.php?id=15913417&tree=Mozilla-Aurora

https://hg.mozilla.org/releases/mozilla-aurora/rev/8ce7c8f9059f
Comment 10 Nathan Froyd [:froydnj] 2012-10-08 08:14:12 PDT
Re-pushed:

https://hg.mozilla.org/releases/mozilla-aurora/rev/5979ad5526e8

This bug was committed after bug 783054, which included this hunk in test_TelemetryPing.js in one of its patches:

https://hg.mozilla.org/integration/mozilla-inbound/rev/08764617f060#l1.12

The absence of bug 783054 on aurora accounts for the failure Justin saw.  I felt that this hunk was trivial enough to bring over without asking for re-approval.
Comment 11 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-10-17 16:01:23 PDT
Is there something QA can do to verify this fix?
Comment 12 Nathan Froyd [:froydnj] 2012-10-18 09:13:09 PDT
There's a testcase included in the patch; I don't think there's a need for QA to do anything beyond that.  (Should this be in-testsuite+, then?)

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