Closed Bug 1509256 Opened 10 months ago Closed 10 months ago

Clarify where to add preferences (prefs.rs vs. marionette.js) and fix application update preferences

Categories

(Testing :: geckodriver, enhancement, P2)

enhancement

Tracking

(firefox64 unaffected, firefox65 fixed)

RESOLVED FIXED
mozilla65
Tracking Status
firefox64 --- unaffected
firefox65 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

Attachments

(2 files)

It came up several times now that new preferences are getting added to both places, but no-one actually knows that it would only be necessary at a single place. As such we should clarify that.

Here a summary:

* Preferences which have to be set BEFORE Firefox gets started have to end-up in prefs.rs (similar to geckoinstance.py).

* Preferences which can be modified during runtime of Firefox and are immediately active have to end-up in marionette.js.

With an upcoming patch I will also remove the two preferences as mentioned in bug 1506917 comment 12.
I don't understand why https://hg.mozilla.org/mozilla-central/rev/663e1f142dd4 has been landed. geckodriver still supports Firefox 53, and as such needs the update preferences in the prefs.rs file. We cannot remove those until we stop supporting Firefox <=62 in geckodriver. Andreas, can you please clarify?
Flags: needinfo?(ato)
Also I'm not sure about the preference `security.turn_off_all_security_so_that_viruses_can_take_over_this_computer`. Do we really need this? Originally it was added by bug 1420514 for the recent updater changes, and that is needed for `Cu.isInAutomation` (https://searchfox.org/mozilla-central/source/js/xpconnect/src/xpcpublic.h#709). But this check fails anyway for Marionette due to the local network restriction check, which we don't support. And the pref is only needed to use `enablePrivilege`, which we don't seem to need at all? 

So why shall we add such a quirk and security hole if it's not necessary.

See also bug 1508726 for the discussion on the `isInAutomation` flag.
Blocks: 1495062
See Also: → 1508726
Masatoshi, can you also please have a look at my last comment? Thanks.
Flags: needinfo?(VYV03354)
(In reply to Henrik Skupin (:whimboo) from comment #1)
> I don't understand why
> https://hg.mozilla.org/mozilla-central/rev/663e1f142dd4 has been landed.
> geckodriver still supports Firefox 53, and as such needs the update
> preferences in the prefs.rs file. We cannot remove those until we stop
> supporting Firefox <=62 in geckodriver. Andreas, can you please clarify?

I believe this is a better question for the patch author.
Flags: needinfo?(ato) → needinfo?(ksteuber)
As background, the support whimboo is referring to is documented here:
https://firefox-source-docs.mozilla.org/testing/geckodriver/geckodriver/Support.html
According to that the minimum geckodriver version is 57, though.
Yes, thanks for the link to the support page. Also we don't want to bump the minimum supported version of Firefox for the geckodriver 0.24 release to 63. I don't see this necessary at this point. But it might change depending on what resolution we will get on bug 1508726.
(In reply to Henrik Skupin (:whimboo) from comment #1)
> I don't understand why
> https://hg.mozilla.org/mozilla-central/rev/663e1f142dd4 has been landed.

`app.update.enabled` was removed according to the comment above it, which read "Once Firefox 62 becomes stable, the line below can be removed". `app.update.auto` was removed because it has no effect when update is disabled.
Flags: needinfo?(ksteuber)
(In reply to Henrik Skupin (:whimboo) from comment #2)
> And the pref is only needed to
> use `enablePrivilege`, which we don't seem to need at all? 
> 
> So why shall we add such a quirk and security hole if it's not necessary.

Talos still depends on enablePrivilege:
https://dxr.mozilla.org/mozilla-central/search?q=enablePrivilege

We will have to fix bug 1435113 before removing enablePrivilege, unfortunately.
Flags: needinfo?(VYV03354)
Also, many mochitests depend on forcePermissiveCOWs that SpecialPowers is using.
(In reply to Kirk Steuber (he/him) [:bytesized] from comment #9)
> `app.update.enabled` was removed according to the comment above it, which
> read "Once Firefox 62 becomes stable, the line below can be removed".

Well, this is confusing because we cannot remove it once 62 becomes stable. As noted originally we also have/want to support older releases of Firefox which currently goes back to 57. 

The only reason why this evaluates the statement to true nowadays are the underlying changes of bug 1277691 which made this pref a no-op since Firefox 49!

> `app.update.auto` was removed because it has no effect when update is
> disabled.

Note, that in Marionette updates are currently NOT disabled! See the comment above. Would re-adding this preference help in the interim until bug 1508726 has been fixed?
Flags: needinfo?(ksteuber)
(In reply to Masatoshi Kimura [:emk] from comment #10)
> Talos still depends on enablePrivilege:
> https://dxr.mozilla.org/mozilla-central/search?q=enablePrivilege
> 
> We will have to fix bug 1435113 before removing enablePrivilege,
> unfortunately.

Talos doesn't have a reference to Marionette at all. So why should it matter here if we remove this preference in Marionette only?

> Also, many mochitests depend on forcePermissiveCOWs that SpecialPowers is
> using.

Mochitests only use Marionette to install the MochiKit extension. That's all. Also the profiles are setup by the Mochitest harness itself, and not Marionette. So I don't see why it should matter here too.

Maybe I miss something?
Flags: needinfo?(VYV03354)
(In reply to Henrik Skupin (:whimboo) from comment #12)
> > `app.update.auto` was removed because it has no effect when update is
> > disabled.
> 
> Note, that in Marionette updates are currently NOT disabled! See the comment
> above. Would re-adding this preference help in the interim until bug 1508726
> has been fixed?

I confirmed that this preference still exists, and is evaluated:

https://searchfox.org/mozilla-central/rev/d35199ef15ffa57d190886e20d5df6471faf0870/toolkit/mozapps/update/nsUpdateService.js#2402-2427

So re-adding it to the list of preferences when creating the profile with Marionette works, and updates are no longer downloaded.  I only have a problem verifying that it works in general due to the following failure I always see locally:

> *** AUS:SVC Checker:onLoad - Getting sslStatus failed.
> *** AUS:SVC Checker:onLoad - there was a problem checking for updates. Exception: TypeError: this._request.responseXML is null; can't access its "documentElement" property
> *** AUS:SVC Checker:onLoad - request.status: 200

As such we should re-add this preference as soon as possible, and leave it in-place until we found a proper solution for bug 1508726.
Note that `app.update.auto` was removed on bug 1458308 for Firefox 65. As such we thankfully don't have a regression with geckodriver yet as long as we re-add it before the next release (bug 1495062).

https://hg.mozilla.org/mozilla-central/rev/663e1f142dd4

While the removal from `marionette.js` was fine, we have to move the pref to `geckoinstance.py`.
With the try build none of the Marionette driven tests show any update activities anymore. Also removing the preference "security.turn_off_all_security_so_that_viruses_can_take_over_this_computer" doesn't have any negative effect to before mentioned Mochitests nor Talos.
It should be made clear that only those preferences should be added
to marionette.js which have an immediate effect. All others which
eg. require a restart, or have to be set before the application
starts the first time, must be added to the appropriate client's
profile generation code.
Because in Marionette the application updates cannot be turned off
via the preference "app.update.disabledForTesting" (see bug 1508726),
the preference "app.update.auto" has to be used for the time being
to control that by default no updates should be downloaded and
installed.

Even "app.update.disabledForTesting" doesn't work right now,
it might be good to leave it. It would help us in the future
so that we won't have to release new Marionette client and
geckodriver releases, if it gets supported in Firefox.

Depends on D13059
Summary: Clarify where to add preferences (prefs.rs vs. marionette.js) → Clarify where to add preferences (prefs.rs vs. marionette.js) and fix application update preferences
Thanks for fixing this, whimboo!  It would be good to get the second
patch landed as soon as possible.
(In reply to Henrik Skupin (:whimboo) from comment #12)
> Would re-adding this preference help in the interim until bug 1508726
> has been fixed?

Relying on `app.update.auto` on Windows may yield inconsistent results. Windows no longer stores that setting value in that preference. Instead, the setting is stored in a file in the update directory so that it is accessible to the Background Update Agent.

Firefox will migrate the `app.update.auto` pref value to the settings file, but only if:
  The `app.update.auto.migrated` pref is false or non-existent, and
  The settings file does not already exist.

I do not know how the Marionette test setup works, so I do not know if you can rely on the settings file being non-existent. If I remember correctly, you mentioned having problems with Firefox updating when running Marionette tests multiple times. This leads me to suspect that Marionette tests do not delete the update directory after they run. However, it is possible that the update directory gets removed at a different point.

So, if Marionette tests remove the files in the update directory at some point, you could add these preferences as a temporary fix:

app.update.auto = false
app.update.auto.migrated = false

If Marionette tests never remove the files in the update directory, then I am afraid I cannot offer a simple pref-based fix for this problem since the setting is not stored in a pref anymore.
Flags: needinfo?(ksteuber)
(In reply to Kirk Steuber (he/him) [:bytesized] from comment #21)
> (In reply to Henrik Skupin (:whimboo) from comment #12)
> > Would re-adding this preference help in the interim until bug 1508726
> > has been fixed?
> 
> Relying on `app.update.auto` on Windows may yield inconsistent results.
> Windows no longer stores that setting value in that preference. Instead, the
> setting is stored in a file in the update directory so that it is accessible
> to the Background Update Agent.

That is unfortunate, but maybe a side-effect we would have to live with until a real solution has been found? Also as it looks like this behavior isn't new, but already exists for a while? So it shouldn't interfere with bringing this preference temporarily back, right? At least we can make sure that tests on Linux and MacOS (where most of the automated tests are probably executed) don't run into that update mess.
 
> Firefox will migrate the `app.update.auto` pref value to the settings file,
> but only if:
>   The `app.update.auto.migrated` pref is false or non-existent, and
>   The settings file does not already exist.

Where is this file located? I assume this path can vary, and that we would have to request it through `aus.getUpdatesDirectory().path`, or is it located somewhere else? Also which bug implemented that? 

> I do not know how the Marionette test setup works, so I do not know if you
> can rely on the settings file being non-existent. If I remember correctly,

Marionette basically sets-up a default profile and by default assumes that no update will ever happen, and as such no extra files at random places have to be deleted. The same also applies to geckodriver. As such it doesn't matter if the file exists or not.

> you mentioned having problems with Firefox updating when running Marionette
> tests multiple times. This leads me to suspect that Marionette tests do not
> delete the update directory after they run. However, it is possible that the
> update directory gets removed at a different point.

The problem is not only when you run Marionette tests against the same binary, but also within the same test. It only has to take long enough, and also to restart Firefox. Then this upgrade can happen at random times.

> So, if Marionette tests remove the files in the update directory at some
> point, you could add these preferences as a temporary fix:
> 
> app.update.auto = false
> app.update.auto.migrated = false
> 
> If Marionette tests never remove the files in the update directory, then I
> am afraid I cannot offer a simple pref-based fix for this problem since the
> setting is not stored in a pref anymore.

When are the files in the update directory created if they don't exist? At each startup of Firefox? If that is the case then there is clearly no way for Marionette to remove those files especially for restart tests, because then it would interfere with the test itself, and might cause race conditions.

Reading all this I get the impression that we should land this band-aid fix as soon as possible, and find a real solution for this problem on bug 1508726. But maybe answers of my new questions could bring up another idea. Thanks
Flags: needinfo?(ksteuber)
(In reply to Henrik Skupin (:whimboo) from comment #22)
> Also as it looks like this behavior
> isn't new, but already exists for a while?
Actually, these changes were merged in Firefox 65.

> Where is this file located? I assume this path can vary, and that we would
> have to request it through `aus.getUpdatesDirectory().path`, or is it
> located somewhere else?
The typical location is "C:\ProgramData\Mozilla\updates\<hash>\update-config.json", but the directory path should be obtained from the directory service using the "UpdRootD" key. You do not want `aus.getUpdatesDirectory()` as that will return the patch directory rather than the update root.

> Also which bug implemented that?
Bug 1458308

> When are the files in the update directory created if they don't exist?
The preference value will be migrated to the config file when it is first accessed (that is to say, when `getAutoUpdateIsEnabled()` is called [1]). This assumes that the preference has not already been migrated.

[1] Note that `getAppUpdateAutoEnabled()` is about to be moved and renamed to `getAppUpdateAutoEnabled()` (
Flags: needinfo?(ksteuber)
(In reply to Kirk Steuber (he/him) [:bytesized] from comment #23)
> (In reply to Henrik Skupin (:whimboo) from comment #22)
> > Also as it looks like this behavior
> > isn't new, but already exists for a while?
> Actually, these changes were merged in Firefox 65.

Okay, that is good! It gives us still a bit of time until this patch series reaches beta and release.

So lets try to find a final solution for bug 1508726 as soon as possible.

> The typical location is
> "C:\ProgramData\Mozilla\updates\<hash>\update-config.json", but the
> directory path should be obtained from the directory service using the
> "UpdRootD" key. You do not want `aus.getUpdatesDirectory()` as that will
> return the patch directory rather than the update root.

Note that this would require at least a single successful connection from Marionette client or geckodriver to the Marionette server, to exchange this information. If something is broken we won't be able to remove that file.

> > Also which bug implemented that?
> Bug 1458308

Thanks. I actually found it already a bit earlier today.

> > When are the files in the update directory created if they don't exist?
> The preference value will be migrated to the config file when it is first
> accessed (that is to say, when `getAutoUpdateIsEnabled()` is called [1]).
> This assumes that the preference has not already been migrated.
> 
> [1] Note that `getAppUpdateAutoEnabled()` is about to be moved and renamed
> to `getAppUpdateAutoEnabled()` (

I would actually not spend the time for this workaround but better on bug 1508726 for the final solution in getting this fixed. Keeping this bug and patch simply for backward compatibility.
Flags: needinfo?(VYV03354)
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/50ff40cd5813
Clarify where to add preferences for geckodriver and Marionette. r=ato
https://hg.mozilla.org/integration/autoland/rev/0ed1cec73c66
Fix application update preferences for backward compatibility in Marionette and geckodriver. r=ato,bytesized
https://hg.mozilla.org/mozilla-central/rev/50ff40cd5813
https://hg.mozilla.org/mozilla-central/rev/0ed1cec73c66
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.