1508726 - When running Marionette tests Firefox downloads and installs updates even when turned off via prefs

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Diff view of original and modified Firefox application bundle

Today I tried to investigate the problem on https://bugzilla.mozilla.org/show_bug.cgi?id=1507803#c12, and as such downloaded such a debug build from autoland.

Running the Marionette tests I can see that Firefox checks for updates, downloads those, and even installs them when a test restarts Firefox! This is not acceptable, especially because all prefs should have turned updates off!

https://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette_driver/geckoinstance.py#498-502

So in my case the debug build got updated to an opt build on mozilla-central! That's what diff tells me when running it afterward on the original and modified Firefox application bundle directory. Should that ever be possible?

I didn't test builds from beta yet but could do later if that is necessary.

Robert, why does that happen? Is any of the recent changes responsible for that?

Comment 1

[Andreas Tolfsen ❲:ato❳]

See also https://hg.mozilla.org/mozilla-central/rev/663e1f142dd4.

Comment 2

[Robert Strong (they/them - no direct email)]

Kirk handled the disabling of updates in bug 1420514 via the app.update.disabledForTesting preference. Kirk might be able to answer this for you.

Comment 3

[Robert Strong (they/them - no direct email)]

Here is the relevant changeset
https://hg.mozilla.org/mozilla-central/rev/7ca13add114d

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I tried to narrow it down but currently I only get the following failures for the following update URL:

https://aus5.mozilla.org/update/6/Firefox/65.0a1/20181119100448/Darwin_x86_64-gcc3/en-US/nightly/Darwin%2017.7.0/ISET:SSE4_2,MEM:16384/default/default/update.xml?force=1

> *** AUS:SVC Checker:onLoad - there was a problem checking for updates. Exception: TypeError: this._request is null; can't access its "responseXML" property
> *** AUS:SVC Checker:onLoad - request.status: 200
> *** AUS:SVC getStatusTextFromCode - transfer error: Update XML file malformed (200), code: 200
> JavaScript error: jar:file:///Volumes/data/code/gecko/Firefox%20Nightly.app/Contents/Resources/omni.ja!/components/nsUpdateService.js, line 3413: TypeError: this._callback is null; can't access its "onError" property

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

But should it be allowed to update a debug build from autoland to a normal nightly build on central at all?

Comment 6

[Robert Strong (they/them - no direct email)]

I don't think that autoland shouldn't update but that should be controlled during the build. iirc in the past it was by setting the channel name to a name different than m-c but that was a long time ago. Someone from build should be able to help you with that.

Comment 7

[Robin Steuber (they/them) [:bytesized]]

I'm looking to see if I can reproduce this, but in the meantime, here is how this system is supposed to work:

  Marionette tests set two prefs, here [1]
  Automation sets the `MOZ_DISABLE_NONLOCAL_CONNECTIONS` env value, here [2] (I think)
  IsInAutomation checks one of those prefs [3] and the env value[4]
  `UpdateService.disabledForTesting` [5] returns `true`, causing `UpdateService.disabledByPolicy` [6] to return true
  This disables update the same way that the enterprise policy AppUpdateDisabled does, which is to say, completely.

[1] https://searchfox.org/mozilla-central/rev/55895c49f55073d82d977cb74ec1d3a71ae4b25f/testing/marionette/components/marionette.js#64-65
[2] https://searchfox.org/mozilla-central/rev/55895c49f55073d82d977cb74ec1d3a71ae4b25f/build/automation.py.in#208
[3] https://searchfox.org/mozilla-central/rev/55895c49f55073d82d977cb74ec1d3a71ae4b25f/js/xpconnect/src/xpcpublic.h#709
[4] https://searchfox.org/mozilla-central/rev/55895c49f55073d82d977cb74ec1d3a71ae4b25f/js/xpconnect/src/xpcpublic.h#694
[5] https://searchfox.org/mozilla-central/rev/55895c49f55073d82d977cb74ec1d3a71ae4b25f/toolkit/mozapps/update/nsUpdateService.js#2457
[6] https://searchfox.org/mozilla-central/rev/55895c49f55073d82d977cb74ec1d3a71ae4b25f/toolkit/mozapps/update/nsUpdateService.js#2464

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So the problem here is:
https://searchfox.org/mozilla-central/rev/55895c49f55073d82d977cb74ec1d3a71ae4b25f/toolkit/mozapps/update/nsUpdateService.js#2458

When marionette is run via it's cli command `Cu.isInAutomation` is false, and as such will always enable the update mechanism for Marionette tests.

So this is a regression from bug 1420514, and goes all the way back to Firefox 63! :(

I assume that also all of our customers of geckodriver are affected by that behavior, and get updates shipped. But thankfully not while tests are executed given that restarts aren't allowed there yet.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Kirk Steuber (he/him) [:bytesized] from comment #7)
>   Marionette tests set two prefs, here [1]
>   Automation sets the `MOZ_DISABLE_NONLOCAL_CONNECTIONS` env value, here [2]

No! This environment variable is not set whether by Marionette nor by geckodriver! As such we can NOT use it to determine if Firefox is under automation or not. See my comment which I already gave on the regression bug months ago (bug 420514 comment 52)

Comment 10

[Andreas Tolfsen ❲:ato❳]

MOZ_DISABLE_NONLOCAL_CONNECTIONS is for historical reasons not used
by Marionette tests (turning it on is tracked in
https://bugzilla.mozilla.org/show_bug.cgi?id=1272255), but we
shouldn’t discount that Marionette is being used by consumers out
of tree.

geckodriver uses Marionette to implement WebDriver but this doesn’t
set MOZ_DISABLE_NONLOCAL_CONNECTIONS for obvious reasons.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Can we just make it the following check?

> get disabledForTesting() {
>    return Cu.isInAutomation ||
>           Services.prefs.getBoolPref(PREF_APP_UPDATE_DISABLEDFORTESTING, false);
> }

Comment 12

[Robin Steuber (they/them) [:bytesized]]

(In reply to Henrik Skupin (:whimboo) from comment #11)
> Can we just make it the following check?
> 
> > get disabledForTesting() {
> >    return Cu.isInAutomation ||
> >           Services.prefs.getBoolPref(PREF_APP_UPDATE_DISABLEDFORTESTING, false);
> > }

That would not be my preferred solution to this problem. We removed `app.update.enabled`, in large part, to prevent people from setting a value and then forgetting to ever update again. The change that you are proposing would reintroduce a pref that allows exactly this. It is intended to be a testing-only pref, and I would really like to keep it this way.

Comment 13

[Robert Strong (they/them - no direct email)]

The decision to remove the ability for clients to disable updates came from product. That's not something we can change and tests will need to accommodate this.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Then eg. a different environment variable would be necessary beside `MOZ_DISABLE_NONLOCAL_CONNECTIONS` to inform Firefox that it is run under automation. As we said multiple times in Marionette and geckodriver we cannot set this environment variable because people are specifically testing their websites, which are not hosted on localhost.

Ted, do you know if there is already an existing environment variable which could be used here to disable application updates for Marionette tests? Also see comment 8.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #6)
> I don't think that autoland shouldn't update but that should be controlled
> during the build. iirc in the past it was by setting the channel name to a

This is actually also a regression bug but in the build system. FYI I filed it as bug 1508836.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Maybe we can use something from bug 1169290 like the nsIMarionette interface to determine if Firefox is under automation, and set `Cu.isInAutomation` properly?

Comment 17

[Marcia Knous [:marcia]]

ni on robert.strong to answer Comment 16.

Comment 18

[Robert Strong (they/them - no direct email)]

I don't know the intricacies of Cu.isInAutomation and someone that does should answer this.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I had a look where `isInAutomation` is set, and the problem is located here:

https://searchfox.org/mozilla-central/rev/8f89901f2d69d9783f946a7458a6d7ee70635a94/js/xpconnect/src/xpcpublic.h#720

> return sAutomationPrefIsSet && AreNonLocalConnectionsDisabled();

The changes which affected Marionette happened on bug 1277691 and bug 1278250 by Masatoshi.

I wonder if we can relax that a bit, and do not return true/false based on the value of the environment variable, but based that the env variable is set or not? See the previous comments in why we would need that.

Masatoshi, and Blake, what do you think?

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

It regressed for us a really long time ago, which is Firefox 49.

Comment 21

[Masatoshi Kimura [:emk]]

I added the MOZ_DISABLE_NONLOCAL_CONNECTIONS check to `isInAutomation` because it is used to block super-danger features such as `enablePrivilege` outside test suites. I'm not confortable with opening such a huge security hole in production.

Comment 22

[Masatoshi Kimura [:emk]]

If Marionette wants to disable updates, it should be specially checked instead of changing isInAutomation blanketly.

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Masatoshi Kimura [:emk] from comment #21)
> I added the MOZ_DISABLE_NONLOCAL_CONNECTIONS check to `isInAutomation`
> because it is used to block super-danger features such as `enablePrivilege`
> outside test suites. I'm not confortable with opening such a huge security
> hole in production.

I'm not saying that we should remove that, but simply ignore its value. As such enable the automation flag based on if the environment variable is set.

(In reply to Masatoshi Kimura [:emk] from comment #22)
> If Marionette wants to disable updates, it should be specially checked
> instead of changing isInAutomation blanketly.

Not sure if I understand that comment correctly. But are you saying that with Marionette enabled `isInAutomation` should not be true? Note that Marionette is a test harness for automation, and its server component is baked into Firefox. As already mentioned above it is not restricted on purpose to only open local pages.

So which other option would you see? Shall we use the nsIMarionette interface to additionally check if Marionette is active, and include it in the condition of `isInAutomation`?

Putting a different check for Marionette somewhere else would assume that Marionette tests are still not treated as automation in Firefox, which is wrong.

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So I found a quick solution for now which helps us to not upgrade Firefox for users while running their tests with Marionette/geckodriver. As noticed on bug 1509256 also the preference `app.update.auto` has been removed. And given that `app.update.enabled`, and `app.update.disabledForTesting` are not evaluated as expected, the download of updates happen automatically.

As such bug 1509256 has to re-add `app.update.auto` set to `false`, and once fixed we can continue on this bug.

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So with the fix on bug 1509256 landed we have a band-aid workaround in-place which at least fixes the problem for our users from Firefox 57 to 64 (which are the supported releases by geckodriver). For Firefox 65 we will need a proper fix now.

I had a quick discussion with Kirk yesterday which was specifically about the updater component, but applying that to XPCOM might also be feasible. 

Basically `Cu.isInAutomation` is a flag, which should be set to `true` when Firefox is under automation. That should be independent from whatever test framework is in use, and as we know this works fine for eg. mochitests, reftests, and others. But actually not for Marionette which is special, given that its code is baked into Firefox as a component.

Lets have a look at the definition of `isInAutomation`:
https://searchfox.org/mozilla-central/rev/f2028b4c38bff2a50ed6aa1763f6dc5ee62b0cc4/js/xpconnect/src/xpcpublic.h#708-721

Here you can see that a per-requisite is a preference called `security.turn_off_all_security_so_that_viruses_can_take_over_this_computer`, which has to be set by any of those other test harnesses to make `enablePrivilege` and specialPowers to work. Hereby note that Marionette doesn't need a single usage of this method, and as such will never have to set this obscure preference. Based by that `sAutomationPrefIsSet` is always false. 

Further `AreNonLocalConnectionsDisabled()` is evaluated and taken into account. As we mentioned above this is a feature Marionette will never be able to use. And yes, this is on purpose. Marionette is special, and compared to (nearly) all other test harnesses also used outside of the tree - like in combination with geckodriver and Selenium.

My proposal would now be to have an `or` check beside the two flags from above in `isInAutomation`. Which could look like that way:

> return (sAutomationPrefIsSet && AreNonLocalConnectionsDisabled()) or isMarionetteRunning;

This could be implemented by using the nsIMarionette interface, which was added for Firefox 55 and contains a `running` attribute:

https://searchfox.org/mozilla-central/source/testing/marionette/components/nsIMarionette.idl#16

I don't know if we would have access to this interface in xpcpublic.h, to get a check implemented. Feedback therefore would be welcome.

Also I would need some feedback from product, and/or those people who know the security implications by that.

Boris or Blake, do you have an advise? Maybe you can already help with it? 

Thanks a lot.

I'm flagging this for Firefox 65 now, because for us it's a blocker to not ship updates of Firefox to our testing users.

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The underlying regression bug for Firefox 65 is bug 1458308.

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Boris, I missed to set the needinfo for you. Maybe you could have a look at comment 25. Thanks.

Comment 28

[Boris Zbarsky [:bzbarsky]]

That sounds more like a question for Bobby.

But in general the isInAutomation value means "Firefox CI test automation, where we control and trust all the code that's running".  It does not mean "any sort of automation setup".  It should probably not be set for random people running tests on their websites via marionette, and _definitely_ not set for random people running tests on someone else's websites.

When isInAutomation is true, at the very least we:

1) Expose the JS testing functions on workers.  These expose all sorts of unsafe behavior.

2) Allow enablePrivilege.  This does NOT depend on the "turn_off_all_security" pref, afaict, apart from its effects on isInAutomation.  We'd like to get rid of 

For purposes of Marionette, I think we should just have a preference _and_ env var that need to be set together to disable updates and have Marionette set them.  So basically another boolean with precautions similar to isInAutomation but having the semantics we want instead of trying to redefine isInAutomation.

Comment 29

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #28)
> "any sort of automation setup".  It should probably not be set for random
> people running tests on their websites via marionette, and _definitely_ not
> set for random people running tests on someone else's websites.

I think this is key. A Gecko browser loading a page over a non-local connection when isInAutomation is true is just asking for that page to exploit the computer that's running the browser. There has to be a separate flag for Marionette to turn off updates.

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I see. So I have misunderstood what isInAutomation actually mean. The name is a bit confusing if you aren't aware of the actually use case.

Ok, so shall we still add something to xpcpublic.h for Marionette, or trying to avoid this file completely? For me the latter case sounds more ideal, and I would just add the `nsIMarionette.running` check to the application updater code here:

https://searchfox.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#2435

Like:

>    return (Cu.isInAutomation || marionette.running) &&
>           Services.prefs.getBoolPref(PREF_APP_UPDATE_DISABLEDFORTESTING, false);
 

Kirk, I think that this is what you also meant yesterday?

Comment 31

[Robin Steuber (they/them) [:bytesized]]

Yes, this is exactly what I had in mind.

Comment 32

[Robert Strong (they/them - no direct email)]

Henrik, you still need to come up with something that Marionette sets that clients wouldn't set similar to IsInAutomation that application update can use to determine that Marionette is running. Once that is done then an application update bug can be filed to add that one check as in comment #30 either in that bug or a new bug.

Comment 33

[Boris Zbarsky [:bzbarsky]]

> so shall we still add something to xpcpublic.h for Marionette

Unless you want xpconnect behavior to change based on whether marionette is being used, don't add stuff to xpcpublic. I suspect you don't want to touch this file.

Comment 34

[Robert Strong (they/them - no direct email)]

(In reply to Robert Strong (Robert he/him) [:rstrong] (use needinfo to contact me) from comment #32)
> Henrik, you still need to come up with something that Marionette sets that
> clients wouldn't set similar to IsInAutomation that application update can
> use to determine that Marionette is running. Once that is done then an
> application update bug can be filed to add that one check as in comment #30
> either in that bug or a new bug.
Kirk informed me that nsIMarionette running was already implemented. Yay! As long as that covers all of the Marionette use cases then that should be good to go.

Comment 35

[Bobby Holley (:bholley)]

+1 to what both Blake and Boris have said. Let me know if any other information is needed from me, and please get review from me if you do touch xpconnect.

Comment 36

[Henrik Skupin [:whimboo][⌚️UTC+1]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=29cfb90c4c12fe1e2752b41d415d8dbb8d3ba022

Comment 37

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1508726 - Prevent application updates when Marionette is enabled. r?bytesized

Marionette is a remote protocol which allows to automate the
application. Because it is not only run in CI automation but
also by users outside of the tree, the flag "Cu.isInAutomation"
cannot be used to determine if updates can be downloaded and
installed.

But it can b achieved by relying on the "running" state as
exposed by Marionette through the nsIMarionette interface.

Comment 38

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1508726 - [geckodriver] Use "app.update.disabledForTesting" to disable application updates. r?ato

Until Firefox 64 the preference "app.update.auto" controlled if updates
can be downloaded and applied. This changed in Firefox 65, and now the
preference "app.update.disabledForTesting" controls it.

Depends on D13515

Comment 39

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1508726 - [marionette] Use "app.update.disabledForTesting" to disable application updates. r?ato

Until Firefox 64 the preference "app.update.auto" controlled if updates
can be downloaded and applied. This changed in Firefox 65, and now the
preference "app.update.disabledForTesting" controls it.

Depends on D13516

Comment 40

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1508726 - Temporary fix from Bug 1508726 to allow overriding of preferences.

Comment 41

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Because trees are currently closed I had to temporarily add the fix for bug 1511311 to this queue to be able to finish the patch. Here a new try build for Windows only, and the new Marionette unit test which checks that updates are disabled by default included:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=26f876ff1907e1b037a24c5e232e4633049deccd

The last try already looked good and on all platforms canCheckForUpdates returns false, and doesn't allow updates for Marionette.

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ffc42da8138e
Prevent application updates when Marionette is enabled. r=bytesized

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/766ba8a6c875
[geckodriver] Use "app.update.disabledForTesting" to disable application updates. r=ato
https://hg.mozilla.org/integration/autoland/rev/5c59407599f6
[marionette] Use "app.update.disabledForTesting" to disable application updates. r=ato

Comment 44

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f1f136ea674c
Backed out 2 changesets as requested by whimboo for TestMarionette.test_application_update_disabled permafails.

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ae2c582146e7
[marionette] Use "app.update.disabledForTesting" to disable application updates. r=ato

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f4d2799a8993
[geckodriver] Use "app.update.disabledForTesting" to disable application updates. r=ato

Comment 47

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/ae2c582146e7
https://hg.mozilla.org/mozilla-central/rev/f4d2799a8993