Closed Bug 878606 Opened 6 years ago Closed 6 years ago

SSL Warning UI Telmetry should only measure top level frame warnings

Categories

(Firefox :: Security, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 24
Tracking Status
firefox23 --- fixed
firefox24 --- verified

People

(Reporter: devd, Assigned: dev.akhawe)

Details

Attachments

(1 file, 1 obsolete file)

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.
No longer depends on: 850435
Attachment #757160 - Flags: review?(felipc)
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
Attachment #757160 - Flags: review?(felipc) → review+
Attachment #757160 - Attachment is obsolete: true
Comment on attachment 758239 [details] [diff] [review]
Only measure SSL UI clicks for top level frames

carrying over the r+ from felipe
Attachment #758239 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8382b3aa1bf2
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
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.
Attachment #758239 - Flags: approval-mozilla-aurora?
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.
Attachment #758239 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
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.
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!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.