597247 - Implement alive ping counter for blocklist to strengthen user privacy

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Daniel Einspanjer [:dre] [:deinspanjer]]

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.

Comment 1

[Justin Scott [:fligtar]]

This is a client bug. (Though I support the idea!)

Comment 2

[Robert Strong (they/them - no direct email)]

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?

Comment 3

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

Comment 4

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

forgot to refresh first

Comment 5

[Daniel C]

How does this strengthen user privacy?

Comment 6

[Daniel Einspanjer [:dre] [:deinspanjer]]

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 7

[Daniel Einspanjer [:dre] [:deinspanjer]]

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?

Comment 8

[Dave Townsend [:mossop]]

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?

Comment 9

[Robert Strong (they/them - no direct email)]

(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

Comment 10

[Daniel Einspanjer [:dre] [:deinspanjer]]

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.

Comment 11

[Robert Strong (they/them - no direct email)]

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.

Comment 12

[Daniel Einspanjer [:dre] [:deinspanjer]]

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.

Comment 13

[Robert Strong (they/them - no direct email)]

Attachment: patch rev2

Comment 14

[Robert Strong (they/them - no direct email)]

Attachment: patch rev2 - check if pingCount is less than zero

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

Comment 15

[Dave Townsend [:mossop]]

Comment on attachment 476886 [details] [diff] [review]
patch rev2 - check if pingCount is less than zero

This looks good, any tests?

Comment 16

[Robert Strong (they/them - no direct email)]

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.

Comment 17

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/c1ded4058bad

I'll add a test for this as time permits

Comment 18

[Robert Strong (they/them - no direct email)]

Attachment: test bustage fix

Comment 19

[Robert Strong (they/them - no direct email)]

Comment on attachment 477055 [details] [diff] [review]
test bustage fix

Turns out there is a blocklist url test. doh!

Comment 20

[Robert Strong (they/them - no direct email)]

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.

Comment 21

[Mark Banner (:standard8)]

(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?

Comment 22

[Robert Strong (they/them - no direct email)]

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?

Comment 23

[Mark Banner (:standard8)]

(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.

Comment 24

[Robert Strong (they/them - no direct email)]

A test was added for this in bug 620837

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 26

[Daniel Einspanjer [:dre] [:deinspanjer]]

Just to make sure, the request goes through with 1/1 but the value stored in the profile is 2/2?

Comment 27

[Robert Strong (they/them - no direct email)]

Correct