Closed Bug 1606335 Opened 5 months ago Closed 5 months ago

Marionette no longer runs in non-e10s mode

Categories

(Testing :: Marionette, defect, P1)

Version 3
defect

Tracking

(firefox-esr68 wontfix, firefox72 wontfix, firefox73 wontfix, firefox74 fixed)

RESOLVED FIXED
mozilla74
Tracking Status
firefox-esr68 --- wontfix
firefox72 --- wontfix
firefox73 --- wontfix
firefox74 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

(Regression)

Details

(Keywords: regression)

Attachments

(2 files)

As it looks like since bug 1548941 has been fixed Marionette can no longer be started in non-e10s mode anyway:

$ mach marionette-test --disable-e10s
..
AssertionError: BaseMarionetteTestRunner configuration (self.e10s) does not match browser appinfo (self.is_e10s)

  File "/Volumes/code/gecko/testing/marionette/mach_commands.py", line 109, in marionette_test
    return run_marionette(tests, topsrcdir=self.topsrcdir, **kwargs)
  File "/Volumes/code/gecko/testing/marionette/mach_commands.py", line 58, in run_marionette
    failed = MarionetteHarness(MarionetteTestRunner, args=vars(args)).run()
  File "/Volumes/code/gecko/testing/marionette/harness/marionette_harness/runtests.py", line 71, in run
    runner.run_tests(tests)
  File "/Volumes/code/gecko/testing/marionette/harness/marionette_harness/runner/base.py", line 902, in run_tests
    raise AssertionError("BaseMarionetteTestRunner configuration (self.e10s) "

Reason is that Marionette is no longer allowed to set the preference browser.tabs.remote.autostart to false. This is because Marionette and geckodriver are not covered by xpc::IsInAutomation because they are also running outside of our tree.

Gijs, given that we got fixes for other test harnesses, I assume we should also fix Marionette?

Flags: needinfo?(gijskruitbosch+bugs)
Priority: -- → P3

Sorry, I don't understand the question because there's a load of context I don't have.

Bug 1548941 landed in May. So I have questions, like, do we currently try to run marionette in non-e10s mode on infra or locally? If so, why? And why wasn't this picked up sooner? And, does this affect e.g. the non-e10s mochitest runs that we currently run on infra, (AIUI) on top of marionette? If not, why not?

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #0)

This is because Marionette and geckodriver are not covered by xpc::IsInAutomation because they are also running outside of our tree.

This is confusing - is this only the case outside of the tree, or is isInAutomation also false when marionette/geckodriver run inside Firefox's automation? The exception from comment #0 seems to be about in-tree, so I'm confused why that wouldn't be covered by isInAutomation.

Gijs, given that we got fixes for other test harnesses, I assume we should also fix Marionette?

I'm not sure that assumption holds. Why does Marionette/Geckodriver need to run with e10s turned off?

Also, is that error actually about the behaviour caused by bug 1548941 ? We still "allow" you to set the pref you cite, it will just not have any effect. What is marionette checking in terms of configuration?

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(hskupin)

(In reply to :Gijs (back Jan 2; he/him) from comment #1)

Sorry, I don't understand the question because there's a load of context I don't have.

Bug 1548941 landed in May. So I have questions, like, do we currently try to run marionette in non-e10s mode on infra or locally? If so, why? And why wasn't this picked up sooner? And, does this affect e.g. the non-e10s mochitest runs that we currently run on infra, (AIUI) on top of marionette? If not, why not?

I found the problem 3 days ago when I noticed that we force all consumers of Marionette to run in non-e10s mode by default due to setting browser.tabs.remote.autostart to False. This should have been removed a long time ago. So I fixed that over on bug 1606325. (Note that Marionette still supports older Firefox releases not including your patch from bug 1548941)

While verifying that we can still run in both modes and that the --disable-e10s mach option can turn e10s off, I just noticed the problem. Nothing in CI uses this option, so it just happens for locally run tests. And as such we never noticed that before.

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #0)

This is because Marionette and geckodriver are not covered by xpc::IsInAutomation because they are also running outside of our tree.

This is confusing - is this only the case outside of the tree, or is isInAutomation also false when marionette/geckodriver run inside Firefox's automation? The exception from comment #0 seems to be about in-tree, so I'm confused why that wouldn't be covered by isInAutomation.

Marionette tests don't run with MOZ_DISABLE_NONLOCAL_CONNECTIONS set (bug 1272255) and as such isInAutomation is always false due to https://searchfox.org/mozilla-central/rev/331f0c3b25089c9a16be65f4dc8c601aeaac8cc4/js/xpconnect/src/xpcpublic.h#683. So yes, we hit this situation also all the time in tree. Even if Marionette would support it the Firefox ui remote test jobs, which have to allow external addresses, would still be affected.

We had a similar situation with updates over on bug 1508726

Gijs, given that we got fixes for other test harnesses, I assume we should also fix Marionette?

I'm not sure that assumption holds. Why does Marionette/Geckodriver need to run with e10s turned off?

I think it's a fair question, and we could consider not allowing that anymore. Personally I would be all for that. But I would need the buy-in from the team. Andreas, what's your take here?

Also, is that error actually about the behaviour caused by bug 1548941 ? We still "allow" you to set the pref you cite, it will just not have any effect. What is marionette checking in terms of configuration?

It ensures that the expected e10s mode is set. It does it via Services.appinfo.browserTabsRemoteAutostart:

https://searchfox.org/mozilla-central/rev/cac340f10816707e91c46db6d285f80259420f07/testing/marionette/harness/marionette_harness/runner/base.py#887-893
https://searchfox.org/mozilla-central/rev/cac340f10816707e91c46db6d285f80259420f07/testing/marionette/harness/marionette_harness/runner/base.py#845-858

Flags: needinfo?(hskupin) → needinfo?(ato)

Removing the E10s support from the harness code seems reasonable to me, since it’s not a test configuration we use or want to test on Treeherder anymore.

It used to be the case that developers would use --disable-e10s to make the debugging experience nicer, forcing everything to run within the same process. But I think the situation here has now improved.

Flags: needinfo?(ato)

While verifying that we can still run in both modes and that the --disable-e10s mach option can turn e10s off, I just noticed the problem. Nothing in CI uses this option, so it just happens for locally run tests. And as such we never noticed that before.

So to be clear, you're sure that the non-e10s mochitest plain and/or other tests that we do still run on infra don't use this switch, but this disable e10s "on their own"?

Flags: needinfo?(hskupin)

Mochitest is doing it on its own, yes. The place where it is decided is here:
https://searchfox.org/mozilla-central/rev/331f0c3b25089c9a16be65f4dc8c601aeaac8cc4/testing/mochitest/runtests.py#1922

Flags: needinfo?(hskupin)

When I find the time I will have a patch for that by latest next week. One more improvement here is that we safe us the extra startup of Firefox when removing the code block from base.py. Maybe that helps us with bug 1382162 a little bit.

Gijs, I assume the changes you did also affect mobile? Given that it renders Marionette useless for Fennec, I don't see any blockers from really removing it from central. If anyone still needs it for testing Fennec the Marionette version from the esr68 branch should be used.

Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Priority: P3 → P1

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #7)

Gijs, I assume the changes you did also affect mobile?

Unclear which mobile product you mean.

Fennec is unaffected - it will (still) run with e10s disabled. I explicitly called this out in bug 1548941 comment 0.

Geckoview is "interesting", but you can't meaningfully disable e10s there anyway, AIUI. See some of the other deps.

Given that it renders Marionette useless for Fennec,

I don't know what "it" is here, but given that the default is "no e10s" on fennec, that's probably what should be used for fennec. Whether current central marionette needs to support fennec from esr68 is not something I can answer.

I don't see any blockers from really removing it from central. If anyone still needs it for testing Fennec the Marionette version from the esr68 branch should be used.

(In reply to :Gijs (he/him) from comment #9)

Fennec is unaffected - it will (still) run with e10s disabled. I explicitly called this out in bug 1548941 comment 0.

Good to know. So even we would continue support older Fennec releases it won't be affected. I also filed bug 1607210 to get rid of Fennec support at all.

Geckoview is "interesting", but you can't meaningfully disable e10s there anyway, AIUI. See some of the other deps.

The Marionette harness doesn't support it and even won't in the future.

I don't know what "it" is here, but given that the default is "no e10s" on fennec, that's probably what should be used for fennec. Whether current central marionette needs to support fennec from esr68 is not something I can answer.

Sorry, should have been "this". But that wasn't a question, more a statement as one of the Marionette peers.

Thanks.

Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e53f0ca31d75
[marionette] Remove non-e10s support from Marionette harness. r=marionette-reviewers,ato
https://hg.mozilla.org/integration/autoland/rev/5112947294e4
[marionette] Remove e10s support from build and mozharness. r=marionette-reviewers,ato
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla74

Not sure how critical this is for uplifting, but feel free to ping if we should.

This will ride the train.

Regressions: 1607602
You need to log in before you can comment on or make changes to this bug.