781018 - Telemetry data for Mixed Content (display and script)

Status: VERIFIED FIXED

(Firefox :: Security, defect)

Description

[Tanvi Vyas[:tanvi]]

Collect telemetry data to determine what percentage of pages that users visit have mixed display content and/or mixed script content.
https://developer.mozilla.org/en/Security/MixedContent

Comment 1

[Devdatta Akhawe [:devd]]

I don't understand: don't you want to log this data regardless of whether the user has set the preference for blocking mixed content/ mixed display?

Comment 2

[Tanvi Vyas[:tanvi]]

Yes.  But the patch in bug 62178 doesn't make that easy to do right now.

Comment 3

[Tanvi Vyas[:tanvi]]

Dev, any chance you want to take this bug on?

Comment 4

[Devdatta Akhawe [:devd]]

Unfortunately, it is unlikely that I will get time to do this.

Comment 5

[Tanvi Vyas[:tanvi]]

I believe what we need to do here is capture when we end up at line 6651 here: https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#6643.

This means we have mixed content.  (Or potentially something else; I am not familiar with all the meanings of STATE_IS_BROKEN - ex: does an invalid cert yield this mode?)

We also need to capture the Content Type when ShouldLoad() is called mxr.mozilla.org/mozilla-central/source/content/base/src/nsMixedContentBlocker.cpp#207.

The combination of these two will tell us how many pages have Mixed Content and what type of mixed content is most frequent (is it active or passive?  is it xhr or a font?).

(Ideally, we'd also love to know whether it is an advertisement or not, but we can add logic to check that in a second iteration.)

Comment 6

[Tanvi Vyas[:tanvi]]

Just spoke with Brian.  One way to do this is to have a bit mask where each bit corresponds to a content type.  As mixed content loads, the bits corresponding to the content types loaded would be set.

Then in the Documents destructor, we could put each unique combination into it's own bucket (we'd have a few thousand buckets for each possible content type combination).  That way, we'd know the number of pages that have mixed content, along with what type of mixed content it had.

Since there is a limit on bucket size for telemetry, we may have to group some of the content types together (for example, we could group script and stylesheet), but we'll see.

After that (second patch), we could create another bit mask with all the content types where the bit for a content type is only set if we think that the content is advertising related (and hence wouldn't break the page).  At the end, we will know how many pages out of all the pages with mixed content had advertising mixed content.  What we won't know (and the problem with this idea) is how many pages had only advertising mixed content (and no other kind of mixed content).  Will think more about this.

Comment 7

[Devdatta Akhawe [:devd]]

If I am not wrong, mixed content blocking should not worry about TYPE_REFRESH or TYPE_DOCUMENT. MDN says TYPE_PING is disabled in Firefox. 

This leaves you with 13 bits of buckets which is easily within the telemetry bucket max, afaik.

Comment 8

[Tanvi Vyas[:tanvi]]

This bug hasn't been updated in a while.  Here's a new summary of what we want to do.

We need 15 bit bitmask for the following 14 content types.  When one of the content types is detected as mixed content, a bit, its corresponding bit will be set.  The 15th bit is set if the content is blocked, unset if the content is allowed (because the user has overridden the blocking or because the about:config pref to block content is not set)

case TYPE_IMAGE:
case TYPE_MEDIA:
case TYPE_OBJECT_SUBREQUEST:
case TYPE_PING:
case TYPE_CSP_REPORT:
case TYPE_DTD:
case TYPE_FONT:
case TYPE_OBJECT:
case TYPE_SCRIPT:
case TYPE_STYLESHEET:
case TYPE_SUBDOCUMENT:
case TYPE_XBL:
case TYPE_XMLHTTPREQUEST:
case TYPE_OTHER:

Once the page load has completed and the bitmask has been fully populated, we want to save it.  We will have 2^15 buckets (32768) and increment the value in the bucket corresponding to the bitmask.  I have to look into the telemetry limits and see if this is within the limit.

Comment 9

[Tanvi Vyas[:tanvi]]

(In reply to Tanvi Vyas [:tanvi] from comment #8)

> The 15th bit is set if the content is blocked, unset if the
> content is allowed (because the user has overridden the blocking or because
> the about:config pref to block content is not set)

Do we want to use the 15th bit for Mixed Active Content and add a 16th bit for Mixed Display Content?

Comment 10

[Tanvi Vyas[:tanvi]]

ddahl and I got together to talk about this more.

To start with, these are the questions we want to answer:
1) What percentage of pages on the web have Mixed Active Content, Mixed Passive Content, or both? (These would be unique pages.  We would get 4 values in the end.  W% have none, X% have both, Y% have just mixed active, Z% have just mixed passive.)

2) How many pages (not unique) do users encounter that have Mixed Active Content, Mixed Passive Content, or both.

3) On pages that have Mixed Active Content, is the Mixed Active Content just because of mixed iframes?  (The result would say X% of the time Mixed Active Content is triggered by just mixed frames, Y% of the time Mixed Active Content is triggered because of frames + other stuff, and Z% of the times there is Mixed Active Content with no frames).

4) The same questions as number 3, except with fonts instead of frames.

5) On pages that have mixed object subrequests (which is Mixed Passive), is there Mixed Active content on the page?  (The result would say X% of the time there are object subrequests but no Mixed Active content on pages with object subrequests.  Y% of the time, there are object subrequests and also Mixed Active content.)

We think we may be able to answer these questions with two telemetry probes.

The first is simpler and we can start with that one:
In the checkIdentity function in browser.js, add a telemetry probe that sends the server a url hash, a flag telling the server whether there is mixed active content (STATE_BLOCKED_MIXED_ACTIVE_CONTENT or STATE_LOADED_MIXED_ACTIVE_CONTENT flag set), a flag telling the server whether there is mixed passive content (STATE_BLOCKED_MIXED_DISPLAY_CONTENT or STATE_LOADED_MIXED_DISPLAY_CONTENT flag set).  The server will aggregate data from unique url hashes.  This will answer question 1) above.

The second probe is a bit more complicated and I will describe it in the next comment.

Comment 11

[Tanvi Vyas[:tanvi]]

The second probe will help answer questions 2-5) above.

Add an event listener before a page starts loading.  When the load event is finished, the event listener will call a function (say MixedContentTelemetry()). (We are not sure where we would add this event listener).

Create a bit mask with 8 bits. (We are not sure where we would create this bitmask)

1. Is there one or more mixed content fonts on this page?
2. Is there one or more mixed content objects on this page?
3. Is there one or more mixed content frames on this page?
4. Is there one or more mixed content script/css/xhr/csp_report/DTD/XBL/other on this page.  If there are none of these types, the value will be 0.  If there are one or more of these types, the value will be 1.
5. Is there one or more mixed content object subrequests on this page?
6. Is there one or more mixed content image or media on this page?
7. Is Mixed Active Content blocked on this page?  1 if it is, 0 if it is not.
8. Is Mixed Passive Content blocked on this page?  1 if it is, 0 if it is not.

The bits will be slowly populated on each call to nsMixedContentBlocker.cpp.

When the load has completed, the event listener will call MixedContentTelemetry().

MixedContentTelemetry will send the bit mask number (between 0-256) to the server.  The server will take the number, find the corresponding bucket, and increment it by 1.

There will be a lot of pages with the value 0 (no mixed content of any type).  The number of 0's will help us answer question 2) in the previous comment.  Since sending values to the server for every page load (even ones without mixed content) can cause a performance issue, we could aggregate the 0's on the client side and send them once a day.  If sending the 0's is too complicated or something we don't want to do right now, we could just rely on the first telemetry probe to help us answer question 1) and forget about question 2).

The resulting data would be 256 buckets, each corresponding to a unique case, and the number of times it was encountered.  Parsing the data would help us answer the rest of the questions.

Comment 12

[Tanvi Vyas[:tanvi]]

Sid, can you take a look at the proposal in Comment 10 and 11?  Thanks!

Comment 13

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Tanvi Vyas [:tanvi] from comment #12)
> Sid, can you take a look at the proposal in Comment 10 and 11?  Thanks!

I think in general the approach seems good.  One nit: you mention sending URL hashes to the server and in comment 11 you mention sending the accumulated bits to the server for aggregation...  My impression is that in both of these cases, you'll want to accumulate the histogram locally and then let the telemetry service take care of communication with the server, right?

Also, with the URL hashes ... "the server will aggregate data from unique URL hashes" seems underspecified.  What's the output gonna look like?  (What's the histogram or counter?)

Comment 14

[samlh]

I have some concern about the url hashes. How will they be made such that the pages visited could not be reconstructed?

Comment 15

[David Dahl :ddahl]

(In reply to samuel.l.harrington from comment #14)
> I have some concern about the url hashes. How will they be made such that
> the pages visited could not be reconstructed?

Indeed, now that I am looking at the design more closely, I wonder if sending URL hashes will be OK. If so, we will need to add a method to https://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/Telemetry.cpp to handle the collection of the data for probe 1 as the existing telemetry APIs cannot send strings + enums.

Do we need to involve our privacy/legal folks here?

Comment 16

[Sid Stamm [:geekboy or :sstamm]]

Wait, can't you do the counting/aggregation on the client?  Instead of shipping hashes over to the server, can we just make the aggregate counts you want (on idle-daily, or before telemetry ships the ping)?

Comment 17

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

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #16)
> Wait, can't you do the counting/aggregation on the client?  Instead of
> shipping hashes over to the server, can we just make the aggregate counts
> you want (on idle-daily, or before telemetry ships the ping)?

Yes, we should avoid sending URL hashes to telemetry. I am pretty sure telemetry doesn't support that anyway. Perhaps we should punt on the "unique page" counts completely, in the interest of getting something working.

Comment 18

[David Dahl :ddahl]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #16)
> Wait, can't you do the counting/aggregation on the client?  Instead of
> shipping hashes over to the server, can we just make the aggregate counts
> you want (on idle-daily, or before telemetry ships the ping)?

The aggregation method is unclear to me. It does not seem like Telemetry provides a facility for client side aggregation.

I'm thinking the netrics team does the aggregation, in which case it seems like the histogram will look like:

"MIXED_CONTENT_HGRAM1": {
  "kind": "enumerated",
  "n_buckets": 4,
  "description": "Page load mixed content indicators"
}

// usage
let histogram = Telemetry.getHistogramById("MIXED_ACTIVE_CONTENT");
histogram.add(1); // observed mixed active content
histogram.add(2); // observed mixed passive content

Does this mean that we need to call histogram.add(0) for no mixed content, or should we only worry about recording occurrences of mixed content? Is this going to call the telemetry service too much if we make a recording on each page loaded by every browser? Also, is there a perf issue?

Comment 19

[David Dahl :ddahl]

(In reply to David Dahl :ddahl from comment #18)

This is incorrect...
> // usage
> let histogram = Telemetry.getHistogramById("MIXED_ACTIVE_CONTENT");

Should be:
let histogram = Telemetry.getHistogramById("MIXED_CONTENT_HGRAM1");

Comment 20

[Sid Stamm [:geekboy or :sstamm]]

(In reply to David Dahl :ddahl from comment #18)
> The aggregation method is unclear to me. It does not seem like Telemetry
> provides a facility for client side aggregation.

You're right.  If you want to measure anything across "distinct sites", you have to roll your own data structure to record non-aggregate data, then roll it up later in prep for telemetry ping.

For example:
http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/NewTabUtils.jsm#713

If you just want to measure things across "page encounters", it's easier, but I think you probably -- eventually -- want to do both.  Maybe start with "page encounters" and go from there?  For example, what percent of page loads exhibit the properties in question (this is #2-5 in comment 10)?

> Does this mean that we need to call histogram.add(0) for no mixed content,
> or should we only worry about recording occurrences of mixed content? 

Doing aggregation on the client side (which I maintain is going to be the easiest for all of us in the long run), you'll need something with which to compare "number of pages with mixed content" -- or the data won't be useful.  Ultimately, it sounds like you'd like to know the % of "page encounters" that have mixed content.  So you can either record total # of page encounters or split the total count into the various partitions {no mixed, mixed active, mixed passive}.

> Is
> this going to call the telemetry service too much if we make a recording on
> each page loaded by every browser? Also, is there a perf issue?

Probably not.  Once per page (nsDocument) load should not be a big perf hit.

Comment 21

[David Dahl :ddahl]

Attachment: Taking a Stab on probe 1 - WIP

Just a quick WIP, based on the idea that we are trying to produce a pie chart with all page loads, page loads with mixed active, page loads with mixed passive - of course the mixed content overlaps

Comment 22

[David Dahl :ddahl]

I just realized my patch is wrong, I need to calculate everything and call add() outside of the if/else if statements

Comment 23

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 744294 [details] [diff] [review]
Taking a Stab on probe 1 - WIP

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

::: browser/base/content/browser.js
@@ +6397,5 @@
>     *                           .hostname and .port)
>     */
>    checkIdentity : function(state, location) {
> +    let mixedContentHgram = 
> +      Services.telemetry.getHistogramById("MIXED_CONTENT_HGRAM1");

You'll want to verify that checkIdentity() is only called once per page load.  I'm not sure it is.

::: toolkit/components/telemetry/Histograms.json
@@ +3377,5 @@
> +  },
> +  "MIXED_CONTENT_HGRAM1": {
> +    "kind": "enumerated",
> +    "n_buckets": 4,
> +    "description": "Page load mixed content indicators"

description is a little unclear... maybe something like "accumulates type of content (mixed, mixed passive, unmixed...) per page load".

Comment 24

[David Dahl :ddahl]

Attachment: WIP 2

If the checkIdentity function is called only once, this new patch might work. I would like to know what the result data looks like, I *imagine* it would look like this:

[
 {time: 12345678, value: 0},
 {time: 12357890, value: 3}
]

With each array element corresponding to a page load

Comment 25

[David Dahl :ddahl]

Attachment: WIP 3

Dolske:

If you have a moment, just wanted to see if this makes sense - I want to collect telemetry on mixed content, but am unsure where to place the collection code. I want each collected bit to (roughly) correspond to a single page load.

Do I also need to check for aWebProgress.STATE_IS_WINDOW first? 

If this listener is added to each loading window, do I need to keep some state in the listener object before add()ing the telemetry data on STATE_STOP?

Comment 26

[Justin Dolske [:Dolske]]

(In reply to David Dahl :ddahl from comment #25)

> If you have a moment, just wanted to see if this makes sense - I want to
> collect telemetry on mixed content, but am unsure where to place the
> collection code. I want each collected bit to (roughly) correspond to a
> single page load.
> 
> Do I also need to check for aWebProgress.STATE_IS_WINDOW first?

Yes, and more. You'll probably basically want to clone what the "Collect telemetry data about tab load times" code right above this does. The onStateChange callback can be invoked multiple times while a page is loading.

This is probably good enough, but note that adding the code here only catches mixed-content that happens before the page's |load| event fires. So it'll miss things like https://people.mozilla.com/~dolske/tmp/mixroll.html

You could instead add probes in the code path that's actually showing the mixed-content notification (or just in onSecurityChange), but that has its own complexities and depends on what you're trying to actually measure.

Is the intent to grow this patch to the bitmask stuff discussed earlier in the bug? That seems overengineered, and not really a good fit for telemetry.

Comment 27

[David Dahl :ddahl]

(In reply to Justin Dolske [:Dolske] from comment #26)

> Is the intent to grow this patch to the bitmask stuff discussed earlier in
> the bug? That seems overengineered, and not really a good fit for telemetry.

Indeed, this is merely a probe that tells us of all pages loaded, how many are 1) mixed active, 2) mixed passive 3) not mixed content at all

Comment 28

[David Dahl :ddahl]

(In reply to Justin Dolske [:Dolske] from comment #26)

> > Do I also need to check for aWebProgress.STATE_IS_WINDOW first?
> 
> Yes, and more. You'll probably basically want to clone what the "Collect
> telemetry data about tab load times" code right above this does. The
> onStateChange callback can be invoked multiple times while a page is loading.
> 
Ok, thanks. I am curious does Ci.nsIWebProgressListener.STATE_STOP indicate that we have loaded all resources, but the actual load event probably hasn't fired yet?

Comment 29

[David Dahl :ddahl]

Attachment: WIP-4

I have a patch that saves the state of the mixedcontent discoveries to the browser window, deleting the state property when the STATE_STOP is fired. The weird thing is that with https://people.mozilla.com/~bsterne/tests/62178/test.html the discovered mixed content is 'passive' instead of active. Am I testing for the wrong flags & prefs combination? Is this just not cut and dry with the state changes and flags we have available to us on a per document-load basis?

Comment 30

[Justin Dolske [:Dolske]]

(In reply to David Dahl :ddahl from comment #29)

> The weird thing is that with
> https://people.mozilla.com/~bsterne/tests/62178/test.html the discovered
> mixed content is 'passive' instead of active.

That test is twiddling things after the |load| event fires:

  <body onload="runTests();">

Although I don't see any passive content in here. Curious. The iframe will be loaded (and the script there executed) before the top-level doc's load event fires, so that should have triggered active mixed content. Well, I'm not 100% sure exactly when the <script src="http://"> load is triggered relative to that. I'd suggest either catching the passive trigger in the debugger, or reduce the test until it's obvious why.

Comment 31

[Justin Dolske [:Dolske]]

(In reply to David Dahl :ddahl from comment #28)

> I am curious does Ci.nsIWebProgressListener.STATE_STOP indicate
> that we have loaded all resources, but the actual load event probably hasn't
> fired yet?

I'm actually not sure of the exact timing, but I'd guess that the observer is triggered immediately before the actual |load| event is dispatched.

Comment 32

[Justin Dolske [:Dolske]]

Comment on attachment 753526 [details] [diff] [review]
WIP-4

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

::: browser/base/content/browser.js
@@ +4276,5 @@
> +            delete aBrowser._mixedContent;
> +          }
> +        }
> +
> +        if ((aStateFlags & Ci.nsIWebProgressListener.STATE_IS_BROKEN)) {

Hmm, I think you need to have a TabsProgressListener.onSecurityChange() to catch this correctly. Otherwise you're just lucky to have onStatusChange being called with it before STATE_STOP.

@@ +4279,5 @@
> +
> +        if ((aStateFlags & Ci.nsIWebProgressListener.STATE_IS_BROKEN)) {
> +          if ((aStateFlags &
> +               Ci.nsIWebProgressListener.STATE_LOADED_MIXED_ACTIVE_CONTENT) &&
> +               gPrefService.getBoolPref("security.mixed_content.block_active_content")) {

Do you want only measure actually-blocked pages, or all pages that have mixed content irrespective of blocking? I'd suspect the latter (no pref check).

@@ +5978,5 @@
>          nonPopupPresent = true;
> +      // If the current window is not in private browsing mode we don't need to
> +      // look for other pb windows, we can leave the loop when finding the
> +      // first non-popup window. If however the current window is in private
> +      // browsing mode then we need at least one other pb and one non-popup

You've got a bunch of unrelated whitespaceish changes here and elsewhere.

Comment 33

[David Dahl :ddahl]

(In reply to Justin Dolske [:Dolske] from comment #32)
> Comment on attachment 753526 [details] [diff] [review]
> WIP-4
> 
> Review of attachment 753526 [details] [diff] [review]:

Thank you for the informative feedback. Patch for review coming up.

Comment 34

[David Dahl :ddahl]

Attachment: Patch for review (it works!)

This patch works. Added onSecurityChange and am getting mixedContent status from the DocShell. Now, I am just using STATE_STOP in onStateChange to send the telemetry.

Comment 35

[David Dahl :ddahl]

Attachment: mixed-content about:telemetry screen grab

Screenshot of the histogram while testing

Comment 36

[Tanvi Vyas[:tanvi]]

Comment on attachment 755446 [details] [diff] [review]
Patch for review (it works!)

This patch looks good.  Do the https://people.mozilla.com/~dolske/tmp/mixroll.html and https://people.mozilla.com/~bsterne/tests/62178/test.html test cases work with it?

+    if (ds.hasMixedActiveContentLoaded || ds.hasMixedActiveContentBlocked) {
+      mixedContent = 1;
+      if (ds.hasMixedDisplayContentLoaded || ds.hasMixedDisplayContentBlocked) {
+        mixedContent = 3;
+      }
+    } else if (ds.hasMixedDisplayContentLoaded || ds.hasMixedDisplayContentBlocked) {
+      mixedContent = 2;
+      if (ds.hasMixedActiveContentLoaded || ds.hasMixedActiveContentBlocked) {
+        mixedContent = 3;
+      }

This if is not necessary (and will never execute), since you have already checked if hasMixedActiveContentLoaded and hasMixedActiveContentBlocked in a previous if.  If both of those flags are false, then it goes into the else if for Mixed Display.  If one is true, then we never enter the else if for Mixed Display.

Thanks for your help with this David!

Comment 37

[Tanvi Vyas[:tanvi]]

(In reply to Justin Dolske [:Dolske] from comment #26)
> Is the intent to grow this patch to the bitmask stuff discussed earlier in
> the bug? That seems overengineered, and not really a good fit for telemetry.

Since we punted on sending url hashes, David's probe will answer question 2 in comment 10.

The intent was to start with the first probe and then eventually work on a second probe that would answer questions 3-5.  Since then, we've learned some new things that make those questions less important to answer.

QA has been doing testing on Alexa top sites.  77 of the top 1000 Alexa Sites have some kind of Mixed Active Content blocked on Firefox.  Of those 77, 75 are blocked on Internet Explorer and/or Chrome.  Only 2 of the top 1000 sites are effected on Firefox exclusively.  Hence, I am less worried about site compatibility issues and less worried about answering these detailed questions.

Also, Chrome is working on blocking Mixed Content frames by default in the near future.  With all three browser blocking Mixed Frames, question 3 becomes less important.

Comment 38

[David Dahl :ddahl]

(In reply to Tanvi Vyas [:tanvi] from comment #36)
> Comment on attachment 755446 [details] [diff] [review]
> Patch for review (it works!)
> 
> This patch looks good.  Do the
> https://people.mozilla.com/~dolske/tmp/mixroll.html and
> https://people.mozilla.com/~bsterne/tests/62178/test.html test cases work
> with it?
> 
> +    if (ds.hasMixedActiveContentLoaded || ds.hasMixedActiveContentBlocked) {
> +      mixedContent = 1;
> +      if (ds.hasMixedDisplayContentLoaded ||
> ds.hasMixedDisplayContentBlocked) {
> +        mixedContent = 3;
> +      }
> +    } else if (ds.hasMixedDisplayContentLoaded ||
> ds.hasMixedDisplayContentBlocked) {
> +      mixedContent = 2;
> +      if (ds.hasMixedActiveContentLoaded ||
> ds.hasMixedActiveContentBlocked) {
> +        mixedContent = 3;
> +      }
> 
> This if is not necessary (and will never execute), since you have already

What is "this"?

> checked if hasMixedActiveContentLoaded and hasMixedActiveContentBlocked in a
> previous if.  If both of those flags are false, then it goes into the else
> if for Mixed Display.  If one is true, then we never enter the else if for
> Mixed Display.

How do you think this should be handled? onSecurityChange can potentially be called many times during a page load. Perhaps we need to examine the current value of the _mixedContent property first?

I did not check the test suite with this patch, will do so presently. Thanks.

Comment 39

[David Dahl :ddahl]

Attachment: Patch 2 - better yet

Based on Tanvi's feedback, this is a better patch, still not perfect. I left the logging in for you to test.

https://ie.microsoft.com/testdrive/browser/mixedcontent/assets/woodgrove.htm <- pass
https://people.mozilla.com/~dolske/tmp/mixroll.html <- fails due to XHR, which is OK for now, perhaps we should file a new bug for this.
https://people.mozilla.com/~bsterne/tests/62178/test.html <- pass
https://people.mozilla.com/~tvyas/mixeddisplay.html <- pass

Comment 40

[Tanvi Vyas[:tanvi]]

(In reply to David Dahl :ddahl from comment #38)
> > 
> > +    if (ds.hasMixedActiveContentLoaded || ds.hasMixedActiveContentBlocked) {
> > +      mixedContent = 1;
> > +      if (ds.hasMixedDisplayContentLoaded ||
> > ds.hasMixedDisplayContentBlocked) {
> > +        mixedContent = 3;
> > +      }
> > +    } else if (ds.hasMixedDisplayContentLoaded ||
> > ds.hasMixedDisplayContentBlocked) {
> > +      mixedContent = 2;
> > +      if (ds.hasMixedActiveContentLoaded ||
> > ds.hasMixedActiveContentBlocked) {
> > +        mixedContent = 3;
> > +      }
> > 
> > This if is not necessary (and will never execute), since you have already
> 
> What is "this"?
Sorry; "this" refers to the last if().


> > checked if hasMixedActiveContentLoaded and hasMixedActiveContentBlocked in a
> > previous if.  If both of those flags are false, then it goes into the else
> > if for Mixed Display.  If one is true, then we never enter the else if for
> > Mixed Display.
> 
> How do you think this should be handled? onSecurityChange can potentially be
> called many times during a page load. Perhaps we need to examine the current
> value of the _mixedContent property first?
onSecurityChange will be called multiple times, but only the last call to it will matter for the value of _mixedContent (which is fine).  After the last call to onSecurityChange, there will later be a call to onStateChange when we reach STATE_STOP.  That will report the most recent _mixedContent value.

The _mixedContent value is determined by the flags associated with the docShell.  Assume an https page begins to load.  An http image is requested and loaded.  The docShell flag for hasMixedDisplayContentLoaded will be set to true.  onSecurityChange, the _mixedContent value would be set to 1.  Then an http script is requested and blocked.  The docshell flag for hasMixedActiveContentBlocked will be set to true (hasMixedDisplayContentLoaded won't be reset, it will still be true).  onSecurityChange, the _mixedContent value would be set to 3.  Assume there are no other loads on the page, and onStateChange is called with STATE_STOP.  The value reported to the telemetry service will be 3.  Does that make sense?

I am not sure how to handle a case where Mixed Content does not appear on the page until after STATE_STOP (Dolske's example - https://people.mozilla.com/~dolske/tmp/mixroll.html).  In these cases, the HTTPS doesn't have Mixed Active Content until the user interacts with the page and invokes the Mixed Content.  This could be many minutes after STATE_STOP is called.

Comment 41

[Tanvi Vyas[:tanvi]]

I spoke to ddahl and jst about the mixroll example, and what we could do to report accurate telemetry on pages like that (where the mixed content loads after STATE_STOP has been reached).

jst said TabsProgressListener is behaving as intended.  We never really know when a page is loading it's last resource.  We could decide to dismiss these edge cases (where Mixed Content loads are invoked after the user interacts with the page) or we could change the telemetry probe to instead report the _mixedContent value on document destroy (http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#7714).  

When you close a tab, a document is destroyed.  When a tab is navigated from one location to the next a new document is created, but the previous document isn't necessarily destroyed (since you can still hit back and go back to the page in it's previous state).  The previous document is destroyed when it is no longer in the bfcache.  If we rely on document destroy, we might still miss some cases (a different edge case).  When the browser is shut down, all the documents in open tabs and in the bfcache will be destroyed, but it might be too late to report the telemetry for these documents to the server.

It is hard to say which probe would provide us the most complete data.  We don't really know how many pages invoke mixed content after user interaction (and we haven't done any QA testing on this; the QA work we've done to test sites with Mixed Content is so far just on page loads of the Alexa Top 1000 homepages).  I also don't know how common it is to shut down the browser with multiple tabs open and with multiple documents in the bfcache (although, maybe someone at Mozilla does have some data about this).

We could create two probes.  We could start with David's current patch and see what data it returns.  Then we can decide whether or not we also want to create the second probe that will catch cases like mixroll.html.  The number of user reports and compatibility complaints will also help determine whether or not this second probe is needed.

Comment 42

[Daniel Veditz [:dveditz]]

We definitely want to catch cases like mixroll. With more and more popular sites going largely dynamic (facebook, twitter, etc -- the fad for endless pages) we'll miss a large amount of user brokenness if we're only measuring onload.

I wonder if the second probe could be a simple count of the number of times pages "go bad" after onload (does the first probe have a count of total pages visited?). Don't need to wait until document destruction, if we hit the code that changes state (and have finished loading) then increment a counter right then. (I'm handwaving over the details, you'd want passive and active counters, and maybe another for pages with both depending on how you want to track that.)

Comment 43

[Tanvi Vyas[:tanvi]]

I think we should go with two probes here.  The one ddahl has already submitted.  And then one more that captures Mixed Content loads that occur after page interaction.

Justin, have you had a chance to look at David's patch?  Thanks!

Comment 44

[Alex Keybl [:akeybl]]

We'd like this to land on the order of days, so that we can start looking at Aurora data prior to this landing on Beta.

Comment 45

[Justin Dolske [:Dolske]]

Comment on attachment 756192 [details] [diff] [review]
Patch 2 - better yet

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

I think it would also be helpful to state, concisely, what you're trying to do here overall. This bug has become a bit convoluted, so it's not really clear to me what the current question is.

::: browser/base/content/browser.js
@@ +4246,5 @@
>  };
>  
>  var TabsProgressListener = {
> +  onSecurityChange:
> +  function TPL_onSecurityChange(aBrowser, aDocShell, aRequest, aStateFlags) {

You don't need to name method functions like this (TPB_xxx) anymore, and at the method below this one already isn't.

@@ +4251,5 @@
> +    let location = aBrowser.contentDocument.location;
> +    if (location.protocol == "chrome:" || location.protocol == "about:") {
> +      return;
> +    }
> +    if (aBrowser._mixedContent == 3) {

The _mixedContent property and values are both rather vague and undefined. It would be helpful to have a more descriptive name and something noting what the values are intended to mean.

@@ +4283,5 @@
> +      }
> +    }
> +    dump(location + "\n\n");
> +    dump(mixedContent + "\n\n");
> +    aBrowser._mixedContent = mixedContent;

I don't really understand what you're trying to do here.

The code here is just caching a value computed from the docshell's properties. Why not just compute it once upon STATE_STOP, in the onStateChange code (as in previous versions of the patch)?

Yes, if you want to detect mixed content triggered after page load, you could detect that in onSecurityChange. But you never report a value cached after STATE_STOP, so this code won't accomplish that.

@@ +4307,5 @@
> +          // Send mixed content status via Telemetry
> +          let mixedContentHgram =
> +            Services.telemetry.getHistogramById("MIXED_CONTENT_PAGE_LOAD");
> +          mixedContentHgram.add(aBrowser._mixedContent);
> +          dump("final: " + aBrowser.contentDocument.location + ": " + aBrowser._mixedContent + "\n\n")

No dump()s shipping code.

@@ +4308,5 @@
> +          let mixedContentHgram =
> +            Services.telemetry.getHistogramById("MIXED_CONTENT_PAGE_LOAD");
> +          mixedContentHgram.add(aBrowser._mixedContent);
> +          dump("final: " + aBrowser.contentDocument.location + ": " + aBrowser._mixedContent + "\n\n")
> +          delete aBrowser._mixedContent;

Why delete it?

Comment 46

[Tanvi Vyas[:tanvi]]

David is out today, but hopefully he can address some of these review comments tomorrow.  I can answer a few questions now.

(In reply to Justin Dolske [:Dolske] from comment #45)
> Comment on attachment 756192 [details] [diff] [review]
> Patch 2 - better yet
> 
> Review of attachment 756192 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think it would also be helpful to state, concisely, what you're trying to
> do here overall. This bug has become a bit convoluted, so it's not really
> clear to me what the current question is.
> 

This patch will give us telemetry data the tells us how many pages users encounter (not unique) that have Mixed Active Content, Mixed Passive Content, both, or neither on initial load.  From the data, we will be able to say something like:
W% of pages that our users visit had no Mixed Content
X% of pages that our users visit had Mixed Display Content
Y% of pages that our users visit had Mixed Active Content
Z% if pages that our users visit had Mixed Active Content and Mixed Display Content.

The important numbers here will be Y and Z.  Y+Z will tell us the percentages of pages where users will encounter Mixed Active Content.  Note that whether or not the content was blocked or not does not matter here; we are just counting how many pages attempted to load it.  Some users may turn of the pref or "disable protection", but we aren't trying to count that.

We may add subsequent telemetry patches that give us more data if necessary.  But we'd like to start with this.

 
> @@ +4251,5 @@
> > +    let location = aBrowser.contentDocument.location;
> > +    if (location.protocol == "chrome:" || location.protocol == "about:") {
> > +      return;
> > +    }
> > +    if (aBrowser._mixedContent == 3) {
> 
> The _mixedContent property and values are both rather vague and undefined.
> It would be helpful to have a more descriptive name and something noting
> what the values are intended to mean.
> 

* mixedContent = 0 - There is no Mixed Content on the page
* mixedContent = 1 - The page attempted to load Mixed Display Content
* mixedContent = 2 - The page attempted to load Mixed Active Content
* mixedContent = 3 - The page attempted to load Mixed Display & Mixed Active Content

What should the name be?  mixedContentLevel?  mixedContentTelemetryLevel?

Comment 47

[Benjamin Smedberg]

How will the measurements from comment 46 help us make decisions? Let's say that 5% (or 0.5%) of pages report mixed content errors... that still doesn't give us any usable data about whether we can safely deploy this feature.

Obviously what we really care about are pages that are unusably broken by mixed-content blocking. I suspect that we don't have any good way to measure "brokenness", but we should at *least* be measuring how many times users ended up disabling mixed-content protections via the UI. This won't measure the usability of the UI, but it will give us warning signs if there are broken sites out there.

We should also measure whether users have changed the security.mixed_content.block_active_content pref... I discovered that I had flipped it to "false" in my main profile, although I don't know why. Any significant portion of users who have flipped that is an indication that we need more data and understanding.

Comment 48

[Benjamin Smedberg]

Also, since it seems that the mixed-content warning doesn't drop down automatically, we should also measure the number of times users click to show the doorhanger.

Comment 49

[David Dahl :ddahl]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #47)
> How will the measurements from comment 46 help us make decisions? Let's say
> that 5% (or 0.5%) of pages report mixed content errors... that still doesn't
> give us any usable data about whether we can safely deploy this feature.

Good question. I was under the impression we were just trying to measure how many times our users encounter mixed content.

Tanvi: Will measuring the # of times we encounter mixed content help us the way you want it to?

Comment 50

[Tanvi Vyas[:tanvi]]

This bug was to gather telemetry on how many pages have mixed content.  If we want telemetry on how many times a user clicks on the UI or clicks "disable protection on this page", please file a separate bug for that.

Comment 51

[David Dahl :ddahl]

Attachment: Patch 3, comments addressed

Tanvi: going to remove the dump() b4 requesting review, take a look, run it. dolske has schooled me again;)

Comment 52

[David Dahl :ddahl]

Attachment: Patch 3 Comments addressed

Comments addressed. Added some notes about what everything means, made variable names more descriptive. Also, removed onSecurityChange as you pointed out is not needed for this particular patch

Comment 53

[David Dahl :ddahl]

Comment on attachment 761643 [details] [diff] [review]
Patch 3 Comments addressed

a quick edit b4 review...

Comment 54

[David Dahl :ddahl]

Attachment: Patch 3, comments addressed

OK, this one is for review

Comment 55

[Justin Dolske [:Dolske]]

Comment on attachment 761652 [details] [diff] [review]
Patch 3, comments addressed

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

You're still only recording the mixedContent state at STATE_STOP, which isn't going to tell us much about modern web pages that do work after pageload (like gmail, facebook, etc). Without that I don't think the data from these probes is all that useful.

Other than that the patch is technically fine.

::: browser/base/content/browser.js
@@ +4284,5 @@
> +          let ds = aWebProgress.QueryInterface(Ci.nsIDocShell);
> +          if ((ds.hasMixedActiveContentLoaded || ds.hasMixedActiveContentBlocked) &&
> +            (ds.hasMixedDisplayContentLoaded || ds.hasMixedDisplayContentBlocked)) {
> +            mixedContentLevel = 3;
> +          }

Oh, nit: these conditionals would all get simpler to read if you did a little precomputation.

var hasMixedActive = (ds.hasMixedActiveContentLoaded || ds.hasMixedActiveContentBlocked);
var hasMixedDisplay = (ds.hasMixedDisplayContentLoaded || ds.hasMixedDisplayContentBlocked);

if (hasMixedActive && hasMixedDisplay)
  mixedContentLevel = 3;
else if (hasMixedActive && !hasMixedDisplay)
  mixedContentLevel = 1
...etc...

Comment 56

[Tanvi Vyas[:tanvi]]

(In reply to Justin Dolske [:Dolske] from comment #55)
> Comment on attachment 761652 [details] [diff] [review]
> Patch 3, comments addressed
> 
> Review of attachment 761652 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You're still only recording the mixedContent state at STATE_STOP, which
> isn't going to tell us much about modern web pages that do work after
> pageload (like gmail, facebook, etc). Without that I don't think the data
> from these probes is all that useful.
> 
> Other than that the patch is technically fine.
> 

A patch that will record data after STATE_STOP we will lose data from a different set of pages (see comment 41).  Can we have both probes, as proposed in comment 43.  We can start with this one, since the patch is done, and add a second one.  The second one will be a bit more complicated to implement and we haven't looked into how to do that yet.

Comment 57

[David Dahl :ddahl]

Attachment: Patch 4

Fixed nits. I also looked into handling the xhr cases as well. I came up with an observer that will figure out the mixed-content state on dom-window-destroy but getting back to the DocShell of the original window was not working for me. I will start a new patch for it. http://pastebin.mozilla.org/2520989 

Tanvi: did we file a bug yet?

Comment 58

[Tanvi Vyas[:tanvi]]

I filed a separate bug for the document destroy case.  So to recap, here is a summary:

* This bug 781018 is to implement a telemetry probe that tells how many pages users encounter (not unique) that have Mixed Active Content, Mixed Passive Content, both, or neither on initial load.  The probe will not tell us if Mixed Content appeared on the page after initial page load (ex: on user interaction).  From the data, we will be able to say something like:
W% of pages that our users visit had no Mixed Content
X% of pages that our users visit had Mixed Display Content
Y% of pages that our users visit had Mixed Active Content
Z% if pages that our users visit had Mixed Active Content and Mixed Display Content
This probe is ready to go, pending a final review from dolske.


* Bug 883235 is to implement a telemetry probe that tells us how many pages users encounter (not unique) that have Mixed Active Content, Mixed Passive Content, both, or neither on document destroy.  This means that we will lose data for some documents (both cached and open in current tabs) that are not destroyed until shutdown/browser crash.  From the data, we will be able to say something like:
W% of pages that our users visit had no Mixed Content
X% of pages that our users visit had Mixed Display Content
Y% of pages that our users visit had Mixed Active Content
Z% if pages that our users visit had Mixed Active Content and Mixed Display Content
This probe is not implemented, but ddahl has started thinking about what the patch would look like - http://pastebin.mozilla.org/2520989

* Neither of the above probes will be perfect, because in both of them we don't capture 100% of the pages/documents a user visits.  But they are both sufficient.

* We also have bug 882467, to count how many times a user clicks "disable protection on this page".
This probe is not implemented.

Comment 59

[Justin Dolske [:Dolske]]

Comment on attachment 762320 [details] [diff] [review]
Patch 4

Sigh. I just don't see how the data from this probe results in useful data. Tons of pages and most major modern sites do work after the |load| event fires, and so data on how many pages trigger mixed content during the initial load isn't terribly useful. At best it's a lower-bound on how often mixed-content is triggered, and that doesn't seem helpful either.

If the data's not going to be useful, then we shouldn't be adding the code.

Comment 60

[David Dahl :ddahl]

(In reply to Justin Dolske [:Dolske] from comment #59)
> Comment on attachment 762320 [details] [diff] [review]
> Patch 4
> 
> If the data's not going to be useful, then we shouldn't be adding the code.

I think I have figured out how to get any and all mixed content data. I was having trouble getting the correct DocShell from a window passed into an observer as subject for topic "dom-window-destroyed". I imagine I want to get the WebProgressListener from the DOMWindow, but was unsuccessful. The code I was tinkering is here: http://pastebin.mozilla.org/2520989

That code throws when I try to get gBrowser- as I was trying to get the original browser element. Which I now realize was unneeded.

What should I be doing here? From the patch on this bug, it looks like the WebProgressListener is provided to the onStateChange event. How can I get a handle to the webProgressListener that corresponds to the domwindow inside the observer?

Comment 61

[David Dahl :ddahl]

Attachment: New Approach

For Tanvi

Comment 62

[Justin Dolske [:Dolske]]

Random thoughts:

* You might be able to use onLocationChange + TabClose instead of actual document destruction, if that's any easier.

* What if instead you record all top-level document loads as a baseline, and also record when things go mixed (from onSecurityChange)? EG, instead of trying to exactly record "90,000 not-mixed, 10,000 mixed", you record "100,000 loads, 10,000 were mixed". You would then reconstruct the actual W/X/Y/Z from server-reported data. (100K - 10K = 90K, and so forth).

Comment 63

[David Dahl :ddahl]

Attachment: Native Patch

Trying a new native approach. Crash in nsDocShell::GetHasMixedActiveContentLoaded 

(gdb) bt
#0  0x00007fe9c26e184d in nanosleep () from /lib/x86_64-linux-gnu/libc.so.6
#1  0x00007fe9c26e16ec in sleep () from /lib/x86_64-linux-gnu/libc.so.6
#2  0x00007fe9bb43387d in ah_crap_handler (signum=11)
    at /home/ddahl/code/moz/mc/src/toolkit/xre/nsSigHandlers.cpp:88
#3  0x00007fe9bb441951 in nsProfileLock::FatalSignalHandler (signo=11, 
    info=0x7fff2b9f5c70, context=0x7fff2b9f5b40) at nsProfileLock.cpp:190
#4  <signal handler called>
#5  0x00007fe9bcfc63fd in nsDocShell::GetHasMixedActiveContentLoaded (
    this=0x7fe9944b7800, aHasMixedActiveContentLoaded=
    0x7fe9bb4f6eae <nsCOMPtr<nsIBaseWindow>::nsCOMPtr(nsQueryInterface)+62>)
    at /home/ddahl/code/moz/mc/src/docshell/base/nsDocShell.cpp:2036
#6  0x00007fe9bcfd643e in nsDocShell::Destroy (this=0x7fe9944b7800)
    at /home/ddahl/code/moz/mc/src/docshell/base/nsDocShell.cpp:4907
#7  0x00007fe9bcfd6bbc in non-virtual thunk to nsDocShell::Destroy() ()
   from /home/ddahl/code/moz/mc/obj-x86_64-unknown-linux-gnu-debug/dist/bin/libxul.so
#8  0x00007fe9bc0402ed in nsFrameLoader::Finalize (this=0x7fe9945fae80)
    at /home/ddahl/code/moz/mc/src/content/base/src/nsFrameLoader.cpp:574
#9  0x00007fe9bbfee39e in nsDocument::MaybeInitializeFinalizeFrameLoaders (
    this=0x7fe9a6dd9000)
    at /home/ddahl/code/moz/mc/src/content/base/src/nsDocument.cpp:6267
#10 0x00007fe9bc01e848 in nsRunnableMethodImpl<void (nsDocument::*)(), true>::Run (this=0x7fe98fdd31f0) at ../../../dist/include/nsThreadUtils.h:350
---Type <return> to continue, or q <return> to quit--- 
#11 0x00007fe9bbf87771 in nsContentUtils::AddScriptRunner (
    aRunnable=0x7fe98fdd31f0)
    at /home/ddahl/code/moz/mc/src/content/base/src/nsContentUtils.cpp:4831
#12 0x00007fe9bbff96ea in nsDocument::FinalizeFrameLoader (
    this=0x7fe9a6dd9000, aLoader=0x7fe9945fae80)
    at /home/ddahl/code/moz/mc/src/content/base/src/nsDocument.cpp:6223
#13 0x00007fe9bc045961 in nsFrameLoader::Destroy (this=0x7fe9945fae80)
    at /home/ddahl/code/moz/mc/src/content/base/src/nsFrameLoader.cpp:1365
#14 0x00007fe9bc472fdb in nsGenericHTMLFrameElement::DestroyContent (
    this=0x7fe9945f6950)
    at /home/ddahl/code/moz/mc/src/content/html/content/src/nsGenericHTMLFrameElement.cpp:274
#15 0x00007fe9bbf2bf16 in mozilla::dom::FragmentOrElement::DestroyContent (
    this=0x7fe9a73fdd30)
    at /home/ddahl/code/moz/mc/src/content/base/src/FragmentOrElement.cpp:950
#16 0x00007fe9bbf2bf16 in mozilla::dom::FragmentOrElement::DestroyContent (
    this=0x7fe9a1675aa0)
    at /home/ddahl/code/moz/mc/src/content/base/src/FragmentOrElement.cpp:950
#17 0x00007fe9bbf2bf16 in mozilla::dom::FragmentOrElement::DestroyContent (
    this=0x7fe99cd8cab0)
    at /home/ddahl/code/moz/mc/src/content/base/src/FragmentOrElement.cpp:950
#18 0x00007fe9bc00037f in nsDocument::Destroy (this=0x7fe9a6dd9000)
    at /home/ddahl/code/moz/mc/src/content/base/src/nsDocument.cpp:7736
---Type <return> to continue, or q <return> to quit---
#19 0x00007fe9bc498e60 in nsHTMLDocument::Destroy (this=0x7fe9a6dd9000)
    at /home/ddahl/code/moz/mc/src/content/html/document/src/nsHTMLDocument.h:78
#20 0x00007fe9bbb0ea65 in nsDocumentViewer::Destroy (this=0x7fe99c01d360)
    at /home/ddahl/code/moz/mc/src/layout/base/nsDocumentViewer.cpp:1618

Comment 64

[David Dahl :ddahl]

Attachment: Native patch 2

Smaug: I hope you have time for an initial review - this is probably not dine, but we are unsure how to make sure we are querying the docshell of the root document. Right now it seems like we get multiple results for each destroyed document.

Comment 65

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 764497 [details] [diff] [review]
Native patch 2

I wouldn't add anything to destructor of document. It is not ensured that there is window anymore, or whether the relevant docshell is alive anymore.

Just update telemetry data when various flags are first time set?
The hardcoded numbers 0-3 are odd. Should use enum or #define.
To access docshell:
  nsCOMPtr<nsISupports> container = GetContainer();
  nsCOMPtr<nsIDocShell> docshell = do_QueryInterface(container);

So, I think the setter methods could be something like
SetHasMixedActiveContentLoaded(bool aHasMixedActiveContentLoaded)
{
  if (!mHasMixedActiveContentLoaded && aHasMixedActiveContentLoaded) {
     UpdateMixedContentTelemetry(eMixedActiveContentLoaded);
  }
  mHasMixedActiveContentLoaded = aHasMixedActiveContentLoaded; 
}

And then add UpdateMixedContentTelemetry method and make it access docshell etc.

Comment 66

[Tanvi Vyas[:tanvi]]

Comment on attachment 764497 [details] [diff] [review]
Native patch 2

I believe these are the changes needed to make this work (crossing my fingers that it actually works)

1) Create bool isTopLevel, GetIsTopLevel() and SetIsTopLevel() in nsDocument.h/cpp

2) Set IsTopLevel on the Document in http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#4134

And then these changes to the patch:

># HG changeset patch
># Parent ec0b9531dda95e22b1a9b4bee8200a0ee385fd8a
>
>diff --git a/content/base/src/nsDocument.cpp b/content/base/src/nsDocument.cpp
>--- a/content/base/src/nsDocument.cpp
>+++ b/content/base/src/nsDocument.cpp
>@@ -16,16 +16,17 @@
> #ifdef MOZ_LOGGING
> // so we can get logging even in release builds
> #define FORCE_PR_LOG 1
> #endif
> #include "prlog.h"
> #include "plstr.h"
> #include "prprf.h"
> 
>+#include "mozilla/Telemetry.h"
> #include "nsIInterfaceRequestor.h"
> #include "nsIInterfaceRequestorUtils.h"
> #include "nsDocument.h"
> #include "nsUnicharUtils.h"
> #include "nsContentList.h"
> #include "nsIObserver.h"
> #include "nsIBaseWindow.h"
> #include "mozilla/css/Loader.h"
>@@ -1403,27 +1404,76 @@ ClearAllBoxObjects(nsIContent* aKey, nsP
> 
> nsIDocument::~nsIDocument()
> {
>   if (mNodeInfoManager) {
>     mNodeInfoManager->DropDocumentReference();
>   }
> }
> 
>-
> nsDocument::~nsDocument()
> {
> #ifdef PR_LOGGING
>   if (gDocumentLeakPRLog)
>     PR_LOG(gDocumentLeakPRLog, PR_LOG_DEBUG,
>            ("DOCUMENT %p destroyed", this));
> #endif
> 
>   NS_ASSERTION(!mIsShowing, "Destroying a currently-showing document");
> 


>+  nsPIDOMWindow* win = GetWindow();
>+  if (win) {
>+    nsCOMPtr<nsIDocShell> docShell = win->GetDocShell();

Change this to 

nsCOMPtr<nsIDocShell> docShell = do_QueryReferent(mDocumentContainer);
if (docShell) {

>+    int32_t itemType;
>+    docShell->GetItemType(&itemType);

Add

bool isTopLevel;
docShell->GetIsTopLevel(&isTopLevel)

>+    if (itemType == nsIDocShellTreeItem::typeContent) {
>+

Change to 
if (itemType == nsIDocShellTreeItem::typeContent && isTopLevel) {

>+      nsCOMPtr<nsIDocShellTreeItem> sameTypeRoot;
>+      docShell->GetSameTypeRootTreeItem(getter_AddRefs(sameTypeRoot));
>+      NS_ASSERTION(sameTypeRoot, "No document shell root tree item from document shell tree item!");
>+
>+      nsCOMPtr<nsIDocument> rootDoc = do_GetInterface(sameTypeRoot);
>+      NS_ASSERTION(rootDoc, "No root document from document shell root tree item.");
>+      // if (rootDoc == this) {
>+        // Record the mixed content status of the docshell in Telemetry 
>+        // mixedContentLevel = 0: There is no Mixed Content on the page
>+        // mixedContentLevel = 1: The page attempted to load Mixed Display Content
>+        // mixedContentLevel = 2: The page attempted to load Mixed Active Content
>+        // mixedContentLevel = 3: The page attempted to load Mixed Display & Mixed Active Content
>+
>+        bool mixedActiveLoaded = rootDoc->GetHasMixedActiveContentLoaded();
>+        bool mixedActiveBlocked = rootDoc->GetHasMixedActiveContentBlocked();
>+
>+        bool mixedDisplayLoaded = rootDoc->GetHasMixedDisplayContentLoaded();
>+        bool mixedDisplayBlocked = rootDoc->GetHasMixedDisplayContentBlocked();
>+      
>+        bool hasMixedDisplay;
>+        hasMixedDisplay = (mixedDisplayBlocked || mixedDisplayLoaded);
>+        bool hasMixedActive;
>+        hasMixedActive = (mixedActiveBlocked || mixedActiveLoaded);
>+
>+        uint32_t mixedContentLevel = 0;
>+        if (hasMixedDisplay && hasMixedActive) {
>+          mixedContentLevel = 3;
>+        }
>+        if (hasMixedDisplay && !hasMixedActive) {
>+          mixedContentLevel = 1;
>+        }
>+        if (hasMixedActive && !hasMixedDisplay) {
>+          mixedContentLevel = 2;
>+        }
>+        if (!hasMixedActive && !hasMixedDisplay) {
>+          mixedContentLevel = 0;
>+        }
>+        printf("mixedContentLevel: %i\n\n", mixedContentLevel);
>+        Accumulate(Telemetry::MIXED_CONTENT_PAGE_LOAD, mixedContentLevel);
>+      // }
>+    }
>+  }
>+
>   mInDestructor = true;
>   mInUnlinkOrDeletion = true;
> 
>   mCustomPrototypes.Clear();
> 
>   nsISupports* supports;
>   QueryInterface(NS_GET_IID(nsCycleCollectionISupports), reinterpret_cast<void**>(&supports));
>   NS_ASSERTION(supports, "Failed to QI to nsCycleCollectionISupports?!");

Comment 67

[David Dahl :ddahl]

(In reply to Tanvi Vyas [:tanvi] from comment #66)

> 2) Set IsTopLevel on the Document in
> http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.
> cpp

I am getting:

1:34.65 nsDocument.cpp
 1:43.66 /home/ddahl/code/moz/mc/src/content/base/src/nsDocument.cpp:1429:15: error: no member named 'GetIsTopLevel' in 'nsIDocShell'
 1:43.66     docShell->GetIsTopLevel(&isTopLevel);

Then I tried just calling GetIsTopLevel(true); thinking that the nsIDocument would know how to call the nsDocument setter.

What is the proper method for QIing to nsDocument from inside of an nsIDocument instance?

Comment 68

[Tanvi Vyas[:tanvi]]

(In reply to David Dahl :ddahl from comment #67)
> (In reply to Tanvi Vyas [:tanvi] from comment #66)
> 
> > 2) Set IsTopLevel on the Document in
> > http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.
> > cpp
> 
> I am getting:
> 
> 1:34.65 nsDocument.cpp
>  1:43.66
> /home/ddahl/code/moz/mc/src/content/base/src/nsDocument.cpp:1429:15: error:
> no member named 'GetIsTopLevel' in 'nsIDocShell'
>  1:43.66     docShell->GetIsTopLevel(&isTopLevel);

Are you putting the getters and setters in the docshell or the document?  I think this goes back to why I have getters and setters for the hasMixed* flags in both document and docshell.  Maybe you need both.  Can I see a draft of the patch?

Comment 69

[David Dahl :ddahl]

(In reply to Tanvi Vyas [:tanvi] from comment #68)

> Are you putting the getters and setters in the docshell or the document?  I
> think this goes back to why I have getters and setters for the hasMixed*
> flags in both document and docshell.  Maybe you need both.  Can I see a
> draft of the patch?

nsDocument.cpp/.h - uploading the patch now.

Comment 70

[David Dahl :ddahl]

Attachment: Native Patch

Comment 71

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 766051 [details] [diff] [review]
Native Patch

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

Hi David,

I hope you don't mind these drive-by comments :)

Monica

::: content/base/src/nsDocument.cpp
@@ +1454,5 @@
> +
> +      uint32_t mixedContentLevel = 0;
> +      if (hasMixedDisplay && hasMixedActive) {
> +        mixedContentLevel = 3;
> +      }

you can save a few nanoseconds by changing the below to else ifs, or doing

mixedContentLevel = hasMixedDisplay << 1 | hasMixedActive

or doing

#define HAS_MIXED_DISPLAY 0x00000001
#define HAS_BLOCKED_DISPLAY 0x00000002
#define HAS_MIXED_ACTIVE  0x00000003
#define HAS_BLOCKED_ACTIVE 0x00000004
uint32_t sample;

if (rootDoc->GetHasMixedDisplayContent()) {
  sample |= HAS_MIXED_DISPLAY;
}
etc

That way later on you can get any combination out of the accumulator, e.g. num pages that have mixed display blocked but not loaded, etc.

@@ +4218,5 @@
>  {
>    mDocumentContainer = do_GetWeakReference(aContainer);
>    EnumerateFreezableElements(NotifyActivityChanged, nullptr);
> +  nsRefPtr<nsDocument> doc = do_QueryInterface(this);
> +  doc->SetIsTopLevel(true);

When do we ever SetIsTopLevel(false)?

@@ +4351,5 @@
>  }
> +nsresult
> +nsDocument::GetIsTopLevel(bool aIsTopLevel)
> +{
> +  // XXXddahl: ASSERT here that we have a bool?

Not sure what this means -- you'll get a compile error if aIsTopLevel is not a bool

@@ +4352,5 @@
> +nsresult
> +nsDocument::GetIsTopLevel(bool aIsTopLevel)
> +{
> +  // XXXddahl: ASSERT here that we have a bool?
> +  aIsTopLevel = mIsTopLevel;

primitives are passed by value in C, so this won't work. You'll need to pass a reference to aIsTopLevel.

::: content/base/src/nsDocument.h
@@ +1299,5 @@
>    bool mAllowRelocking:1;
>  
>    bool mAsyncFullscreenPending:1;
>  
> +  bool mIsTopLevel:1;

I know this is legal, but man is it ugly. Much better to initialize members in the constructor or constructor initialization list. I guess your reviewers might prefer consistency over beauty though.

@@ +1301,5 @@
>    bool mAsyncFullscreenPending:1;
>  
> +  bool mIsTopLevel:1;
> +
> +  virtual void GetIsTopLevel(bool aIsTopLevel);

This needs to be a reference. Also, no need to make it virtual unless you forsee children needing to overload it (probably not)

::: toolkit/components/telemetry/Histograms.json
@@ +3579,5 @@
>      "description": "The result of the startup default desktop browser check."
> +  },
> +  "MIXED_CONTENT_PAGE_LOAD": {
> +    "kind": "enumerated",
> +    "n_values": 4,

This would be 16 if you chose the other way of bucketizing

Comment 72

[Tanvi Vyas[:tanvi]]

Attachment: Native Patch v4

I've updated the patch so that it compiles now.  I addressed some of Monica's comments, but not all of them yet since there are some other issues to work out first).

The patch compiles and I get Mixed Content data, but it doesn't seem to be accurate.  I think I'm getting mixed content level 0 loads from about:blank pages (which I thought were nsIDocShellTreeItem::typeChrome, and hence shouldn't be reporting telemetry - http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsIDocShellTreeItem.idl#38).  And in some cases where I visit mixed content pages and then close the associated tabs, I'm not getting Mixed Content telemetry data at all.

Anyway, I'm uploading my current patch and will see if I can do more testing/debugging tomorrow.

Comment 73

[Tanvi Vyas[:tanvi]]

Comment on attachment 767547 [details] [diff] [review]
Native Patch v4

(In reply to Tanvi Vyas [:tanvi] from comment #72)
> Created attachment 767547 [details] [diff] [review]
> Native Patch v4
>
> I think I'm getting mixed content level 0 loads from about:blank
> pages (which I thought were nsIDocShellTreeItem::typeChrome, and hence
> shouldn't be reporting telemetry -
> http://mxr.mozilla.org/mozilla-central/source/docshell/base/
> nsIDocShellTreeItem.idl#38).  
about:blank isn't typeChrome, it is typeContent.  about:addons is probably typeChrome.  This patch doesn't look at the protocol to check if it is http/https/ftp, etc, but I think that is okay.  So we will collect mixed content telemetry for about:blank pages.

> And in some cases where I visit mixed content
> pages and then close the associated tabs, I'm not getting Mixed Content
> telemetry data at all.
>
It seems as though nsDocument's destructor isn't called when you close a tab.  Olli, does this sound right to you?  If this is the case, then we will lose telemetry data for closed tabs (not just on browser shutdown, which is what we originally thought in Comment 58).  I'm starting to think the document destructor isn't a reliable place to get Mixed Content data.

Using gdb to add a breakpoint in the document destructor, I open a new window, navigate to a mixed content page, and then close the window.  After a short lag, I hit the breakpoint 3 or more times; no docshell exists so no telemetry can be reported.

I also tried opening a new tab (instead of window) and navigate to a mixed content page, and then closing the tab.  I again hit the breakpoint multiple times, most times without a docshell.  When a docshell does exist, it gives me a mixedContentLevel of 0 instead of 1, 2, or 3.  

Also, the document destructor is called multiple times (sometimes when I just open the new window and haven't navigated anywhere yet), so it is hard to tell when I've caused the document destructor by closing a tab/window or navigating a page, or when it is called by something else.

Marking the patch r? to smaug because in theory it should work, but I'm stuck here and don't know how to get accurate data.

Comment 74

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Tanvi Vyas [:tanvi] from comment #73)
> > I think I'm getting mixed content level 0 loads from about:blank
> > pages (which I thought were nsIDocShellTreeItem::typeChrome, and hence
> > shouldn't be reporting telemetry -
> > http://mxr.mozilla.org/mozilla-central/source/docshell/base/
> > nsIDocShellTreeItem.idl#38).  
> about:blank isn't typeChrome, it is typeContent.  about:addons is probably
> typeChrome.  This patch doesn't look at the protocol to check if it is
> http/https/ftp, etc, but I think that is okay.  So we will collect mixed
> content telemetry for about:blank pages.
type of a docshell is a property of the docshell object, not related to the url.

Comment 75

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Tanvi Vyas [:tanvi] from comment #73)
> It seems as though nsDocument's destructor isn't called when you close a
> tab.  Olli, does this sound right to you? 
dtor will be called when the document is deleted. So, usually after cycle collector has run and nothing
keeps document alive.
If the dtor isn't ever called, that means we have a leak.

> Using gdb to add a breakpoint in the document destructor, I open a new
> window, navigate to a mixed content page, and then close the window.  After
> a short lag, I hit the breakpoint 3 or more times; no docshell exists so no
> telemetry can be reported.
Yes, sounds right that when document's dtor is called, the docshell may be gone already.

Comment 76

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 767547 [details] [diff] [review]
Native Patch v4


> nsDocument::~nsDocument()
> {
> #ifdef PR_LOGGING
>   if (gDocumentLeakPRLog)
>     PR_LOG(gDocumentLeakPRLog, PR_LOG_DEBUG,
>            ("DOCUMENT %p destroyed", this));
> #endif
> 
>   NS_ASSERTION(!mIsShowing, "Destroying a currently-showing document");
>+  nsCOMPtr<nsIDocShell> docShell = do_QueryReferent(mDocumentContainer);
>+  if (docShell) {
So as I said on IRC or somewhere, this isn't reliable. When the dtor is called the docshell is
possibly gone already.

Comment 77

[Olli Pettay [:smaug][[e-mail]]]

You can report telemetry when dtor is called, but you need to collect relevant data before it, 
like check the docshell type.

Comment 78

[David Dahl :ddahl]

(In reply to Olli Pettay [:smaug] from comment #77)
> You can report telemetry when dtor is called, but you need to collect
> relevant data before it, 
> like check the docshell type.

What would be the best/efficient way to collect this data? Where would you store it?

Comment 79

[Olli Pettay [:smaug][[e-mail]]]

Check the docshell type when document is bound to a docshell. (container is set)

Comment 80

[Tanvi Vyas[:tanvi]]

Attachment: Native Patch v5

Checking the itemType and rootDocShell in SetContainer, instead of the document destructor.  This patch seems to work now.

Comment 81

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 767914 [details] [diff] [review]
Native Patch v5


>+    bool hasMixedDisplay;
>+    hasMixedDisplay = (mixedDisplayBlocked || mixedDisplayLoaded);
bool hasMixedDisplay = mixedDisplayBlocked || mixedDisplayLoaded;


>+    bool hasMixedActive;
>+    hasMixedActive = (mixedActiveBlocked || mixedActiveLoaded);
bool hasMixedActive = mixedActiveBlocked || mixedActiveLoaded;
>+
>+    uint32_t mixedContentLevel = 0;
>+    if (hasMixedDisplay && hasMixedActive) {
>+      mixedContentLevel = 3;
>+    }
>+    if (hasMixedDisplay && !hasMixedActive) {
>+      mixedContentLevel = 1;
>+    }
>+    if (hasMixedActive && !hasMixedDisplay){
>+      mixedContentLevel = 2;
>+    }
>+    if (!hasMixedActive && !hasMixedDisplay) {
>+      mixedContentLevel = 0;
>+    }
You have set mixedContentLevel to 0 already.

Please make some enum for the levels. Would make the code easier to read than these random numbers.


>+    printf("\n\n-----------------mixedContentLevel: %i\n\n", mixedContentLevel);
Remove this printf


>+NS_IMETHODIMP
>+nsDocument::GetIsTopLevel(bool* aIsTopLevel)
>+{
>+  // XXXddahl: ASSERT here that we have a bool?
>+  *aIsTopLevel = mIsTopLevel;
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsDocument::SetIsTopLevel(bool aIsTopLevel)
>+{
>+  // XXXddahl: ASSERT here that we have a bool?
XXXddahl comments are just super-odd :) Remove them.


>diff --git a/content/base/src/nsDocument.h b/content/base/src/nsDocument.h
>--- a/content/base/src/nsDocument.h
>+++ b/content/base/src/nsDocument.h
>@@ -1116,16 +1116,22 @@ protected:
> public:
>   // Get our title
>   virtual void GetTitle(nsString& aTitle) MOZ_OVERRIDE;
>   // Set our title
>   virtual void SetTitle(const nsAString& aTitle, mozilla::ErrorResult& rv) MOZ_OVERRIDE;
> 
>   static void XPCOMShutdown();
> 
>+  bool mIsTopLevel:1;
>+
>+  nsresult GetIsTopLevel(bool* aIsTopLevel);
>+
>+  nsresult SetIsTopLevel(bool aIsTopLevel);

No need for these to return nsresult.
Just make them return bool and call them
IsTopLevel and SetIsTopLevel

Comment 82

[Tanvi Vyas[:tanvi]]

Attachment: Native Patch v5

Addressed review comments and fixed the issues I was having with inaccuracy.  Also removed reporting for pages with an about: principal.

Note that if a user visits a page with mixed active content and disables protection on the page, that will get counted as 2 separate mixed active content pages since it requires 2 separate loads.  This is okay with me as it is 2 different loads.  I just realized this wasn't documented anywhere, so I wanted to include it in the bug.

Comment 84

[Tanvi Vyas[:tanvi]]

Attachment: Native Patch v6

Changing if's to else if's and removed the printf.

Comment 85

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 768032 [details] [diff] [review]
Native Patch v6


>+  bool isTopLevel = IsTopLevel();
>+
>+  if (isTopLevel) {
if (IsTopLevel())

But I think IsTopLEvel should be renamed to IsTopLevelContentDocument()
A bit long, but at least it tells what it means.


;
>+      if (hasMixedDisplay && hasMixedActive) {
>+        mixedContentLevel = MIXED_DISPLAY_AND_ACTIVE_CONTENT;
>+      } else if (hasMixedActive && !hasMixedDisplay){
You can drop  && !hasMixedDisplay

>+        mixedContentLevel = MIXED_ACTIVE_CONTENT;
>+      } else if (hasMixedDisplay && !hasMixedActive) {
you can drop  && !hasMixedActive

> nsIDocument::SetContainer(nsISupports* aContainer)
> {
>   mDocumentContainer = do_GetWeakReference(aContainer);
>   EnumerateFreezableElements(NotifyActivityChanged, nullptr);
>+  // Get the Docshell
>+  nsCOMPtr<nsIDocShell> docShell = do_QueryReferent(mDocumentContainer);
nsCOMPtr<nsIDocShell> docShell = do_QueryInterface(aContainer);

>+nsDocument::IsTopLevel()
>+{
>+  return mIsTopLevel;
>+}
>+
>+void
>+nsDocument::SetIsTopLevel(bool aIsTopLevel)
>+{
>+  mIsTopLevel = aIsTopLevel;
>+}
As I said earlier, rename these to indicate that it is about top level content document

Comment 86

[Tanvi Vyas[:tanvi]]

Attachment: Native Patch v7

Comments addressed.  Carrying over r+ from Olli.

Comment 87

[Tanvi Vyas[:tanvi]]

Attachment: Native Patch 8

Missed a couple of things.  Updated patch.

Comment 88

[Tanvi Vyas[:tanvi]]

Noticed an open question in the patch...

bool isAboutScheme = true;  // Should this fail open or closed?  If there is no uri, then do we report or not report?

I'm not sure in which cases a uri wouldn't exist.  But in cases where it doesn't, we won't know if it's an about: uri or not.  We have to decide whether to report on this anyway (isAboutScheme = false by default) or to ignore cases where the uri does not exist (isAboutScheme = true by default).

Olli, what do you think?  Do you know why the uri might not exist?

Comment 89

[Tanvi Vyas[:tanvi]]

Pushed to try - https://tbpl.mozilla.org/?tree=Try&rev=984796e0a686

Comment 90

[Tanvi Vyas[:tanvi]]

Attachment: Native Patch 9

(In reply to Tanvi Vyas [:tanvi] from comment #88)
> Noticed an open question in the patch...
> 
> bool isAboutScheme = true;  // Should this fail open or closed?  If there is
> no uri, then do we report or not report?
> 
> I'm not sure in which cases a uri wouldn't exist.  But in cases where it
> doesn't, we won't know if it's an about: uri or not.  We have to decide
> whether to report on this anyway (isAboutScheme = false by default) or to
> ignore cases where the uri does not exist (isAboutScheme = true by default).
> 
> Olli, what do you think?  Do you know why the uri might not exist?

I spoke with Olli on irc and he said to only report on normal cases where the nsIURI exists.  Hence, we should stick with isAboutScheme = true.  I removed the unnecessary comment from the patch.  If try looks good, I will land this.

Comment 91

[Tanvi Vyas[:tanvi]]

Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/d8d194d3dcc1

Comment 92

[Tanvi Vyas[:tanvi]]

Comment on attachment 768537 [details] [diff] [review]
Native Patch 9

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Mixed Content Blocker
User impact if declined: We will have less telemetry data.  We won't start collecting Mixed Content Telemetry until Firefox 25, when the feature went out in Firefox 23.
Testing completed (on m-c, etc.): Just landed on inbound.
Risk to taking this patch (and alternatives if risky): Added some code to nsDocument.cpp.
String or IDL/UUID changes made by this patch:

I would like to uplift to Aurora and Beta (which have the Mixed Content Blocker enabled).  It would also be nice to uplift to stable (so that we can get some data from stable users before they are affected by the feature), but is not necessary.  I have marked approval-mozilla-release-? and I leave it up to release management on whether or not they want to uplift this to stable.

Comment 93

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d8d194d3dcc1

Comment 94

[bhavana bajaj [:bajaj]]

Comment on attachment 768537 [details] [diff] [review]
Native Patch 9

Looks good to land on aurora/beta.Does not qualify for a chemspill or ride along for Fx22 hence a- for release.

Comment 95

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a0197043b0e3
https://hg.mozilla.org/releases/mozilla-beta/rev/9c2ae0462c92

Comment 96

[u279076]

Matt, can you please evaluate if this needs some regression testing in Firefox 23 Beta?

Comment 97

[Matt Wobensmith [:mwobensmith][:matt:]]

To answer Anthony's question - this is telemetry, meant to explore the impact of the feature and to validate its usefulness.

This feature is not exposed to a user and has no way for us to meaningfully test. I don't see what QA can do here.

Comment 98

[u279076]

Thanks Matt, tagging this [qa-] based on your comment.

Comment 99

[David Dahl :ddahl]

(In reply to Matt Wobensmith from comment #97)
> To answer Anthony's question - this is telemetry, meant to explore the
> impact of the feature and to validate its usefulness.
> 
> This feature is not exposed to a user and has no way for us to meaningfully
> test. I don't see what QA can do here.

It is possible to test manually, using the urls in  comment 39, you can load them each twice and check in about:telemetry for results. The results to look for are labeled "MIXED_CONTENT_"*

Comment 100

[Matt Wobensmith [:mwobensmith][:matt:]]

Thanks David.

In both today's m-c and Aurora, I used the URLs from comment 39, loaded them twice. I saw the Mixed Content UI.

I then opened up the about:telemetry window and expanded all categories to search for that string.

I don't see anything with that label. 

Am I doing something wrong?

Comment 101

[David Dahl :ddahl]

(In reply to Matt Wobensmith from comment #100)
> 
> I don't see anything with that label. 

Sorry, the label is in "Historgrams" and is 'MIXED_CONTENT_PAGE_LOAD' - working for me in today's Nightly

Comment 102

[Matt Wobensmith [:mwobensmith][:matt:]]

Ah, I think the problem is that this only appears to work on Release builds. I had been using Debug builds, in which the telemetry does not appear to work.

Is this expected?

Comment 103

[Tanvi Vyas[:tanvi]]

(In reply to Matt Wobensmith from comment #102)
> Ah, I think the problem is that this only appears to work on Release builds.
> I had been using Debug builds, in which the telemetry does not appear to
> work.
> 
> Is this expected?

No, it should work on debug builds too.  Do you have to turn reporting on for about:telemetry to work?  Or does it work regardless, and by turning it on you are sending the data to our servers?

Comment 104

[Matt Wobensmith [:mwobensmith][:matt:]]

1. New profile, about:telemetry, turn it telemetry.
2. Visit those URLs, twice each.
3. Return to telemetry page, look under Histogram for data.

On Mac - today's m-c and Aurora, debug - no data or label for MIXED_CONTENT_PAGE_LOAD.

However, if I repeat those steps using today's release build, I do indeed see the corresponding label MIXED_CONTENT_PAGE_LOAD and data.

Comment 105

[Mihai Morar, (:MihaiMorar)]

(In reply to Matt Wobensmith from comment #104)
> On Mac - today's m-c and Aurora, debug - no data or label for
> MIXED_CONTENT_PAGE_LOAD.

I've done a verificaton on Mac OS 10.9 and I can confirm the fix is verified following STR from Comment 104 and using URL from Comment 29. After step 2 you have to refresh telemetry page, so MIXED_CONTENT_PAGE_LOAD to be displayed in Histrograms section.


Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:25.0) Gecko/20130714 Firefox/25.0(20130714030202)

It's also fixed on Windows 7 x64 and Ubuntu 13.04 x86 on Latest Nightly 25.

Comment 106

[Mihai Morar, (:MihaiMorar)]

Matt if you need I can verify this on FF 23beta and Latest Aurora too

Comment 107

[Matt Wobensmith [:mwobensmith][:matt:]]

Hi Mihai - the problem appears, for me, only on debug builds. Can you try a debug build and see what happens?

Comment 108

[Mihai Morar, (:MihaiMorar)]

(In reply to Matt Wobensmith from comment #107)
> Hi Mihai - the problem appears, for me, only on debug builds. Can you try a
> debug build and see what happens?

Sure, please tell me if is necessary to debug on all OS (Mac, Win, Ubuntu) or is enought on one of them. Also please tell me on with branch to debug (Nightly/Aurora/Beta/Release).
Thanks!

Comment 109

[Mihai Morar, (:MihaiMorar)]

Removing [qa-] until I'll finish this investigation.

Comment 110

[Tanvi Vyas[:tanvi]]

(In reply to Mihai Morar, QA [:MarioMi] from comment #108)
> (In reply to Matt Wobensmith from comment #107)
> > Hi Mihai - the problem appears, for me, only on debug builds. Can you try a
> > debug build and see what happens?
> 
> Sure, please tell me if is necessary to debug on all OS (Mac, Win, Ubuntu)
> or is enought on one of them. Also please tell me on with branch to debug
> (Nightly/Aurora/Beta/Release).
> Thanks!

One platform should be fine.  Telemetry should be in the latest Nightly, Aurora, and Beta.  Make sure telemetry is enabled.  Also, if you shut down the browser, the telemetry data will disappear because it is per session.

Comment 111

[Tanvi Vyas[:tanvi]]

When I test this on my local build of mozilla-central (pulled from Friday), I see the telemetry data for MIXED_CONTENT_PAGE_LOAD.

Comment 112

[Matt Wobensmith [:mwobensmith][:matt:]]

I rebuilt yesterday and now I can see the telemetry data in a debug build. So, somewhere between July 11th and now, something changed.

So, good news, let's close this out and move on to better things.