Closed Bug 1371576 Opened 7 years ago Closed 2 years ago

Ensure that Firefox doesn't crash for Marionette tests when MOZ_DISABLE_NONLOCAL_CONNECTIONS is set

Categories

(Remote Protocol :: Marionette, enhancement, P3)

55 Branch
enhancement

Tracking

(firefox98 fixed)

RESOLVED FIXED
98 Branch
Tracking Status
firefox98 --- fixed

People

(Reporter: whimboo, Assigned: jdescottes)

References

Details

Attachments

(4 files, 1 obsolete file)

In Marionette and Geckodriver we modify some preferences to stop Firefox reaching some production servers like for the add-ons discovery pane:

"extensions.webservice.discoverURL": "http://%(server)s/dummy/discoveryURL"

The problem with this is that Firefox still wants to reach this remote address and is causing network traffic. In case of a proxy in the middle it will be flooded.

To prevent this we should check how to get this replaced with the address from the locally hosted wptserve instance.

Here the known prefs:

> "datareporting.healthreport.documentServerURI": "http://%(server)s/dummy/healthreport/",
> "datareporting.healthreport.about.reportUrl": "http://%(server)s/dummy/abouthealthreport/",
> "extensions.webservice.discoverURL": "http://%(server)s/dummy/discoveryURL",
> "network.sntp.pools": "%(server)s",
> "services.settings.server": "http://%(server)s/dummy/blocklist/",
> "toolkit.telemetry.server": "https://%(server)s/dummy/telemetry/",

For Marionette unit tests we could do a replacement most likely in geckoinstance.py where those prefs are getting set.

But not sure what to do with the recommended prefs we set in Marionette server. When we run through Geckodriver there might not be a local server running. Just using localhost without a port doesn't seem to be a wise idea.

Andreas, any idea?
Flags: needinfo?
Flags: needinfo?(ato)
(In reply to Henrik Skupin (:whimboo) from comment #0)

> In Marionette and Geckodriver we modify some preferences to stop
> Firefox reaching some production servers like for the add-ons
> discovery pane:
> 
> "extensions.webservice.discoverURL": "http://%(server)s/dummy/discoveryURL"
> 
> The problem with this is that Firefox still wants to reach this remote
> address and is causing network traffic. In case of a proxy in the
> middle it will be flooded.
> 
> To prevent this we should check how to get this replaced with the
> address from the locally hosted wptserve instance.

Why do we want it to hit the wptserve instance?  Could they not just be
cleared?

> Here the known prefs:
> 
> > "datareporting.healthreport.documentServerURI": "http://%(server)s/dummy/healthreport/",
> > "datareporting.healthreport.about.reportUrl": "http://%(server)s/dummy/abouthealthreport/",

As far as I know, it is datareporting.policy.dataSubmissionEnabled
that controls whether we will submit any data to Telemetry.  We
set this to false, and it would be surprising if this caused
us to hit datareporting.healthreport.documentServerURI or
datareporting.healthreport.about.reportUrl.
Flags: needinfo?(ato)
(In reply to Andreas Tolfsen ‹:ato› from comment #1)
> Why do we want it to hit the wptserve instance?  Could they not just be
> cleared?

That's how Mochitest is doing it. But maybe they register valid handlers for all the cases immediately. Given that Marionette is more flexible, we might indeed not wanna do it, and indeed could simply set `localhost` with the default port.

> > > "datareporting.healthreport.documentServerURI": "http://%(server)s/dummy/healthreport/",
> > > "datareporting.healthreport.about.reportUrl": "http://%(server)s/dummy/abouthealthreport/",
> 
> As far as I know, it is datareporting.policy.dataSubmissionEnabled
> that controls whether we will submit any data to Telemetry.  We
> set this to false, and it would be surprising if this caused
> us to hit datareporting.healthreport.documentServerURI or
> datareporting.healthreport.about.reportUrl.

This is just an example which might not cause problems, right. But others could, like for the add-ons discovery pane.
Flags: needinfo?
(In reply to Henrik Skupin (:whimboo) from comment #2)
> (In reply to Andreas Tolfsen ‹:ato› from comment #1)
> > Why do we want it to hit the wptserve instance?  Could they not just be
> > cleared?
> 
> That's how Mochitest is doing it. But maybe they register valid handlers for
> all the cases immediately. Given that Marionette is more flexible, we might
> indeed not wanna do it, and indeed could simply set `localhost` with the
> default port.

I would actually suggest clearing the URLs completely so they don’t hit any
accidental endpoint or service on localhost at all.
Summary: Replace %server% placeholders in modified preferences with the localhost and port → Replace %(server)s placeholders in modified preferences with the localhost and port
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Summary: Replace %(server)s placeholders in modified preferences with the localhost and port → Replace %(server)s placeholders in modified preferences with an empty string
Francois, even with the following preferences disabled I can still see that Firefox tries to connect to the remote address as specified in "browser.safebrowsing.provider.mozilla.updateURL":

        "browser.safebrowsing.blockedURIs.enabled": False,
        "browser.safebrowsing.downloads.enabled": False,
        "browser.safebrowsing.malware.enabled": False,
        "browser.safebrowsing.phishing.enabled": False,

What are we missing here? Is there something else which needs explicitly to be disabled?
Flags: needinfo?(francois)
Lets make this bug broader and finally fix all the necessary preferences.
Blocks: 1272255
Summary: Replace %(server)s placeholders in modified preferences with an empty string → Ensure that Firefox doesn't hit remote addresses by default when Marionette tests are run
(In reply to Henrik Skupin (:whimboo) from comment #4)
> Francois, even with the following preferences disabled I can still see that
> Firefox tries to connect to the remote address as specified in
> "browser.safebrowsing.provider.mozilla.updateURL":
> 
>         "browser.safebrowsing.blockedURIs.enabled": False,
>         "browser.safebrowsing.downloads.enabled": False,
>         "browser.safebrowsing.malware.enabled": False,
>         "browser.safebrowsing.phishing.enabled": False,
> 
> What are we missing here? Is there something else which needs explicitly to
> be disabled?

For the mozilla provider, you need to disable all of the tracking protection and plugin blocking features:

privacy.trackingprotection.enabled = false
privacy.trackingprotection.pbmode.enabled = false
privacy.trackingprotection.annotate_channels = false
plugins.flashBlock.enabled = false
Flags: needinfo?(francois)
Hm, so what would actually be better in terms of disabling the service? Switching all flags to false, or better replacing the URLs of each with an empty string so that we do not hit the network? At least for other test harnesses I can see that they are doing the latter.

Would that also make it easier for the safebrowsing firefox ui tests to enable the service without a restart? Or wouldn't it make a difference?
Flags: needinfo?(francois)
(In reply to Henrik Skupin (:whimboo) from comment #9)
> Hm, so what would actually be better in terms of disabling the service?
> Switching all flags to false, or better replacing the URLs of each with an
> empty string so that we do not hit the network? At least for other test
> harnesses I can see that they are doing the latter.
>
> Would that also make it easier for the safebrowsing firefox ui tests to
> enable the service without a restart? Or wouldn't it make a difference?

It would be easier to re-enable the service in the Safe Browsing UI tests if we only disable the feature flags and leave the URLs as they are.

Otherwise we'll have to remember to update the UI tests everytime we change the URL prefs and so it's likely they will get out of sync.
Flags: needinfo?(francois)
(In reply to François Marier [:francois] from comment #10)
> Otherwise we'll have to remember to update the UI tests everytime we change
> the URL prefs and so it's likely they will get out of sync.

That shouldn't be the case given that we would only have to remove the user defined value (aka reset the preference). Once done the original value (URL) should be immediately available. Would modifying the URLs instead safe us a restart to get safebrowsing enabled?
Flags: needinfo?(francois)
(In reply to Henrik Skupin (:whimboo) from comment #11)
> That shouldn't be the case given that we would only have to remove the user
> defined value (aka reset the preference). Once done the original value (URL)
> should be immediately available. Would modifying the URLs instead safe us a
> restart to get safebrowsing enabled?

I don't think that would change anything because we'd still end up having to re-initialize the list manager with the new URLs.
Flags: needinfo?(francois)
Priority: -- → P3
In bug 1399628 we got the preference `the extensions.shield-recipe-client.api_url` added, which should disable shield by not allowing it to download studies. As informed by Mike this might not be a good idea, and we shouldn't set the URL to an empty string. Instead the component/extension should have been entirely disabled.

Maybe we have to rethink the strategy here in regards that we cannot use an empty string at all. Also not for the other preferences. Not sure if every component/extension has a knob to easily turn it off.

I'm happy to hear options in what to do with the URL given that we cannot always replace `%(server)s` due to a missing locally running webserver.
FYI, for shield we should change the preference to use:

> extensions.shield-recipe-client.enabled = false

I will do it with the next update.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
This is not a dupe but a dependency to get bug 1272255 fixed. I will still have to finish this patch.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Depends on: 1531082
Depends on: 1542244
Assignee: hskupin → nobody
Status: REOPENED → NEW
Summary: Ensure that Firefox doesn't hit remote addresses by default when Marionette tests are run → Ensure that Firefox doesn't crash for Marionette tests when MOZ_DISABLE_NONLOCAL_CONNECTIONS is set
See Also: → 1749981

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: nobody → jdescottes
Status: NEW → ASSIGNED

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!

Flags: needinfo?(francois)

Dimi would be the right person to ask since he's the module owner for Safe Browsing.

Flags: needinfo?(francois) → needinfo?(dlee)

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!

Flags: needinfo?(dlee) → needinfo?(jdescottes)

(In reply to Dimi Lee [:dimi] from comment #22)

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!

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.

Flags: needinfo?(jdescottes) → needinfo?(dlee)

(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.

Flags: needinfo?(dlee)

Sounds like a good option, thanks for the help!

Attachment #9260552 - Attachment is obsolete: true

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

Flags: needinfo?(jmaher)

Also note that setting this environment variable should be done via bug 1272255. So as best lets move the discussion over there.

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?

Flags: needinfo?(rpierzina)

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?

Flags: needinfo?(rpierzina)
Flags: needinfo?(jdescottes)
Flags: needinfo?(chutten)

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.

Flags: needinfo?(chutten)
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0749213a5ba5
Allow to disable all SafeBrowsing updates with the browser.safebrowsing.update.enabled preference r=dimi
https://hg.mozilla.org/integration/autoland/rev/d4f456d73000
[marionette] Avoid connections during marionette tests r=webdriver-reviewers,whimboo,Gijs

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

https://treeherder.mozilla.org/jobs?repo=try&selectedTaskRun=cOjOySeaT_WMzmheMk9D4Q.0&revision=d991ee6c5a39a57b508e3fbba93dc344004b3397

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.

Flags: needinfo?(jdescottes)

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?

Status: ASSIGNED → RESOLVED
Closed: 6 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 98 Branch
Blocks: 1753003

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)

Flags: needinfo?(aryx.bugmail)

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).

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.

Flags: needinfo?(aryx.bugmail)

(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.

Blocks: 1753034
Product: Testing → Remote Protocol
See Also: → 1849972
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: