1325872 - Telemetry experiment for TLS 1.3 [short headers; FF 51]

Status: RESOLVED INVALID

(Toolkit :: Telemetry, defect)

Description

[Eric Rescorla (:ekr)]

Per discussions in bug 1305970, we are going to uplift NSS 3.28 to FF 51 and turn on TLS 1.3 via a telemetry experiment. This experiment will also measure success rates to some chosen sites with known configurations and disable 1.3 immediately if there are indications of failure.

Comment 1

[Eric Rescorla (:ekr)]

Attachment: tls13 ff51 experiment

Comment 2

[Eric Rescorla (:ekr)]

Attachment: Actually write the experiment

Comment 3

[Eric Rescorla (:ekr)]

Attachment: Make a new experiment that's a copy of a previous experiment

Comment 4

[Eric Rescorla (:ekr)]

Attachment: Write the experiment

Comment 5

[Eric Rescorla (:ekr)]

Comment on attachment 8821919 [details] [diff] [review]
Write the experiment

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

BDS, this is a slight modification of the experiment from bug 1310338.

The control parameters (version, etc.) in the install.rdf/manifest.json will need to be updated for deployment but it should otherwise be ready for review

Comment 6

[Benjamin Smedberg]

Comment on attachment 8821919 [details] [diff] [review]
Write the experiment

>diff --git a/build.py b/build.py

>         os.mkdir(os.path.join(dest_path, e.manifest["id"]))
>-
>+        

Nit this probably was included by accident, but please revert trailing whitespace.

>diff --git a/experiments/tls13-compat-ff51/README.md b/experiments/tls13-compat-ff51/README.md

Since we already deployed the former experiment, is this an `hg copy` of the old experiment to a new directory? Remaining comments assume that this is a new directory and the old one isn't changed.

>--- a/experiments/tls13-compat-ff51/README.md
>+++ b/experiments/tls13-compat-ff51/README.md
>@@ -1,11 +1,13 @@
> This experiment compares performance/behavior of various TLS servers
> (which can be configured differently) by doing a GET to each URL and
>-then reporting the results.
>+then reporting the results. It starts by enabling TLS 1.3 and if
>+all the tests succed, it leaves it on as the second half of the
>+experiment.

Does the experiment explicitly record whether it decided to leave TLS1.3 on or off for a particular user? e.g. by setting the experiment branch to a particular value? Or is that recorded somewhere else in the normal telemetry payload/environment?

>diff --git a/experiments/tls13-compat-ff51/code/bootstrap.js b/experiments/tls13-compat-ff51/code/bootstrap.js

>+// - If the TLS 1.3 connection succeeded, leave TLS 1.3 on.

The mechanism you're using for this is setting a user pref, which outlasts this experiment. That seems wrong to me, although perhaps that's intended?

The normal way this should work in an experiment is that you make changes only for the duration of the experiment, usually by setting a default pref on each startup. That way when the experiment times out (or is user-disabled), everything reverts to the correct default.

>diff --git a/experiments/tls13-compat-ff51/code/install.rdf b/experiments/tls13-compat-ff51/code/install.rdf

>+        <em:minVersion>51*</em:minVersion>

51* is not a good maxversion, this should be 51 (51.* is equivalent to 51.9999999999)

We do want to extend this experiment into beta 52?

>diff --git a/experiments/tls13-compat-ff51/manifest.json b/experiments/tls13-compat-ff51/manifest.json
>--- a/experiments/tls13-compat-ff51/manifest.json
>+++ b/experiments/tls13-compat-ff51/manifest.json
>@@ -1,18 +1,18 @@
> {
>   "publish"     : true,
>   "priority"    : 2,
>   "name"        : "TLS 1.3 Compatibility Testing 1",
>   "description" : "Measure the compatibility of TLS 1.3",
>   "info"        : "<p><a href=\"https://bugzilla.mozilla.org/show_bug.cgi?id=1310338\">Related bug</a></p>",
>   "manifest"    : {
>-    "id"               : "[e-mail]",
>+    "id"               : "[e-mail]",
>     "startTime"        : 1477637000,
>-    "endTime"          : 1481760000,
>+    "endTime"          : 2481760000,

2048-08-23 ? This should be a reasonable date, e.g. 1-Mar or something.

>     "maxActiveSeconds" : 86400,

This may not be correct based on the question above about making persistent changes to user config.

>     "appName"          : ["Firefox"],
>-    "channel"          : ["nightly"],
>-    "minVersion"       : "52.0a1*",
>-    "maxVersion"       : "52.0a1",
>+    "channel"          : ["default"],

needs to be "beta"

>+    "minVersion"       : "51*",

ditto minVersion above

Comment 7

[Eric Rescorla (:ekr)]

- Yes, this is a copy.
- This is behaving correctly. There are two experiments being run here in one add-on:
  - Turn on TLS 1.3 for everyone where it seems to succeed. This is in preparation for just flipping the switch for everyone (per discussion with Relman in the bug listed in c1)
  - Take an explicit measurement of TLS performance for a number of configurations
- No, we don't record our decision on the machine but you can back it out from the telemetry report here and then correlate against other telemetry reports.

As noted in c5, the control parameters aren't right. I just picked random ones which let it run on my machine and was planning to correct them prior to deployment (it's pretty inconvenient to have to mess with these while you're developing the experiment), so I just made endTime infinitely far in the future. In terms of moving forward, do the explanations above resolve your concerns? If so, I an prepare a new patch with correct control parameters.

Comment 8

[Benjamin Smedberg]

We discussed directly, and we're going to have revisions. We should make sure we set a minimum buildid which matches the beta build which has all the TLS1.3 fixes we need.

Comment 9

[Eric Rescorla (:ekr)]

Attachment: revised.patch

Comment 10

[Eric Rescorla (:ekr)]

Comment on attachment 8825019 [details] [diff] [review]
revised.patch

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

bsmedberg,

Here is a revised patch along the lines you indicated. I have two things I know I need to fix:

1. Revise the min version to beta12.
2. Revise the channel to beta (I left it as "default") for testing.

Can you point me in the right direction for #1 please? I'm not sure quite what goes in the minVersion field.

Comment 11

[Eric Rescorla (:ekr)]

Oh, one more thing: I will be adding a third server as soon as Cloudflare gives me the domain name

Comment 12

[Benjamin Smedberg]

You don't change the minversion, you set a minimum buildid. See e.g. https://hg.mozilla.org/webtools/telemetry-experiment-server/file/791a488e9527/experiments/flash-protectedmode-beta/manifest.json#l16

Comment 13

[Eric Rescorla (:ekr)]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #12)
> You don't change the minversion, you set a minimum buildid. See e.g.
> https://hg.mozilla.org/webtools/telemetry-experiment-server/file/
> 791a488e9527/experiments/flash-protectedmode-beta/manifest.json#l16

Thanks. Willdo.

Comment 14

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8825019 [details] [diff] [review]
revised.patch

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

::: experiments/tls13-compat-ff51/code/bootstrap.js
@@ +155,5 @@
> +      })
> +      .then(_ => {
> +        let id = experiments.getActiveExperimentID();
> +        console.log("TLS 1.3 experiment branch result was: " + branch);        
> +        experiments.setExperimentBranch(id, branch);

return the value of the last line here (a promise) so that the chain will only continue once the branch setting has finished.

@@ +174,5 @@
> +  // setting.
> +  let userprefs = new Preferences();
> +  if (userprefs.isSet(kVERSION_MAX_PREF)) {
> +    console.log("User has changed TLS max version. Skipping");
> +    disable();

prior to disabling the experiment, it would be slightly better to assign a new branch name so that this user can be filtered when analysing the data. Bsmedberg said that it's probably statistically insignficant, so I'll leave that up to you.

Comment 15

[Eric Rescorla (:ekr)]

RyanVM,

Can you please arrange for test for this. This should have been deployed to staging at this point.

Aside from the conventional tests, this needs a test that:

1. It generates a telemetry ping with the results.
2. It leaves the browser with TLS 1.3 enabled, which can be verified by waiting for the experiment to run and then going to https://www.allizom.org/ and verifying that it negotiates TLS 1.3.

Comment 16

[Eric Rescorla (:ekr)]

jwilliams: do you have an ETA on this?

Comment 17

[Justin [:JW_SoftvisionQA]]

We just got into the office here. I will look over all of the details and give you an ETA on this.

Comment 18

[Justin [:JW_SoftvisionQA]]

Eric I have wrote you some questions on IRC. If I do not hear back from you by EOD, I will send you a follow up email.

Comment 19

[Justin [:JW_SoftvisionQA]]

I have verified that all the information from Comment 15 is working as intended. Marking this as Resolved Verified.

Comment 20

[Eric Rescorla (:ekr)]

Justin, can you please test this XPI against a FF51 Release Candidate. We are planning to release it as a system add-on.

Comment 21

[Justin [:JW_SoftvisionQA]]

I can confirm that all the information from Comment 15 is working as intended on FF51 Release Candidate Build 20170112171116. Marking as Resolved Verified

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

Did this actually ship? I don't think we should mark it verified/fixed until it ships....

Comment 23

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #22)
> Did this actually ship? I don't think we should mark it verified/fixed until
> it ships....

Okay.

RelMan has approved this via release-drivers@. I think we just need the signed XPI referenced in comment 15.

Comment 24

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

Ekr messaged me the location of the signed XPI for shipping as a system add-on. Justin, did you verify against this version?

https://hg.mozilla.org/webtools/telemetry-experiment-server/file/tip/experiments/tls13-compat-ff51

Comment 25

[Justin [:JW_SoftvisionQA]]

yes. that is what I verified.

Comment 26

[Eric Rescorla (:ekr)]

Here is an updated version of this patch that I want to deploy now that we have the results from the first one. It differs in two respects:

- It tries 3 URLs and not 2.
- It has an explicit control group.

Comment 27

[Eric Rescorla (:ekr)]

Attachment: exp.patch

Comment 28

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8836419 [details] [diff] [review]
exp.patch

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

::: experiments/tls13-compat-ff51/manifest.json
@@ +11,3 @@
>      "maxActiveSeconds" : 1209600,
>      "appName"          : ["Firefox"],
> +      "channel"          : ["release", "default"],

"default" is useful for testing locally, but please remove it from the final commit

Comment 29

[:Felipe Gomes (needinfo for replies!)]

Oh, I just noticed. Don't reuse the same experiment folder (experiments/tls13-compat-ff51) because the generated page contains historical info about all past experiments which is very useful to look at a glance (https://telemetry-experiment.cdn.mozilla.net/).

Just make a copy of the folder with a new name.

Comment 30

[Eric Rescorla (:ekr)]

Sorry to give you a new patch, but we had a change of direction to just do a a measurement but not set things permanently.

Comment 31

[Eric Rescorla (:ekr)]

Attachment: experiment.patch

Sorry to give you a new patch to review but we had a change of direction to:

1. Not set the settings permanently
2. Run the experiment with different client parameters (hence the conversion to Task).

Comment 32

[Eric Rescorla (:ekr)]

P.S. The allizom.org URL is a placeholder. I'm waiting for a new URL from cloudflare.

Comment 33

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8839607 [details] [diff] [review]
experiment.patch

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

Just nitpicks below

::: experiments/tls13-compat-ff51-v2/code/bootstrap.js
@@ +18,5 @@
> +  "https://short.tls13.com/" : true,
> +  "https://www.allizom.org/" : false
> +};
> +
> +// Ugh, keys() doesn't work.

Hmm did you try as `Object.keys(kURLs)`? Works for me.

@@ +77,5 @@
>        let nsireq = channel.QueryInterface(Ci.nsIRequest);
>        result.error= nsireq ? nsireq.status : NS_ERROR_NOT_AVAILABLE;
>        recordSecInfo(channel, result);
>        result.elapsed = Date.now() - t0;
> +      prefs.set(kVERSION_MAX_PREF, 3);

as this (and below) is about resetting to the original value, please use prefs.reset(kVERSION_MAX_PREF) instead of manually setting it to 3.

@@ +102,1 @@
>    console.log(result);

please remove or comment out these logging before landing

@@ +157,1 @@
>          prefs.set(kVERSION_MAX_PREF, 3);

same here about prefs.reset (sorry about not catching this the previous time).

::: experiments/tls13-compat-ff51-v2/code/install.rdf
@@ +16,5 @@
>        </Description>
>      </em:targetApplication>
>  
>      <!-- Front End MetaData -->
> +    <em:name>TLS 1.3 Compatibility Testing 3</em:name>

you're using -v2 in the id but "Testing 3" in the name here and in manifest.json. Which version is correct?

::: experiments/tls13-compat-ff51-v2/manifest.json
@@ +12,2 @@
>      "appName"          : ["Firefox"],
> +      "channel"          : ["release", "default"],

reminder to remove "default" from this list.

Comment 34

[Eric Rescorla (:ekr)]

Justin, can you please test this. Signed XPI in: https://bugzilla.mozilla.org/show_bug.cgi?id=1341445

The only tests that are needed are:

1. That it doesn't break ordinary operation.
2. That it actually sends a telemetry ping (using about:telemetry) after the experiment has loaded and run.
3. That if you restart Firefox, it does TLS 1.2 when connecting to https://www.allizom.org/

Comment 35

[Justin [:JW_SoftvisionQA]]

Attachment: alizom.JPG

Eric, What is the telemetry ping that will be sent? I went to about:telemetry and searched TLS but nothing shows. 

Also this is what I am seeing on https://www.allizom.org/ This looks a bit different than my previous testing. Is this expected? Just to note, I have visited this page 5 times and the Privacy & History is not updating as seen in the screenshot.

Comment 36

[Eric Rescorla (:ekr)]

You need the archived ping. It should look like this (in general data):

Name 	Value
type 	tls-13-study-v3
id 	0a0747fb-a653-0a45-bffc-d6f3801da6f1
creationDate 	2017-02-22T17:34:28.297Z
version 	4
application.architecture 	x86-64
application.buildId 	20170211141402
application.name 	Firefox
application.version 	51.0.2
application.displayVersion 	51.0.2
application.vendor 	Mozilla
application.platformVersion 	51.0.2
application.xpcomAbi 	x86_64-gcc3
application.channel 	default 

No, that's now what I would be expecting in the privacy and history. Can you retry with a totally fresh profile?

Comment 37

[Justin [:JW_SoftvisionQA]]

With a totally new profile I am not seeing any of this data on release with the signed xpi found here:

https://bugzilla.mozilla.org/attachment.cgi?id=8839936

Comment 38

[Eric Rescorla (:ekr)]

Does the browser console show it running the experiment?

Comment 39

[Justin [:JW_SoftvisionQA]]

Here are the contents of the browser console:

1487786766784	addons.update-checker	WARN	Update manifest for [e-mail] did not contain an updates property
1487786772873	Browser.Experiments.Experiments	WARN	Experiments #0::onInstallStarted cancelling install of unknown experiment add-on: [e-mail]

Comment 40

[Eric Rescorla (:ekr)]

OK, so that's the problem, presumably.

felipe, any idea what the problem is here?

Comment 41

[Eric Rescorla (:ekr)]

OK, actually we have a bigger problem. The pref set isn't taking effect, so it looks like we're always offering TLS 1.3 even though I wanted to offer 1.2 for www.allizom.org. felipe, I don't have a clear picture of how the pref system works in terms of timing. Should frobbing defaultPrefs have an immediate effect? Do I need to do something else to make that happen? PSM does have an observer that's supposed to detect changes.

Comment 42

[Benjamin Smedberg]

I'm 99.44% sure setting a default pref will immediately change the results from .getBoolPref calls (assuming there is no user pref which overrides it).

According to the limited docs/IDL comments, that should also trigger pref change notifications, but I am much less sure that works and I doubt we have automated tests for it. Let me construct a testcase and see.

Regarding comment 39, it appears that you're trying to directly install the experiment XPI. If it's an experiment, it will only install via the experiment system (a published manifest). It needs an em:type="2" to install as a regular addon.

Comment 43

[Eric Rescorla (:ekr)]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #42)
> I'm 99.44% sure setting a default pref will immediately change the results
> from .getBoolPref calls (assuming there is no user pref which overrides it).
> 
> According to the limited docs/IDL comments, that should also trigger pref
> change notifications, but I am much less sure that works and I doubt we have
> automated tests for it. Let me construct a testcase and see.

Thanks! It could also be some other kind of bug, but I want to make sure I haven't
totally misunderstood how things work.


> Regarding comment 39, it appears that you're trying to directly install the
> experiment XPI. If it's an experiment, it will only install via the
> experiment system (a published manifest). It needs an em:type="2" to install
> as a regular addon.

jthomas, you should be installing this as via the experiment system (we will
be making it a system add-on for final deployment)

Comment 44

[Benjamin Smedberg]

A very quick xpcshell test indicates that the pref observer should work in this case:

[bsmedberg@mozart bin]$ ./run-mozilla.sh ./xpcshell 
js> const Cc = Components.classes;
js> const Ci = Components.interfaces;
js> var prefs = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch);
js> prefs.QueryInterface(Ci.nsIPrefService);
[xpconnect wrapped (nsISupports, nsIPrefBranch, nsIPrefService) @ 0x7fec08511f20 (native @ 0x7fec16779880)]
js> prefs
[xpconnect wrapped (nsISupports, nsIPrefBranch, nsIPrefService) @ 0x7fec08511f20 (native @ 0x7fec16779880)]
js> prefs.getIntPref("security.tls.version.max")
[27957] WARNING: No active window: file /home/bsmedberg/mozilla-hg/js/xpconnect/src/XPCJSContext.cpp, line 1329
4
js> function observer(subject, topic, data) { dump("Pref Observer: " + subject + " topic: " + topic + " data: " + data); }                       
[27957] WARNING: No active window: file /home/bsmedberg/mozilla-hg/js/xpconnect/src/XPCJSContext.cpp, line 1329
js> prefs.addObserver("security.tls.version.max", observer, false);
[27957] WARNING: No active window: file /home/bsmedberg/mozilla-hg/js/xpconnect/src/XPCJSContext.cpp, line 1329
js> var defaults = prefs.getDefaultBranch("");
[27957] WARNING: No active window: file /home/bsmedberg/mozilla-hg/js/xpconnect/src/XPCJSContext.cpp, line 1329
js> defaults
[27957] WARNING: No active window: file /home/bsmedberg/mozilla-hg/js/xpconnect/src/XPCJSContext.cpp, line 1329
[xpconnect wrapped nsIPrefBranch @ 0x7fec08512040 (native @ 0x7fec0e61b980)]
js> defaults.setIntPref("security.tls.version.max", 5);
Pref Observer: [xpconnect wrapped nsISupports @ 0x7fec085121c0 (native @ 0x7fec0e61b900)] topic: nsPref:changed data: security.tls.version.max[27957] WARNING: No active window: file /home/bsmedberg/mozilla-hg/js/xpconnect/src/XPCJSContext.cpp, line 1329
js> prefs.getIntPref("security.tls.version.max")
[27957] WARNING: No active window: file /home/bsmedberg/mozilla-hg/js/xpconnect/src/XPCJSContext.cpp, line 1329
5

Comment 45

[Eric Rescorla (:ekr)]

OK, so I must have some other kind of bug. I'm about to jump into meetings but will try to diagnose this afternoon. jthomas, can you get to the point where you can get the thing to install and run (because I don't think that's on my end) and then I will try to have a fixed xpi this afternoon

Comment 46

[Eric Rescorla (:ekr)]

OK, I've diagnosed the reason it's not re-setting, which is that prefs.reset() doesn't work properly.

I adjusted the code to just have the following snippet:

    let prefs = new Preferences({ defaultBranch: true });
    prefs.set(kVERSION_MAX_PREF, 4);
    debug("SET PREF");
    debug(prefs.get(kVERSION_MAX_PREF));
    debug("RESET PREF");
    prefs.reset(kVERSION_MAX_PREF);
    debug(prefs.get(kVERSION_MAX_PREF));

This produces the output:

SET PREF  bootstrap.js:29
4  bootstrap.js:29
RESET PREF  bootstrap.js:29
4  bootstrap.js:29

felipc: any idea what's wrong? If we can't resolve this quickly, I'd like to just use .set?

Comment 47

[:Felipe Gomes (needinfo for replies!)]

Ok, I just tested this and turns out that prefs.reset doesn't work with the defaultBranch. Sorry about that change.. You can just use set again

Comment 48

[Eric Rescorla (:ekr)]

No worries. Will post a new patch in a bit. Thanks.

Comment 49

[Eric Rescorla (:ekr)]

Attachment: revised.patch

Felipe, here is a revised patch with better debugging, the right URL, and the non-reset changes you asked for patched.

Comment 50

[Eric Rescorla (:ekr)]

Justin, please see new XPI for test.

https://bugzilla.mozilla.org/show_bug.cgi?id=1341445

Comment 51

[Justin [:JW_SoftvisionQA]]

I am still seeing the same issue as before. This testing is being done on a new profile on release.

Browser Console:

1487952413956	addons.update-checker	WARN	Update manifest for [e-mail] did not contain an updates property
1487952419855	Browser.Experiments.Experiments	WARN	Experiments #0::onInstallStarted cancelling install of unknown experiment add-on: [e-mail]

Also there are no telemetry pings.

Comment 52

[Eric Rescorla (:ekr)]

I don't think this is me. BDS, can you help?

Comment 53

[Benjamin Smedberg]

Justin, what are your steps to reproduce? I think my comment 42 still applies as it appears you're trying to install the XPI directly.

Comment 54

[Justin [:JW_SoftvisionQA]]

Comment 51 is me installing it directly with the xpi.

Below is me attempting to run it as an experiment, assuming this is an active experiment (This is the only way I currently know how to run an experiment. If there is another way please let me know).

Browser Console input:

Services.prefs.setBoolPref("xpinstall.signatures.required", false);
Services.prefs.setBoolPref("toolkit.telemetry.enabled", true);
Services.prefs.setBoolPref("experiments.logging.enabled", true);
Services.prefs.setBoolPref("extensions.logging.enabled", true);
Services.prefs.setIntPref("experiments.logging.level", 0);
Services.prefs.setCharPref("experiments.force-sample-value", "0.1");
Services.prefs.setCharPref("experiments.manifest.uri", "https://telemetry-experiment-dev.allizom.org/firefox-manifest.json");

Browser Console Output:
undefined
1487967122267	Browser.Experiments.Experiments	TRACE	Experiments #0::updateManifest()
A promise chain failed to handle a rejection. Did you forget to '.catch', or did you forget to 'return'?
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise

Date: Fri Feb 24 2017 12:12:02 GMT-0800 (Pacific Standard Time)
Full Message: Error: experiments are disabled
Full Stack: Experiments.Experiments.prototype.updateManifest@resource://app/modules/experiments/Experiments.jsm:846:29
PrefObserver.prototype.observe@resource://gre/modules/Preferences.jsm:413:9
@debugger eval code:7:1
WCA_evalWithDebugger@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/webconsole.js:1325:16
WCA_onEvaluateJS@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/webconsole.js:889:20
WCA_onEvaluateJSAsync@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/webconsole.js:859:20
onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:1748:15
LocalDebuggerTransport.prototype.send/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/transport/transport.js:570:13
exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14
exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14

Comment 55

[Benjamin Smedberg]

If experiments.enabled is true and toolkit.telemetry.enabled is true, then you should be able to run it. gfritzsche or maybe Kamil would be the people to assist if that doesn't work: Kamil wrote the QA guide for experiments originally.

Comment 56

[Georg Fritzsche [:gfritzsche]]

(In reply to Justin [:JW_SoftvisionQA] from comment #54)
> Below is me attempting to run it as an experiment, assuming this is an
> active experiment (This is the only way I currently know how to run an
> experiment. If there is another way please let me know).
> 
> Browser Console input:
> 
> Services.prefs.setBoolPref("xpinstall.signatures.required", false);
> Services.prefs.setBoolPref("toolkit.telemetry.enabled", true);
> Services.prefs.setBoolPref("experiments.logging.enabled", true);
> Services.prefs.setBoolPref("extensions.logging.enabled", true);
> Services.prefs.setIntPref("experiments.logging.level", 0);
> Services.prefs.setCharPref("experiments.force-sample-value", "0.1");
> Services.prefs.setCharPref("experiments.manifest.uri",
> "https://telemetry-experiment-dev.allizom.org/firefox-manifest.json");
...
> Date: Fri Feb 24 2017 12:12:02 GMT-0800 (Pacific Standard Time)
> Full Message: Error: experiments are disabled

This probably means that Telemetry is not properly enabled [1].
Are you using an unbranded release build to be able to set "xpinstall.signatures.required"?
I don't know how these work exactly, but i assume that Telemetry is not enabled for those.

You can instead:
- test on official Nightly or Aurora builds
- get the experiment xpi signed
- (not recommended: do some overrides with the debugger)

1: https://dxr.mozilla.org/mozilla-central/rev/7ef1e9abd296a8edc39b7efc8d637767ba2f77ed/browser/experiments/Experiments.jsm#380

Comment 57

[Eric Rescorla (:ekr)]

The XPI is signed.

Comment 58

[:Felipe Gomes (needinfo for replies!)]

Hi Justin, the problem is that the xpi has not been pushed yet to the repo/staging location that's being used.

I just pushed it to a custom location for testing.. In your settings for "experiments.manifest.uri", please use the following:

"https://people-mozilla.org/~fgomes/tls13v3/firefox-manifest.json"

Comment 59

[Justin [:JW_SoftvisionQA]]

(In reply to :Felipe Gomes (needinfo me!) from comment #58)
> Hi Justin, the problem is that the xpi has not been pushed yet to the
> repo/staging location that's being used.
> 
> I just pushed it to a custom location for testing.. In your settings for
> "experiments.manifest.uri", please use the following:
> 
> "https://people-mozilla.org/~fgomes/tls13v3/firefox-manifest.json"

I receive this error when doing so:

A promise chain failed to handle a rejection. Did you forget to '.catch', or did you forget to 'return'?
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise

Date: Mon Feb 27 2017 13:01:49 GMT-0800 (Pacific Standard Time)
Full Message: Error: experiments are disabled
Full Stack: Experiments.Experiments.prototype.updateManifest@resource://app/modules/experiments/Experiments.jsm:846:29
PrefObserver.prototype.observe@resource://gre/modules/Preferences.jsm:413:9
ModifyPref@chrome://global/content/config.js:629:7
ModifySelected@chrome://global/content/config.js:552:5
ondblclick@about:config:1:24

Comment 60

[Eric Rescorla (:ekr)]

This still seems like it's not me.

Comment 61

[:Felipe Gomes (needinfo for replies!)]

Justin: are both of these prefs true for you?

toolkit.telemetry.enabled
experiments.enabled

Comment 62

[Justin [:JW_SoftvisionQA]]

Yes, both are set to true on my end.

Comment 63

[Eric Rescorla (:ekr)]

Did you restart the browser after setting these prefs?

Comment 64

[Eric Rescorla (:ekr)]

Note: this is supposed to be a system add-on. Do we need to do something different for that?

Comment 65

[Justin [:JW_SoftvisionQA]]

Yes here are my steps.

1. Clean profile
2. set
toolkit.telemetry.enabled = true
experiments.enabled = true
3. restart
4. set
experiments.manifest.uri = https://people-mozilla.org/~fgomes/tls13v3/firefox-manifest.json

4 output. is in comment 59

5. restart

There is no sign of the experiment/addon

Comment 66

[Ryan VanderMeulen [:RyanVM]]

FWIW, I haven't been able to install the signed XPI either. When I try on Firefox 51 with a clean profile and the about:config options as noted in comment 65 (and restarting afterwards), I see the following when I drag the XPI onto the browser, and click install on the door hanger dropdown:

1488236825861	addons.update-checker	WARN	Update manifest for [e-mail] did not contain an updates property
[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIURI.hostPort]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: resource://gre/modules/PopupNotifications.jsm :: PopupNotifications_refreshPanel/< :: line 702"  data: no]  (unknown)
1488236827181	Browser.Experiments.Experiments	WARN	Experiments #0::onInstallStarted cancelling install of unknown experiment add-on: [e-mail]

Comment 67

[:Felipe Gomes (needinfo for replies!)]

I'm gonna take a look on this later today

Comment 68

[Benjamin Smedberg]

I am so confused about this.

Question 1: Is this supposed to be a telemetry experiment or a system addon? The comments here are all about an experiment, but comment 64 says this is a system addon.

Comment 1: You cannot use the same .xpi file for an experiment and a system addon. An experiment is em:type=128 an a system addon is em:type=2.  The .xpi file here is built as an experiment, so if we're shipping it as a system addon we need to rebuild it with the correct type.

Comment 2: you cannot install an experiment directly (by dragging it etc). You can only install an experiment through a manifest. So comment 66 is irrelevant.

Comment 3: I really don't know why Justin is getting the "experiments are disabled" message. The person who wrote the experiment QA process is Kamil Jozwiak. The engineering owner of experiments is Georg Fritzsche. Those are the best technical owners following up with this.

Comment 4: in terms of QA priorities, we already know what the addon does, right?. The most important bits to test are the actual deployment settings: the staging of the correct values either on the experiment staging server (experiments) or the balrog staging server (system addon). So setting up tests against people-mozilla or manual .xpi installs seems unnecessary.

Comment 69

[Georg Fritzsche [:gfritzsche]]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #68)
> Comment 3: I really don't know why Justin is getting the "experiments are
> disabled" message. The person who wrote the experiment QA process is Kamil
> Jozwiak. The engineering owner of experiments is Georg Fritzsche. Those are
> the best technical owners following up with this.

Justin, per comment 56, what Firefox version are you using exactly?
Which channel, branded or unbranded?

Comment 70

[Justin [:JW_SoftvisionQA]]

Hello Georg, I am using Release for all testing. This is what I was told to test on.

Comment 71

[Justin [:JW_SoftvisionQA]]

I have tried loading this as a sysaddon with the steps that I know.

1. Clean Profile
2. Change "app.update.channel" to release-sysaddon
3. Open Firefox and view about:support

The addon is not apart of the sysaddons list

Comment 72

[:Felipe Gomes (needinfo for replies!)]

I think the mention of a system add-on here was just a mistake, as everything related to this code is through Telemetry Experiments.  The signed xpi has type=128 (experiment).

I just tested this locally using a fresh profile with Firefox 51.0.1 (Mac) and everything worked well. I'll post the log and the telemetry payload generated by the experiment.

The only one thing that I did differently was enabling telemetry through the about:preferences UI* instead of setting the pref, as I wasn't sure if there are more prefs nowadays that controls it.

* Preferences -> Advanced -> Data Choices -> Share Additional Data

Comment 73

[:Felipe Gomes (needinfo for replies!)]

Attachment: log.txt

Copy&pasted the Browser Console log and highlighted some of the important steps

Comment 74

[:Felipe Gomes (needinfo for replies!)]

Attachment: payload.json

The telemetry payload generated, obtained by:

about:telemetry -> Archived ping data ->  Choose ping "tls-13-study-v3" -> Go to Raw Payload section

Comment 75

[Eric Rescorla (:ekr)]

Benjamin,

Thanks for your detailed comments:

1. We want to deploy this on release, so as I understood, this had to be a system add-on if we want reach. I didn't realize it needed a new em:type, but I can do that.

2. I made enough changes in the add-on that it probably does need to be tested, though maybe we could just test it as a regular experiment in Beta?

Comment 76

[Eric Rescorla (:ekr)]

OK, it sounds like felipe has verified that this works, so we just need to make it a system add-on. Smedberg?

Comment 77

[:Felipe Gomes (needinfo for replies!)]

So I'll work with Justin to figure out what the problem was on his machine, but I believe this is no longer a block to releasing this as an experiment. I think everything is looking good and we could release it now.

Would you like to release it as is for 51 and think about a system add-on afterwards if needed? The detail with Experiments is that they only apply to people who has opted-in to Telemetry reporting, which is not the default on release. But we estimate this to be ~2% of users, which is still a lot. (and you're sampling at 10% but we could increase that)

Comment 78

[:Felipe Gomes (needinfo for replies!)]

Needinfo'ing to confirm my suggestion in comment 77.

And to clarify: the goal right now is just to make the measurements and not set things permanently, right?

Comment 79

[Eric Rescorla (:ekr)]

Felipe:
Yes, our plan is to just take measurements.

I'm still a little vague about the deployment constraints. We'd like to get about 10% of release 51. My understanding was that the cool way to do this was with a system add-on... Is that wrong?

Comment 80

[:Felipe Gomes (needinfo for replies!)]

Both deployment systems have pros and cons. The system add-on can reach up to 95% of users, and it's just a normal, invisible add-on. The Experiments system can only reach users with Telemetry opted-in (2%) but it has some built-in filtering and reporting mechanisms..

Since you're not using these built-in extras it'd be straightforward to convert this to a system add-on. On the other hand, the Experiment is ready to go.. So it's up to you

Comment 81

[Eric Rescorla (:ekr)]

Sample size is more important. Do I need to do anything to make it a system add-on other than the change BDS indicated?

Comment 82

[Justin [:JW_SoftvisionQA]]

With the steps provided by Felipe here:

https://pastebin.mozilla.org/8980600

I can verify that this works as expected.

Comment 83

[Robert Helmer [:rhelmer]]

(In reply to Eric Rescorla (:ekr) from comment #81)
> Sample size is more important. Do I need to do anything to make it a system
> add-on other than the change BDS indicated?

I just tested and this does load as a normal add-on if the type is changed in the manifest.

I'm not sure if Experiments.jsm will work in a normal add-on - it'd be simplest to just make it run for all users, and remove the use of this module.

We can use the update server to only release it to a certain number of users, if you don't want it to go to 100% of release users (it's just a simple throttle mechanism that only serves the update to a certain % of http requests)

Comment 84

[Eric Rescorla (:ekr)]

I would be fine removing the Experiments stuff, assuming that that's OK with Smedberg. Alternately, happy to leave Experiments.jsm if that's better.

Comment 85

[Benjamin Smedberg]

The Experiments code doesn't make sense if this is deployed via system addon, so we'd need to change that. Note that you can't uninstall a system addon, so we'd have to change the behavior to just do nothing on subsequent startups.

rhelmer volunteered to help with system addon stuff since he's the tech lead for all that, so I'm going to loop him in here.

Comment 86

[Robert Helmer [:rhelmer]]

Working up a patch for this.

Comment 87

[Robert Helmer [:rhelmer]]

Attachment: bug1325872.diff

I've applied attachment 8840617 [details] [diff] [review] to the addon in telemetry-experiment-server repo, this is the patch on top of that to convert to a system add-on.

We should move this to a different repo (since it's not a telemetry experiment anymore), but first I wanted to make sure these changes look like what you want.

Comment 88

[Robert Helmer [:rhelmer]]

Oh, I forgot to mention - I tested this locally on 51.0.1 and see a "tls-13-study-v3" payload in about:telemetry.

Comment 89

[Eric Rescorla (:ekr)]

Comment on attachment 8842156 [details] [diff] [review]
bug1325872.diff

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

This looks right except for the version.

::: experiments/tls13-compat-ff51/code/install.rdf
@@ +15,2 @@
>          <em:minVersion>51</em:minVersion>
> +        <em:maxVersion>53.*</em:maxVersion>

I think this should be 52, because we have TLS 1.3 on in 53.

Comment 90

[Robert Helmer [:rhelmer]]

(In reply to Eric Rescorla (:ekr) from comment #89)
> Comment on attachment 8842156 [details] [diff] [review]
> bug1325872.diff
> 
> Review of attachment 8842156 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks right except for the version.
> 
> ::: experiments/tls13-compat-ff51/code/install.rdf
> @@ +15,2 @@
> >          <em:minVersion>51</em:minVersion>
> > +        <em:maxVersion>53.*</em:maxVersion>
> 
> I think this should be 52, because we have TLS 1.3 on in 53.

Oops, that was an artifact from my local testing. Thanks!

I've just tested this via the system add-on install path and it seems to work:

1) about:telemetry shows the custom ping 
2) subsequent startups don't do anything

Comment 91

[Robert Helmer [:rhelmer]]

Here's a PR against the "one-offs" system add-on repo:

https://github.com/mozilla/one-off-system-add-ons/pull/19

Comment 92

[Eric Rescorla (:ekr)]

rhelmer: what happens next?

Comment 93

[Eric Rescorla (:ekr)]

rhelmer: can you just double-check that after the experiment is over, security.tls.version.max == 3 (the default)

Comment 94

[Robert Helmer [:rhelmer]]

Attachment: [e-mail]

Please sign this with the system add-on key. Thanks!

Comment 95

[Robert Helmer [:rhelmer]]

(In reply to Eric Rescorla (:ekr) from comment #93)
> rhelmer: can you just double-check that after the experiment is over,
> security.tls.version.max == 3 (the default)

Yes I do see this.

(In reply to Eric Rescorla (:ekr) from comment #92)
> rhelmer: what happens next?

I've attached the packaged system add-on, once it's signed we're ready to test on release Firefox and ostensibly ready to deploy.

Comment 96

[Jason Thomas [:jason]]

Attachment: [e-mail] signed

Please see attached.

Comment 97

[Robert Helmer [:rhelmer]]

This will be staged and ready for final testing momentarily.

I put together this set of steps to verify before we push:
https://public.etherpad-mozilla.org/p/test_plan_for_bug_1325872

Comment 98

[Robert Helmer [:rhelmer]]

Attachment: [e-mail]_1.0.4.xpi

Could you please sign this version?

We had to do a followup to address some issues in the original XPI and also to add some more telemetry.

Comment 99

[Jason Thomas [:jason]]

Attachment: [e-mail]_1.0.4.xpi signed

Please see attached.

Comment 100

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

Justin, could you please QA this new version?

Comment 101

[Justin [:JW_SoftvisionQA]]

everything works as expected with the new signed xpi.

Comment 102

[Georg Fritzsche [:gfritzsche]]

I assume this bug is done and can be closed?

Comment 103

[Georg Fritzsche [:gfritzsche]]

Moving this out of the current component to prepare for sun setting that component.

Comment 104

[Eric Rescorla (:ekr)]

OBE