Closed Bug 1458308 (update-prefs) Opened 6 years ago Closed 6 years ago

Move update pref(s) including app.update.auto out of profile

Categories

(Toolkit :: Application Update, enhancement, P1)

Product:
Toolkit
Component:
Application Update
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla65
Tracking Flags:
Tracking Status
firefox65 --- verified

People

(Reporter: bytesized, Assigned: bytesized)

References

(Blocks 2 open bugs)

Details

User Story

In order for the Background Update Agent to be able to determine whether updates are disabled, the `app.update.enabled` and `app.update.auto` prefs need to be removed from the user profile.

Steps:
1) Get rid of app.update.enabled entirely (including the corresponding UI option, "Never check for updates")
2) Get rid of checks for app.update prefs from all system addon update code
3) Allow policies to disable app and (separately) system addon update to be used outside of the ESR
4) Ensure policies can be read by the Background Update Agent in order to determine if updates are disabled
5) Move `app.update.auto` to be stored in the update directory.

Attachments

(6 files, 5 obsolete files)

Bug 1458308 - Move app.update.auto to be stored in the update directory on Windows only r=rstrong
46 bytes, text/x-phabricator-request
Details | Review
Bug 1458308 - Change about:preferences to use the new auto update pref mechanism r=jaws
46 bytes, text/x-phabricator-request
flod
: review+
jaws
: review+
Details | Review
Bug 1458308 - Change telemetry to use the new auto update lookup mechanism r=chutten
46 bytes, text/x-phabricator-request
chutten
: review+
Details | Review
Bug 1458308 - Tests for migration and UI of app.update.auto pref r=rstrong
46 bytes, text/x-phabricator-request
Details | Review
Bug 1458308 - Change lookups of app.update.auto to use nsIApplicationUpdateService::(get|set)AutoUpdateIsEnabled r=rstrong
47 bytes, text/x-phabricator-request
Details | Review
Bug 1458308 - Remove app.update.auto and app.update.enabled from prefs.rs and marionette.js r=rstrong,ato
47 bytes, text/x-phabricator-request
Details | Review 
The prefs `app.update.enabled` and `app.update.auto` need to be stored in a central, installation-specific location rather than in the prefs associated with a Firefox profile.

There are several reasons for this:
 - The Background Update Agent does not know where any particular profile lives or which profile it should use to determine if updates are allowed.
 - It doesn't really make sense for those prefs to be stored there in the first place. If one user configures Firefox not to update and another user configures Firefox to update automatically, Firefox will update when the latter profile is running. This doesn't really respect the preferences of the former user.

Since we want to move the preferences outside of the user profile, their new location will likely need to be platform-specific. These are the proposed locations by platform:
 - Windows: The Registry
 - OSX: Somewhere in /Library. Probably in Application Support
 - Linux: Unlike in other operating systems, Firefox installations in Linux cannot elevate their privileges to update. If the current user cannot write to the installation directory, they cannot update. Therefore, it is easiest to store this data in the installation directory.
Assignee: nobody → ksteuber
Hmm. It looks like I did not think this through 100%. Pretty much any location that Firefox can write to and the Background Update Agent can read from will be world-writable, which we do not want for the `app.update.enabled` pref.

The approach that we are looking at is to get rid of the `app.update.enabled` pref altogether so we can just write `app.update.auto` to the update directory.
Depends on: 1420514, 1428459
Depends on: 1460082
Depends on: 1460086
User Story: (updated)
Alias: update-prefs
Depends on: 1461755
No longer depends on: 1461755
Depends on: 1458314
No longer depends on: 1460082
Priority: -- → P1
hg error in cmd: hg push -r . -f try: pushing to ssh://hg.mozilla.org/try
remote: Permission denied (publickey).
abort: no suitable response from remote hg!
hg error in cmd: hg push -r . -f try: pushing to ssh://hg.mozilla.org/try
remote: Permission denied (publickey).
abort: no suitable response from remote hg!
Attachment #8994700 - Flags: review?(robert.strong.bugs)
Attachment #8994701 - Flags: review?(jaws)
Attachment #8994703 - Flags: review?(robert.strong.bugs)
Comment on attachment 8994702 [details]
Bug 1458308 - Change telemetry to use the new auto update lookup mechanism

https://reviewboard.mozilla.org/r/259234/#review267270

I'd also love to see this tested, perhaps in telemetry/test/test_TelemetryEnvironment.js

::: commit-message-e9ba1:2
(Diff revision 2)
> +Bug 1458308 - Change telemetry to use the new auto update lookup mechanism
> +

Please elaborate in your commit message how this changes the timing and availability of these fields.

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
(Diff revision 2)
>    ["app.feedback.baseURL", {what: RECORD_PREF_VALUE}],
>    ["app.support.baseURL", {what: RECORD_PREF_VALUE}],
>    ["accessibility.browsewithcaret", {what: RECORD_PREF_VALUE}],
>    ["accessibility.force_disabled", {what:  RECORD_PREF_VALUE}],
>    ["app.shield.optoutstudies.enabled", {what: RECORD_PREF_VALUE}],
> -  ["app.update.auto", {what: RECORD_PREF_VALUE}],

It might be kind to announce your intent to remove this userpref on fx-data-dev in case someone or some data job is currently using this.

::: toolkit/components/telemetry/TelemetryEnvironment.jsm:903
(Diff revision 2)
>    this._currentEnvironment.profile = {};
>    p.push(this._updateProfile());
>    if (AppConstants.MOZ_BUILD_APP == "browser") {
>      p.push(this._loadAttributionAsync());
>    }
> +  p.push(this._loadAutoUpdateAsync());

Is it possible this might delay the availability of the Telemetry Environment?
Attachment #8994702 - Flags: review-
Comment on attachment 8994701 [details]
Bug 1458308 - Change about:preferences to use the new auto update pref mechanism

https://reviewboard.mozilla.org/r/259232/#review267570

::: browser/locales/en-US/browser/preferences/preferences.ftl:351
(Diff revision 1)
> +update-pref-write-failure-title = Write Failure
> +
> +update-pref-write-failure-message = Unable to save preference. Could not write to file: { $path }

Can you please request review from Brian Jones on this? He can give us some recommendations on friendlier text to use.

My recommendation:
Title: Error saving Update preferences
Message: The Update preferences are stored outside of normal preference storage due to how the Firefox updater works. Unfortunately, an error occured while trying to write to this preference file. (line-break)
File location: { $path }
Attachment #8994701 - Flags: review?(jaws) → review+
Hi Brian,

Can we get your advice on some text here? The preferences for the Update service are stored outside of our normal preference storage, and there is a chance that we could have an error while trying to update the stored values of the preferences. In this odd case, we don't have much that Firefox can do to correct this. The patch that Kirk has written resorts to showing a modal dialog to the user to let them know of the failure, and hopefully either they or their system administrator can fix the file permissions or location so future writes to the file will succeed.

The above comment 12 shows Kirk's proposed text as well as my proposed text. What do you think we should say here?
Flags: needinfo?(brjones)
Hi Jared,
Thanks for the background and context. Couple quick (I think?) questions:
- the 'File location' referenced in Kirk's example is the location of the "non-normal" preference storage?
- if the error is the result of a permission error, do we need to specifically reference that?

Thanks!
Brian
Hi Brian-
The file location is indeed a "non-normal" preferences storage location. The requirements of the new Background Update Agent require that certain prefs be located in the update directory rather than in the profile.

A permission error is one way that we might not have access to that file, but not the only way. I don't know if it makes sense to reference that or not (I was hoping someone with a bit more UI experience could give me some direction on that).

This error should be extremely uncommon though. It would require a permissions change or conflicting directory name in a location that typically sees very little use by users or other applications.

I would really appreciate any suggestions on the string text, so thank you in advance!
Last push was a rebase-only push. Just trying to make future interdiffs a little more readable.
Comment on attachment 8994702 [details]
Bug 1458308 - Change telemetry to use the new auto update lookup mechanism

https://reviewboard.mozilla.org/r/259234/#review269406

Sorry, I missed this one the first time around.

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
(Diff revision 3)
>        telemetryEnabled: Utils.isTelemetryEnabled,
>        locale: getBrowserLocale(),
>        update: {
>          channel: updateChannel,
>          enabled: !Services.policies || Services.policies.isAllowed("appUpdate"),
> -        autoDownload: Services.prefs.getBoolPref(PREF_UPDATE_AUTODOWNLOAD, true),

Please remove from the docs and the tests as well.
Attachment #8994702 - Flags: review-
Comment on attachment 8994701 [details]
Bug 1458308 - Change about:preferences to use the new auto update pref mechanism

Per Jaws comment, requesting final review of strings for dialog.
Attachment #8994701 - Flags: review?(brjones)
Comment on attachment 8994702 [details]
Bug 1458308 - Change telemetry to use the new auto update lookup mechanism

https://reviewboard.mozilla.org/r/259234/#review267270

While I did not add testing, it already exists here: https://searchfox.org/mozilla-central/rev/ca869724246f4230b272ed1c8b9944596e80d920/toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js#442

Is that sufficient, or did you have something else in mind?

> It might be kind to announce your intent to remove this userpref on fx-data-dev in case someone or some data job is currently using this.

Done.

> Is it possible this might delay the availability of the Telemetry Environment?

It is possible that this would introduce a minor delay while the file is read. However, because the Telemetry Environment's initialization is already waiting on `_updateProfile` and `_loadAttributionAsync`, and because reading the pref value is a quick process, I don't think that it is likely to delay the availability significantly.
Comment on attachment 8994702 [details]
Bug 1458308 - Change telemetry to use the new auto update lookup mechanism

https://reviewboard.mozilla.org/r/259234/#review269406

> Please remove from the docs and the tests as well.

This data is not being removed. It is just added in another place: `_updateAutoDownload()`.
Attachment #8994702 - Flags: review?(chutten)
Comment on attachment 8994701 [details]
Bug 1458308 - Change about:preferences to use the new auto update pref mechanism

Since I ended up adding a notification for this value changing, I updated about:preferences to observe the notification and use it to update the UI.

Could you take a look at the changes [1] and make sure they look ok? Thanks!

[1] https://reviewboard.mozilla.org/r/259232/diff/2-3/
Attachment #8994701 - Flags: review+ → review?(jaws)
Attachment #8994701 - Flags: review?(brjones)
Comment on attachment 8994701 [details]
Bug 1458308 - Change about:preferences to use the new auto update pref mechanism

https://reviewboard.mozilla.org/r/259232/#review269536

::: browser/components/preferences/in-content/main.js:1377
(Diff revisions 2 - 3)
>          // All the prefs we observe can affect what we display, so we rebuild
>          // the view when any of them changes.
>          this._rebuildView();
>        }
> +    } else if (aTopic == AUTO_UPDATE_CHANGED_TOPIC) {
> +      document.getElementById("updateRadioGroup").value = aData;

What are the valid values for this? "true" or "false"? radiogroup.value will only work if it is set to a value that is found in one of the radio buttons within the group.

Can you check that the value is equal to "true" or "false", otherwise throw an exception?
Attachment #8994701 - Flags: review?(jaws) → review+
Comment on attachment 8994700 [details]
Bug 1458308 - Move app.update.auto to be stored in the update directory

https://reviewboard.mozilla.org/r/259230/#review268810

::: browser/base/content/aboutDialog-appUpdater.js:146
(Diff revision 3)
>             gAppUpdater.aus.canStageUpdates;
>    },
>  
> -  // true when updating is automatic.
> -  get updateAuto() {
> -    try {
> +  // Returns a promise that resolves to true when updating is automatic.
> +  // Since the pref value needs to be read from a file, we do some caching here.
> +  updateAuto() {

It looks like there is only one path that the value of updateAuto is checked in updateCheckListener:onCheckComplete. Since there isn't an option to recheck in what instance will the cached value be used?

::: toolkit/mozapps/update/nsIUpdateService.idl:367
(Diff revision 3)
>     */
>    readonly attribute boolean canCheckForUpdates;
>  
>    /**
> +   * Determines whether or not the Update Service automatically downloads and
> +   * installs updates. This corresponds to whether or not the user has selected

Please mention that this setting is shared across all profiles for the installation here and below.

::: toolkit/mozapps/update/nsIUpdateService.idl:375
(Diff revision 3)
> +   * Note: This preference is stored in a file on the disk, so this operation
> +   *       involves reading that file.
> +   *
> +   * @return A Promise that resolves with a boolean.
> +   */
> +  jsval autoUpdateIsEnabled();

Is there any reason not to go with getAutoUpdateIsEnabled so it is consistent with setAutoUpdateIsEnabled?

::: toolkit/mozapps/update/nsUpdateService.js:56
(Diff revision 3)
> +// Note that although this pref has the same format as those above, it is very
> +// different. It is not stored as part of the user's prefs. Instead it is stored
> +// in the file indicated by FILE_UPDATE_PREFS, which will be located in the
> +// update directory. This allows it to be accessible from all Firefox profiles
> +// and from the Background Update Agent.
> +const PREF_APP_UPDATE_AUTO                 = "app.update.auto";

To make it clear that this isn't part of the preference system how about naming the const CONFIG_APP_UPDATE_AUTO, JSON_APP_UPDATE_AUTO, or something similar?

::: toolkit/mozapps/update/nsUpdateService.js:58
(Diff revision 3)
> +// in the file indicated by FILE_UPDATE_PREFS, which will be located in the
> +// update directory. This allows it to be accessible from all Firefox profiles
> +// and from the Background Update Agent.
> +const PREF_APP_UPDATE_AUTO                 = "app.update.auto";
> +// The default value for the above pref
> +const DEFAULT_VALUE_APP_UPDATE_AUTO        = true;

nit: naming for defaults in this file don't include VALUE so just DEFAULT_APP_UPDATE_AUTO should suffice.

::: toolkit/mozapps/update/nsUpdateService.js:80
(Diff revision 3)
>  const FILE_UPDATE_LOG        = "update.log";
>  const FILE_UPDATE_MAR        = "update.mar";
>  const FILE_UPDATE_STATUS     = "update.status";
>  const FILE_UPDATE_TEST       = "update.test";
>  const FILE_UPDATE_VERSION    = "update.version";
> +const FILE_UPDATE_PREFS      = "prefs.json";

To make it clear that this isn't part of the general preference system how about calling it update-config.json or something similar? Also, the name of the const would be FILE_UPDATE_CONFIG_JSON for the above name (see other const names above for examples).

::: toolkit/mozapps/update/nsUpdateService.js:2367
(Diff revision 3)
> +  // Used for serializing reads and writes of the app.update.auto pref file.
> +  // This is especially important for making sure writes don't happen out of
> +  // order.
> +  _updateAutoIOPromise: Promise.resolve(),
> +
> +  _readUpdateAutoPref: async function AUS__readUpdateAuto() {

If you go with a different name than pref as suggested previously it would be good to rename _readUpdateAutoPref and _writeUpdateAutoPref to something like _readUpdateAutoConfig and _writeUpdateAutoConfig as well as rename variables, etc.

::: toolkit/mozapps/update/nsUpdateService.js:2398
(Diff revision 3)
> +  },
> +
> +  /**
> +   * See nsIUpdateService.idl
> +   */
> +  autoUpdateIsEnabled: function AUS_autoUpdateIsEnabled() {

Note: It would be nice if a missing file returned the default value to avoid the file read in the typical case but the missing file case is used to migrate from preferences. Since this is async I don't think it is worthwhile changing this.

::: toolkit/mozapps/update/nsUpdateService.js:2406
(Diff revision 3)
> +    // which is when writing fails and the error is logged and re-thrown. All
> +    // other possible exceptions are wrapped in try blocks.
> +    let readPromise = this._updateAutoIOPromise.catch(() => {}).then(async () => {
> +      try {
> +        return await this._readUpdateAutoPref();
> +      } catch (error) {

There are 477 instances of catch (error) and 19,162 instances of catch (e) in dxr. Also, this file typically uses catch (e). Is there a new JavaScript style recommendation to use catch (error)?

::: toolkit/mozapps/update/nsUpdateService.js:2416
(Diff revision 3)
> +          prefValue = Services.prefs.getBoolPref(
> +            UNMIGRATED_PREF_APP_UPDATE_AUTO,
> +            DEFAULT_VALUE_APP_UPDATE_AUTO);
> +
> +          return await this._writeUpdateAutoPref(prefValue);
> +        } catch (error) {

error is being reused in the catch. If you go with e instead the typical naming would be ex.
Attachment #8994700 - Flags: review?(robert.strong.bugs) → review-
This will make other OS's use the file method which won't always work on Linux since the updates directory is in the installation directory which won't always be writable. It might be a good thing to just implement this for Windows for the time being to avoid this and to simplify migration of the value when other OS's support the common location.
Comment on attachment 8994703 [details]
Bug 1458308 - Tests for migration and UI of app.update.auto pref

https://reviewboard.mozilla.org/r/259236/#review269552

I'd like to see this again after it has been updated.

::: toolkit/mozapps/update/tests/browser/browser_updateAutoPrefUI.js:7
(Diff revision 4)
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/.
> + */
> +
> +ChromeUtils.import("resource://gre/modules/osfile.jsm", this);
> +const aus = Cc["@mozilla.org/updates/update-service;1"]

This isn't needed since gAUS is available in these tests

::: toolkit/mozapps/update/tests/browser/browser_updateAutoPrefUI.js:11
(Diff revision 4)
> +ChromeUtils.import("resource://gre/modules/osfile.jsm", this);
> +const aus = Cc["@mozilla.org/updates/update-service;1"]
> +            .getService(Ci.nsIApplicationUpdateService);
> +
> +
> +const FILE_UPDATE_PREFS = "prefs.json";

Please add this to data/shared.js and change the name per the other review

::: toolkit/mozapps/update/tests/browser/browser_updateAutoPrefUI.js:14
(Diff revision 4)
> +
> +
> +const FILE_UPDATE_PREFS = "prefs.json";
> +
> +let prefFile = getUpdatesRootDir();
> +prefFile.append(FILE_UPDATE_PREFS);

Please add a helper function for getting this file in data/shared.js and move the call to get the file to where it is used and use it in the other test as well.

::: toolkit/mozapps/update/tests/browser/browser_updateAutoPrefUI.js:15
(Diff revision 4)
> +
> +const FILE_UPDATE_PREFS = "prefs.json";
> +
> +let prefFile = getUpdatesRootDir();
> +prefFile.append(FILE_UPDATE_PREFS);
> +let decoder = new TextDecoder();

Can't this be moved to where it is used?

::: toolkit/mozapps/update/tests/unit_aus_update/updateAutoPrefMigrate.js:7
(Diff revision 4)
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/.
> + */
> +
> +ChromeUtils.import("resource://gre/modules/osfile.jsm", this);
> +let aus = Cc["@mozilla.org/updates/update-service;1"]

This isn't needed since gAUS is available in these tests
Attachment #8994703 - Flags: review?(robert.strong.bugs) → review-
Comment on attachment 8994702 [details]
Bug 1458308 - Change telemetry to use the new auto update lookup mechanism

https://reviewboard.mozilla.org/r/259234/#review267270

I don't see anything in there specifically testing the timing, but I suppose they do exercise the code for the important cases.
Comment on attachment 8994702 [details]
Bug 1458308 - Change telemetry to use the new auto update lookup mechanism

https://reviewboard.mozilla.org/r/259234/#review269574
Attachment #8994702 - Flags: review?(chutten) → review+
This patch additionally includes support for automatic migration of the pref from its old location to its new location.

This patch does not fix telemetry reporting of app.update.auto - that will be addressed in another patch in the same series.

MozReview-Commit-ID: KjX1mmGVB8M
This change makes TelemetryEnvironment.settings.update.autoDownload load asynchronously. Now, a file needs to be read before that value can be written. This has the potential to delay the initialization of TelemetryEnvironment, but since it should be pretty quick and it runs in parallel with the other asynchronously loading values of TelemetryEnvironment, it should not delay initialization significantly.

MozReview-Commit-ID: 1UUAi4sDopX

Depends on D4591
Comment on attachment 9005003 [details]
Bug 1458308 - Change telemetry to use the new auto update lookup mechanism r=chutten

Chris H-C :chutten has approved the revision.
Attachment #9005003 - Flags: review+
Attachment #8994700 - Attachment is obsolete: true
Attachment #8994701 - Attachment is obsolete: true
Attachment #8994701 - Flags: review?(brjones)
Attachment #8994702 - Attachment is obsolete: true
Attachment #8994703 - Attachment is obsolete: true
Comment on attachment 9005001 [details]
Bug 1458308 - Change about:preferences to use the new auto update pref mechanism r=jaws

Francesco Lodolo [:flod] has approved the revision.
Attachment #9005001 - Flags: review+
Comment on attachment 9005001 [details]
Bug 1458308 - Change about:preferences to use the new auto update pref mechanism r=jaws

Jared Wein [:jaws] (please needinfo? me) has approved the revision.
Attachment #9005001 - Flags: review+
Any reason for this bug not having landed yet? Just noticed it in my phabricator dashboard.
Flags: needinfo?(ksteuber)
(In reply to Francesco Lodolo [:flod] from comment #43)
> Any reason for this bug not having landed yet? Just noticed it in my
> phabricator dashboard.
Bug 1458314 which is still open and this bug depends on
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #44)
> (In reply to Francesco Lodolo [:flod] from comment #43)
> > Any reason for this bug not having landed yet? Just noticed it in my
> > phabricator dashboard.
> Bug 1458314 which is still open and this bug depends on

Thanks, clearing NI. This is going to be a fun rebase.
Flags: needinfo?(ksteuber)
Clearing this NI, we're going to land these strings as is a file a follow up on updating them if we decide we need to.
Flags: needinfo?(brjones)
Pushed by ksteuber@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7370877bd9e1
Move app.update.auto to be stored in the update directory on Windows only r=rstrong
https://hg.mozilla.org/integration/autoland/rev/99317c8cd247
Change about:preferences to use the new auto update pref mechanism r=flod,jaws
https://hg.mozilla.org/integration/autoland/rev/51e675ce6c56
Change telemetry to use the new auto update lookup mechanism r=chutten
https://hg.mozilla.org/integration/autoland/rev/4bf34689d4b6
Tests for migration and UI of app.update.auto pref r=rstrong
See Also: → 1503341
Blocks: 1503341
See Also: 1503341 → 
I'm investigating
Flags: needinfo?(ksteuber)
There are also failures on browser chrome that started from here: 
Treeherder link: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&searchStr=browser%2Cchrome&selectedJob=208686487&revision=4bf34689d4b62ed8299e1239ec9c01a4a6833e38 

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=208686487&repo=autoland&lineNumber=5528

18:26:10     INFO - Console message: AUS:SVC Downloader:_verifyDownload called
18:26:10     INFO - Console message: AUS:SVC Downloader:_verifyDownload downloaded size == expected size.
18:26:10     INFO - Console message: AUS:SVC isServiceInstalled - returning true
18:26:10     INFO - Console message: AUS:SVC shouldUseService - returning true
18:26:10     INFO - Console message: AUS:SVC getCanStageUpdates - staging updates is disabled by preference app.update.staging.enabled
18:26:10     INFO - Console message: AUS:SVC Downloader:onStopRequest - setting state to: pending-service
18:26:10     INFO - Console message: AUS:SVC getCanStageUpdates - staging updates is disabled by preference app.update.staging.enabled
18:26:10     INFO - Console message: AUS:SVC showUpdateDownloaded - Notifying observers that an update was downloaded. topic: update-downloaded, status: pending-service
18:26:10     INFO - Buffered messages finished
18:26:10     INFO - TEST-UNEXPECTED-FAIL | toolkit/mozapps/update/tests/browser/browser_TelemetryUpdatePing.js | The right notification showed up. - Got update-restart, expected update-available
18:26:10     INFO - Stack trace:
18:26:10     INFO - chrome://mochikit/content/browser-test.js:test_is:1295
18:26:10     INFO - chrome://mochitests/content/browser/toolkit/mozapps/update/tests/browser/head.js:processStep/<:214
18:26:10     INFO - chrome://mochitests/content/browser/toolkit/mozapps/update/tests/browser/head.js:processStep:209
18:26:10     INFO - chrome://mochitests/content/browser/toolkit/mozapps/update/tests/browser/head.js:runUpdateTest/<:148
18:26:10     INFO - chrome://mochitests/content/browser/toolkit/mozapps/update/tests/browser/head.js:runUpdateTest:111
18:26:10     INFO - chrome://mochitests/content/browser/toolkit/mozapps/update/tests/browser/browser_TelemetryUpdatePing.js:testUpdatePingReady:27
18:26:10     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1093
18:26:10     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1084
18:26:10     INFO - chrome://mochikit/content/browser-test.js:nextTest/<:986
18:26:10     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:795
18:26:54     INFO - Not taking screenshot here: see the one that was previously logged
18:26:54     INFO - TEST-UNEXPECTED-FAIL | toolkit/mozapps/update/tests/browser/browser_TelemetryUpdatePing.js | Test timed out - 
18:26:54     INFO - GECKO(5216) | *** AUS:SVC UpdateManager:_loadXMLFileIntoArray - XML file does not exist. path: C:\ProgramData\Mozilla\updates\1C6CEA9BC0919C65\updates.xml
18:26:54     INFO - Console message: AUS:SVC UpdateManager:_loadXMLFileIntoArray - XML file does not exist. path: C:\ProgramData\Mozilla\updates\1C6CEA9BC0919C65\updates.xml
18:26:54     INFO - GECKO(5216) | MEMORY STAT | vsize 1687MB | vsizeMaxContiguous 132100807MB | residentFast 212MB | heapAllocated 65MB
18:26:54     INFO - TEST-OK | toolkit/mozapps/update/tests/browser/browser_TelemetryUpdatePing.js | took 45068ms
Flags: needinfo?(ksteuber)
Flags: needinfo?(ksteuber)
This patch also contains a minor change to clear the user value set to the pref app.update.auto after the value is migrated.

Depends on D4594
`app.update.auto` should actually never have been needed here. `app.update.disabledForTesting`, and before that, `app.update.enabled` will prevent updates altogether. Now that the `app.update.auto` pref is not the correct mechanism for disabling automatic update, this pref should be removed from these files.

`app.update.enabled` can also be removed from prefs.rs at this time as per the comment in the file indicating that it can be removed when Firefox 62 stabilizes.

Depends on D10779
If you look at the try run there are several tests that fail during the parallel run and then pass when they are run sequentially.

toolkit/mozapps/update/tests/unit_base_updater/invalidArgStageDirNotInInstallDirFailure_win.js
toolkit/mozapps/update/tests/unit_base_updater/invalidArgPatchDirPathTraversalFailure.js
toolkit/mozapps/update/tests/unit_base_updater/invalidArgWorkingDirPathRelativeFailure.js
toolkit/mozapps/update/tests/unit_base_updater/marRMRFDirFileInUseStageFailureComplete_win.js
toolkit/mozapps/update/tests/unit_base_updater/marPIDPersistsSuccessComplete_win.js
toolkit/mozapps/update/tests/unit_base_updater/marRMRFDirFileInUseStageFailurePartial_win.js
toolkit/mozapps/update/tests/unit_aus_update/updateAutoPrefMigrate.js
toolkit/mozapps/update/tests/unit_aus_update/uiAutoPref.js

When I only ran mach test toolkit/mozapps/update/tests/unit_aus_update/ I had the following results where the tests failed the parallel run and then passed when they are run sequentially.

1st run parallel failed
toolkit/mozapps/update/tests/unit_aus_update/uiAutoPref.js failed or timed out, will retry.
toolkit/mozapps/update/tests/unit_aus_update/uiSilentPref.js failed or timed out, will retry.
toolkit/mozapps/update/tests/unit_aus_update/uiUnsupportedAlreadyNotified.js failed or timed out, will retry.
toolkit/mozapps/update/tests/unit_aus_update/updateAutoPrefMigrate.js failed or timed out, will retry.
toolkit/mozapps/update/tests/unit_aus_update/updateManagerXML.js failed or timed out, will retry.

2nd run parallel failed
toolkit/mozapps/update/tests/unit_aus_update/urlConstruction.js failed or timed out, will retry.
toolkit/mozapps/update/tests/unit_aus_update/updateAutoPrefMigrate.js failed or timed out, will retry.
toolkit/mozapps/update/tests/unit_aus_update/uiSilentPref.js

3rd run parallel failed
toolkit/mozapps/update/tests/unit_aus_update/urlConstruction.js
toolkit/mozapps/update/tests/unit_aus_update/downloadMissingMar.js
toolkit/mozapps/update/tests/unit_aus_update/updateManagerXML.js
toolkit/mozapps/update/tests/unit_aus_update/downloadResumeForSameAppVersion.js

4th run parallel failed
toolkit/mozapps/update/tests/unit_aus_update/cleanupPendingVersionFileIncorrectStatus.js
toolkit/mozapps/update/tests/unit_aus_update/cleanupSuccessLogMove.js
toolkit/mozapps/update/tests/unit_aus_update/downloadCompleteAfterPartialFailure.js
toolkit/mozapps/update/tests/unit_aus_update/updateDirectoryMigrate.js
toolkit/mozapps/update/tests/unit_aus_update/updateAutoPrefMigrate.js
toolkit/mozapps/update/tests/unit_aus_update/urlConstruction.js
The try run I checked above was the Windows 10 x64 debug
I'm seeing the following during the parallel run
[createWorldWritableAppUpdateDir : 1689] The helper process exit value should be 0 - 1 == 0","stack":"../data/xpcshellUtilsAUS.js:createWorldWritableAppUpdateDir:1689\n../data/xpcshellUtilsAUS.js:setupTestCommon:841"
It looks to me like this is a problem with multiple instances of TestAUSHelper running in parallel and all trying to set permissions on the same directory. Since the problem with the update directory's permissions on our test images was fixed in Bug 1494048, I'll just change TestAUSHelper to no longer do this. Applying this change locally, seems to fix problems running the tests locally.

It occurs to me, however, that commonupdatedir.cpp may have a minor race condition where if the directory is created between the `GetFileAttributesW` call and the `CreateDirectoryW` call, `GetCommonUpdateDirectory` may (incorrectly) return an error. Since this function will be running multiple times in parallel, it is probably important to fix this race condition. I will include that change in my update to the patch.
Attachment #9022318 - Attachment is obsolete: true
Pushed by ksteuber@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/54dd159fd0a6
Move app.update.auto to be stored in the update directory on Windows only r=rstrong
https://hg.mozilla.org/integration/autoland/rev/dc4048b0f691
Change about:preferences to use the new auto update pref mechanism r=jaws,flod
https://hg.mozilla.org/integration/autoland/rev/c9ffe7510d09
Change telemetry to use the new auto update lookup mechanism r=chutten
https://hg.mozilla.org/integration/autoland/rev/c6a1aa25f4fd
Tests for migration and UI of app.update.auto pref r=rstrong
https://hg.mozilla.org/integration/autoland/rev/1b57a28b1e90
Change lookups of app.update.auto to use nsIApplicationUpdateService::(get|set)AutoUpdateIsEnabled r=rstrong
https://hg.mozilla.org/integration/autoland/rev/663e1f142dd4
Remove app.update.auto and app.update.enabled from prefs.rs and marionette.js r=rstrong,ato
Depends on: 1505338
No longer depends on: 1505790
Depends on: 1506983
No longer depends on: 1506983

Following the STR's provided by Kirk via slack:

  1. The value of app.update.auto is properly migrated to the config file after update (config file will be located at C:\ProgramData\Mozilla\updates\<hash>\update-config.json).
  2. Changes to the auto-update setting in about:preferences are propagated to the config file.
  3. After changing the value from one instance of Firefox, that the new value is properly picked up by another instance of Firefox.
  4. If the file is deleted or corrupted, that Firefox will revert to using the default value (app.update.auto = true).
    To ensure that this feature is working as expected I managed to run those tests and got a positive results.
Status: RESOLVED → VERIFIED
status-firefox65: fixed → verified
Summary: Move update prefs out of profile → Move update pref(s) including app.update.auto out of profile
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: