Closed
Bug 1508726
Opened 6 years ago
Closed 6 years ago
When running Marionette tests Firefox downloads and installs updates even when turned off via prefs
Categories
(Toolkit :: Application Update, defect)
Tracking
()
RESOLVED
FIXED
mozilla65
Tracking | Status | |
---|---|---|
firefox-esr60 | --- | unaffected |
firefox63 | --- | unaffected |
firefox64 | --- | unaffected |
firefox65 | + | fixed |
People
(Reporter: whimboo, Assigned: whimboo)
References
Details
(Keywords: regression)
Attachments
(4 files, 1 obsolete file)
(deleted),
text/plain
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
Bug 1508726 - [marionette] Use "app.update.disabledForTesting" to disable application updates. r?ato
(deleted),
text/x-phabricator-request
|
Details |
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?
Flags: needinfo?(robert.strong.bugs)
![]() |
||
Comment 2•6 years ago
|
||
Kirk handled the disabling of updates in bug 1420514 via the app.update.disabledForTesting preference. Kirk might be able to answer this for you.
Flags: needinfo?(robert.strong.bugs) → needinfo?(ksteuber)
![]() |
||
Comment 3•6 years ago
|
||
Here is the relevant changeset https://hg.mozilla.org/mozilla-central/rev/7ca13add114d
Assignee | ||
Comment 4•6 years ago
|
||
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
Assignee | ||
Comment 5•6 years ago
|
||
But should it be allowed to update a debug build from autoland to a normal nightly build on central at all?
![]() |
||
Comment 6•6 years ago
|
||
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•6 years ago
|
||
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
Assignee | ||
Comment 8•6 years ago
|
||
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.
Updated•6 years ago
|
Flags: needinfo?(ksteuber)
Assignee | ||
Comment 9•6 years ago
|
||
(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•6 years ago
|
||
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.
Assignee | ||
Comment 11•6 years ago
|
||
Can we just make it the following check?
> get disabledForTesting() {
> return Cu.isInAutomation ||
> Services.prefs.getBoolPref(PREF_APP_UPDATE_DISABLEDFORTESTING, false);
> }
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(ksteuber)
Comment 12•6 years ago
|
||
(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.
Updated•6 years ago
|
Flags: needinfo?(ksteuber)
![]() |
||
Comment 13•6 years ago
|
||
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.
Assignee | ||
Comment 14•6 years ago
|
||
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.
Flags: needinfo?(ted)
Assignee | ||
Comment 15•6 years ago
|
||
(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.
Assignee | ||
Comment 16•6 years ago
|
||
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•6 years ago
|
||
ni on robert.strong to answer Comment 16.
Flags: needinfo?(robert.strong.bugs)
![]() |
||
Comment 18•6 years ago
|
||
I don't know the intricacies of Cu.isInAutomation and someone that does should answer this.
Flags: needinfo?(robert.strong.bugs)
Assignee | ||
Comment 19•6 years ago
|
||
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?
Component: Application Update → XPConnect
Flags: needinfo?(mrbkap)
Flags: needinfo?(VYV03354)
Product: Toolkit → Core
Assignee | ||
Comment 20•6 years ago
|
||
It regressed for us a really long time ago, which is Firefox 49.
Blocks: 1277691
Comment 21•6 years ago
|
||
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.
Flags: needinfo?(VYV03354)
Comment 22•6 years ago
|
||
If Marionette wants to disable updates, it should be specially checked instead of changing isInAutomation blanketly.
Assignee | ||
Comment 23•6 years ago
|
||
(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.
Flags: needinfo?(VYV03354)
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 24•6 years ago
|
||
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.
Depends on: 1509256
Assignee | ||
Comment 25•6 years ago
|
||
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.
tracking-firefox65:
--- → ?
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 26•6 years ago
|
||
The underlying regression bug for Firefox 65 is bug 1458308.
Blocks: update-prefs
Assignee | ||
Comment 27•6 years ago
|
||
Boris, I missed to set the needinfo for you. Maybe you could have a look at comment 25. Thanks.
Flags: needinfo?(bzbarsky)
![]() |
||
Comment 28•6 years ago
|
||
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.
Flags: needinfo?(bzbarsky) → needinfo?(bobbyholley)
Comment 29•6 years ago
|
||
(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.
Flags: needinfo?(mrbkap)
Updated•6 years ago
|
Assignee | ||
Comment 30•6 years ago
|
||
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?
Flags: needinfo?(ted)
Flags: needinfo?(ksteuber)
Flags: needinfo?(VYV03354)
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Component: XPConnect → Application Update
Product: Core → Toolkit
![]() |
||
Comment 32•6 years ago
|
||
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•6 years ago
|
||
> 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•6 years ago
|
||
(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•6 years ago
|
||
+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.
Flags: needinfo?(bobbyholley)
Assignee | ||
Comment 36•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=29cfb90c4c12fe1e2752b41d415d8dbb8d3ba022
Assignee | ||
Comment 37•6 years ago
|
||
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.
Assignee | ||
Comment 38•6 years ago
|
||
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
Assignee | ||
Comment 39•6 years ago
|
||
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
Assignee | ||
Comment 40•6 years ago
|
||
Assignee | ||
Comment 41•6 years ago
|
||
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•6 years ago
|
||
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ffc42da8138e Prevent application updates when Marionette is enabled. r=bytesized
Comment 43•6 years ago
|
||
Pushed by hskupin@mozilla.com: 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•6 years ago
|
||
Backout by csabou@mozilla.com: 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•6 years ago
|
||
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ae2c582146e7 [marionette] Use "app.update.disabledForTesting" to disable application updates. r=ato
Comment 46•6 years ago
|
||
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f4d2799a8993 [geckodriver] Use "app.update.disabledForTesting" to disable application updates. r=ato
Comment 47•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ae2c582146e7 https://hg.mozilla.org/mozilla-central/rev/f4d2799a8993
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Updated•6 years ago
|
status-firefox-esr60:
--- → unaffected
Assignee | ||
Updated•4 years ago
|
Updated•4 years ago
|
Attachment #9028927 -
Attachment is obsolete: true
You need to log in
before you can comment on or make changes to this bug.
Description
•