add telemetry to measure effect of blocking some third party cookies

RESOLVED FIXED in mozilla24

Status

()

defect
RESOLVED FIXED
7 years ago
2 years ago

People

(Reporter: geekboy, Assigned: Yoric)

Tracking

Trunk
mozilla24
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 17 obsolete attachments)

11.97 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
6.36 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
13.03 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
No description provided.
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).
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.
Assignee: nobody → jono
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."
  }
Flags: needinfo?(ehsan)
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++.
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.
Flags: needinfo?(ehsan) → needinfo?(taras.mozilla)
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.
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.
Flags: needinfo?(taras.mozilla)
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.
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.
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
Thanks Sid, that's a lot more clear.

Assigning to Yoric since Jono isn't available.
Assignee: jono → dteller
Here's what I've been workign on; not sure the use of nsPrintfCString is correct but at least this is a starting point.
Attachment #764596 - Flags: review?(taras.mozilla)
Attachment #764596 - Flags: review?(taras.mozilla) → feedback?(dteller)
Getting started. Who should I r? when time comes?
Posted patch WIP (obsolete) — Splinter Review
Attaching a WIP.
Testing in progress.
Attachment #764596 - Attachment is obsolete: true
Attachment #764596 - Flags: feedback?(dteller)
Attachment #764716 - Flags: feedback?(sstamm)
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?
I have an experimental patch, which I'm currently testing.
I hope I can make it by Friday.
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
(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 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
Attachment #764716 - Attachment is obsolete: true
Attachment #764716 - Flags: feedback?(sstamm)
Attachment #765011 - Flags: review?(ehsan)
Posted patch 3. Unit tests (obsolete) — Splinter Review
Attachment #765016 - Flags: review?(nfroyd)
Attachment #765016 - Flags: review?(ehsan)
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.
v2 of the patch, now implementing both the algorithms detailed at https://wiki.mozilla.org/SecurityEngineering/ThirdPartyCookies/Telemetry
Attachment #765015 - Attachment is obsolete: true
Attachment #765015 - Flags: review?(nfroyd)
Attachment #765036 - Flags: review?(nfroyd)
Posted patch 3. Unit tests, v2 (obsolete) — Splinter Review
Now testing both algorithms.
Attachment #765016 - Attachment is obsolete: true
Attachment #765016 - Flags: review?(nfroyd)
Attachment #765016 - Flags: review?(ehsan)
Attachment #765037 - Flags: review?(nfroyd)
Attachment #765037 - Flags: review?(ehsan)
(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
(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 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.
Attachment #765011 - Flags: review?(ehsan) → review+
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.
Attachment #765037 - Flags: review?(ehsan) → review+
Applied feedback.
Attachment #765011 - Attachment is obsolete: true
Attachment #765258 - Flags: review+
Attachment #765258 - Attachment is patch: true
Posted patch 3. Unit tests, v3 (obsolete) — Splinter Review
Applied feedback.
Attachment #765037 - Attachment is obsolete: true
Attachment #765037 - Flags: review?(nfroyd)
Attachment #765263 - Flags: review?(nfroyd)
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.
Attachment #765036 - Flags: review?(nfroyd) → feedback+
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.
Attachment #765263 - Flags: review?(nfroyd) → review+
(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.
(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...
(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?
Same one, minus typo.
Attachment #765258 - Attachment is obsolete: true
Attachment #765375 - Flags: review+
Posted patch 3. Unit tests, v4 (obsolete) — Splinter Review
Added unregistration.
Attachment #765263 - Attachment is obsolete: true
Attachment #765378 - Flags: review+
Attachment #765036 - Attachment is obsolete: true
Attachment #765380 - Flags: review?(nfroyd)
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?
Attachment #765380 - Flags: review?(nfroyd) → review+
Applied feedback. Now waiting for Sid's clarification.
Attachment #765380 - Attachment is obsolete: true
Attachment #765387 - Flags: review+
ni? on Sid for comment 37.
Flags: needinfo?(sstamm)
Posted patch 3. Unit tests, v5 (obsolete) — Splinter Review
Added a commit message.
Attachment #765378 - Attachment is obsolete: true
Attachment #765389 - Flags: review+
(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.
(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.
Flags: needinfo?(sstamm)
(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]
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.
Flags: needinfo?(sstamm)
(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...
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.
(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.
Flags: needinfo?(sstamm)
(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.
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?).
Clarified assertion.
Attachment #765375 - Attachment is obsolete: true
Attachment #765822 - Flags: review+
Now normalizing stuff upon uptime.
Attachment #765387 - Attachment is obsolete: true
Attachment #765825 - Flags: review+
Posted patch 3. Unit tests, v6 (obsolete) — Splinter Review
Adapted tests.
Attachment #765389 - Attachment is obsolete: true
Attachment #765827 - Flags: review+
Ready to land. Unfortunately, Try is closed due to infra bustage, so I can't push it atm.
Keywords: checkin-needed
Fixed moz.build.
Attachment #765825 - Attachment is obsolete: true
Attachment #766018 - Flags: review+
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
Backed out again:

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

(Nit: please fix the added moz.build whitespace.)
Defending better against instances of nsIChannel that, for some reason, don't have a uri.
Attachment #765822 - Attachment is obsolete: true
Attachment #766075 - Flags: review+
And the Mac try, since for some reason the previous isn't launched on Mac:
https://tbpl.mozilla.org/?tree=Try&rev=28fda42ce489
Removing time-dependency in tests. This should get rid of the remaining (benign) intermittent orange.
Attachment #765827 - Attachment is obsolete: true
Attachment #766233 - Flags: review+
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?
Flags: needinfo?(sstamm)
followup it.
(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.
Flags: needinfo?(sstamm)
And... you already did.  Please ignore my last comment.
Blocks: 886389
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
John, I've just filed your problem as bug 935973.
You need to log in before you can comment on or make changes to this bug.