878606 - SSL Warning UI Telmetry should only measure top level frame warnings

Status: VERIFIED FIXED

(Firefox :: Security, defect)

Description

[Devdatta Akhawe [:devd]]

Bug 767676 added security ui telemetry. If an iframe has an SSL error, it shows an error in the iframe. We should measure the case for top level warning only.

Comment 1

[Devdatta Akhawe [:devd]]

Attachment: Only measure SSL UI clicks for top level frames

Comment 2

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 757160 [details] [diff] [review]
Only measure SSL UI clicks for top level frames

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

::: browser/base/content/browser.js
@@ +2371,5 @@
>          break;
>  
>        case "getMeOutOfHereButton":
> +        if (isTopFrame) {
> +        secHistogram.add(Ci.nsISecurityUITelemetry.WARNING_BAD_CERT_TOP_GET_ME_OUT_OF_HERE);

missing indentation here and on the two below.

::: security/manager/boot/public/nsISecurityUITelemetry.idl
@@ +140,5 @@
> +const uint32_t WARNING_BAD_CERT_TOP_CONFIRM_ADD_EXCEPTION_BASE = 84;
> +const uint32_t WARNING_BAD_CERT_TOP_CONFIRM_ADD_EXCEPTION_FLAG_UNTRUSTED = 1;
> +const uint32_t WARNING_BAD_CERT_TOP_CONFIRM_ADD_EXCEPTION_FLAG_DOMAIN = 2;
> +const uint32_t WARNING_BAD_CERT_TOP_CONFIRM_ADD_EXCEPTION_FLAG_TIME = 4;
> +// This uses up buckets till 91 (including)

Can you add an extra comment saying that our last bucket is 99 (or is it 100?)? I don't want someone reading the file to think that the upper limit is a power-of-two value for binary reasons

Comment 3

[Devdatta Akhawe [:devd]]

Attachment: Only measure SSL UI clicks for top level frames

Comment 4

[Devdatta Akhawe [:devd]]

Comment on attachment 758239 [details] [diff] [review]
Only measure SSL UI clicks for top level frames

carrying over the r+ from felipe

Comment 5

[Devdatta Akhawe [:devd]]

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

Try looks clean to me.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8382b3aa1bf2

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8382b3aa1bf2

Comment 8

[Devdatta Akhawe [:devd]]

Comment on attachment 758239 [details] [diff] [review]
Only measure SSL UI clicks for top level frames

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 850435 goes live in Firefox 23. This is a related fix that should have been done in 850435 but I screwed up.
User impact if declined: We will have to wait 6 weeks for accurate numbers. The numbers are being used to improve warnings across different browsers (incl. Chrome) and this will delay the change.
Testing completed (on m-c, etc.): tested on m-c. Try run is in comments.
Risk to taking this patch (and alternatives if risky): no risk. The patch is very simple. Only constants values are changed. These constants are only used in our own internal telemetry and no one else.
String or IDL/UUID changes made by this patch: UUID of an IDL that lists all constants is changed. This IDL does not have any real "interface"---it is just a list of constants.

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 758239 [details] [diff] [review]
Only measure SSL UI clicks for top level frames

We don't have to worry about IDL changes in Aurora - approving this low risk uplift so we're getting accurate data.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f8a9b16a2afd

Comment 11

[Mihai Morar, (:MihaiMorar)]

How can QA verify this fix, because there is only one visible section about security in about:telemetry->Histograms, called CACHE_SERVICE_LOCK_WAIT_MAINTHREAD_NSCACHEENTRYDESCRIPTOR_SETSECURITYINFO

Build id: 20130501031041

Comment 12

[Devdatta Akhawe [:devd]]

You will need to go to a site with a bad certificate. So, for example, I just went to https://reddit.com in a tab and then opened about:telemetry->Histograms and found a new value for bucket 68 in the SECURITY_UI histogram. I just did this in my aurora install and it works fine.

Comment 13

[Mihai Morar, (:MihaiMorar)]

Works as expected for me on Mac OS 10.8.3 and Windows 7 x64, using latest Nightly
Build id: 20130610031147

Thanks Devdatta for info!