François Marier [:francois] Reporter
	Description • 7 years ago

Shield studies could in theory interfere with the functionality of any marionette tests. It already did once in bug 1399311.

To shield (pun intended) the tests from this source of uncertainty, we should disable all such experiments:

app.shield.optoutstudies.enabled = false

This corresponds to "Allow Firefox to install and run studies" in about:preferences#privacy | Nightly Data Collection and Use

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 1 • 7 years ago

To be in sync with other test harnesses we should set `the extensions.shield-recipe-client.api_url` to an empty string. This will prevent the extension from reaching the network at all, which includes fetching of experiments.

	Andreas Tolfsen ❲:ato❳
	Comment 3 • 7 years ago
mozreview-review

Comment on attachment 8908293 [details]
Bug 1399628 - Disable Shield extension in Marionette and geckodriver.

https://reviewboard.mozilla.org/r/179936/#review185388

	Pulsebot
	Comment 4 • 7 years ago

Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3baecf7b25f2
Disable Shield extension in Marionette and geckodriver. r=ato

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 5 • 7 years ago

The patch for geckodriver has a missing closing bracket and as such it causes build bustage. I requested a backout from sheriffs via IRC, but no-one seems to be around.

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 6 • 7 years ago

Backed out for bustage:

https://hg.mozilla.org/integration/autoland/rev/5500f432a88828303595de17bf31d127baa12ff9

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=3baecf7b25f2d4a0d8cda5f773878a0f9d941ea5&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Build log: https://treeherder.mozilla.org/logviewer.html#?job_id=131268422&repo=autoland

[task 2017-09-15T12:15:22.012Z] 12:15:22     INFO -  error: incorrect close delimiter: `]`
[task 2017-09-15T12:15:22.012Z] 12:15:22     INFO -     --> /builds/worker/workspace/build/src/testing/geckodriver/src/prefs.rs:234:5
[task 2017-09-15T12:15:22.012Z] 12:15:22     INFO -      |
[task 2017-09-15T12:15:22.012Z] 12:15:22     INFO -  234 |     ];
[task 2017-09-15T12:15:22.012Z] 12:15:22     INFO -      |     ^
[task 2017-09-15T12:15:22.012Z] 12:15:22     INFO -      |
[task 2017-09-15T12:15:22.013Z] 12:15:22     INFO -  note: unclosed delimiter
[task 2017-09-15T12:15:22.013Z] 12:15:22     INFO -     --> /builds/worker/workspace/build/src/testing/geckodriver/src/prefs.rs:151:9
[task 2017-09-15T12:15:22.013Z] 12:15:22     INFO -      |
[task 2017-09-15T12:15:22.013Z] 12:15:22     INFO -  151 |         ("extensions.shield-recipe-client.api_url", Pref::new(""),
[task 2017-09-15T12:15:22.014Z] 12:15:22     INFO -      |         ^
[task 2017-09-15T12:15:22.014Z] 12:15:22     INFO -  error[E0308]: mismatched types
[task 2017-09-15T12:15:22.014Z] 12:15:22     INFO -     --> /builds/worker/workspace/build/src/testing/geckodriver/src/prefs.rs:151:9
[task 2017-09-15T12:15:22.014Z] 12:15:22     INFO -      |
[task 2017-09-15T12:15:22.015Z] 12:15:22     INFO -  151 | /         ("extensions.shield-recipe-client.api_url", Pref::new(""),
[task 2017-09-15T12:15:22.015Z] 12:15:22     INFO -  152 | |
[task 2017-09-15T12:15:22.015Z] 12:15:22     INFO -  153 | |         ("extensions.showMismatchUI", Pref::new(false)),
[task 2017-09-15T12:15:22.016Z] 12:15:22     INFO -  154 | |
[task 2017-09-15T12:15:22.016Z] 12:15:22     INFO -  ...   |
[task 2017-09-15T12:15:22.016Z] 12:15:22     INFO -  233 | |         ("toolkit.telemetry.server", Pref::new("https://%(server)s/dummy/telemetry/")),
[task 2017-09-15T12:15:22.016Z] 12:15:22     INFO -  234 | |     ];
[task 2017-09-15T12:15:22.016Z] 12:15:22     INFO -      | |_____^ expected a tuple with 2 elements, found one with 28 elements
[task 2017-09-15T12:15:22.016Z] 12:15:22     INFO -      |
[task 2017-09-15T12:15:22.017Z] 12:15:22     INFO -      = note: expected type `(&'static str, mozprofile::preferences::Pref)`
[task 2017-09-15T12:15:22.017Z] 12:15:22     INFO -                 found type `(&'static str, mozprofile::preferences::Pref, (&'static str, mozprofile::preferences::Pref), (&'static str, mozprofile::preferences::Pref), (&'static str, mozprofile::preferences::Pref), (&'static str, mozprofile::preferences::Pref), (&'static str, mozprofile::preferences::Pref), (&'static str, mozprofile::preferences::Pref), (&'static str, mozprofile::preferences::Pref), (&'static str, mozprofile::preferences::Pref), (&'static str, mozprofile::preferences::Pref), (&'static str, mozprofile::preferences::Pref), (&'static str, mozprofile::preferences::Pref), (&'static str, mozprofile::preferences::Pref), (&'static str, mozprofile::preferences::Pref), (&'static str, mozprofile::preferences::Pref), (&'static str, mozprofile::preferences::Pref), (&'static str, mozprofile::preferences::Pref), (&'static str, mozprofile::preferences::Pref), (&'static str, mozprofile::preferences::Pref), (&'static str, mozprofile::preferences::Pref), (&'static str, mozprofile::preferences::Pref), (&'static str, mozprofile::preferences::Pref), (&'static str, mozprofile::preferences::Pref), (&'static str, mozprofile::preferences::Pref), (&'static str, mozprofile::preferences::Pref), (&'static str, mozprofile::preferences::Pref), (&'static str, mozprofile::preferences::Pref))`
[task 2017-09-15T12:15:22.017Z] 12:15:22     INFO -  error: aborting due to previous error(s)
[task 2017-09-15T12:15:22.017Z] 12:15:22     INFO -  error: Could not compile `geckodriver`.
[task 2017-09-15T12:15:22.018Z] 12:15:22     INFO -  Caused by:
[task 2017-09-15T12:15:22.019Z] 12:15:22     INFO -    process didn't exit successfully: `/builds/worker/workspace/build/src/sccache2/sccache /builds/worker/workspace/build/src/rustc/bin/rustc --crate-name geckodriver /builds/worker/workspace/build/src/testing/geckodriver/src/main.rs --crate-type bin --emit=dep-info,link -C opt-level=3 -C metadata=72879fabd8533377 -C extra-filename=-72879fabd8533377 --out-dir /builds/worker/workspace/build/src/obj-firefox/testing/geckodriver/./x86_64-unknown-linux-gnu/release/deps --target x86_64-unknown-linux-gnu -C linker=/builds/worker/workspace/build/src/build/cargo-linker -L dependency=/builds/worker/workspace/build/src/obj-firefox/testing/geckodriver/./x86_64-unknown-linux-gnu/release/deps -L dependency=/builds/worker/workspace/build/src/obj-firefox/testing/geckodriver/./release/deps --extern clap=/builds/worker/workspace/build/src/obj-firefox/testing/geckodriver/./x86_64-unknown-linux-gnu/release/deps/libclap-99e3bfa8e6d4a648.rlib --extern slog_stdlog=/builds/worker/workspace/build/src/obj-firefox/testing/geckodriver/./x86_64-unknown-linux-gnu/release/deps/libslog_stdlog-befb37d3bceb6b44.rlib --extern mozrunner=/builds/worker/workspace/build/src/obj-firefox/testing/geckodriver/./x86_64-unknown-linux-gnu/release/deps/libmozrunner-845225c340bb6f2b.rlib --extern zip=/builds/worker/workspace/build/src/obj-firefox/testing/geckodriver/./x86_64-unknown-linux-gnu/release/deps/libzip-77fa1f392c7dcbed.rlib --extern slog=/builds/worker/workspace/build/src/obj-firefox/testing/geckodriver/./x86_64-unknown-linux-gnu/release/deps/libslog-93cbcbbdf82ea409.rlib --extern slog_stream=/builds/worker/workspace/build/src/obj-firefox/testing/geckodriver/./x86_64-unknown-linux-gnu/release/deps/libslog_stream-51b856d9717529ef.rlib --extern mozversion=/builds/worker/workspace/build/src/obj-firefox/testing/geckodriver/./x86_64-unknown-linux-gnu/release/deps/libmozversion-14c14c082457d4f5.rlib --extern hyper=/builds/worker/workspace/build/src/obj-firefox/testing/geckodriver/./x86_64-unknown-linux-gnu/release/deps/libhyper-b938c4d4c431d9c8.rlib --extern log=/builds/worker/workspace/build/src/obj-firefox/testing/geckodriver/./x86_64-unknown-linux-gnu/release/deps/liblog-1f5475e10c01ed14.rlib --extern mozprofile=/builds/worker/workspace/build/src/obj-firefox/testing/geckodriver/./x86_64-unknown-linux-gnu/release/deps/libmozprofile-aab57bb98c47b3c3.rlib --extern chrono=/builds/worker/workspace/build/src/obj-firefox/testing/geckodriver/./x86_64-unknown-linux-gnu/release/deps/libchrono-3d6f695ecef86b2b.rlib --extern rustc_serialize=/builds/worker/workspace/build/src/obj-firefox/testing/geckodriver/./x86_64-unknown-linux-gnu/release/deps/librustc_serialize-4e819f02090d323a.rlib --extern webdriver=/builds/worker/workspace/build/src/obj-firefox/testing/geckodriver/./x86_64-unknown-linux-gnu/release/deps/libwebdriver-2f8654378948ae97.rlib --extern uuid=/builds/worker/workspace/build/src/obj-firefox/testing/geckodriver/./x86_64-unknown-linux-gnu/release/deps/libuuid-83c6367f255e262a.rlib --extern regex=/builds/worker/workspace/build/src/obj-firefox/testing/geckodriver/./x86_64-unknown-linux-gnu/release/deps/libregex-654d17cf911fa301.rlib --extern lazy_static=/builds/worker/workspace/build/src/obj-firefox/testing/geckodriver/./x86_64-unknown-linux-gnu/release/deps/liblazy_static-f98f083de486c507.rlib --extern slog_atomic=/builds/worker/workspace/build/src/obj-firefox/testing/geckodriver/./x86_64-unknown-linux-gnu/release/deps/libslog_atomic-949396f07b779e90.rlib -C debuginfo=2 -L native=/builds/worker/workspace/build/src/obj-firefox/testing/geckodriver/./x86_64-unknown-linux-gnu/release/build/backtrace-sys-ddb37249d3f213a0/out/.libs -L native=/builds/worker/workspace/build/src/obj-firefox/testing/geckodriver/./x86_64-unknown-linux-gnu/release/build/bzip2-sys-f95fe9b8584513b0/out -L native=/builds/worker/workspace/build/src/obj-firefox/testing/geckodriver/./x86_64-unknown-linux-gnu/release/build/miniz-sys-264fda27642dc530/out` (exit code: 101)
[task 2017-09-15T12:15:22.019Z] 12:15:22     INFO -  /builds/worker/workspace/build/src/config/rules.mk:1033: recipe for target 'force-cargo-program-build' failed
[task 2017-09-15T12:15:22.019Z] 12:15:22     INFO -  gmake[5]: *** [force-cargo-program-build] Error 101
[task 2017-09-15T12:15:22.019Z] 12:15:22     INFO -  gmake[5]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/testing/geckodriver'
[task 2017-09-15T12:15:22.019Z] 12:15:22     INFO -  /builds/worker/workspace/build/src/config/recurse.mk:73: recipe for target 'testing/geckodriver/target' failed
[task 2017-09-15T12:15:22.019Z] 12:15:22     INFO -  gmake[4]: *** [testing/geckodriver/target] Error 2

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 7 • 7 years ago

The bustage was not detected with the try build due to "--artifact" :( It means whenever we make changes to geckodriver we have to remember to not run an artifact build!

	Pulsebot
	Comment 9 • 7 years ago

Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e5660fb96ca6
Disable Shield extension in Marionette and geckodriver. r=ato

	Ryan VanderMeulen [:RyanVM]
	Comment 10 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/e5660fb96ca6

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 11 • 7 years ago

Francois, I assume that we have to uplift this patch to beta?

	François Marier [:francois] Reporter
	Comment 12 • 7 years ago

(In reply to Henrik Skupin (:whimboo) from comment #11)
> Francois, I assume that we have to uplift this patch to beta?

That won't be needed for Safe Browsing since Beta 57 will work fine.

However, that might be a good idea in general since Shield could interfere with other marionette tests.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 13 • 7 years ago

Ok, backporting to beta is a no-op now given that the merge from m-c to m-b has already been done and this code is available on beta.

The question would be if an uplift to release would make sense, but I don't think we should do it. Better would be to just release a new Marionette driver package. Users of geckodriver already have the pref set with the 0.19 release went out on Saturday.

	Michael Cooper [:mythmon]
	Comment 14 • 7 years ago

Sorry for being late here, but no one informed anyone from Shield about this change until yesterday. The best pref to use for this would be extensions.shield-recipe-client.enabled, which will cleanly disable the entire system.

Setting app.shield.optoutstudies.enabled = false will only affect a portion of Shield, and will not disable network traffic.

Adjusting extensions.shield-recipe-client.api_url will change where Shield attempts to connect to, and setting it to the empty string will technically have the effect of stopping any activity, but this is not guaranteed to always be true.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 15 • 7 years ago

Mike, I just used what we have in the following file:
https://dxr.mozilla.org/mozilla-central/source/testing/profiles/prefs_general.js#328

This change went in via bug 1385177 and was reviewed by yourself. So why is it coming up to be wrong now? If that is the case I would suggest that you fix it correctly for each test harness as present in tree.

	Michael Cooper [:mythmon]
	Comment 16 • 7 years ago

In bug 1385177 the goal was to prevent network traffic. As I understand it, the goal of this bug is to disable Shield entirely during Marionette runs. Since the goals are different, it makes sense that prefs would be different as well. Have I misunderstood this bug?

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 17 • 7 years ago

Generally we also want to prevent network traffic, so that the Marionette harness behaves the same  way as other test harnesses. As such we are mainly copying the preferences from the above mentioned file. 

In regards of this bug we don't care if it is completely disabled, or just doesn't retrieve shield studies. Given your reply on this bug, does it mean that even with preventing the network access it would still be possible for the addon to run studies? I strongly belief that other harnesses also don't want that due to flakiness in the tests, and as such set this preference.

	Michael Cooper [:mythmon]
	Comment 18 • 7 years ago

It wouldn't be possible to run studies without network access, since there wouldn't be any way to fetch studies to then execute. My concern is mostly that "" is not a valid URL, where as pointing shield at a dummy service, like "https://%(server)s/" is a valid URL. This confuses Shield less, and makes my life easier. I'm not arguing we should enable network access, or that studies should be running, I'm mostly selfishly trying to save myself some work.

If you want to disable Shield entirely, the correct pref to do that is the enabled pref.

If you want to prevent Shield from contacting the network, setting api_url to "" would technically work, but it would be more polite to set it to a valid URL pointing to a test-safe dummy server.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 19 • 7 years ago

(In reply to Mike Cooper [:mythmon] from comment #18)
> It wouldn't be possible to run studies without network access, since there
> wouldn't be any way to fetch studies to then execute. My concern is mostly
> that "" is not a valid URL, where as pointing shield at a dummy service,
> like "https://%(server)s/" is a valid URL. This confuses Shield less, and
> makes my life easier. I'm not arguing we should enable network access, or
> that studies should be running, I'm mostly selfishly trying to save myself
> some work.
> If you want to disable Shield entirely, the correct pref to do that is the
> enabled pref.

The problem here is (as also described and worked on in bug 1371576) that we cannot always replace `%(server)s` with a valid URL. Simply because we do not always have a local webserver running. If you have a better solution please propose one in the before mentioned bug. I'm happy to incorporate that. And sorry if using a blank value here confuses Shield.

	Michael Cooper [:mythmon]
	Comment 20 • 7 years ago

In these situations, Shield shouldn't be running at all, so disabling it entirely is appropriate, with extensions.shield-recipe-client.enabled = false. I don't have a more clever solution for other services.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 21 • 7 years ago

I will care about it over on bug 1371576. Thanks.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 22 • 7 years ago

As we can see on bug 1399312 the test failures are still present for Firefox 56 (release branch). This patch landed for 57.

Ryan, do you think it is feasible and wanted to have an uplift of this patch for mozilla-release? Just in case we need another release, whereby we are close for 57 already.

	Ryan VanderMeulen [:RyanVM]
	Comment 23 • 7 years ago

I don't think it's worth it at this point, no.