Closed Bug 597247 Opened 9 years ago Closed 9 years ago

Implement alive ping counter for blocklist to strengthen user privacy

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla2.0b7

People

(Reporter: dre, Assigned: rstrong)

References

()

Details

Attachments

(2 files, 3 obsolete files)

I'd like us to do something similar to what Canonical is doing for their channel distribution metrics.

http://theravingrick.blogspot.com/2010/08/can-we-count-users-without-uniquely.html
https://launchpad.net/ubuntu/+source/canonical-census

For this bug, the only needed patch is adding an integer url parameter to the blocklist request that increments on every request.
This is a client bug. (Though I support the idea!)
Component: Blocklisting → Add-ons Manager
Product: addons.mozilla.org → Toolkit
QA Contact: blocklisting → add-ons.manager
Assignee: nobody → robert.bugzilla
Status: NEW → ASSIGNED
I talked with Daniel about this and iirc he would like to reset the counter on version change.

Another thing we could do is send 0 for the count if this is the first ping for a profile and 1 if it isn't (e.g. version change or rollover of the counter which really should never happen).

Daniel, does this sound correct?
Attached patch patch rev1 (obsolete) — Splinter Review
Attachment #476613 - Flags: review?(dtownsend)
Attached patch patch rev1 (obsolete) — Splinter Review
forgot to refresh first
Attachment #476613 - Attachment is obsolete: true
Attachment #476614 - Flags: review?(dtownsend)
Attachment #476613 - Flags: review?(dtownsend)
How does this strengthen user privacy?
Because if we can use this mechanism to be able to count active installations without collecting any personal information, we'll be able to remove the cookie that is currently listed in the privacy policy as being part of blocklist.
Comment on attachment 476614 [details] [diff] [review]
patch rev1

If the user replaces the integer with a negative value, I think we should also reset it to 1.

I'm not sure in what case the getIntPref might throw an exception, but if it does, would it be useful to set pingCount = 1; in the catch block as well so that the pref will be reset in that case?
Attachment #476614 - Flags: review-
Comment on attachment 476614 [details] [diff] [review]
patch rev1

(In reply to comment #2)
> Another thing we could do is send 0 for the count if this is the first ping for
> a profile and 1 if it isn't (e.g. version change or rollover of the counter
> which really should never happen).

Need a decision on this.
>   }
> #ifdef XP_MACOSX
>   // Mac universal build should report a different ABI than either macppc
>   // or mactel.
>   let macutils = Cc["@mozilla.org/xpcom/mac-utils;1"].
>                  getService(Ci.nsIMacUtils);
> 
>   if (macutils.isUniversalBinary)
>-    abi = "Universal-gcc3";
>+    abi += "-u-" + macutils.architecturesInBinary;

Is this meant to be in here?

>+    var pingCount = 0;
>+    try {
>+      pingCount = gPref.getIntPref(PREF_BLOCKLIST_PINGCOUNT);
>+      pingCount++;
>+      if (pingCount > 2147483647) {
>+        // Rollover to 1 if the value is greater than what is support by an
>+        // integer preference. The 1 indicates that this is an existing profile.
>+        pingCount = 1;
>+      }
>+    }
>+    catch (e) {
>+    }
>+    gPref.setIntPref(PREF_BLOCKLIST_PINGCOUNT, pingCount);

This is kind of confusing to me as it only increments when there was a ping count in the prefs. I think it would be more understandable to just read the pingCount from prefs, do the url replacement, then increment and write to prefs.

Do we actually want to roll-over to 1 when the number gets too high?
Attachment #476614 - Flags: review?(dtownsend) → review-
(In reply to comment #8)
> Comment on attachment 476614 [details] [diff] [review]
> patch rev1
> 
> (In reply to comment #2)
> > Another thing we could do is send 0 for the count if this is the first ping for
> > a profile and 1 if it isn't (e.g. version change or rollover of the counter
> > which really should never happen).
> 
> Need a decision on this.
> >   }
> > #ifdef XP_MACOSX
> >   // Mac universal build should report a different ABI than either macppc
> >   // or mactel.
> >   let macutils = Cc["@mozilla.org/xpcom/mac-utils;1"].
> >                  getService(Ci.nsIMacUtils);
> > 
> >   if (macutils.isUniversalBinary)
> >-    abi = "Universal-gcc3";
> >+    abi += "-u-" + macutils.architecturesInBinary;
> 
> Is this meant to be in here?
es, this brings the blocklist service up to date with what app update does which allows us to determine which universal binary is being used and the architecture of the system.

> >+    var pingCount = 0;
> >+    try {
> >+      pingCount = gPref.getIntPref(PREF_BLOCKLIST_PINGCOUNT);
> >+      pingCount++;
> >+      if (pingCount > 2147483647) {
> >+        // Rollover to 1 if the value is greater than what is support by an
> >+        // integer preference. The 1 indicates that this is an existing profile.
> >+        pingCount = 1;
> >+      }
> >+    }
> >+    catch (e) {
> >+    }
> >+    gPref.setIntPref(PREF_BLOCKLIST_PINGCOUNT, pingCount);
> 
> This is kind of confusing to me as it only increments when there was a ping
> count in the prefs. I think it would be more understandable to just read the
> pingCount from prefs, do the url replacement, then increment and write to
> prefs.
Will clean up

> Do we actually want to roll-over to 1 when the number gets too high?
I think we do but I am waiting on Daniel's response to comment #2
I do want the number to reset to 1 if appChanged is true.  That helps keep the numbers low (anonymity in large groups), and it further protects the user's privacy because we don't know how many days the profile has been active for all versions of Firefox in total, only the number of days the profile has been active for this version.  While I think there are a lot of interesting metrics that could be derived if we didn't reset it on version change, I think it is a stronger value to the user's privacy if the profile resets the counter on every version change.

It is definitely reasonable to roll it over if the number gets high.  Keep in mind the number should only be incrementing once per day for as long as the appChanged variable stays false.  Seeing numbers in the thousands would be an indication of something being broken.
Daniel, what about?
(In reply to comment #2)
>...
> Another thing we could do is send 0 for the count if this is the first ping for
> a profile and 1 if it isn't (e.g. version change or rollover of the counter
> which really should never happen).
> 
> Daniel, does this sound correct?
So, if a profile has never pinged send 0 if it has and the app version has changed or the count has rolled over send 1.
Yes, sorry I wasn't more clear.  The behavior described in comment #11 sounds fine.

So if this change were to go in to a version of Firefox that was installed as an upgrade to an existing profile, does that means that the very first blocklist ping for that profile would contain a 1 rather than a 0?  I'd like that because it would distinguish between a new profile (i.e. likely a new user) and an upgrade of an existing profile.
Attached patch patch rev2 (obsolete) — Splinter Review
Attachment #476614 - Attachment is obsolete: true
Attachment #476882 - Flags: review?(dtownsend)
Dave, this also handles the case where the pingCount is less than zero that Daniel asked for. Submitted both in case you have a preference here
Attachment #476886 - Flags: review?(dtownsend)
Comment on attachment 476886 [details] [diff] [review]
patch rev2 - check if pingCount is less than zero

This looks good, any tests?
Attachment #476886 - Flags: review?(dtownsend) → review+
Attachment #476882 - Attachment is obsolete: true
Attachment #476882 - Flags: review?(dtownsend)
Not yet and I'll add xpcshell tests for this and blocklist url replacement after I get my blocker work done... hopefully within a week.
Attachment #476886 - Flags: approval2.0+
Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/c1ded4058bad

I'll add a test for this as time permits
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite?
Flags: in-litmus-
Resolution: --- → FIXED
Comment on attachment 477055 [details] [diff] [review]
test bustage fix

Turns out there is a blocklist url test. doh!
Attachment #477055 - Attachment is patch: true
Attachment #477055 - Attachment mime type: application/octet-stream → text/plain
Attachment #477055 - Flags: review+
cc'ing a bunch of folks in case they want this change in their products. To add this you need to add %PING_COUNT%/ to the end of your extensions.blocklist.url preference.
Target Milestone: --- → mozilla2.0b7
(In reply to comment #20)
> cc'ing a bunch of folks in case they want this change in their products. To add
> this you need to add %PING_COUNT%/ to the end of your extensions.blocklist.url
> preference.

Is this likely to be back-ported to 1.9.2/1.9.1 branches?
There are no plans to at the moment though it should be fairly trivial to do so. Is this something you would like to see backported to 1.9.2/1.9.1?
(In reply to comment #22)
> There are no plans to at the moment though it should be fairly trivial to do
> so. Is this something you would like to see backported to 1.9.2/1.9.1?

Only if metrics want it. It was more just so I know where we'll need to land the patches.
Blocks: 599284
Blocks: 598977
A test was added for this in bug 620837
Depends on: 620837
Flags: in-testsuite? → in-testsuite+
Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110209 Firefox/4.0b12pre ID:20110209030359

Something which confused me while testing the blocklist pings is, that we already store the incremented value in the prefs. That means with a fresh profile the pingCount and totalPingCount will have 2 as value after the first blocklist ping.
Status: RESOLVED → VERIFIED
Just to make sure, the request goes through with 1/1 but the value stored in the profile is 2/2?
You need to log in before you can comment on or make changes to this bug.