Open Bug 1371576 Opened 2 years ago Updated 4 months ago

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

Categories

(Testing :: Marionette, enhancement, P3)

55 Branch
enhancement

Tracking

(Not tracked)

People

(Reporter: whimboo, Unassigned)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(2 files)

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: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1272255
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 → ---
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
You need to log in before you can comment on or make changes to this bug.