1598804 - make Cu.isInAutomation set for xpcshell tests

Status: NEW

(WebExtensions :: General, task, P3)

Description

[Shane Caraveo (:mixedpuppy)]

We would like the flag set, at least for extension tests. Then we can remove an environment check done in D53981.

Comment 1

[Rob Wu [:robwu]]

The goal here is to keep a useful browser.test API without exposing dangerous functionality to extensions.

Comment 2

[Rob Wu [:robwu]]

I looked into this, and it seems that the cleanest way to do this is to add a constant that is only set at compile time in the xcpshell binary. If needed, we can decouple "is xpcshell" and "is xpcshell test" by adding an additional check for an environment variable. Possibly around https://searchfox.org/mozilla-central/rev/bc3600def806859c31b2c7ac06e3d69271052a89/js/xpconnect/src/XPCShellImpl.cpp#1204

I'm suggesting something hard-coded at compile time, because toggles in prefs, environment variables and command-line arguments can inadvertently or maliciously be set. The flag "automation mode" should never be true in normal browser usage.

Comment 3

[:Gijs (he/him)]

(In reply to Rob Wu [:robwu] from comment #2)

I looked into this, and it seems that the cleanest way to do this is to add a constant that is only set at compile time in the xcpshell binary. If needed, we can decouple "is xpcshell" and "is xpcshell test" by adding an additional check for an environment variable. Possibly around https://searchfox.org/mozilla-central/rev/bc3600def806859c31b2c7ac06e3d69271052a89/js/xpconnect/src/XPCShellImpl.cpp#1204

I'm suggesting something hard-coded at compile time, because toggles in prefs, environment variables and command-line arguments can inadvertently or maliciously be set. The flag "automation mode" should never be true in normal browser usage.

There's already an environment variable, MOZ_DISABLE_NONLOCAL_CONNECTIONS, which causes us to crash if we try to talk to anything but localhost. There aren't really useful ways of having that set and doing "normal browser usage". The gap with e.g. mochitests is that xpcshell doesn't set the let_viruses_take_over_this_computer pref. See also bug 1548941 comment 11. Changing that pref is a lot simpler than writing xpcshell-specific compiled code. You might even be able to set the pref in the xpcshell manifest for specific tests.

Either way there's likely to be fallout from code behind isInAutomation checks that doesn't expect to run in xpcshell right now.

Comment 4

[:Gijs (he/him)]

Oh, and indeed some webextension tests do exactly that, cf. https://searchfox.org/mozilla-central/rev/7b1de5e29d878cc163dec7beaf9b57a2f0f41aaa/toolkit/mozapps/extensions/test/xpcshell/test_seen.js#5-9

Comment 5

[Mathieu Leplatre [:leplatrem]]

I would also be interested in that. Currently, we detect being in XPCShell tests using Cu.isInAutomation || env.exists("XPCSHELL_TEST_PROFILE_DIR") in our code.

Comment 6

[Mark Banner (:standard8)]

Out of interest, I took a quick poke at this and pushed a patch to try: https://treeherder.mozilla.org/jobs?repo=try&revision=b230069d5330f5b903e51db1cb1718e7e9180ae2

Most of the desktop failures there are attempted loads of SessionStore.jsm or AttributionCode.jsm (and other modules) in non browser/ locations code. I think there's also things like static pref changes in various android tests and possibly some gfx items as well.

Tackling the loads issue will likely fix most of it. Fixing the loads issue might be beneficial to help prevent us accessing things we shouldn't be. Unfortunately I'm not quite sure how exactly we should handle it. I could see various ways of providing SessionStore/AttributionCode stubs, or maybe even having a special ChromeUtils.defineModuleGetter that allows for the file not existing.

Comment 7

[Mark Banner (:standard8)]

Attachment: WIP: Bug 1598804 - Make Cu.isInAutomation true for xpcshell tests (incomplete).

Comment 8

[Mark Banner (:standard8)]

Note: the patch I just attached is the one I tried in comment 6. Just adding it here for easy reference, I'm not planning on working on this.