1260502 - Fix preference handling of enforce_gecko_prefs() and do a safe shutdown

Status: NEW

(Testing :: Marionette Client and Harness, defect, P3)

Description

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

Right now the enforce_gecko_prefs() method forces a hard-restart of Firefox from the Python side. This has certain disadvantages because lots of data which is usually written during shutdown of Firefox cannot be written. This will cause test failures for certain tests which need a restart.

https://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette_driver/marionette.py#1053

There is GeckoInstance.restart() which allows that.

Comment 1

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

We should wait for bug 1141483 here which keeps the context from before the restart.

Comment 2

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

So originally this feature has been implemented in bug 1030442. The requirements were:

* set non-default settings for prefs
* allow those to be available on startup
* should stick for all the tests

With the current implementation I see the following issues:

1. We replace any preferences as given via the command line by the ones as specified by enforce_gecko_prefs(). Shouldn't we update the dict?

https://dxr.mozilla.org/mozilla-central/rev/3f4c3a3cabaf94958834d3a8935adfb4a887942d/testing/marionette/client/marionette_driver/marionette.py#1078
https://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette_driver/geckoinstance.py#182

2. There is no way to get rid of those extra preferences except specifying an empty dict which due to the above issue would reset everything.

I don't say that we should remove this method. We still need a feature like that if a test has to modify preferences which Marionette forces as default.

My proposal would be that we add a separate prefs dictionary to the Gecko instance class which only holds preferences as set by enforce_gecko_prefs(). That way we can build a final prefs dict which would allow us to overwrite Marionette defaults, and keep the originally set preferences from the command line. Also we could easily remove all those prefs by calling `enforce_gecko_prefs()` again.

Regarding doing a safe restart we will have to do a `quit(in_app=True)`, modify the profile via `_update_profile()`, and start Firefox via `start_session()` again.

David, would you have any feedback? Or do you see issues with that? IMO we should get this fixed sooner than later given that our fx-ui-tests are at least affected by not being able to reset those prefs and will cause eg. safebrowsing to stay active.

Comment 3

[David Burns :automatedtester]

Do what you think needs doing

Comment 4

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

As seen via bug 1291844 a call to this method also resets formerly set preferences. So it's not at all obvious in how to use this method.

Comment 5

[Chris H-C :chutten]

Another use case for you, this time from bug 1550718

I tried to use enforce_gecko_prefs() in a test to check behaviour that happens as a result of the preferences being set. This might not be a good fit, but it would be nice to have a chance to force the prefs without being forced to restart the instance.

Also, the interaction with prefs already set is unclear. If I set the prefs on the instance manually, enforce_gecko_prefs might no-op when I ask it to enforce that the set prefs persist across restarts. However, if I then manually restart the instance the prefs aren't actually enforced. Perhaps enforce_gecko_prefs should check against the dict it is using to enforce the prefs in addition to checking the prefs in the currently-running instance.

Comment 6

[Sam Foster [:sfoster] (he/him)]

In Bug 1899346, I need to test some browser startup/initialization behavior with different pref states. The pref wasn't defined at all, one pref was false, another true at startup etc. Because I needed to ensure startup happened so the initialization code under test for sure ran (to avoid false positives), I found I was not able to use enforce_gecko_prefs(). This might be a niche case, but hopefully its a useful datapoint.