Note: There are a few cases of duplicates in user autocompletion which are being worked on.

develop system add-on to facilitate sha-1 disabling rollout

VERIFIED FIXED in Firefox 52

Status

()

Core
Security: PSM
P1
normal
VERIFIED FIXED
7 months ago
5 months ago

People

(Reporter: keeler, Assigned: keeler)

Tracking

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed, firefox53 fixed)

Details

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

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 4 obsolete attachments)

+++ This bug was initially created as a clone of Bug #1311479 +++

This is based on the work in bug 1311479. The approach is to perform the same measurement again, and if the data indicates we won't brick the user (and if they haven't already expressed a preference for always allowing or always forbidding sha1), we'll disable sha1 in signatures on certificates issued by publicly-trusted roots in 10% of the beta population.
(Assignee)

Comment 1

7 months ago
Created attachment 8824207 [details] [diff] [review]
patch
Attachment #8824207 - Flags: review?(jjones)

Comment 2

7 months ago
Comment on attachment 8824207 [details] [diff] [review]
patch

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

Code itself looks fine. I've only nits about metadata.

::: experiments/disable-sha1-beta52-part1/code/bootstrap.js
@@ +205,2 @@
>    console.log(result);
> +  return TelemetryController.submitExternalPing("disable-sha1-beta52-part1",

Is there any way we can extern this string, so that we don't need to re-build/sign/test this package?

Alternatively, maybe we could just name it "disable-sha1" and pick up the deltas in telemetry based on their timestamps?

::: experiments/disable-sha1-beta52-part1/manifest.json
@@ +7,4 @@
>    "manifest"    : {
> +    "id"               : "disable-sha1-beta52-part1@experiments.mozilla.org",
> +    "startTime"        : 1483562000,
> +    "endTime"          : 1488326000,

I'm not really sure how the endTime interacts with the sample; I'd worry that we can't push another rev of this (with the same name, anyway) until endTime hits -- relevant maybe with my other comment.
Attachment #8824207 - Flags: review?(jjones) → review+
(Assignee)

Comment 3

7 months ago
Created attachment 8824507 [details] [diff] [review]
patch v2

(In reply to J.C. Jones [:jcj] from comment #2)
...
> ::: experiments/disable-sha1-beta52-part1/code/bootstrap.js
> @@ +205,2 @@
> >    console.log(result);
> > +  return TelemetryController.submitExternalPing("disable-sha1-beta52-part1",
> 
> Is there any way we can extern this string, so that we don't need to
> re-build/sign/test this package?

Yes, I think I figured out a way to do this (by getting the id of the current experiment and using that). With that change and giving the add-on a more generic name, I think we now only need to update the metadata (i.e. manifest.json) for each phase of the process.

> ::: experiments/disable-sha1-beta52-part1/manifest.json
> @@ +7,4 @@
> >    "manifest"    : {
> > +    "id"               : "disable-sha1-beta52-part1@experiments.mozilla.org",
> > +    "startTime"        : 1483562000,
> > +    "endTime"          : 1488326000,
> 
> I'm not really sure how the endTime interacts with the sample; I'd worry
> that we can't push another rev of this (with the same name, anyway) until
> endTime hits -- relevant maybe with my other comment.

I'll update the start/end times when we have a better idea of exactly when we'll deploy.
Attachment #8824207 - Attachment is obsolete: true
Attachment #8824507 - Flags: review+
(Assignee)

Comment 4

7 months ago
Comment on attachment 8824507 [details] [diff] [review]
patch v2

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

data-review? to :bsmedberg (this is very similar to bug 1311479).
Attachment #8824507 - Flags: review?(benjamin)
Comment on attachment 8824507 [details] [diff] [review]
patch v2

After email discussion, an experiment is not the right vehicle for this progressive deployment. FWIW I have no issues with the data collection bits though.
Attachment #8824507 - Flags: review?(benjamin) → review-
(Assignee)

Comment 6

6 months ago
Ok - it's mighty morphin' bug time. The new approach is to develop a system add-on that we can ship updates to to do a staged roll-out.
Summary: [telemetry-experiment] attempt to disable sha1 in signatures on certificates issued by publicly-trusted roots in 10% of the beta population → develop system add-on to facilitate sha-1 disabling rollout
(Assignee)

Updated

6 months ago
Attachment #8824507 - Attachment is obsolete: true
Comment hidden (mozreview-request)

Comment 8

6 months ago
mozreview-review
Comment on attachment 8826340 [details]
bug 1328718 - implement system add-on to facilitate rollout of disabling SHA-1  data-review=bsmedberg

https://reviewboard.mozilla.org/r/104272/#review105020

Only a string-changing comment. Code looks good - of course, we went through it for the telemetry experiment already. Thanks!

::: browser/extensions/disableSHA1rollout/install.rdf.in:29
(Diff revision 1)
>          <em:maxVersion>@MOZ_APP_MAXVERSION@</em:maxVersion>
>        </Description>
>      </em:targetApplication>
>  
>      <!-- Front End MetaData -->
> -    <em:name>Multi-process staged rollout</em:name>
> +    <em:name>SHA-1 disabling staged rollout</em:name>

might want to use "SHA-1 deprecation staged rollout" instead; similar in the following lines.
Attachment #8826340 - Flags: review?(jjones) → review+
Is it intentional that you'll re-run makeRequest every time you update the addon? (like when updating to increase the threshold).

In theory you could do the checks and set the prefs just once, and later updates will just check what was stored, and the cohortSample, and adjust the prefs accordingly. But I'm not seeing anything that short-circuits that here.

I'll wait to understand if this is on purpose or not before continuing with the review.
Flags: needinfo?(dkeeler)
My intention was to be conservative here to avoid potential breakage. If you think it's unnecessary or likely to cause more problems than it will prevent, I can re-work it.
Flags: needinfo?(dkeeler)

Comment 11

6 months ago
mozreview-review
Comment on attachment 8826340 [details]
bug 1328718 - implement system add-on to facilitate rollout of disabling SHA-1  data-review=bsmedberg

https://reviewboard.mozilla.org/r/104272/#review106360

Alright. Some minor nits below, and 3 general comments:

1 - As long as these requests and checks are not expensive (either on the client or the server), you can keep it this way.

2 - You report the data through an external telemetry ping, but nothing reports this change with the main ping or in a crashreport annotation, so you wouldn't be able to make correlations later if you wanted to. Is that something not needed?  One easy way would be to add the PREF_SHA1_POLICY here: http://searchfox.org/mozilla-central/source/toolkit/components/telemetry/TelemetryEnvironment.jsm#121 (but that requires a data-collection review)

3 - The associated cohort name is not resilient to the user making a manual change after the code has run. I imagine that's not a concern either (since the payload is reported only once when these are set), but just wanted to double check. If the user changes PREF_SHA1_POLICY, then PREF_COHORT_NAME and POLICY_SET_BY_ADDON will be innacurate.

::: browser/extensions/disableSHA1rollout/README.md:10
(Diff revision 1)
> +population into a test group and a control group (currently on a 10%/90%
> +split). The test group will have the policy changed. After doing this, a

change "currently" to e.g. "starting" to avoid having to change this comment every time :P

::: browser/extensions/disableSHA1rollout/bootstrap.js:101
(Diff revision 1)
> +    didNotDisableSHA1Because = currentPrefValue == SHA1_MODE_ALLOW
> +                             ? "preference:allow"
> +                             : "preference:forbid";
> +  }
> +  result.didNotDisableSHA1Because = didNotDisableSHA1Because;
> +  console.log(result);

remove console.log
Attachment #8826340 - Flags: review?(felipc) → review+
To clarify, this point:

(In reply to :Felipe Gomes (needinfo me!) from comment #11)
> 1 - As long as these requests and checks are not expensive (either on the
> client or the server), you can keep it this way.
> 

was referring to your comment 10
Thanks!

(In reply to :Felipe Gomes (needinfo me!) from comment #11)
...
> 1 - As long as these requests and checks are not expensive (either on the
> client or the server), you can keep it this way.

They're not expensive (we already did a similar thing in bug 1311479 without any apparent issues).

> 2 - You report the data through an external telemetry ping, but nothing
> reports this change with the main ping or in a crashreport annotation, so
> you wouldn't be able to make correlations later if you wanted to. Is that
> something not needed?  One easy way would be to add the PREF_SHA1_POLICY
> here:
> http://searchfox.org/mozilla-central/source/toolkit/components/telemetry/
> TelemetryEnvironment.jsm#121 (but that requires a data-collection review)

I don't think we'll need to do any correlations, so I think we're fine without this (famous last words).

> 3 - The associated cohort name is not resilient to the user making a manual
> change after the code has run. I imagine that's not a concern either (since
> the payload is reported only once when these are set), but just wanted to
> double check. If the user changes PREF_SHA1_POLICY, then PREF_COHORT_NAME
> and POLICY_SET_BY_ADDON will be innacurate.

Good point. If they set it to always allow or always forbid, we'll see they opted out when the add-on gets updated, but if they reset it to the current default, we'll just change it on them again, which isn't a great experience. I'll add an observer to catch this case and treat that as another kind of opting out.

> ::: browser/extensions/disableSHA1rollout/README.md:10
> (Diff revision 1)
> > +population into a test group and a control group (currently on a 10%/90%
> > +split). The test group will have the policy changed. After doing this, a
> 
> change "currently" to e.g. "starting" to avoid having to change this comment
> every time :P

Sounds good.

> ::: browser/extensions/disableSHA1rollout/bootstrap.js:101
> (Diff revision 1)
> > +    didNotDisableSHA1Because = currentPrefValue == SHA1_MODE_ALLOW
> > +                             ? "preference:allow"
> > +                             : "preference:forbid";
> > +  }
> > +  result.didNotDisableSHA1Because = didNotDisableSHA1Because;
> > +  console.log(result);
> 
> remove console.log

Done.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
I also updated the add-on to not make the request or send telemetry if it's not running on a channel we're targeting, since that won't give us useful information. Additionally, I'm carrying over the data-review+ from comment 5.
Comment hidden (mozreview-request)

Comment 18

6 months ago
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1da29f893381
implement system add-on to facilitate rollout of disabling SHA-1 r=Felipe,jcj data-review=bsmedberg
I had to back this out for Win7pgo xperf failures like https://treeherder.mozilla.org/logviewer.html#?job_id=70469925&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/80d616e63bc522ec73b0a5cb2c2c5f1c4ca86246
Flags: needinfo?(dkeeler)
Ah sorry, I always forget about this. You have to add an entry to http://searchfox.org/mozilla-central/source/testing/talos/talos/xtalos/xperf_whitelist.json

We never managed to explain why this happens on PGO only, but investigating that always fell very low on the list of priorities..
Comment hidden (mozreview-request)
Ok - thanks for the pointer.
Flags: needinfo?(dkeeler)

Comment 23

6 months ago
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e8abb8252fb0
implement system add-on to facilitate rollout of disabling SHA-1 r=Felipe,jcj data-review=bsmedberg
https://hg.mozilla.org/mozilla-central/rev/e8abb8252fb0
Status: NEW → RESOLVED
Last Resolved: 6 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Hi Matt - I was thinking a good approach might be to have QA verify this on a try build based on the beta branch before actually uplifting it. OS X and Windows builds should be here:

https://archive.mozilla.org/pub/firefox/try-builds/dkeeler@mozilla.com-35c3f8a35b13d5227e76a5ef137d305e7546a6ba/

(For some reason the linux64 builds aren't in that same directory. Here's the treeherder link if anyone knows how to them based on that: https://treeherder.mozilla.org/#/jobs?repo=try&revision=35c3f8a35b13d5227e76a5ef137d305e7546a6ba )

Basically what should happen is after the add-on runs (at the first startup after it's installed or updated), the about:config preference security.pki.sha1_enforcement_level should change from 4 to 3 10% of the time (if disableSHA1.rollout.cohortSample is less than .1, essentially). Other important prefs are disableSHA1.rollout.cohort ("test" if the SHA-1 preference changes, "control" otherwise), and disableSHA1.rollout.policySetByAddOn (true if the add-on changes the pref).

Other edge cases are if security.pki.sha1_enforcement_level has already been changed by the user. If it's already 3, disableSHA1.rollout.cohort will be "optedIn". If it's 0 or 1, disableSHA1.rollout.cohort will be "optedOut". (2 isn't a valid option any longer, so we treat that as if it's the default of 4 for the purposes of this add-on.)

Thanks!
Flags: needinfo?(mwobensmith)
Redirecting need-info to Kanchan (see comment 25, but feel free to redirect as appropriate).
Flags: needinfo?(mwobensmith) → needinfo?(kkumari)
Is this addon already added into these builds or do we need to be provided an xpi?
Flags: needinfo?(kkumari) → needinfo?(dkeeler)
It should already be present in the linked builds - let me know if that isn't the case.
Flags: needinfo?(dkeeler)
Yes I am thinking this is not present in the linked builds because:

1. security.pki.sha1_enforcement_level = 4 every time
2. disableSHA1.rollout.cohortSample, disableSHA1.rollout.cohort, & disableSHA1.rollout.policySetByAddOn are not in about:config. 

I checked this against the windows builds and MacOS. We could not get the linux build for the link provided.
Flags: needinfo?(dkeeler)
Hmmm - I'm seeing the same thing. Felipe, would you know the best way to test this on beta before actually uplifting it to beta? Do we have to do something extra to get the add-on built/signed or something? Thanks!
Flags: needinfo?(dkeeler) → needinfo?(felipc)
I haven't looked at the linked builds, but I'm guessing the code is properly there but bailing out early because the update channel of a try build is "default" instead of "beta". I forgot the steps to do it but it's relatively easy to unpack the build and change its channel name in the prefs file. I suggest trying that and changing the app.update.channel pref to "beta".
Flags: needinfo?(felipc)
(In reply to :Felipe Gomes (needinfo me!) from comment #31)
> I haven't looked at the linked builds, but I'm guessing the code is properly
> there but bailing out early because the update channel of a try build is
> "default" instead of "beta". I forgot the steps to do it but it's relatively
> easy to unpack the build and change its channel name in the prefs file. I
> suggest trying that and changing the app.update.channel pref to "beta".

I navigated to C:\Program Files (x86)\Nightly\defaults\pref and changed app.update.channel pref to "beta" in the channel-prefs.js file and the prefs from comment 29 are still not showing. Is there anything else you can suggest?
Flags: needinfo?(felipc)
Also, about:support in those builds doesn't show the disableSHA1rollout add-on (whereas it does show e.g. e10srollout).
Did the commit of that try push include the addition to browser/extensions/moz.build?
Flags: needinfo?(felipc)
https://hg.mozilla.org/try/file/https://hg.mozilla.org/try/file/35c3f8a35b13d5227e76a5ef137d305e7546a6ba/browser/extensions/moz.build/browser/extensions/moz.build
nope! That would explain it :)
(In reply to :Felipe Gomes (needinfo me!) from comment #35)
> https://hg.mozilla.org/try/file/https://hg.mozilla.org/try/file/
> 35c3f8a35b13d5227e76a5ef137d305e7546a6ba/browser/extensions/moz.build/
> browser/extensions/moz.build
> nope! That would explain it :)

You lost me there Felipe (: so will I need another try build or do I just add the addition to browser/extensions/moz.build (if so how do I do that)?
Flags: needinfo?(felipc)
(Fixing the broken copy-and-pasted link: https://hg.mozilla.org/try/file/35c3f8a35b13d5227e76a5ef137d305e7546a6ba/browser/extensions/moz.build ) 

Yeah, you'll need to use a new try build from Keeler.
Flags: needinfo?(felipc)
D'oh - thanks for catching that. I've started a new try build here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=31a91c5083a8d54f65ca314bb39cd4d987e01439
(it might take a few hours for that to complete)
(In reply to David Keeler [:keeler] (use needinfo?) from comment #38)
> D'oh - thanks for catching that. I've started a new try build here:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=31a91c5083a8d54f65ca314bb39cd4d987e01439
> (it might take a few hours for that to complete)

Looks like all of the builds have failed.
Flags: needinfo?(dkeeler)
Thanks for letting me know. Looks like the mozilla-beta build is busted at the moment, so I rebased this on (what should be) a good revision: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b378df121c0df4081c68f678e5f4b781f1cc114
Flags: needinfo?(dkeeler)
Comment 29 still stands true

(In reply to Justin [:JW_SoftvisionQA] from comment #29)
> 
> 1. security.pki.sha1_enforcement_level = 4 every time
> 2. disableSHA1.rollout.cohortSample, disableSHA1.rollout.cohort, &
> disableSHA1.rollout.policySetByAddOn are not in about:config. 
> 
> I checked this against the windows builds and MacOS. We could not get the
> linux build for the link provided.

Differences from the previous build (35c3f8a35b13d5227e76a5ef137d305e7546a6ba):

1. "SHA-1 deprecation staged rollout" is showing on about:support as true
2. E10's is defaulted to false (don't know if this effects anything)
Flags: needinfo?(dkeeler)
I heard this wants to be shipped via a system add-on. Please refer to the Go Faster documentation[0] for information on this process. It should be noted that uptake of the system add-on release channel is currently being investigated[1] with some estimations as low as 55%.

The next steps would be

    - attach to this bug a signed and QA verified XPI file
    - send an `Intent to Ship` email to the release-drivers and gofaster mailing lists

More info can be found in the documentation[0] on both these steps. From reading the bug, I presume :keeler, and/or :jcj would own these items.

[0] https://wiki.mozilla.org/Firefox/Go_Faster/System_Add-ons/Process
[1] https://wiki.mozilla.org/Firefox/Go_Faster/System_Add-ons/Verification_Chain
Whiteboard: [psm-assigned] → [psm-assigned][go-faster-system-addon]
So it sounds like verifying via a try build is not the way to go here. I'll get a signed xpi and we can move forward with that.
Flags: needinfo?(dkeeler)
Your beta rollout phase can be done through in-tree code and can be verified with the try builds. The GoFaster process will enter in the picture when you want to do over-the-air updates to the release channel to increase your rollout there (without needing to do a dot release)
OK, sounds good.

Justin, if I understand correctly, in comment 41 you're saying that in the build from comment 40, the disableSHA1.rollout.* prefs aren't being created. Did you change the channel (as you described in comment 32) before running the build?

(FWIW, it's a good sign that the SHA-1 entry shows up in about:support. I think the e10s setting is unrelated to our purposes here.)
Flags: needinfo?(jwilliams)
Forgot to set the channel to Beta. All the prefs are showing now that this is set. I will proceed with testing.
Flags: needinfo?(jwilliams)
Information on our findings:

1. There were 4 of us testing one on each platform (Win 7, 8.1, 10, MacOSX 10.12) and we were placed into the control cohort 100% of the time.
2. security.pki.sha1_enforcement_level was 4 every time. When security.pki.sha1_enforcement_level = 4 it works as expected.
3. disableSHA1.rollout.cohortSample was always greater than 0.1
4. When manually changing the disableSHA1.rollout.cohortSample or security.pki.sha1_enforcement_level nothing happened to the other prefs (figured if changing security.pki.sha1_enforcement_level to 0, 1, or 3 or changing disableSHA1.rollout.cohortSample to be less than 0.1 we would be placed in a different cohort)

Is there any way to force myself into optedOut or optedIn?
Flags: needinfo?(dkeeler)
The add-on only runs once - when "installed". I believe this happens either when you start with a new profile or run a build that doesn't have the add-on and then run a build that does have the add-on under the same profile. This means that changing the cohortSample after it's already been created won't change anything.

Unfortunately with a sample of 10%, it's expected that you'll have to run with a new profile ~10 times before being put in the test cohort.

To force optedOut, set the preference security.pki.sha1_enforcement_level to 0 or 1 before running the add-on (basically, run a profile in a different build, set the preference, and then run the build in comment 40). To force optedIn, set the preference to 3 before running the add-on.
Flags: needinfo?(dkeeler)
I can verify that this feature works as expected.
Status: RESOLVED → VERIFIED
Comment on attachment 8826340 [details]
bug 1328718 - implement system add-on to facilitate rollout of disabling SHA-1  data-review=bsmedberg

Approval Request Comment
[Feature/Bug causing the regression]: SHA-1 deprecation staged rollout
[User impact if declined]: users won't be protected against potential collisions found against certificates signed with SHA-1
[Is this code covered by automated tests?]: n/a
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: already done
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not very
[Why is the change risky/not risky?]: it's been QA'd, it's just a system add-on that, in the worst case, we can disable (and all it does is make a simple HTTP request and then conditionally changes a pref)
[String changes made/needed]: none
Attachment #8826340 - Flags: approval-mozilla-beta?
Thanks Justin!
Created attachment 8830573 [details] [diff] [review]
patch-for-beta.diff
Comment on attachment 8830573 [details] [diff] [review]
patch-for-beta.diff

sha1 deprecation system add-on for beta52

Moving approval flag from the other patch since AIUI this is the one that should land in beta.
Attachment #8830573 - Flags: approval-mozilla-beta+
Comment on attachment 8826340 [details]
bug 1328718 - implement system add-on to facilitate rollout of disabling SHA-1  data-review=bsmedberg

resetting beta uplift flag here, moved to the other patch on this bug.
Attachment #8826340 - Flags: approval-mozilla-beta?

Comment 55

6 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/ce7143aed04f
status-firefox52: --- → fixed
Backed out from Beta for causing non-local connection attempts to telemetry.mozilla.org on all opt builds.
https://treeherder.mozilla.org/logviewer.html#?job_id=72383928&repo=mozilla-beta

https://hg.mozilla.org/releases/mozilla-beta/rev/0ca9aead23c24c40fb84b1800914db550385aa25
status-firefox52: fixed → affected
Flags: needinfo?(dkeeler)
Created attachment 8831217 [details] [diff] [review]
patch-for-beta v2

I updated the add-on to not run if the telemetry server configuration is not the default (which indicates we're in a test environment and shouldn't connect to external hosts).
Attachment #8830573 - Attachment is obsolete: true
Flags: needinfo?(dkeeler)
Attachment #8831217 - Flags: review?(jjones)
Comment on attachment 8831217 [details] [diff] [review]
patch-for-beta v2

Looks like JC's traveling. Felipe, can you have a quick look? Thanks!
Attachment #8831217 - Flags: review?(jjones) → review?(felipc)
Justin, if you could do a quick check that this build still does the expected (when it's available), that would be great: https://treeherder.mozilla.org/#/jobs?repo=try&revision=21c047fefa634cea9139d989af7b6c3a2a95ac27
(I think you'll still have to set the channel manually before running.) Thanks!
Flags: needinfo?(jwilliams)
Attachment #8831217 - Flags: review?(felipc) → review+
I can verify that this feature works as expected.
Flags: needinfo?(jwilliams)
Comment on attachment 8831217 [details] [diff] [review]
patch-for-beta v2

carrying over beta approval from previous patch
Attachment #8831217 - Flags: approval-mozilla-beta+

Comment 62

6 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/bef0eb878659
status-firefox52: affected → fixed
Backed again for non-local connection crashes.
https://treeherder.mozilla.org/logviewer.html#?job_id=72777598&repo=mozilla-beta

https://hg.mozilla.org/releases/mozilla-beta/rev/eee47824f4687534a901cd9c28126041bd19d261
status-firefox52: fixed → affected
 https://hg.mozilla.org/releases/mozilla-beta/rev/cd23479f1e746be41d872f4f2ea71e33cd0f07ae
sorry missed comment #63, backed out again.

David could you take a look at this so that we can get this landed on beta
Flags: needinfo?(dkeeler)
It looks like different test harnesses have different sets of preferences they use to prevent outbound connections (particularly for telemetry). reftests appear to set toolkit.telemetry.unified to false instead of setting toolkit.telemetry.server to localhost. Using both preferences prevents the add-on from making any outbound connections in tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=375f4d38179b289cdb6f62e165589a5a18cd4cf8
(provided I set up that patch correctly - I had to make sure it would attempt to run if the channel was also "default" - that part won't go in the final patch)
Also, note that checking toolkit.telemetry.unified is something we want to do anyway in case the user has opted out of fhr reporting.
So, unless anyone knows of a better way to check if we're in a test environment, I'll put up a new patch, get the addition reviewed, get it re-QA'd, and get it ready for landing again.
Flags: needinfo?(dkeeler)
Created attachment 8831846 [details] [diff] [review]
patch-for-beta v3

I ran a manual diff on the patches and got this:

5,6c5,6
< # Node ID 90b06dbacdd848c099f00c54a61e83b65516a3a8
< # Parent  f28c092abe746a9c7b39fb5c715141810a9c87d4
---
> # Node ID a4f8129002e756978fa4e6d5a90680f5644d88c0
> # Parent  787766dd27b7373797a3fa6ab7115c85f27c04f2
117c117
< @@ -0,0 +1,312 @@
---
> @@ -0,0 +1,317 @@
157a158,161
> +  // Also only run if the user has unified telemetry enabled (because we don't
> +  // want to submit a telemetry ping if they've opted out).
> +  let unifiedTelemetryEnabled = Preferences.get("toolkit.telemetry.unified",
> +                                                undefined);
159c163,164
< +      telemetryServerURL == "https://incoming.telemetry.mozilla.org") {
---
> +      telemetryServerURL == "https://incoming.telemetry.mozilla.org" &&
> +      unifiedTelemetryEnabled === true) {
Attachment #8831217 - Attachment is obsolete: true
Attachment #8831846 - Flags: review?(felipc)
Another option would be to set something in http://searchfox.org/mozilla-central/source/testing/profiles/prefs_general.js. That covers mochitest and I think there's a different file for xpcshell.

The usage of this pref file for things hitting the network is usually like this: the URL to hit is set in a pref and overridden in the prefs file: http://searchfox.org/mozilla-central/rev/f31f538622d0dfb2d869d4babc4951f6001e9be2/testing/profiles/prefs_general.js#55

But given that the server that you're hitting is the telemetry one, I think this approach makes sense too.
Comment on attachment 8831846 [details] [diff] [review]
patch-for-beta v3

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

Up to you if you want to keep this or change to the other suggestion.
Attachment #8831846 - Flags: review?(felipc) → review+
Thanks! I think since we want to check the unified telemetry enabled pref anyway, I'll stick with how it is currently.

Justin, if you could verify these builds again, that would be great: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a243b1c8606fc36e9d48ddcdbbaa6d0fff387171
Flags: needinfo?(jwilliams)
Everything looks good with these builds.
Flags: needinfo?(jwilliams)
Julien, would it be alright to carry over beta approval again? Based on the try run in comment 66 I'm (more) confident that the add-on won't try to make any network requests during tests (see comment 67 for the interdiff, although bugzilla's built-in interdiff seems to be working fine this time around).
Flags: needinfo?(jcristau)
Comment on attachment 8831846 [details] [diff] [review]
patch-for-beta v3

let's try again for beta.
Flags: needinfo?(jcristau)
Attachment #8831846 - Flags: approval-mozilla-beta+

Comment 74

6 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/6b6f15c4bece
status-firefox52: affected → fixed

Updated

5 months ago
Blocks: 1338228

Updated

5 months ago
Blocks: 1339662
You need to log in before you can comment on or make changes to this bug.