947676 - MIXED_CONTENT_UNBLOCK_COUNTER telemetry probe seems to not be measuring the right thing

Status: NEW

(Core :: DOM: Security, defect)

Description

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

+++ This bug was initially created as a clone of Bug #882467 +++

See http://telemetry.mozilla.org/#path=release/25/MIXED_CONTENT_UNBLOCK_COUNTER

This has the constant value "1" on all channels for months. It seems like this probe is and has been broken for a long time.

See http://hg.mozilla.org/mozilla-central/annotate/edac8cba9f78/content/base/src/nsDocument.cpp#l1484:

    // Record the page load
    uint32_t pageLoaded = 1;
    Accumulate(Telemetry::MIXED_CONTENT_UNBLOCK_COUNTER, pageLoaded); 

1. no need to use a variable that will always have a constant value.

2. Probe Telemetry::MIXED_CONTENT_UNBLOCK_COUNTER is the same probe that is used to measure how many times the did "Disable Protection." I am having trouble understanding how this is supposed to work.

Also, see dolske's comments in bug 882467, where he says the same thing I say in #2 above.

Comment 1

[Tanvi Vyas[:tanvi]]

I must have missed this bug!  Looks like it was filed when I was on leave.  Will look into this and see what's off with the probe.

Philipp, Aislinn - this means that the number of times users disable protection that we discussed last week may be inaccurate.

Comment 2

[Daniel Veditz [:dveditz]]

All this is doing it incrementing the count of all secure pages encountered in bucket 1 ("pageLoaded"). When the user unblocks active content then bucket 2 is incremented over in urlbarBindings.xml. It would be clearer is "pageLoaded" were a constant and had a better name but it's not just counting 1 for everyone -- there are a (very small) number of people adding to the count in bucket 2.

For unblocking the number of all secure page loads might not be a useful thing to compare against. I'd recommend moving this down in the function and only incrementing bucket 1 if hasActiveContent is true.

Oh, actually it looks like bucket one (and bucket zero of MIXED_CONTENT_PAGE_LOAD) are counting _all_ non-about documents, secure and not. Why exclude about documents but include file:// and data: and such? Would it make more sense to instead just count docs with the https scheme? What we don't know--but would really like to--is how many https pages the user loads. It's possible 90% of all TLS pages have mixed content, but all we know is the 1% of _all_ pages have mixed content.

Comment 3

[Tanvi Vyas[:tanvi]]

Brian, I don't think this probe is broken.  It may not be the best probe we could have, but it still seems functional.
* This is where we set a value of 1 for all top-level non-about pages - http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp#1669

* And here we record everytime disable protection is clicked by setting a value of 2 - http://mxr.mozilla.org/mozilla-central/source/browser/base/content/urlbarBindings.xml#2350

Looking at release 36, we see that there have been 830M submissions (top-level non-about page loads) where we do not disable protection and 6.36K pages where we do.  Note that of the 830M, many will not even be HTTP pages.  Many will be HTTPS pages with no mixed active content.  To compare the 6.36M against the 830M doesn't make any sense, but we can compare it against the MIXED_CONTENT_PAGE_LOAD probe which tell us how many pages have mixed active content.
http://telemetry.mozilla.org/#filter=release%2F36%2FMIXED_CONTENT_PAGE_LOAD%2Fsaved_session%2FFirefox&aggregates=multiselect-all!Submissions&evoOver=Builds&locked=true&sanitize=true&renderhistogram=Graph

For release 36, we see that 5.37M (3.84+1.57) pages have mixed active content (either loaded or blocked).  In 6.36K of those pages loads, the user disabled protection (which persists while the user is on that tab on that domain).

Hence, I think we can file bugs for better probes (of the number of pages with mixed active content blocked, how many times is disable protection clicked?), but I don't think this probe is useless.

Comment 4

[Tanvi Vyas[:tanvi]]

(In reply to Daniel Veditz [:dveditz] from comment #2)
> Would it make more sense to instead just count docs with the https scheme?
> What we don't know--but would really like to--is how many https pages the
> user loads. It's possible 90% of all TLS pages have mixed content, but all
> we know is the 1% of _all_ pages have mixed content.

Yes, this would be nice to know.  We can file a bug for more mixed content telemetry probes.

Comment 5

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

It may be the case I was mistaken when I filed this bug. I just remember that when I looked at the aggregate data it didn't make sense, and when I read the code it didn't make sense to me. But, it may have been a misunderstanding on my part. In any case, I'm sure it will be easy to sort out.

Comment 6

[Daniel Veditz [:dveditz]]

Attachment: mixed-telemetry.patch

This patch mostly changes the MIXED_CONTENT_UNBLOCK_COUNTER probe to make it more useful. I've left bucket 2 as the count of unblock events, but bucket 1 being "all documents other than about:" was not that useful in comparison. Instead the relevant comparisons are the number of pages with mixed active content that might be unblocked. The un-used bucket 0 is now pages that have BLOCKED mixed active content (iow, 0 current active content). Bucket 1 has been changed to be the number of pages that _do_ contain active mixed content. Hopefully this number bears some relation to the number of unblock events, but currently the counts for bucket 2 are so small that we're having trouble believing them and this will be a useful calibration.

I also made a change to the MIXED_CONTENT_PAGE_LOAD probe: bucket 0 is now the number of HTTPS pages with no mixed content rather than including all the insecure page loads in that number. If we want to compare secure and insecure page loads we have the HTTP_PAGELOAD_IS_SSL probe. In theory the buckets in MIXED_CONTENT_PAGE_LOAD should now add up to bucket 1 of HTTP_PAGELOAD_IS_SSL. They're so far off now that it's hard to tell if we're measuring the wrong thing.

Comment 7

[Tanvi Vyas[:tanvi]]

Comment on attachment 8584953 [details] [diff] [review]
mixed-telemetry.patch

Thank you Dan for working on this bug!


(In reply to Daniel Veditz [:dveditz] from comment #6)
> Created attachment 8584953 [details] [diff] [review]
> mixed-telemetry.patch
> 
> 
> I also made a change to the MIXED_CONTENT_PAGE_LOAD probe: bucket 0 is now
> the number of HTTPS pages with no mixed content rather than including all
> the insecure page loads in that number. 
>
Instead, can we leave bucket 0 as all page loads and use bucket 4 which is currently unused as all HTTPS page loads.  This way, comparing against historical data will be easier.

Also, if you have an HTTP page with HTTPS frame that sources HTTP content, then that type of load won't get counted in your patch.  It will if we add a 4th bucket.  Since that type of load is pretty rare, I'm not too concerned about it.  But I would like to be able to compare new stats against historical ones.

So keep the about: check.  Also add a bool isHTTPS.  Add an enum:

  enum {
    NO_MIXED_CONTENT = 0, // There is no Mixed Content on the page
    MIXED_DISPLAY_CONTENT = 1, // The page attempted to load Mixed Display Content
    MIXED_ACTIVE_CONTENT = 2, // The page attempted to load Mixed Active Content
    MIXED_DISPLAY_AND_ACTIVE_CONTENT = 3 // The page attempted to load Mixed Display & Mixed Active Content
    HTTPS_NO_MIXED_CONTENT = 4 // There is no Mixed Content on this HTTPS top level page.
  };

And update the if else:

uint32_t mixedContentLevel = NO_MIXED_CONTENT;
if (hasMixedDisplay && hasMixedActive) {
   mixedContentLevel = MIXED_DISPLAY_AND_ACTIVE_CONTENT;
} else if (hasMixedActive){
   mixedContentLevel = MIXED_ACTIVE_CONTENT;
} else if (hasMixedDisplay) {
    mixedContentLevel = MIXED_DISPLAY_CONTENT;
} else if (isHTTPS)
    mixedContentLevel = HTTPS_NO_MIXED_CONTENT;
}

> Hopefully this number bears some relation to the number of unblock
> events, but currently the counts for bucket 2 are so small that we're having
> trouble believing them and this will be a useful calibration.
> 

It will bear some relation, but the number of pages with mixed active content loaded won't be 1-1 with the number of disable events because:
1) we have persistence, so an unblock on one page can persist for quite a few pages before the user switches domains or quits the tab.
2) users might disable mixed content blocker all together.  Can we add probes for security.mixed_content.block_active_content and security.mixed_content.block_display_content?  We could also easily do this in another bug so that we don't slow down progress on this bug.

Comment 8

[Tanvi Vyas[:tanvi]]

Dan, do you need help fixing up this patch?

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Dan, is this still a valid bug? If yes, do you have time to work on it. If no, I can find another owner.

Comment 10

[Daniel Veditz [:dveditz]]

I don't know if we need this information any more, best to ask Tanvi.

Comment 11

[Daniel Veditz [:dveditz]]

Although if we don't want the data then we should morph the bug into removing the telemetry probe.

Comment 12

[Tanvi Vyas[:tanvi]]

The probe is okay as it is, it does give us some information.  This bug is to expand the probe to give us more information.  Right now we record:
1) number of pages where no disable event happened (regardless of whether the page is http, https or even has mixed content)
2) number of pages where a disable event happened.

It would be nice to dive deeper intow 1), by adding a 3 and 4:
3) number of pages that are https where no disable event happens
4) number of pages that are https with mixed content where no disable event happens.

Right now, we can kind of estimate at 3 and 4 by looking at MIXED_CONTENT_PAGE_LOAD and HTTP_PAGELOAD_IS_SSL probes.

Hence, this bug is still valid.  And doesn't need to be morphed.  For the moment, it is low priority since we don't see many unblock events.  This could become higher priority as we either
* get further in HTTP deprecation goals
* move passive content types to active classifications (like TYPE_OBJECT_SUBREQUEST)