Implement Security UI Telemetry

RESOLVED FIXED in Firefox 17

Status

()

Firefox
Security
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: devd, Assigned: devd)

Tracking

unspecified
Firefox 17
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 18 obsolete attachments)

11.84 KB, patch
Felipe
: review+
Details | Diff | Splinter Review
6.85 KB, patch
Felipe
: review+
Details | Diff | Splinter Review
2.30 KB, patch
mayhemer
: review+
briansmith
: review+
Details | Diff | Splinter Review
4.57 KB, patch
devd
: review+
Details | Diff | Splinter Review
11.48 KB, patch
devd
: review+
Details | Diff | Splinter Review
23.87 KB, patch
gkw
: review+
gkw
: checkin+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
More details on why here (currently a little rough)

wiki.mozilla.org/Security/Features/UI_Telemetry
(Assignee)

Comment 1

5 years ago
Created attachment 636036 [details] [diff] [review]
Base patch that setups the IDL

first time actually using queues, this 'base patch' might be stupid.
Assignee: nobody → dev.akhawe+mozilla
Status: NEW → ASSIGNED
(Assignee)

Comment 2

5 years ago
Created attachment 636037 [details] [diff] [review]
Measure UI and Clickthrough rates for Addon install flow
(Assignee)

Comment 3

5 years ago
Created attachment 636038 [details] [diff] [review]
Measure UI and Clickthrough rates for modal dialogs thrown by nsSecurityWarnings.cpp
(Assignee)

Comment 4

5 years ago
Created attachment 638966 [details] [diff] [review]
Telemetry for Malware/Phishing Warnings
(Assignee)

Comment 5

5 years ago
Created attachment 638967 [details] [diff] [review]
Telemetry for SSL Warnings UI
(Assignee)

Comment 6

5 years ago
Created attachment 639168 [details] [diff] [review]
Telemetry for SSL Warnings UI
Attachment #638967 - Attachment is obsolete: true
I'm excited to see this happening! Are these works in progress, or should we be finding reviewers?
(Assignee)

Comment 8

5 years ago
@johnath: I am waiting on reviews.
(In reply to Devdatta Akhawe from comment #8)
> @johnath: I am waiting on reviews.

Typically, then, you should set the review:? flag and name specific reviewers for each patch. If you need help finding reviewers, I suspect dherman can help out.
(Assignee)

Comment 10

5 years ago
uggh.. I messed up :(

I confused this bug with 707275 (SSL telemetry) where I am waiting for review.

For this bug, I want to get initial feedback from Sid Stamm. I am meeting with Sid today, and we will be discussing this. I will then request a review. Sorry for the confusing comment earlier.

Thanks for your interest. If you have thoughts on the feature and what things we should measure, I would love to hear them.
(Assignee)

Updated

5 years ago
Attachment #636036 - Flags: feedback?(sstamm)
(Assignee)

Updated

5 years ago
Attachment #636037 - Flags: feedback?(sstamm)
(Assignee)

Updated

5 years ago
Attachment #636038 - Flags: feedback?(sstamm)
(Assignee)

Updated

5 years ago
Attachment #638966 - Flags: feedback?(sstamm)
(Assignee)

Updated

5 years ago
Attachment #639168 - Flags: feedback?(sstamm)
Comment on attachment 636036 [details] [diff] [review]
Base patch that setups the IDL

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

::: toolkit/components/telemetry/TelemetryHistograms.h
@@ +452,5 @@
>  
> +/**
> + * Security UI Telemetry
> + */
> +HISTOGRAM_ENUMERATED_VALUES(SECURITY_UI, 1000, "Security UI Telemetry")

1000?  Are there really 1000 things we're gonna measure?
Attachment #636036 - Flags: feedback?(sstamm)
(Assignee)

Comment 12

5 years ago
Nope. I will change the final value once I am done with all of them.
Comment on attachment 636037 [details] [diff] [review]
Measure UI and Clickthrough rates for Addon install flow

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

Your patches need more lines of context.  I'm only getting three which is not enough for reviewing.  Need 8.  https://developer.mozilla.org/en/Creating_a_patch_that_can_be_checked_in

The approach looks reasonable.  Fix my nits, then we'll flag it for review (probably mossop on this patch).

::: browser/base/content/browser-addons.js
@@ +3,4 @@
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +

Unnecessary whitespace change.  Remove this please.

::: security/manager/ssl/public/Makefile.in
@@ +67,4 @@
>      nsIKeyModule.idl \
>      nsIProtectedAuthThread.idl \
>      nsIDataSignatureVerifier.idl \
> +    nsISecurityUITelemetry.idl \

Please fix this typo in the patch that adds this line.

::: security/manager/ssl/public/nsISecurityUITelemetry.idl
@@ +14,5 @@
> + * Addon installation warnings
> + */
> +
> +//Firefox prevented this site from asking you to install addon
> +const PRUint32 WARNING_ADDON_ASKING_PREVENTED = 1;

This name confuses me... perhaps use something like WARNING_PREVENTED_ADDON_INSTALL

@@ +16,5 @@
> +
> +//Firefox prevented this site from asking you to install addon
> +const PRUint32 WARNING_ADDON_ASKING_PREVENTED = 1;
> +//User clicks through and allows site to ask to install addons
> +const PRUint32 WARNING_ADDON_ASKING_PREVENTED_CLICK_THROUGH = 2;

And then here... something like WARNING_CLICKEDTHRU_PREVENTED_ADDON_INSTALL.  Or if you describe them well in the IDL, you could use shorter names like:
ADDON_INSTALL_1, ADDON_INSTALL_2, etc.

@@ +22,5 @@
> +//Are you sure you want to install this addon? Only install addons you trust
> +const PRUint32 WARNING_CONFIRM_ADDON_INSTALL = 3;
> +
> +//User clicked she is sure after waiting 3secs
> +const PRUint32 WARNING_CONFIRM_ADDON_INSTALL_CLICK_THROUGH  = 4;

Nit: Use blank lines here consistently.

::: toolkit/mozapps/extensions/amWebInstallListener.js
@@ +173,4 @@
>      args.wrappedJSObject = args;
>  
>      try {
> +      let secHistogram = Cc["@mozilla.org/base/telemetry;1"].getService(Ci.nsITelemetry).getHistogramById("SECURITY_UI");

Nit: this line is way too long, please wrap it.
Attachment #636037 - Flags: feedback?(sstamm)
(Assignee)

Comment 14

5 years ago
Sorry about the context, I was pretty sure I had fixed my .hgrc. 

There are two popups in the addon install flow. The first is "Firefox prevented this site from asking you to install", once you allow the site to ask you, the site actually tries to install and Firefox prevents that and shows up a dialog saying "Are you sure you want to install?" This is why I chose "ADDON_ASKING_PREVENTED" as the name here: PREVENTED_ADDON_INSTALL doesn't seem right.


browser.js already has some pretty  long JS lines; so I figured that line wrapping was only recommended for C/C++. What's the recommended way to wrap JS lines? I tried breaking on property accesses (the dots), but it looked pretty ugly to me.
Comment on attachment 636038 [details] [diff] [review]
Measure UI and Clickthrough rates for modal dialogs thrown by nsSecurityWarnings.cpp

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

Please upload new patches with more lines of context and re-flag for feedback.  While you're at it, please look for any unnecessary newlines you've added -- there is at least one in this patch.
Attachment #636038 - Flags: feedback?(sstamm)
(Assignee)

Comment 16

5 years ago
Created attachment 639858 [details] [diff] [review]
UI Telemetry for SSL Warnings
(Assignee)

Updated

5 years ago
Attachment #639168 - Attachment is obsolete: true
Attachment #639168 - Flags: feedback?(sstamm)
(Assignee)

Comment 17

5 years ago
Created attachment 639859 [details] [diff] [review]
Measure prevalence and clickthrough of malware/phishing warnings
(Assignee)

Updated

5 years ago
Attachment #638966 - Attachment is obsolete: true
Attachment #638966 - Flags: feedback?(sstamm)
(Assignee)

Comment 18

5 years ago
Created attachment 639860 [details] [diff] [review]
Measure UI and Clickthrough rates for modal dialogs thrown by nsSecurityWarnings.cpp
(Assignee)

Updated

5 years ago
Attachment #636038 - Attachment is obsolete: true
(Assignee)

Comment 19

5 years ago
Created attachment 639861 [details] [diff] [review]
Measure UI and Clickthrough rates for Addon install flow
(Assignee)

Updated

5 years ago
Attachment #636037 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #639861 - Flags: feedback?(sstamm)
(Assignee)

Updated

5 years ago
Attachment #639860 - Flags: feedback?(sstamm)
(Assignee)

Updated

5 years ago
Attachment #639859 - Flags: feedback?(sstamm)
(Assignee)

Updated

5 years ago
Attachment #639858 - Flags: feedback?(sstamm)
(Assignee)

Updated

5 years ago
Attachment #636036 - Flags: feedback?(sstamm)
(Assignee)

Comment 20

5 years ago
@geekboy: I think I have fixed all the nits. Should I go ahead and flag mossop for review?
Yes... and please update the names on the "imported patches" (attachment 639860 [details] [diff] [review] and attachment 639861 [details] [diff] [review]) so it's clear what they do.
(Assignee)

Updated

5 years ago
Attachment #639860 - Attachment description: imported patch modal.nsSecurityWarnings → Measure UI and Clickthrough rates for modal dialogs thrown by nsSecurityWarnings.cpp
(Assignee)

Updated

5 years ago
Attachment #639861 - Attachment description: imported patch addons-secui-tel → Measure UI and Clickthrough rates for Addon install flow
(Assignee)

Updated

5 years ago
Attachment #636036 - Flags: feedback?(sstamm) → review?(dtownsend+bugmail)
(Assignee)

Updated

5 years ago
Attachment #639858 - Flags: feedback?(sstamm) → review?(dtownsend+bugmail)
(Assignee)

Updated

5 years ago
Attachment #639859 - Flags: feedback?(sstamm) → review?(dtownsend+bugmail)
(Assignee)

Updated

5 years ago
Attachment #639860 - Flags: feedback?(sstamm) → review?(dtownsend+bugmail)
(Assignee)

Updated

5 years ago
Attachment #639861 - Flags: feedback?(sstamm) → review?(dtownsend+bugmail)
Attachment #636036 - Flags: review?(dtownsend+bugmail)
Attachment #639860 - Flags: review?(dtownsend+bugmail) → feedback?(sstamm)
Comment on attachment 639859 [details] [diff] [review]
Measure prevalence and clickthrough of malware/phishing warnings

global code style nits:

>+        if(isMalware){

if (isMalware) {

>+          secHistogram.add(Ci.nsISecurityUITelemetry.WARNING_MALWARE_PAGE_GET_ME_OUT_OF_HERE);
>+        }else{

} else {

>+          //We log even if malware info URL couldn't be found: the measurement is for how

// We [...]
Attachment #639859 - Flags: review?(dtownsend+bugmail) → feedback?(sstamm)
Comment on attachment 639858 [details] [diff] [review]
UI Telemetry for SSL Warnings

I had only looked at the one add-on UI patch and perhaps I wasn't clear that we should get mossop to review that one.  I still want to look at the rest (and will help identify reviewers for those).
Attachment #639858 - Flags: review?(dtownsend+bugmail) → feedback?(sstamm)
(Assignee)

Comment 24

5 years ago
@dao: thanks. I will fix those along with other nits I will have to fix after feedback.
(Assignee)

Comment 25

5 years ago
I just talked to Taras, and he told me that changing the number of buckets in a Histogram would require changing the name of the histogram. In that case, it seems sensible to me to have a large bucket count for enumerated values such as these.
Comment on attachment 639860 [details] [diff] [review]
Measure UI and Clickthrough rates for modal dialogs thrown by nsSecurityWarnings.cpp

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

General approach looks fine.  We should get mayhemer to look at this one.

::: security/manager/boot/src/nsSecurityWarningDialogs.cpp
@@ +129,4 @@
>    : mPrompt(aPrompt), mPrefName(aPrefName),
>      mDialogMessageName(aDialogMessageName),
>      mShowAgainName(aShowAgainName), mPrefBranch(aPrefBranch),
> +    mStringBundle(aStringBundle) {mTelemetryId = aTelemetryId;}

Use the same pattern as the rest of the default initializers here: mTelemetryId(aTelemetryId).

@@ +155,5 @@
>    // Stop if alert is not requested
>    if (!prefValue) return NS_OK;
>  
> +  MOZ_ASSERT(NS_IsMainThread());
> +  mozilla::Telemetry::Accumulate(mozilla::Telemetry::SECURITY_UI, mTelemetryId);

If you're looking to make this threadsafe, instead of MOZ_ASSERT, you should actually check if it's the main thread with an if statement, and only call Accumulate if it is.

@@ +218,2 @@
>    NS_ENSURE_TRUE(alert, NS_ERROR_OUT_OF_MEMORY);
> +  return aAsync ? NS_DispatchToCurrentThread(alert):alert->Run();

Please don't change the spacing here.

@@ +331,5 @@
>    if (NS_FAILED(rv)) return rv;
>  
>    *_result = (buttonPressed != 1);
> +  if(*_result){
> +  mozilla::Telemetry::Accumulate(mozilla::Telemetry::SECURITY_UI,aTelemetryId+1);

What is the +1 here?  And please fix if block and indent formatting.
Attachment #639860 - Flags: review?(honzab.moz)
Attachment #639860 - Flags: feedback?(sstamm)
Attachment #639860 - Flags: feedback+
Comment on attachment 639858 [details] [diff] [review]
UI Telemetry for SSL Warnings

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

Looks good.  Fix nits, then flag gavin for review on this patch.

::: security/manager/pki/resources/content/exceptionDialog.js
@@ +11,5 @@
>  var gChecking;
>  var gBroken;
>  var gNeedReset;
>  
> +let secHistogram = Components.classes["@mozilla.org/base/telemetry;1"].getService(Components.interfaces.nsITelemetry).getHistogramById("SECURITY_UI");

Declare and use this like the other globals above it.  Call it gSomething, and initialize it in initExceptionDialog.

Also, this line is really long... please wrap it.

@@ +215,5 @@
>        var uts = "addExceptionUnverifiedShort";
>        var utl = "addExceptionUnverifiedLong";
>        var use1 = false;
>        if (gSSLStatus.isDomainMismatch) {
> +        telemetryId += Components.interfaces.nsISecurityUITelemetry.WARNING_BAD_CERT_ADD_EXCEPTION_FLAG_DOMAIN;

Ah, this += is a bitwise OR, then?  Please actually use a bitwise OR to avoid mistakes where the same flag is included twice.

For shorter lines, assign Components.interfaces.nsISecurityUITelemetry to some local var (say nsISecTel) and use that instead of x.y.z.foo.
Attachment #639858 - Flags: feedback?(sstamm) → feedback+
Comment on attachment 639859 [details] [diff] [review]
Measure prevalence and clickthrough of malware/phishing warnings

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

Looks good too... please wrap long lines and flag gavin (or whomever he recommends) for review of this patch.
Attachment #639859 - Flags: feedback?(sstamm) → feedback+
Comment on attachment 636036 [details] [diff] [review]
Base patch that setups the IDL

Brian, can you either r/sr this new IDL (constants for telemetry) or tell us who can r/sr the patch?
Attachment #636036 - Flags: review?(bsmith)
(Assignee)

Comment 30

5 years ago
Created attachment 640397 [details] [diff] [review]
Measure UI and clickthrough rates for modal dialogs thrown by nsSecurityWarningDialogs.cpp
(Assignee)

Updated

5 years ago
Attachment #639860 - Attachment is obsolete: true
Attachment #639860 - Flags: review?(honzab.moz)
(Assignee)

Updated

5 years ago
Attachment #640397 - Flags: review?(honzab.moz)
(Assignee)

Comment 31

5 years ago
Created attachment 640405 [details] [diff] [review]
UI Telemetry for SSL Warnings
(Assignee)

Updated

5 years ago
Attachment #639858 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #640405 - Flags: review?(gavin.sharp)
Comment on attachment 640405 [details] [diff] [review]
UI Telemetry for SSL Warnings

forwarding to felipe!
Attachment #640405 - Flags: review?(gavin.sharp) → review?(felipc)
(Assignee)

Comment 33

5 years ago
Created attachment 640413 [details] [diff] [review]
Measure prevalence and clickthrough of malware/phishing warnings
(Assignee)

Updated

5 years ago
Attachment #639859 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #640413 - Flags: review?(gavin.sharp)
(Assignee)

Updated

5 years ago
Attachment #640413 - Flags: review?(gavin.sharp) → review?(felipc)
Comment on attachment 636036 [details] [diff] [review]
Base patch that setups the IDL

Place nsISecurityUITelemetry.idl in security/manager/boot/public.
Comment on attachment 640397 [details] [diff] [review]
Measure UI and clickthrough rates for modal dialogs thrown by nsSecurityWarningDialogs.cpp

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

::: security/manager/boot/src/nsSecurityWarningDialogs.cpp
@@ +332,5 @@
>  
>    *_result = (buttonPressed != 1);
> +  if (*_result) {
> +  //For confirmation dialogs, the clickthrough constant is 1 more than the constant
> +  //for the dialog.

Space after //.

You should reflect this also in the idl file and where ConfirmDialog is called.

@@ +333,5 @@
>    *_result = (buttonPressed != 1);
> +  if (*_result) {
> +  //For confirmation dialogs, the clickthrough constant is 1 more than the constant
> +  //for the dialog.
> +  mozilla::Telemetry::Accumulate(mozilla::Telemetry::SECURITY_UI,aTelemetryId+1);

space after , and spaces around +.
Attachment #640397 - Flags: review?(honzab.moz) → review+
(Assignee)

Comment 36

5 years ago
Created attachment 640776 [details] [diff] [review]
Measure UI and clickthrough rates for modal dialogs thrown by nsSecurityWarningDialogs.cpp
(Assignee)

Updated

5 years ago
Attachment #640397 - Attachment is obsolete: true
(Assignee)

Comment 37

5 years ago
Thanks Honza. I have fixed the patch; it seems I can't actually carry over the review+ though. Can you please set the flag for the new attachment?

Also, re moving the idl file: I am not certain whether that is also the correct place for it (maybe it is better to keep it in components/toolkit/telemetry). Since updating the location means updating all the patches, I am going to keep it in the current place for now and move it once I have further input on the correct place.
Comment on attachment 640776 [details] [diff] [review]
Measure UI and clickthrough rates for modal dialogs thrown by nsSecurityWarningDialogs.cpp

r=honzab
Attachment #640776 - Flags: review+
Comment on attachment 639861 [details] [diff] [review]
Measure UI and Clickthrough rates for Addon install flow

r+ for the add-ons bits. You should probably double-check the nsISecurityUITelemetry.idl changes with someone appropriate.
Attachment #639861 - Flags: review?(dtownsend+bugmail) → review+
(In reply to Devdatta Akhawe from comment #25)
> I just talked to Taras, and he told me that changing the number of buckets
> in a Histogram would require changing the name of the histogram. In that
> case, it seems sensible to me to have a large bucket count for enumerated
> values such as these.

It is also important to limit the number of buckets. I suggest cutting the number from 1000 to 100 or less.
Comment on attachment 640405 [details] [diff] [review]
UI Telemetry for SSL Warnings

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

How does the enumerated histogram work? Does it value gets its own bucket, which don't necessarily have any statistical correlation? That sounds ok, just trying to understand if that's what it means.

The OR'ing of values you're doing in exceptionDialog.js seems weird to me. You should just add a data point entry for each of the cases, otherwise the data for a single case will be under-represented for when it's merged (OR'ed) with another value. (sure in theory the data will still be there but it will make analysis harder)
Also you're already at the powers of 2 limit because adding another one will make it fall into the next group (e.g. 30 + 4 + 8 will be mixed with 40 + 2).
If there's any value in the merged data you can always report it in an extra bucket.


And the variable name 'telemetryId' isn't really great because all this data is going to the same histogram (with id="SECURITY_UI"), so if you still need to use it there or elsewhere I'd suggest "telemetryBucket" or "bucketId" for example.
(Assignee)

Comment 42

5 years ago
@bsmith: sure. Do you also have input on where to put the idl ?
(Assignee)

Comment 43

5 years ago
Created attachment 641167 [details] [diff] [review]
UI Telemetry for SSL Warnings
(Assignee)

Updated

5 years ago
Attachment #640405 - Attachment is obsolete: true
Attachment #640405 - Flags: review?(felipc)
(Assignee)

Updated

5 years ago
Attachment #641167 - Flags: review?(felipc)
(Assignee)

Comment 44

5 years ago
thanks Felipe. I have uploaded an updated patch.
Comment on attachment 636036 [details] [diff] [review]
Base patch that setups the IDL

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

I think all or most of the PSM UI stuff is in boot/, so put this interface in security/manager/boot/ instead of security/manager/ssl/.

Please file bugs for removing the prompts, especially the prompts that originate from within PSM.

::: toolkit/components/telemetry/TelemetryHistograms.h
@@ +452,5 @@
>  
> +/**
> + * Security UI Telemetry
> + */
> +HISTOGRAM_ENUMERATED_VALUES(SECURITY_UI, 1000, "Security UI Telemetry")

Reduce this to 100 or even less unless you think we already have close to 100 different warnings that we are going to measure here.
Attachment #636036 - Flags: review?(bsmith) → review+
Comment on attachment 640413 [details] [diff] [review]
Measure prevalence and clickthrough of malware/phishing warnings

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

feedback+=felipe

Just nits for a review+, but I wanna take a quick look at the final patch

::: browser/base/content/browser.js
@@ +2531,5 @@
>        // The event came from a button on a malware/phishing block page
>        // First check whether it's malware or phishing, so that we can
>        // use the right strings/links
>        var isMalware = /e=malwareBlocked/.test(ownerDoc.documentURI);
>  

You can reduce the telemetry footprint being added in this file by doing:

let bucketName = isMalware ? "WARNING_MALWARE_PAGE_" : "WARNING_PHISHING_PAGE_";

and then on each about:* case you just need to add one line:

secHistogram.add(Ci.nsISecurityUITelemetry[bucketName + "GET_ME_OUT_OF_HERE"]);

::: docshell/base/nsDocShell.cpp
@@ +4047,5 @@
>              Preferences::GetCString("urlclassifier.alternate_error_page");
>          if (alternateErrorPage)
>              errorPage.Assign(alternateErrorPage);
>  
> +        PRUint32 telemetryid;

Same comment as the previous patch, change this name to bucketId;
Attachment #640413 - Flags: review?(felipc) → feedback+
Comment on attachment 641167 [details] [diff] [review]
UI Telemetry for SSL Warnings

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

r=felipe with the change to updateCertStatus and removal of IgnoreCase (if that is not needed)

::: docshell/base/nsDocShell.cpp
@@ +4039,5 @@
>                          "security.alternate_certificate_error_page");
>                  if (alternateErrorPage)
>                      errorPage.Assign(alternateErrorPage);
> +
> +                if (errorPage.EqualsIgnoreCase("certerror")) 

do you need IgnoreCase? I believe Equals should be enough

::: security/manager/pki/resources/content/exceptionDialog.js
@@ +15,1 @@
>  

Suggestion: you could define nsISecTel as a const here only once and use it elsewhere, including in the viewCertButtonClick.

@@ +218,5 @@
>        var uts = "addExceptionUnverifiedShort";
>        var utl = "addExceptionUnverifiedLong";
>        var use1 = false;
>        if (gSSLStatus.isDomainMismatch) {
> +        bucketId |= nsISecTel.WARNING_BAD_CERT_ADD_EXCEPTION_FLAG_DOMAIN;

forgot to change the bitwise OR to addition in this section
Attachment #641167 - Flags: review?(felipc) → review+
(Assignee)

Comment 48

5 years ago
Created attachment 641739 [details] [diff] [review]
UI Telemetry for SSL Warnings
(Assignee)

Updated

5 years ago
Attachment #641167 - Attachment is obsolete: true
(Assignee)

Comment 49

5 years ago
Created attachment 641740 [details] [diff] [review]
Measure prevalence and clickthrough of malware/phishing warnings
(Assignee)

Updated

5 years ago
Attachment #640413 - Attachment is obsolete: true
(Assignee)

Comment 50

5 years ago
Created attachment 641741 [details] [diff] [review]
Measure UI and clickthrough rates for modal dialogs thrown by nsSecurityWarningDialogs.cpp
(Assignee)

Updated

5 years ago
Attachment #640776 - Attachment is obsolete: true
(Assignee)

Comment 51

5 years ago
Created attachment 641742 [details] [diff] [review]
Measure UI and Clickthrough rates for Addon install flow
(Assignee)

Updated

5 years ago
Attachment #639861 - Attachment is obsolete: true
(Assignee)

Comment 52

5 years ago
Created attachment 641744 [details] [diff] [review]
Base patch that setups the IDL
(Assignee)

Updated

5 years ago
Attachment #636036 - Attachment is obsolete: true
(Assignee)

Comment 53

5 years ago
Thanks, bsmith! I have moved the location of the IDL, and modified the histogram. We are already nearing 50; I propose keeping it at 100. 

Moving the location changed all the patches: I don't have the privileges to carry over the review flags. Can someone with the requisite privileges carry the review +s ?
(Assignee)

Comment 54

5 years ago
Thanks felipe. I have modified all the patches. As discussed on IRC, I am keeping EqualsIgnoreCase since aboutRedirector.cpp lower cases everything before redirecting. Thus, about:CertError could also work; so I am being conservative.
Attachment #641740 - Flags: review+
Attachment #641739 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #641744 - Flags: review?(honzab.moz)
(Assignee)

Updated

5 years ago
Attachment #641741 - Flags: review?(bsmith)
Attachment #641744 - Flags: review?(honzab.moz) → review+
(Assignee)

Comment 55

5 years ago
Created attachment 642277 [details] [diff] [review]
Measure UI and Clickthrough rates for Addon install flow
(Assignee)

Updated

5 years ago
Attachment #641742 - Attachment is obsolete: true
(Assignee)

Comment 56

5 years ago
Created attachment 642279 [details] [diff] [review]
Measure UI and clickthrough rates for modal dialogs thrown by nsSecurityWarningDialogs.cpp
(Assignee)

Updated

5 years ago
Attachment #641741 - Attachment is obsolete: true
Attachment #641741 - Flags: review?(bsmith)
(Assignee)

Updated

5 years ago
Attachment #642279 - Flags: review?(bsmith)

Comment 57

5 years ago
Try run for 4cca78c3a102 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=4cca78c3a102
Results (out of 219 total builds):
    success: 198
    warnings: 12
    failure: 9
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dakhawe@mozilla.com-4cca78c3a102

Comment 58

5 years ago
Try run for 4cca78c3a102 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=4cca78c3a102
Results (out of 225 total builds):
    success: 203
    warnings: 13
    failure: 9
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dakhawe@mozilla.com-4cca78c3a102
(Assignee)

Updated

5 years ago
Attachment #641744 - Flags: review?(bsmith)
Comment on attachment 642279 [details] [diff] [review]
Measure UI and clickthrough rates for modal dialogs thrown by nsSecurityWarningDialogs.cpp

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

::: security/manager/boot/public/nsISecurityUITelemetry.idl
@@ +30,5 @@
> +
> +const PRUint32 WARNING_ENTERING_SECURE_SITE = 5;
> +const PRUint32 WARNING_ENTERING_WEAK_SITE = 6;
> +const PRUint32 WARNING_LEAVING_SECURE_SITE = 7;
> +const PRUint32 WARNING_MIXED_CONTENT  = 8;

Insert newline here.

@@ +32,5 @@
> +const PRUint32 WARNING_ENTERING_WEAK_SITE = 6;
> +const PRUint32 WARNING_LEAVING_SECURE_SITE = 7;
> +const PRUint32 WARNING_MIXED_CONTENT  = 8;
> +// For confirmation dialogs, the clickthrough constant needs to be 1
> +// more than the dialog constant

... so that WARNING_CONFIRM_<X> + 1 == WARNING_CONFIRM_<X>_CLICK_THROUGH.

::: security/manager/boot/src/nsSecurityWarningDialogs.cpp
@@ +139,5 @@
>    nsString                  mDialogMessageName;
>    nsString                  mShowAgainName;
>    nsCOMPtr<nsIPrefBranch>   mPrefBranch;
>    nsCOMPtr<nsIStringBundle> mStringBundle;
> +  PRUint32                  mTelemetryId;

Should be const.

Do not name these variables "telemetryId" because mozilla::Telemetry::ID is something different (the ID of the probe). "telemetryValue" would be better as it's the value (not the ID) used for the telemetry.

@@ +155,5 @@
>  
>    // Stop if alert is not requested
>    if (!prefValue) return NS_OK;
>  
> +  mozilla::Telemetry::Accumulate(mozilla::Telemetry::SECURITY_UI, mTelemetryId);

"

@@ +200,5 @@
>                                        const char* aPrefName,
>                                        const PRUnichar* aDialogMessageName,
>                                        const PRUnichar* aShowAgainName,
> +                                      bool aAsync,
> +                                      PRUint32 aTelemetryId)

"

@@ +212,5 @@
>                                                    aDialogMessageName,
>                                                    aShowAgainName,
>                                                    mPrefBranch,
> +                                                  mStringBundle,
> +                                                  aTelemetryId);

"

@@ +225,5 @@
>  nsSecurityWarningDialogs::ConfirmPostToInsecure(nsIInterfaceRequestor *ctx, bool* _result)
>  {
>    nsresult rv;
>  
> +  // The Telemetry clickthrough constant is 1 more than the constant for the dialog.

Do not need this comment.

@@ +254,5 @@
>  nsresult
>  nsSecurityWarningDialogs::ConfirmDialog(nsIInterfaceRequestor *ctx, const char *prefName,
>                              const PRUnichar *messageName, 
>                              const PRUnichar *showAgainName, 
> +                            PRUint32 aTelemetryId,

"
Attachment #642279 - Flags: review?(bsmith) → review+
(Assignee)

Comment 60

5 years ago
Created attachment 647404 [details] [diff] [review]
Measure UI and clickthrough rates for modal dialogs thrown by nsSecurityWarningDialogs.cpp
(Assignee)

Updated

5 years ago
Attachment #642279 - Attachment is obsolete: true
(Assignee)

Comment 61

5 years ago
Comment on attachment 642277 [details] [diff] [review]
Measure UI and Clickthrough rates for Addon install flow

carrying over r+ from dtownsend
Attachment #642277 - Flags: review+
(Assignee)

Comment 62

5 years ago
Comment on attachment 647404 [details] [diff] [review]
Measure UI and clickthrough rates for modal dialogs thrown by nsSecurityWarningDialogs.cpp

carrying over r+ from honzab and bsmith
Attachment #647404 - Flags: review+
Attachment #641744 - Flags: review?(bsmith) → review+
(Assignee)

Comment 63

5 years ago
Created attachment 648590 [details] [diff] [review]
Folded Patch

Merged into a single patch.
Comment on attachment 648590 [details] [diff] [review]
Folded Patch

Bringing forward r+
Attachment #648590 - Flags: review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/c2090a46d34e
Target Milestone: --- → Firefox 17
https://hg.mozilla.org/mozilla-central/rev/c2090a46d34e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Comment on attachment 648590 [details] [diff] [review]
Folded Patch

Just to be clear, the landing was for this rollup patch of all the above patches.
Attachment #648590 - Flags: checkin+
(Assignee)

Comment 68

5 years ago
I would like to propose a probe for the geolocation  prompt. I didn't implement this originally because I wasn't aware of the WiFi+IP based location capability. See Bug 787738
You need to log in before you can comment on or make changes to this bug.