Ensure that Firefox doesn't crash for Marionette tests when MOZ_DISABLE_NONLOCAL_CONNECTIONS is set
Categories
(Remote Protocol :: Marionette, enhancement, P3)
Tracking
(firefox98 fixed)
Tracking | Status | |
---|---|---|
firefox98 | --- | fixed |
People
(Reporter: whimboo, Assigned: jdescottes)
References
Details
Attachments
(4 files, 1 obsolete file)
Reporter | ||
Updated•7 years ago
|
Comment 1•7 years ago
|
||
Reporter | ||
Comment 2•7 years ago
|
||
Comment 3•7 years ago
|
||
Reporter | ||
Updated•7 years ago
|
Reporter | ||
Updated•7 years ago
|
Reporter | ||
Comment 4•7 years ago
|
||
Reporter | ||
Comment 5•7 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 8•7 years ago
|
||
Reporter | ||
Comment 9•7 years ago
|
||
Comment 10•7 years ago
|
||
Reporter | ||
Comment 11•7 years ago
|
||
Comment 12•7 years ago
|
||
Updated•7 years ago
|
Reporter | ||
Comment 13•7 years ago
|
||
Reporter | ||
Comment 14•7 years ago
|
||
Updated•7 years ago
|
Reporter | ||
Comment 16•7 years ago
|
||
Reporter | ||
Updated•5 years ago
|
Reporter | ||
Updated•5 years ago
|
Reporter | ||
Comment 17•3 years ago
|
||
With the help of the Gecko profiler it's easier to see which external connections Firefox still makes use of when using Marionette. Here a profile that I just created: https://share.firefox.dev/3Aeoo7U
I'll have a look over the next days to get this fixed. Once done we should set the environment variable for the Marionette related jobs.
Assignee | ||
Comment 18•3 years ago
|
||
Updated•3 years ago
|
Assignee | ||
Comment 19•3 years ago
|
||
Assignee | ||
Comment 20•3 years ago
|
||
Hi Francois
I have a follow up question to a very old thread from this bug (comment #4 to comment #12). We can disable requests related to tracking protection / safe browsing, but it requires flipping many preferences (I have 14 so far).
It simplify this if we had a single preference to turn off the feature, as done on the WIP patch at https://phabricator.services.mozilla.com/D136849 for example. Would it be ok to introduce such a preference?
(I know I would also have to read the pref on Android around https://searchfox.org/mozilla-central/rev/02bd3e02b72d431f2c600a7328697a521f87d9b6/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/ContentBlocking.java#928 just didn't do it for the WIP)
Thanks!
Comment 21•3 years ago
|
||
Dimi would be the right person to ask since he's the module owner for Safe Browsing.
Comment 22•3 years ago
•
|
||
Hi Julian,
Adding a pref in SafeBrowsing.jsm
doesn't get rid of sending request to production servers entirely.
Could you help check whether setting the following three preferences to empty string also works? (without adding the pref that turns off all features)
browser.safebrowsing.provider.google4.updateURL
-browser.safebrowsing.provider.google4.gethashURL
browser.safebrowsing.provider.mozilla.updateURL
Thanks!
Assignee | ||
Comment 23•3 years ago
|
||
(In reply to Dimi Lee [:dimi] from comment #22)
Hi Julian,
Adding a pref inSafeBrowsing.jsm
doesn't get rid of sending request to production servers entirely.
Could you help check whether setting the following three preferences to empty string also works? (without adding the pref that turns off all features)
browser.safebrowsing.provider.google4.updateURL
-browser.safebrowsing.provider.google4.gethashURL
browser.safebrowsing.provider.mozilla.updateURL
Thanks!
Thanks for the feedback! Overriding URLs works and this was my initial idea as well : https://phabricator.services.mozilla.com/D136528?id=530375#inline-751976 . I had a few more URLs overridden ( google.updateURL and google.gethashURL, legacy only, maybe irrelevant, and mozilla.gethashURL).
The issue with this is that we still have one test which expects safe browsing to work: https://searchfox.org/mozilla-central/source/testing/firefox-ui/tests/functional/safebrowsing/test_initial_download.py (see also comment #10). So my goal was to find a way to disable safe browsing / tracking protection only using boolean flags, because they are easy to flip back on for this specific test...
Ideally all marionette tests should be able to run without performing network requests, but for now we have to find a solution which allows us to disable network requests for all marionette tests... except the remote firefox-ui functional tests.
With that in mind, do you think disabling features individually (as done in https://phabricator.services.mozilla.com/D136528#C4693332NL37) is the best approach or would you still suggest to use the preferences for URLs? I imagine I can modify the way the test harness works in order to avoid setting the safe browsing preferences only for the remote firefox-ui functional tests if that is the preferred approach.
Comment 24•3 years ago
|
||
(In reply to Julian Descottes [:jdescottes] from comment #23)
(In reply to Dimi Lee [:dimi] from comment #22)
With that in mind, do you think disabling features individually (as done in https://phabricator.services.mozilla.com/D136528#C4693332NL37) is the best approach or would you still suggest to use the preferences for URLs? I imagine I can modify the way the test harness works in order to avoid setting the safe browsing preferences only for the remote firefox-ui functional tests if that is the preferred approach.
I think it is okay to add something like browser.safebrowsing.update.disabled
to disable update for all features (the naming browser.safebrowsing.disabled
might be misleading because we only disable update here, not lookup). The benefit of using single pref is that we don't have to keep updating code here if we support more features in the future.
If you plan to add the pref, I'll suggest doing this in readPrefs
https://searchfox.org/mozilla-central/rev/02bd3e02b72d431f2c600a7328697a521f87d9b6/toolkit/components/url-classifier/SafeBrowsing.jsm#423-430
so it may look like:
let update = Services.prefs.getBoolPref("browser.safebrowsing.update.enabled", true);
this.features = [];
...
update: FEATURES[i].update() && update,
};
...
Note. The above code doesn't disable lookup for Safe Browsing, but since there is no table being download, lookup request will not be triggered. But it is still possible we send a lookup request if somehow we have local SafeBrowsing files while running these tests.
Assignee | ||
Comment 25•3 years ago
|
||
Sounds like a good option, thanks for the help!
Updated•3 years ago
|
Assignee | ||
Comment 26•3 years ago
|
||
Assignee | ||
Comment 27•3 years ago
|
||
Hi Joel,
In https://phabricator.services.mozilla.com/D136528#4455996 :whimboo suggested forcing MOZ_DISABLE_NONLOCAL_CONNECTIONS via taskcluster configuration for all marionette based tests except firefox-ui remote. As far as I know that means:
- taskcluster/ci/test/marionette.yml
- taskcluster/ci/test/firefox-ui.yml
But I am not sure how I can force an environment variable for those tasks. I have seen other tasks defining environment variables in worker
or run
, but trying to add a run
section doesn't work. https://treeherder.mozilla.org/logviewer?job_id=365507595&repo=try&lineNumber=1797
What would be the best way to set an environment variable for those tasks?
Thanks
Reporter | ||
Comment 28•3 years ago
|
||
That indeed doesn't seem to work. So in this case you can make use of the mozharness scripts instead:
https://searchfox.org/mozilla-central/source/testing/mozharness/scripts/marionette.py
https://searchfox.org/mozilla-central/source/testing/mozharness/mozharness/mozilla/testing/firefox_ui_tests.py
https://searchfox.org/mozilla-central/source/testing/mozharness/scripts/telemetry/telemetry_client.py
Reporter | ||
Comment 29•3 years ago
|
||
Also note that setting this environment variable should be done via bug 1272255. So as best lets move the discussion over there.
Assignee | ||
Comment 30•3 years ago
|
||
Hi Raphael,
We are trying to avoid any connection during Marionette tests.
TelemetryTestCase (telemetry-tests-client) extend MarionetteTestCase and as far as I can tell some tests hit the network when the search
helper is used: https://searchfox.org/mozilla-central/rev/e3a7a72713e1ba8696cb2af55e928059bc822572/toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py#75
For instance toolkit/components/telemetry/tests/marionette/tests/client/test_event_ping.py will load https://www.google.com/search?client=firefox-b-d&q=mozilla+firefox when doing self.search("mozilla firefox")
.
I am trying to see if there's any point in chasing other requests I see when running those tests.
For instance I can still see a ping to incoming.telemetry.mozilla.org on try, even if I override toolkit.telemetry.server to a dummy URL.
Is it required for those tests to hit the network?
Comment 31•3 years ago
|
||
Hi Julian,
Thank you for the ping!
The telemetry-tests-client test suite helps us ensure that the Firefox Telemetry component is working as expected. We verify that the correct probes are recorded and that pings are submitted for ingestion, however all HTTP requests with pings are expected to be sent to a local HTTP server running on http://localhost:8000
, not incoming.telemetry.mozilla.org
.
We set the the toolkit.telemetry.server
pref to "http://localhost:8000"
for that in the test runner. So this is not expected.
Adding Chutten to this thread.
Julian, can you please share a link to a try run where we can see the outgoing HTTP requests?
Comment 32•3 years ago
|
||
Recall also that the Glean SDK will attempt to send to incoming.telemetry.mozilla.org
unless you set telemetry.fog.test.localhost_port
to a non-zero value like we do in the test runner. See the docs for details about what the values mean.
IIRC the search-specific tests presently expect to hit the network because the method by which a search interaction turns into search data is odd, so the test aims to reproduce as much of reality as is practicable in order to best ensure we have a valid test. If accessing the wider 'net proves to be troublesome we should probably get some search engineers in here to help us narrow the scope.
Comment 33•3 years ago
|
||
Assignee | ||
Comment 34•3 years ago
|
||
Thanks for the feedback!
Julian, can you please share a link to a try run where we can see the outgoing HTTP requests?
Not sure how to do that. The closest thing I have is an attempt where I ran the telemetry tests with MOZ_DISABLE_NONLOCAL_CONNECTIONS set to 1, but they failed very early on a ping to incoming.telemetry.mozilla.org
, even though telemetry.fog.test.localhost_port
seems set to -1, as pointed out by :chutten
So not sure where this ping comes from, as I don't get it locally. The last telemetry trace before the crash is TelemetrySession::observe - user-interaction-inactive notified.
Maybe there is a codepath which ignores telemetry.fog.test.localhost_port
?
If accessing the wider 'net proves to be troublesome we should probably get some search engineers in here to help us narrow the scope.
My understanding is that tier 1 tests should never hit the network, but maybe there are exceptions? Also even though the tests hit the network, they still seem to pass if you are not connected to any network. So maybe that's ok from a harness/sheriffing point of view, and it just means we can't enable MOZ_DISABLE_NONLOCAL_CONNECTIONS for those.
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 35•3 years ago
|
||
I tested locally to stop setting telemetry.fog.test.localhost_port
to -1, and then I indeed get a very early crash for a gleam ping. This means that this pref successfully silences some pings. But we still get a crash on try.
So I really think there is another ping to incoming.telemetry.mozilla.org
, somehow only triggered on try and which ignores telemetry.fog.test.localhost_port
. The inactivity one would make sense to not happen for me locally, so that might be something worth looking into?
Comment 36•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0749213a5ba5
https://hg.mozilla.org/mozilla-central/rev/d4f456d73000
Assignee | ||
Comment 37•3 years ago
|
||
I filed Bug 1753003 to investigate the remaining ping in telemetry tests.
For the overall question about using network connections (for search tests), let's ask before filing an other bug.
(In reply to Julian Descottes [:jdescottes] from comment #34)
If accessing the wider 'net proves to be troublesome we should probably get some search engineers in here to help us narrow the scope.
My understanding is that tier 1 tests should never hit the network, but maybe there are exceptions? Also even though the tests hit the network, they still seem to pass if you are not connected to any network. So maybe that's ok from a harness/sheriffing point of view, and it just means we can't enable MOZ_DISABLE_NONLOCAL_CONNECTIONS for those.
Aryx: Hi, maybe you can answer here. We have tier 1 tests (telemetry-tests-client) which hit the network by design when they perform a search with the URL bar. They will indirectly load the result page for the current search provider (most likely, google). So the test definitely hits the network, but it doesn't seem to require a valid response to pass . At least I could get one of them to succeed while I had no internet connection.
Is this acceptable for a tier 1 test or do we need to find a way to avoid any network connection?
(see comment #32 from :chutten for more info)
Reporter | ||
Comment 38•3 years ago
|
||
I think that the problem here is that the default search engine is used and as such the tests hit google.com. Telemetry tests should change the default search engine to a locally hosted one. Lets discuss that in the newly filed bug given that it is not related to Marionette itself (this bug).
Comment 39•3 years ago
|
||
Answered by Henrik, further discussion in bug 1753003. If there was a connectivity issue which started the test to fail, it's a high risk to be mistaken as commit related issue. Tier 2 is the appropriate test level for tests requiring network connection. Firefox functional certificate tests are (were?) a precedent of such a setup.
Assignee | ||
Comment 40•3 years ago
|
||
(In reply to Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout) from comment #39)
Answered by Henrik, further discussion in bug 1753003. If there was a connectivity issue which started the test to fail, it's a high risk to be mistaken as commit related issue. Tier 2 is the appropriate test level for tests requiring network connection. Firefox functional certificate tests are (were?) a precedent of such a setup.
Thanks. Bug 1753003 is about a ping to incoming.telemetry.mozilla.org
, unrelated to the search engine.
The tests do not fail when there is no connectivity. They don't require a network connection, the connection is a side effect. But this prevents from setting MOZ_DISABLE_NONLOCAL_CONNECTIONS, because the connection would create a crash.
I was trying to understand if tier1 tests are simply required not to "rely" on any connection. Or are they required to be free from any connection, which means they should all run with MOZ_DISABLE_NONLOCAL_CONNECTIONS=1. Still not clear for me, but I will file another bug for that.
Updated•2 years ago
|
Description
•