837326 - add telemetry to measure effect of blocking some third party cookies

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Comment 1

[Sid Stamm [:geekboy or :sstamm]]

Oh look, I failed to enter a description when I filed this bug!

Pretty straightforward: bug 818340 introduced partial third-party cookie blocking.  We should measure how often this is triggered (what percent of HTTP requests' cookies are blocked due to this and how many different sites' cookies are blocked due to this).

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

A meta-tip based on previous bad experience:

Some teams have started to collect telemetry like this and then realized that we collected the wrong numbers, only after we went to analyze the results. For example, let's say you find out that Xmillion HTTP requests had cookies blocked. What do we do with that number.

Some numbers you might want to include are things like "4xx responses for subresources requests with blocked cookies vs 4xx responses for subresources without blocked cookies." This may give you an indication of how many sites you are breaking. Probably also want to consider 3xx responses here (redirects to login page).

Another question worth answering is "how many fewer (resp. more) eTLD+1s are we sending third-party cookies to (per session?) compared to the default cookie settings (resp. the old third-party cookie blocking)". In other words, how many sites actually received less information due to this change?

I also recommend that you push these probes to -aurora and -beta so that you can get some useful before/after numbers sooner.

Comment 3

[Jono Xia]

Data collection spec is here: https://wiki.mozilla.org/SecurityEngineering/ThirdPartyCookies/Telemetry

Comment 4

[Jono Xia]

Here are the definitions for the histograms I'm planning on using. Any feedback on e.g. the names we use for them?

  "COOKIES_3RDPARTY_NUM_SITES": {
    "kind": "linear",
    "low": "5",
    "high": "145",
    "n_buckets": "30",
    "description": "The number of sites on which cookies for the same third-party site appear."
  },
  "COOKIES_3RDPARTY_NUM_SITES_BLOCKED": {
    "kind": "linear",
    "low": "5",
    "high": "145",
    "n_buckets": "30",
    "description": "The number of sites on which cookies for the same third-party site are blocked by the new policy."
  },
  "COOKIES_3RDPARTY_NUM_ATTEMPTS": {
    "kind": "linear",
    "low": "10",
    "high": "500",
    "n_buckets": "50",
    "description": "The number of attepts to set cookies for the same third-party site."
  },
  "COOKIES_3RDPARTY_NUM_ATTEMPTS_BLOCKED": {
    "kind": "linear",
    "low": "10",
    "high": "500",
    "n_buckets": "50",
    "description": "The number of attempts to set cookies for the same third-party site which are blocked by the new policy."
  }

Comment 5

[Jono Xia]

Ehsan, here's the thing I wanted to ask you about:

What I'm thinking of doing is adding some data to the notification that is sent by nsCookieService::NotifyRejected. Currently the third argument of mObserverService->NotifyObservers is not used -- we just pass nullptr for "data". 

I propose modifying nsCookieService.cpp so that the "data" payload of the notification contains a string giving some information about the reason for the rejection. e.g. a code indicating "this was rejected because it's a third-party cookie from a domain you've never visited" along with the domain name.

With this data available, the logic of the desired telemetry probe could be done in Javascript instead of C++. JS could listen for observer notifications where a cookie is rejected due to the new policy, and record to the telemetry histogram.

The win here is that other Javascript code could also make use of this notification data. I'm thinking specifically of Collusion, but any cookie-related add-ons could potentially benefit.

Since the data payload of the notification is currently null, I don't think we'd be breaking anything by adding some data there. However, this does represent a change to the public interface of the cookie service. If that interface should not be changed, or if it would be very difficult to get review approval for changing it, then I can give up on this idea and implement the telemetry probe entirely in C++.

Comment 6

[(no longer active)]

I'm fine with using the data argument here.  My bigger question is, is telemetry the right tool for this?  I know that in the past people have been burned because they were using Telemetry in the wrong ways to gather this kind of data, but I'm not sure about the details.  Hopefully Taras would know.

Comment 7

[(no longer active)]

Also, perhaps a good starting point would be to modify your local build with these probes, run it for 24 hours and look at your profile's telemetry data.

Comment 8

[(dormant account)]

I think telemetry is not going to give you useful data. It's not interesting to see how many sites break(lots of irrelevant sites on internet). The important thing is how many important/interesting sites break, for that you really want urls.

I think doing a smaller-scale study where you harvest the urls (eg by asking people to install an addon + send a report) will provide much more interesting data.

I think for a decision like this quality of the data(eg urls) > quantity that can be provided by telemetry.

Comment 9

[Sid Stamm [:geekboy or :sstamm]]

taras: we're hoping to do both.  mmc and ddahl are working on an extension-based qualitative measurement thingy. (https://github.com/mozilla/cookiemonster/)

My (personal) goal with the telemetry work is to identify how frequently people will encounter problems and how it affects the whole volume of cookie traffic.

Comment 10

[Jono Xia]

For the extension-based approach, it would be extra useful to have the cookie-rejected notification contain the extra info, so that an extension can get it from Javascript. So this is what I'm focusing on for now.

There's two pieces of information that would be useful to include in the data string: the reason for rejection (a numerical code most likely), and also the name of the referring domain which there is currently no way to get.

(the "subject" argument of the observer notification already contains the domian for which the cookie is being set, but it would be very handy to have the referring domain as well, so we know that the user went to site A and it tried to set a cookie for site B.)

Now, there are various ways to stuff two pieces of data inside a string, none of them exactly elegant. For instance, the string could say something like

"reason=3;referrer=www.example.com"

or maybe just

"3;www.example.com"

or full-on json:

"{'reason': '3', 'referrer': 'www.example.com'}"

Is there any precedent in the cookie manager or Observer service for this sort of thing which I should follow? Or, does anybody have a strong preference for a certain way of encoding this information?

Thanks.

Comment 11

[Sid Stamm [:geekboy or :sstamm]]

I added some pseudocode to [0] to make it clearer what's being collected.  

Jono's histogram names correspond to the wiki page as such:

First section (breadth/number of sites where blocked/allowed):
 "COOKIES_3RDPARTY_NUM_SITES"
 "COOKIES_3RDPARTY_NUM_SITES_BLOCKED"

Second section (persistence/number of requests where blocked/allowed):
 "COOKIES_3RDPARTY_NUM_ATTEMPTS_BLOCKED"
 "COOKIES_3RDPARTY_NUM_ATTEMPTS"

We also need to include the value of the "network.cookie.cookieBehavior" pref so we know if the histograms correspond to a profile with the from-visited cookie policy, the allow-all policy, or firstparty-only policy.

[0] https://wiki.mozilla.org/SecurityEngineering/ThirdPartyCookies/Telemetry

Comment 12

[(dormant account)]

Thanks Sid, that's a lot more clear.

Assigning to Yoric since Jono isn't available.

Comment 13

[Jono Xia]

Attachment: Rough draft of a patch to add rejection reason and referring host to notification payload

Here's what I've been workign on; not sure the use of nsPrintfCString is correct but at least this is a starting point.

Comment 14

[David Teller [:Yoric] - still alive but not very active]

Getting started. Who should I r? when time comes?

Comment 15

[David Teller [:Yoric] - still alive but not very active]

Attachment: WIP

Attaching a WIP.
Testing in progress.

Comment 16

[u279076]

Is this still on schedule to land for Firefox 23 before migration to Beta on Monday? If not should we be disabling bug 818340 in Firefox 23?

Comment 17

[David Teller [:Yoric] - still alive but not very active]

I have an experimental patch, which I'm currently testing.
I hope I can make it by Friday.

Comment 18

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 764716 [details] [diff] [review]
WIP

Review of attachment 764716 [details] [diff] [review]:
-----------------------------------------------------------------

Haven't had a ton of time to look at this.  Some quick first thoughts, will dig in again soon.

::: netwerk/cookie/nsICookieService.idl
@@ +65,5 @@
> + *           to set the cookie.
> + * data   :  the referrer, or "?" if unknown
> + *
> + * topic  : "third-party-cookie-rejected"
> + *           broadcast whenever a third party cookie was rejected

How does this topic/event relate to the "cookie-rejected" topic?  Will they both fire if a third party cookie is rejected?  If not, the consumers of "cookie-rejected" might get confused.

::: toolkit/components/telemetry/ThirdPartyCookieProbe.jsm
@@ +114,5 @@
> +};
> +
> +function normalizeHost(host) {
> +  return host.substring(host.lastIndexOf(".", host.lastIndexOf(".") - 1) + 1);
> +};

Since the "third-partyness" depends not on the last token of the host but the ETLD+1 we should be sure to use the TLD service (not just host tokenization) for normalization.  For example, *.foo.co.uk vs *.com.  See:
http://mxr.mozilla.org/mozilla-central/source/content/base/src/ThirdPartyUtil.cpp#250
http://mxr.mozilla.org/mozilla-central/source/content/base/src/ThirdPartyUtil.cpp#153
http://mxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsCookieService.cpp#1539
http://mxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsCookieService.cpp#3281

Comment 19

[David Teller [:Yoric] - still alive but not very active]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #18)
> Comment on attachment 764716 [details] [diff] [review]
> WIP
> 
> Review of attachment 764716 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Haven't had a ton of time to look at this.  Some quick first thoughts, will
> dig in again soon.
> 
> ::: netwerk/cookie/nsICookieService.idl
> @@ +65,5 @@
> > + *           to set the cookie.
> > + * data   :  the referrer, or "?" if unknown
> > + *
> > + * topic  : "third-party-cookie-rejected"
> > + *           broadcast whenever a third party cookie was rejected
> 
> How does this topic/event relate to the "cookie-rejected" topic?  Will they
> both fire if a third party cookie is rejected?  If not, the consumers of
> "cookie-rejected" might get confused.

Both fire.

> ::: toolkit/components/telemetry/ThirdPartyCookieProbe.jsm
> @@ +114,5 @@
> > +};
> > +
> > +function normalizeHost(host) {
> > +  return host.substring(host.lastIndexOf(".", host.lastIndexOf(".") - 1) + 1);
> > +};
> 
> Since the "third-partyness" depends not on the last token of the host but
> the ETLD+1 we should be sure to use the TLD service (not just host
> tokenization) for normalization.  For example, *.foo.co.uk vs *.com.  See:
> http://mxr.mozilla.org/mozilla-central/source/content/base/src/
> ThirdPartyUtil.cpp#250
> http://mxr.mozilla.org/mozilla-central/source/content/base/src/
> ThirdPartyUtil.cpp#153
> http://mxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsCookieService.
> cpp#1539
> http://mxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsCookieService.
> cpp#3281

Good to know, thanks.

Comment 20

[(dormant account)]

Comment on attachment 764716 [details] [diff] [review]
WIP

+    if (topic == "idle-daily" || topic == "test-third-party-cookie-probe-idle-daily") {
+      // Time to update statistics

That's not right. You should listen to 'gather-telemetry' observer

Comment 21

[David Teller [:Yoric] - still alive but not very active]

Attachment: 1. Extracting meaningful data from nsCookieService

Comment 22

[David Teller [:Yoric] - still alive but not very active]

Attachment: 2. Gathering data and feeding it into Telemetry

Comment 23

[David Teller [:Yoric] - still alive but not very active]

Attachment: 3. Unit tests

Comment 24

[David Teller [:Yoric] - still alive but not very active]

Just uploaded a first version of the patch.
Limitations:
- everything is in memory, no statistics other than the histogram itself is persisted across sessions;
- this first version only implements 1.1 of https://wiki.mozilla.org/SecurityEngineering/ThirdPartyCookies/Telemetry.

Comment 25

[David Teller [:Yoric] - still alive but not very active]

Attachment: 2. Gathering data and feeding it into Telemetry, v2

v2 of the patch, now implementing both the algorithms detailed at https://wiki.mozilla.org/SecurityEngineering/ThirdPartyCookies/Telemetry

Comment 26

[David Teller [:Yoric] - still alive but not very active]

Attachment: 3. Unit tests, v2

Now testing both algorithms.

Comment 27

[Brendan Eich [:brendan]]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #16)
> Is this still on schedule to land for Firefox 23 before migration to Beta on
> Monday? If not should we be disabling bug 818340 in Firefox 23?

We are holding the visited-based cookie-blocking patch in Aurora, so please make that mode non-default in 23 beta. See https://brendaneich.com/2013/06/the-cookie-clearinghouse/ and http://cch.law.stanford.edu/ -- and we'll need a bug to implement CCH list-based exceptions in due course. Thanks,

/be

Comment 28

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Brendan Eich [:brendan] from comment #27)
> We are holding the visited-based cookie-blocking patch in Aurora, so please
> make that mode non-default in 23 beta. 

Will create a patch for the same thing we did in 22 beta - see bug 851606.

> and we'll need a bug to implement CCH
> list-based exceptions in due course. Thanks,

I filed bug 885136.

Comment 29

[(no longer active)]

Comment on attachment 765011 [details] [diff] [review]
1. Extracting meaningful data from nsCookieService

Review of attachment 765011 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/cookie/nsCookieService.cpp
@@ +1716,5 @@
> +    nsresult rv = aChannel->GetURI(getter_AddRefs(channelURI));
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));
> +    nsAutoCString referringHost;
> +    rv = channelURI->GetHost(referringHost);
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));

Unless you can guarantee that rv is never a failure in these two cases, you should change these into an NS_ASSERTION.

@@ +1726,5 @@
> +  } else {
> +    mObserverService->
> +      NotifyObservers(aHostURI,
> +                      topic,
> +                      NS_LITERAL_STRING("?").get());

Please fix the indentation here to put the object name, function name and the first argument all on the same line.

Comment 30

[(no longer active)]

Comment on attachment 765037 [details] [diff] [review]
3. Unit tests, v2

Review of attachment 765037 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/tests/unit/test_ThirdPartyCookieProbe.js
@@ +26,5 @@
> +// copied from toolkit/mozapps/extensions/test/xpcshell/head_addons.js
> +const XULAPPINFO_CONTRACTID = "@mozilla.org/xre/app-info;1";
> +const XULAPPINFO_CID = Components.ID("{c763b610-9d49-455a-bbd2-ede71682a1ac}");
> +
> +function createAppInfo(id, name, version, platformVersion) {

Please either move this into head.js and remove it from http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/tests/unit/test_TelemetryPing.js too, or file a follow-up to do that.

@@ +148,5 @@
> +          // should be collapsed in the resulting histograms
> +            tryToSetCookie({
> +              request: prefix + "prism" + tld,
> +              referrer: prefix + "domain" + i + tld
> +            });

Nit: indentation.

Comment 31

[David Teller [:Yoric] - still alive but not very active]

Attachment: 1. Extracting meaningful data from nsCookieService, v2

Applied feedback.

Comment 32

[David Teller [:Yoric] - still alive but not very active]

Attachment: 3. Unit tests, v3

Applied feedback.

Comment 33

[Nathan Froyd [:froydnj]]

Comment on attachment 765036 [details] [diff] [review]
2. Gathering data and feeding it into Telemetry, v2

Review of attachment 765036 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/Histograms.json
@@ +3331,5 @@
> +    "kind": "linear",
> +    "low": "5",
> +    "high": "145",
> +    "n_buckets": "30",
> +    "description": "The number of sites on which cookies for the same third-party site are blocked by the new policy."

I think I would remove "by the new policy"--or at least make it time-agnostic.  New policies have a way of becoming current policies.

@@ +3345,5 @@
> +    "kind": "linear",
> +    "low": "10",
> +    "high": "500",
> +    "n_buckets": "50",
> +    "description": "The number of attempts to set cookies for the same third-party site which are blocked by the new policy."

Here too.

::: toolkit/components/telemetry/TelemetryPing.js
@@ +759,5 @@
>     */
>    setup: function setup() {
> +    // Initialize some probes that are kept in their own modules
> +    let thirdPartyCookies = new ThirdPartyCookieProbe();
> +    thirdPartyCookies.init();

I don't think we want to initialize this unless we are further down in this function (when we have determined we can gather data).

::: toolkit/components/telemetry/ThirdPartyCookieProbe.jsm
@@ +41,5 @@
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver]),
> +  init: function() {
> +    Services.obs.addObserver(this, "gather-telemetry", false);
> +    Services.obs.addObserver(this, "third-party-cookie-accepted", false);
> +    Services.obs.addObserver(this, "third-party-cookie-rejected", false);

You need to remove these observers.  Another hook into TelemetryPing is probably sufficient.

@@ +97,5 @@
> +      rejectedSites.add(data.countRejectedSites);
> +      acceptedRequests.add(data.countAcceptedRequests);
> +      rejectedRequests.add(data.countRejectedRequests);
> +    }
> +    this._thirdPartyCookies.clear();

On the spec page, it says that these internal tables should be "reset per session".  This code interprets "session" as "periods between pings" whereas a seemingly natural reading would be "browser session", which might encompass several pings.  For a site that prolifically sets third-party cookies, say ~150 cookies per browser session, this is the difference between recording it as one "big" site or several smaller sites.  Is this intentional?

@@ +111,5 @@
> + * @constructor
> + */
> +let RejectStats = function() {
> +  /**
> +   * The set of all sites for which we have accepted the third-party cookies.

Nit: just "...accepted third-party cookies."

@@ +115,5 @@
> +   * The set of all sites for which we have accepted the third-party cookies.
> +   */
> +  this._acceptedSites = new Set();
> +  /**
> +   * The set of all sites for which we have rejected the third-party cookies.

Here too.

Comment 34

[Nathan Froyd [:froydnj]]

Comment on attachment 765263 [details] [diff] [review]
3. Unit tests, v3

Review of attachment 765263 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/tests/unit/test_ThirdPartyCookieProbe.js
@@ +56,5 @@
> + * @param obj An object with the following fields
> + * - {string} request The uri of the request setting the cookie.
> + * - {string} referrer The uri of the referrer for this request.
> + */
> +function tryToSetCookie(obj) {

I don't know that there's any reason to have an object here rather than just (request, referrer).

@@ +86,5 @@
> +          // NUMBER_OF_REJECTS entries.
> +          // Histogram requestsRejected should count
> +          // NUMBER_OF_REJECTS * NUMBER_OF_REPEATS * 2
> +          tryToSetCookie({
> +            request: prefix + "echelon" + tld,

I approve of the request URIs.

Comment 35

[David Teller [:Yoric] - still alive but not very active]

(In reply to Nathan Froyd (:froydnj) from comment #34)
> Comment on attachment 765263 [details] [diff] [review]
> 3. Unit tests, v3
> 
> Review of attachment 765263 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/telemetry/tests/unit/test_ThirdPartyCookieProbe.js
> @@ +56,5 @@
> > + * @param obj An object with the following fields
> > + * - {string} request The uri of the request setting the cookie.
> > + * - {string} referrer The uri of the referrer for this request.
> > + */
> > +function tryToSetCookie(obj) {
> 
> I don't know that there's any reason to have an object here rather than just
> (request, referrer).

Simply because it is easy to get confused in the order of arguments that other have the same type. I got that order wrong too many times, so I decided to label my arguments. If you want me to remove this, I can do it.

> @@ +86,5 @@
> > +          // NUMBER_OF_REJECTS entries.
> > +          // Histogram requestsRejected should count
> > +          // NUMBER_OF_REJECTS * NUMBER_OF_REPEATS * 2
> > +          tryToSetCookie({
> > +            request: prefix + "echelon" + tld,
> 
> I approve of the request URIs.

Thank you.

Comment 36

[Nathan Froyd [:froydnj]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #35)
> > ::: toolkit/components/telemetry/tests/unit/test_ThirdPartyCookieProbe.js
> > @@ +56,5 @@
> > > + * @param obj An object with the following fields
> > > + * - {string} request The uri of the request setting the cookie.
> > > + * - {string} referrer The uri of the referrer for this request.
> > > + */
> > > +function tryToSetCookie(obj) {
> > 
> > I don't know that there's any reason to have an object here rather than just
> > (request, referrer).
> 
> Simply because it is easy to get confused in the order of arguments that
> other have the same type. I got that order wrong too many times, so I
> decided to label my arguments. If you want me to remove this, I can do it.

Nope, that makes sense.  Thanks for the clarification!  Oh for more widespread keyword arguments...

Comment 37

[David Teller [:Yoric] - still alive but not very active]

(In reply to Nathan Froyd (:froydnj) from comment #33)
> @@ +97,5 @@
> > +      rejectedSites.add(data.countRejectedSites);
> > +      acceptedRequests.add(data.countAcceptedRequests);
> > +      rejectedRequests.add(data.countRejectedRequests);
> > +    }
> > +    this._thirdPartyCookies.clear();
> 
> On the spec page, it says that these internal tables should be "reset per
> session".  This code interprets "session" as "periods between pings" whereas
> a seemingly natural reading would be "browser session", which might
> encompass several pings.  For a site that prolifically sets third-party
> cookies, say ~150 cookies per browser session, this is the difference
> between recording it as one "big" site or several smaller sites.  Is this
> intentional?

Oh, I had understood session as "once per day", given several references to "(in 24 hours)" in the spec. I believe that this interpretation of once per day makes more sense, but I am not sure.

Sid, can you clarify this point?

Comment 38

[David Teller [:Yoric] - still alive but not very active]

Attachment: 1. Extracting meaningful data from nsCookieService, v3

Same one, minus typo.

Comment 39

[David Teller [:Yoric] - still alive but not very active]

Attachment: 3. Unit tests, v4

Added unregistration.

Comment 40

[David Teller [:Yoric] - still alive but not very active]

Attachment: 2. Gathering data and feeding it into Telemetry, v3

Comment 41

[Nathan Froyd [:froydnj]]

Comment on attachment 765380 [details] [diff] [review]
2. Gathering data and feeding it into Telemetry, v3

Review of attachment 765380 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the below change and the caveat that we need to wait for Sid's clarification on the meaning of "session".

::: toolkit/components/telemetry/ThirdPartyCookieProbe.jsm
@@ +44,5 @@
> +    Services.obs.addObserver(this, "third-party-cookie-accepted", false);
> +    Services.obs.addObserver(this, "third-party-cookie-rejected", false);
> +  },
> +  dispose: function() {
> +    Services.obs.removeObserver(this, "gather-telemetry");

Ah, so we have to be careful when we call this: if we remove ourselves too early, we won't catch the gather-telemetry notification for saving pings.

Can you move the uninstall() call here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetryPing.js#1111

to after the conditional?

Comment 42

[David Teller [:Yoric] - still alive but not very active]

Attachment: 2. Gathering data and feeding it into Telemetry, v4

Applied feedback. Now waiting for Sid's clarification.

Comment 43

[Nathan Froyd [:froydnj]]

ni? on Sid for comment 37.

Comment 44

[David Teller [:Yoric] - still alive but not very active]

Attachment: 3. Unit tests, v5

Added a commit message.

Comment 45

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd (:froydnj) from comment #41)
> Comment on attachment 765380 [details] [diff] [review]
> 2. Gathering data and feeding it into Telemetry, v3
> 
> Review of attachment 765380 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the below change and the caveat that we need to wait for Sid's
> clarification on the meaning of "session".
> 
> ::: toolkit/components/telemetry/ThirdPartyCookieProbe.jsm
> @@ +44,5 @@
> > +    Services.obs.addObserver(this, "third-party-cookie-accepted", false);
> > +    Services.obs.addObserver(this, "third-party-cookie-rejected", false);
> > +  },
> > +  dispose: function() {
> > +    Services.obs.removeObserver(this, "gather-telemetry");
> 
> Ah, so we have to be careful when we call this: if we remove ourselves too
> early, we won't catch the gather-telemetry notification for saving pings.
> 
> Can you move the uninstall() call here:
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/
> TelemetryPing.js#1111
> 
> to after the conditional?

So I was mistaken here.  We don't issue gather-telemetry on shutdown, since some observers may compute quite expensive stats.  You should instead flush your stats at profile-before-change and disconnect observers there.

Comment 46

[Sid Stamm [:geekboy or :sstamm]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #37)
> Oh, I had understood session as "once per day", given several references to
> "(in 24 hours)" in the spec. I believe that this interpretation of once per
> day makes more sense, but I am not sure.

Having a consistent time window will make the data easier to interpret, so any roughly consistent window is good enough.  (I think browsing sessions vary too much, so we might wanna stick to 24 hours if we can get a nearly consistent window, which it sounds like we can).

Re: flushing on exit... I think the goal should be to paint a consistent picture of the last 24 hours if it's not too hard.  If we flush on exit, will we end up with histograms that represent significantly less than 24 hours?  I'm not familiar with the average session length here.

Comment 47

[Nathan Froyd [:froydnj]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #46)
> Re: flushing on exit... I think the goal should be to paint a consistent
> picture of the last 24 hours if it's not too hard.  If we flush on exit,
> will we end up with histograms that represent significantly less than 24
> hours?

Yes.  Well, you'd end up with a histogram that's

[results from 1st 24h] + [results from 2nd 24h] + ... + [results from N hours after last ping]

Comment 48

[David Teller [:Yoric] - still alive but not very active]

I believe that we have two possibilities:
- if we don't flush, we have relatively consistent windows of 24h, but we don't get any information from users who close their browser at the end of the day;
- if we do flush, we have less consistent windows, but we do get information from these users.

I would go for "not flushing" in a first version and refine later.

Comment 49

[Nathan Froyd [:froydnj]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #48)
> I believe that we have two possibilities:
> - if we don't flush, we have relatively consistent windows of 24h, but we
> don't get any information from users who close their browser at the end of
> the day;
> - if we do flush, we have less consistent windows, but we do get information
> from these users.
> 
> I would go for "not flushing" in a first version and refine later.

If we think that people with telemetry are the sort of people who just have their browser open for the workday, then I'd argue that we do want to flush on shutdown.  That browser session *was* a relatively representative view on what they'd browse over a 24h period.  Or people who have the browser open for a workweek (update Aurora on Monday, close it on Friday sort of thing).  Maybe we have aggregate uptime statistics somewhere...

Comment 50

[David Teller [:Yoric] - still alive but not very active]

Ok, you convinced me.
We should probably have a followup bug to round the edges with respect to this period of 24h, by storing some data to disk.

Comment 51

[Sid Stamm [:geekboy or :sstamm]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #48)
> I would go for "not flushing" in a first version and refine later.

Sounds good.

Comment 52

[(dormant account)]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #46)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #37)
> > Oh, I had understood session as "once per day", given several references to
> > "(in 24 hours)" in the spec. I believe that this interpretation of once per
> > day makes more sense, but I am not sure.
> 
> Having a consistent time window will make the data easier to interpret, so
> any roughly consistent window is good enough.  (I think browsing sessions
> vary too much, so we might wanna stick to 24 hours if we can get a nearly
> consistent window, which it sounds like we can).

I'm really not sure why we need the arbitrary 24hour window. We can just divide number of samples by uptime sent along with them.

Comment 53

[Sid Stamm [:geekboy or :sstamm]]

Good call, Taras.  If we normalize the samples via uptime (time between flushes, right?) we have the rate of "cookying".

Longer-lived samples are better for the "breadth" question (on how many sites is a third-party's cookies blocked?).

Comment 54

[David Teller [:Yoric] - still alive but not very active]

Attachment: 1. Extracting meaningful data from nsCookieService, v4

Clarified assertion.

Comment 55

[David Teller [:Yoric] - still alive but not very active]

Attachment: 2. Gathering data and feeding it into Telemetry, v5

Now normalizing stuff upon uptime.

Comment 56

[David Teller [:Yoric] - still alive but not very active]

Attachment: 3. Unit tests, v6

Adapted tests.

Comment 57

[David Teller [:Yoric] - still alive but not very active]

Ready to land. Unfortunately, Try is closed due to infra bustage, so I can't push it atm.

Comment 58

[David Teller [:Yoric] - still alive but not very active]

Try: https://tbpl.mozilla.org/?tree=Try&rev=bce9070bb975

Comment 59

[David Teller [:Yoric] - still alive but not very active]

Pushed to inbound.
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cbc024b3c77
https://hg.mozilla.org/integration/mozilla-inbound/rev/75fa13b20c1d
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b0640fa5b18

Comment 60

[Ed Morley [:emorley]]

Backed out for Windows-only compilation failures:
https://tbpl.mozilla.org/php/getParsedLog.php?id=24429705&tree=Mozilla-Inbound

Notably: (thanks philor! :-))
http://pastebin.mozilla.org/2549652

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/918fd1e35b39
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/429090f9109a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/74d3dd56cee0

Comment 61

[David Teller [:Yoric] - still alive but not very active]

Attachment: 2. Gathering data and feeding it into Telemetry, v6

Fixed moz.build.

Comment 62

[Phil Ringnalda (:philor)]

And Mac mochitest-chrome assertions, and weirdest of all, Android Armv6 xpcshell failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=24434474&tree=Mozilla-Inbound

Comment 63

[David Teller [:Yoric] - still alive but not very active]

Pushed again with fixed moz.build:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/68e5d9fe91fc
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a0b216d3be12
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/65804eae62b1

Comment 64

[Nathan Froyd [:froydnj]]

Backed out again:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ce7857353a6

(Nit: please fix the added moz.build whitespace.)

Comment 65

[David Teller [:Yoric] - still alive but not very active]

Attachment: 1. Extracting meaningful data from nsCookieService, v5

Defending better against instances of nsIChannel that, for some reason, don't have a uri.

Comment 66

[David Teller [:Yoric] - still alive but not very active]

Try: https://tbpl.mozilla.org/?tree=Try&rev=470487f20efd

Comment 67

[David Teller [:Yoric] - still alive but not very active]

And the Mac try, since for some reason the previous isn't launched on Mac:
https://tbpl.mozilla.org/?tree=Try&rev=28fda42ce489

Comment 68

[David Teller [:Yoric] - still alive but not very active]

Attachment: 3. Unit tests, v7

Removing time-dependency in tests. This should get rid of the remaining (benign) intermittent orange.

Comment 69

[David Teller [:Yoric] - still alive but not very active]

Inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/649a50fee077
https://hg.mozilla.org/integration/mozilla-inbound/rev/6424bcea2460
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed159d7b45eb

Comment 70

[David Teller [:Yoric] - still alive but not very active]

Sid, I need to double-check, but I am starting to wonder whether the private browsing tests that caused me to add the patch of comment 65 could mean that we do not have access to the first-party uri. If so, with the current patch, we will have skewed results on histograms COOKIES_3RDPARTY_NUM_SITES_{ACCEPTED, BLOCKED} when users are in private browsing mode.

Should I attempt to address this immediately or can this wait for a followup bug?

Comment 71

[(dormant account)]

followup it.

Comment 72

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/649a50fee077
https://hg.mozilla.org/mozilla-central/rev/6424bcea2460
https://hg.mozilla.org/mozilla-central/rev/ed159d7b45eb

Comment 73

[Sid Stamm [:geekboy or :sstamm]]

(In reply to David Rajchenbach Teller [:Yoric] <on workweek, will not follow bugzilla until July 1st> from comment #70)

> Should I attempt to address this immediately or can this wait for a followup
> bug?

Yeah (as Taras said in comment 71), please file a followup.  We might want to stop measuring entirely when people are in private mode since the cookie behavior is different.

Comment 74

[Sid Stamm [:geekboy or :sstamm]]

And... you already did.  Please ignore my last comment.

Comment 75

[John Bacon]

Well I don't care what stats. you gather. I just don't want my error console cluttered up with messages like this:
ThirdPartyCookieProbe: Uncaught error [Exception... "Component returned failure code: 0x804b0051 (NS_ERROR_HOST_IS_IP_ADDRESS) [nsIEffectiveTLDService.getBaseDomainFromHost]"  nsresult: "0x804b0051 (NS_ERROR_HOST_IS_IP_ADDRESS)"  location: "JS frame :: resource://gre/modules/ThirdPartyCookieProbe.jsm :: normalizeHost :: line 181"  data: no]
undefined

Comment 76

[David Teller [:Yoric] - still alive but not very active]

John, I've just filed your problem as bug 935973.