1849972 - [wdspec] Tests have to run with "MOZ_DISABLE_NONLOCAL_CONNECTIONS" set

Status: RESOLVED FIXED

(Testing :: web-platform-tests, defect, P1)

Description

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

By surprise I noticed today that for WebDriver tests we currently remove the already set MOZ_DISABLE_NONLOCAL_CONNECTIONS environment variable:

https://searchfox.org/mozilla-central/rev/4e22b886bbd4c274f268c4a285ab7da00e95c99b/testing/web-platform/tests/tools/wptrunner/wptrunner/browsers/firefox.py#901-905

The reason is: in those cases we get the default geckodriver profile which doesn't guarantee zero network access.

That is actually not true. We worked hard on getting Marionette tests running with this environment variable set and this also applies to geckodriver which shares the same preferences to disable certain features.

If there is a crash happening with geckodriver in WebDriver tests we should certainly figure out why that is happening and disabling the feature.

Also removing this environment variable actually makes the Wdspec tests not a Tier-1 test suite, but we are running as Tier-1 for quite some time.

Note that removing this environment variable actually has an influence to the Cu.isInAutomation check as well and triggers crashes when we are trying to send widget level events (see bug 1848957)

As such lets make sure that we can keep the environment variable set for wdspec tests. Checking the history the lines got added on bug 1678044 for Firefox 90.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1678044

:jgraham, since you are the author of the regressor, bug 1678044, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 2

[James Graham [:jgraham]]

I don't think this is a regression. Before that change we didn't set any custom environment variables at all for wdspec tests i.e. we've never set this variable.

Assuming you're correct and with a default profile geckodriver no longer does any network access it should simply be safe to remove that block of code.

Comment 3

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

As it looks like there is only a single pref that is not yet set via recommended preferences in Remote Agent. Also some tests use example.com which is not a valid wpt host. Changing the latter to use example.org should probably do it.

Comment 4

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

Attachment: Bug 1849972 - [wptrunner] Do not remove "MOZ_DISABLE_NONLOCAL_CONNECTIONS" for wdspec tests.

Comment 5

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

Attachment: Bug 1849972 - [wdspec] Only use local network in WebDriver BiDi's reload.py.

Depends on D186823

Comment 6

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

Attachment: Bug 1849972 - [remote] Sync preferences from Marionette to disable services trying to connect to remote addresses.

Depends on D186824

Comment 7

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

Attachment: Bug 1849972 - [geckodriver] Disable updates for SafeBrowsing.

Depends on D186825

Comment 8

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

We did the same for Marionette over on bug 1371576. As such setting the same prefs so that we can share that between geckodriver and WebDriver BiDi would be a good idea.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/24b824e226c5
[wdspec] Only use local network in WebDriver BiDi's reload.py. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/57929c2e184a
[remote] Sync preferences from Marionette to disable services trying to connect to remote addresses. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/d8ac28803dd6
[wptrunner] Do not remove "MOZ_DISABLE_NONLOCAL_CONNECTIONS" for wdspec tests. r=jdescottes

Comment 10

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/41661 for changes under testing/web-platform/tests

Comment 11

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/24b824e226c5
https://hg.mozilla.org/mozilla-central/rev/57929c2e184a
https://hg.mozilla.org/mozilla-central/rev/d8ac28803dd6

Comment 12

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:whimboo, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox118 to wontfix.

For more information, please visit BugBot documentation.

Comment 14

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

The upstream PR has finally been merged!

Lets wait a couple more days for possible regressions of this change. If it's all still fine by next week I'll request an uplift so that wdspec is a Tier-1 for sure.

Comment 15

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by sideshowbarker

Comment 16

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

Given the performance regressions this landing has been caused and the findings (bug 1850950 comment 16) what's still missing I'm asking for a backout of this patch. For a next landing we have to get two things fixed and make sure to not cause perf regressions again.

Sheriffs can you please backout all the changesets? Thanks!

Comment 17

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/41862 for changes under testing/web-platform/tests

Comment 18

[Norisz Fay [:noriszfay]]

Backed out 57929c2e184a & d8ac28803dd6 as requested by whimboo
Backout link: https://hg.mozilla.org/integration/autoland/rev/24c5f0ac0033f113e83081775c276aa128156462

Comment 19

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 20

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

I've pushed a try build with the preference change for safebrowsing in the default profile for Firefox (see bug 1850950) included:
https://treeherder.mozilla.org/jobs?repo=try&revision=45010ad3bf58e5c9575f59b66d0fb4c438fd95e9

The wdspec test webdriver/tests/classic/new_session/default_values.py | test_no_capabilities should still fail in that try build due to an expected crash. As such I'll disable the test for now and we can try to get it running again via bug 1852252.

Comment 21

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

Attachment: Bug 1849972 - [wdspec] Improve stability for desired and no capabilities new session tests.

Depends on D186825

Comment 22

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

Attachment: Bug 1849972 - [wdspec] mozilla/tests/webdriver/classic/new_session/binary.py has to correctly pass capabilities.

Depends on D187780

Comment 23

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

With the recent patches there is no performance regression visible. So nothing should block us from landing those patches now!

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d418c3caa7de
[remote] Sync preferences from Marionette to disable services trying to connect to remote addresses. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/43dd7fd52252
[wdspec] Improve stability for desired and no capabilities new session tests. r=webdriver-reviewers,jgraham
https://hg.mozilla.org/integration/autoland/rev/7949e9eb4564
[wdspec] mozilla/tests/webdriver/classic/new_session/binary.py has to correctly pass capabilities. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/6b6020b8d61f
[wptrunner] Do not remove "MOZ_DISABLE_NONLOCAL_CONNECTIONS" for wdspec tests. r=jdescottes

Comment 25

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/41919 for changes under testing/web-platform/tests

Comment 26

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/d418c3caa7de
https://hg.mozilla.org/mozilla-central/rev/43dd7fd52252
https://hg.mozilla.org/mozilla-central/rev/7949e9eb4564
https://hg.mozilla.org/mozilla-central/rev/6b6020b8d61f

Comment 27

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 28

[Andra Esanu (needinfo me)]

(In reply to Norisz Fay [:noriszfay] from comment #18)

Backed out 57929c2e184a & d8ac28803dd6 as requested by whimboo
Backout link: https://hg.mozilla.org/integration/autoland/rev/24c5f0ac0033f113e83081775c276aa128156462

== Change summary for alert #39482 (as of Tue, 12 Sep 2023 07:07:38 GMT) ==

Regressions:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
10%	fandom loadtime	macosx1015-64-shippable-qr	cold fission webrender	886.09 -> 977.43	Before/After
7%	fandom loadtime	linux1804-64-shippable-qr	bytecode-cached fission warm webrender	201.70 -> 215.23	Before/After
6%	fandom SpeedIndex	linux1804-64-shippable-qr	bytecode-cached fission warm webrender	204.22 -> 215.48	Before/After
5%	fandom PerceptualSpeedIndex	linux1804-64-shippable-qr	fission warm webrender	232.22 -> 244.89	Before/After

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
5%	fandom ContentfulSpeedIndex	linux1804-64-shippable-qr	bytecode-cached cold fission webrender	711.90 -> 676.97	Before/After
4%	fandom LastVisualChange	linux1804-64-shippable-qr	cold fission webrender	928.75 -> 892.22	Before/After
4%	fandom ContentfulSpeedIndex	linux1804-64-shippable-qr	cold fission webrender	708.12 -> 681.49	Before/After
4%	fandom LastVisualChange	linux1804-64-shippable-qr	cold fission webrender	928.67 -> 894.38	Before/After
2%	wikia ContentfulSpeedIndex	windows10-64-shippable-qr	fission warm webrender	796.85 -> 780.28
2%	imgur fcp	windows10-64-shippable-qr	fission warm webrender	424.37 -> 415.83	Before/After

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=39482

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/be02ed103cf0
[wpt PR 41661] - [Gecko Bug 1849972] [wdspec] Only use local network in WebDriver BiDi's reload.py., a=testonly

Comment 30

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

As discussed with perf sheriffs there is no action to take.

Comment 31

[Cosmin Sabou [:CosminS]]

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

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Do we need this on ESR?

Comment 33

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

Comment on attachment 9350185 [details]
Bug 1849972 - [wptrunner] Do not remove "MOZ_DISABLE_NONLOCAL_CONNECTIONS" for wdspec tests.

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Affects tests and makes sure that we fulfill the Tier1 requirements for wdspec tests by causing crashes of Firefox when trying to access a remote address.

Beside the patches on this bug we also need bug 1821981 and bug 1853147.

• User impact if declined: No user impact.
• Fix Landed on Version: 119
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Only changes some default values of preferences to not cause Firefox to reach external network addresses.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9350185 [details]
Bug 1849972 - [wptrunner] Do not remove "MOZ_DISABLE_NONLOCAL_CONNECTIONS" for wdspec tests.

test-only change, doesn't need approval

Comment 35

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/fd600df7f05f
https://hg.mozilla.org/releases/mozilla-esr115/rev/4a3e1107ae2d
https://hg.mozilla.org/releases/mozilla-esr115/rev/1fd932656221
https://hg.mozilla.org/releases/mozilla-esr115/rev/e8660da6a427