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

REOPENED
Assigned to

Status

Firefox Health Report
Client: Desktop
REOPENED
8 months ago
a day ago

People

(Reporter: ekr, Assigned: ekr)

Tracking

unspecified
Points:
---

Firefox Tracking Flags

(firefox51+ wontfix)

Details

(Whiteboard: [go-faster-system-addon])

Attachments

(10 attachments, 6 obsolete attachments)

13.87 KB, patch
Felipe
: review+
Details | Diff | Splinter Review
130.24 KB, image/jpeg
Details
14.89 KB, patch
Felipe
: review+
Details | Diff | Splinter Review
16.54 KB, text/plain
Details
1.34 KB, application/json
Details
4.91 KB, patch
ekr
: review+
Details | Diff | Splinter Review
2.69 KB, application/octet-stream
Details
6.57 KB, application/octet-stream
Details
3.14 KB, application/octet-stream
Details
7.02 KB, application/x-xpinstall
Details
(Assignee)

Description

8 months ago
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.
(Assignee)

Comment 1

8 months ago
Created attachment 8821913 [details] [diff] [review]
tls13 ff51 experiment
(Assignee)

Comment 2

8 months ago
Created attachment 8821916 [details] [diff] [review]
Actually write the experiment
(Assignee)

Comment 3

8 months ago
Created attachment 8821918 [details] [diff] [review]
Make a new experiment that's a copy of a previous experiment
Attachment #8821913 - Attachment is obsolete: true
(Assignee)

Comment 4

8 months ago
Created attachment 8821919 [details] [diff] [review]
Write the experiment
Attachment #8821916 - Attachment is obsolete: true
(Assignee)

Comment 5

8 months ago
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
Attachment #8821919 - Flags: review?(benjamin)

Comment 6

8 months ago
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"               : "tls13-compat-nightly52@experiments.mozilla.org",
>+    "id"               : "tls13-compat-ff51@experiments.mozilla.org",
>     "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
Attachment #8821919 - Flags: review?(benjamin) → review-
(Assignee)

Comment 7

8 months ago
- 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.
(Assignee)

Updated

8 months ago
Flags: needinfo?(benjamin)

Updated

8 months ago
status-firefox51: --- → affected

Comment 8

8 months ago
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.
Flags: needinfo?(benjamin)
(Assignee)

Comment 9

7 months ago
Created attachment 8825019 [details] [diff] [review]
revised.patch
Attachment #8821919 - Attachment is obsolete: true
(Assignee)

Comment 10

7 months ago
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.
Attachment #8825019 - Flags: review?(benjamin)
(Assignee)

Comment 11

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

Updated

7 months ago
Attachment #8825019 - Flags: review?(benjamin) → review?(felipc)

Comment 12

7 months ago
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
(Assignee)

Comment 13

7 months ago
(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 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.
Attachment #8825019 - Flags: review?(felipc) → review+
(Assignee)

Comment 15

7 months ago
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.
(Assignee)

Updated

7 months ago
Flags: needinfo?(ryanvm)
Flags: needinfo?(ryanvm) → needinfo?(rares.bologa)

Updated

7 months ago
Flags: needinfo?(rares.bologa) → needinfo?(jwilliams)
(Assignee)

Comment 16

7 months ago
jwilliams: do you have an ETA on this?
We just got into the office here. I will look over all of the details and give you an ETA on this.
Flags: needinfo?(jwilliams) → needinfo?(ekr)
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.
I have verified that all the information from Comment 15 is working as intended. Marking this as Resolved Verified.
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Flags: needinfo?(ekr)
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
(Assignee)

Updated

7 months ago
Blocks: 1331153
(Assignee)

Comment 20

7 months ago
Justin, can you please test this XPI against a FF51 Release Candidate. We are planning to release it as a system add-on.
Status: VERIFIED → REOPENED
Flags: needinfo?(jwilliams)
Resolution: FIXED → ---
I can confirm that all the information from Comment 15 is working as intended on FF51 Release Candidate Build 20170112171116. Marking as Resolved Verified
Status: REOPENED → RESOLVED
Last Resolved: 7 months ago7 months ago
Flags: needinfo?(jwilliams)
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
Did this actually ship? I don't think we should mark it verified/fixed until it ships....
(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.
Status: VERIFIED → RESOLVED
Last Resolved: 7 months ago7 months ago
Flags: needinfo?(ekr)
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
Flags: needinfo?(jwilliams)
yes. that is what I verified.
Flags: needinfo?(jwilliams)
(Assignee)

Comment 26

6 months ago
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.
Status: RESOLVED → REOPENED
Flags: needinfo?(ekr)
Resolution: FIXED → ---
(Assignee)

Comment 27

6 months ago
Created attachment 8836419 [details] [diff] [review]
exp.patch
Attachment #8836419 - Flags: review?(felipc)
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
Attachment #8836419 - Flags: review?(felipc) → review+
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.
(Assignee)

Comment 30

6 months ago
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.
(Assignee)

Comment 31

6 months ago
Created attachment 8839607 [details] [diff] [review]
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).
Attachment #8821918 - Attachment is obsolete: true
Attachment #8825019 - Attachment is obsolete: true
Attachment #8836419 - Attachment is obsolete: true
Attachment #8839607 - Flags: review?(felipc)
(Assignee)

Comment 32

6 months ago
P.S. The allizom.org URL is a placeholder. I'm waiting for a new URL from cloudflare.
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.
Attachment #8839607 - Flags: review?(felipc) → review+
(Assignee)

Comment 34

6 months ago
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/
Flags: needinfo?(jwilliams)
Created attachment 8839992 [details]
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.
Flags: needinfo?(jwilliams) → needinfo?(ekr)
(Assignee)

Comment 36

6 months ago
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?
Flags: needinfo?(ekr)
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
(Assignee)

Comment 38

6 months ago
Does the browser console show it running the experiment?
Here are the contents of the browser console:

1487786766784	addons.update-checker	WARN	Update manifest for tls13-compat-ff51-v3@experiments.mozilla.org did not contain an updates property
1487786772873	Browser.Experiments.Experiments	WARN	Experiments #0::onInstallStarted cancelling install of unknown experiment add-on: tls13-compat-ff51-v3@experiments.mozilla.org
(Assignee)

Comment 40

6 months ago
OK, so that's the problem, presumably.

felipe, any idea what the problem is here?
Flags: needinfo?(felipc)
(Assignee)

Comment 41

6 months ago
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.
Flags: needinfo?(benjamin)

Comment 42

6 months ago
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.
Flags: needinfo?(benjamin)
(Assignee)

Comment 43

6 months ago
(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

6 months ago
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
(Assignee)

Comment 45

6 months ago
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
Assignee: nobody → ekr
(Assignee)

Comment 46

6 months ago
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?
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
Flags: needinfo?(felipc)
(Assignee)

Comment 48

6 months ago
No worries. Will post a new patch in a bit. Thanks.
(Assignee)

Comment 49

6 months ago
Created attachment 8840617 [details] [diff] [review]
revised.patch

Felipe, here is a revised patch with better debugging, the right URL, and the non-reset changes you asked for patched.
Attachment #8840617 - Flags: review?(felipc)
Attachment #8840617 - Flags: review?(felipc) → review+
(Assignee)

Comment 50

6 months ago
Justin, please see new XPI for test.

https://bugzilla.mozilla.org/show_bug.cgi?id=1341445
Flags: needinfo?(jwilliams)
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 tls13-compat-ff51-v3@experiments.mozilla.org did not contain an updates property
1487952419855	Browser.Experiments.Experiments	WARN	Experiments #0::onInstallStarted cancelling install of unknown experiment add-on: tls13-compat-ff51-v3@experiments.mozilla.org

Also there are no telemetry pings.
Flags: needinfo?(jwilliams)
(Assignee)

Comment 52

6 months ago
I don't think this is me. BDS, can you help?
Flags: needinfo?(benjamin)

Comment 53

6 months ago
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.
Flags: needinfo?(benjamin) → needinfo?(jwilliams)
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
Flags: needinfo?(jwilliams) → needinfo?(benjamin)

Comment 55

6 months ago
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.
Flags: needinfo?(benjamin)
(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
(Assignee)

Comment 57

6 months ago
The XPI is signed.
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"
(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
(Assignee)

Comment 60

6 months ago
This still seems like it's not me.
Justin: are both of these prefs true for you?

toolkit.telemetry.enabled
experiments.enabled
Yes, both are set to true on my end.
(Assignee)

Comment 63

6 months ago
Did you restart the browser after setting these prefs?
(Assignee)

Comment 64

6 months ago
Note: this is supposed to be a system add-on. Do we need to do something different for that?
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 hidden (offtopic)
I'm gonna take a look on this later today
Flags: needinfo?(felipc)

Comment 68

6 months ago
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.

Updated

6 months ago
Flags: needinfo?(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?
Flags: needinfo?(gfritzsche)
Hello Georg, I am using Release for all testing. This is what I was told to test on.
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
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
Flags: needinfo?(felipc)
Created attachment 8842014 [details]
log.txt

Copy&pasted the Browser Console log and highlighted some of the important steps
Created attachment 8842017 [details]
payload.json

The telemetry payload generated, obtained by:

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

Comment 75

6 months ago
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?
(Assignee)

Comment 76

6 months ago
OK, it sounds like felipe has verified that this works, so we just need to make it a system add-on. Smedberg?
Flags: needinfo?(benjamin)
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)
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?
Flags: needinfo?(ekr)

Updated

6 months ago
Flags: needinfo?(benjamin)
(Assignee)

Comment 79

6 months ago
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?
Flags: needinfo?(ekr)
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
Flags: needinfo?(ekr)
(Assignee)

Comment 81

6 months ago
Sample size is more important. Do I need to do anything to make it a system add-on other than the change BDS indicated?
Flags: needinfo?(ekr)
With the steps provided by Felipe here:

https://pastebin.mozilla.org/8980600

I can verify that this works as expected.
(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)
Flags: needinfo?(ekr)
(Assignee)

Comment 84

6 months ago
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.
Flags: needinfo?(ekr) → needinfo?(benjamin)

Comment 85

6 months ago
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.
Flags: needinfo?(benjamin) → needinfo?(rhelmer)
Working up a patch for this.
Flags: needinfo?(rhelmer)
Created attachment 8842156 [details] [diff] [review]
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.
Attachment #8842156 - Flags: review?(ekr)
Oh, I forgot to mention - I tested this locally on 51.0.1 and see a "tls-13-study-v3" payload in about:telemetry.
(Assignee)

Comment 89

6 months ago
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.
Attachment #8842156 - Flags: review?(ekr) → review+
(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
Here's a PR against the "one-offs" system add-on repo:

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

Comment 92

6 months ago
rhelmer: what happens next?
Flags: needinfo?(rhelmer)
(Assignee)

Comment 93

6 months ago
rhelmer: can you just double-check that after the experiment is over, security.tls.version.max == 3 (the default)
Created attachment 8842163 [details]
tls13-compat-ff51@mozilla.org.xpi

Please sign this with the system add-on key. Thanks!
Flags: needinfo?(rhelmer) → needinfo?(jthomas)
(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.
Created attachment 8842169 [details]
tls13-compat-ff51@mozilla.org.xpi signed

Please see attached.
Flags: needinfo?(jthomas)
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
tracking-firefox51: --- → +
Created attachment 8844105 [details]
tls13-compat-ff51@mozilla.org_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.
Flags: needinfo?(jthomas)
Whiteboard: [go-faster-system-addon]
Created attachment 8844133 [details]
tls13-compat-ff51@mozilla.org_1.0.4.xpi signed

Please see attached.
Flags: needinfo?(jthomas)
Justin, could you please QA this new version?
Flags: needinfo?(jwilliams)
everything works as expected with the new signed xpi.
Flags: needinfo?(jwilliams)
status-firefox51: affected → wontfix
You need to log in before you can comment on or make changes to this bug.