Closed
Bug 1458308
(update-prefs)
Opened 7 years ago
Closed 6 years ago
Move update pref(s) including app.update.auto out of profile
Categories
(Toolkit :: Application Update, enhancement, P1)
Toolkit
Application Update
Tracking
()
VERIFIED
FIXED
mozilla65
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)
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
flod
:
review+
jaws
:
review+
|
Details |
(deleted),
text/x-phabricator-request
|
chutten
:
review+
|
Details |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details |
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 | ||
Updated•7 years ago
|
Assignee: nobody → ksteuber
Assignee | ||
Comment 1•6 years ago
|
||
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.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
User Story: (updated)
Updated•6 years ago
|
Alias: update-prefs
Updated•6 years ago
|
Priority: -- → P1
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 8•6 years ago
|
||
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!
Comment 9•6 years ago
|
||
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!
Assignee | ||
Comment 10•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Attachment #8994700 -
Flags: review?(robert.strong.bugs)
Attachment #8994701 -
Flags: review?(jaws)
Attachment #8994703 -
Flags: review?(robert.strong.bugs)
Comment 11•6 years ago
|
||
mozreview-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'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 12•6 years ago
|
||
mozreview-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+
Comment 13•6 years ago
|
||
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)
Comment 14•6 years ago
|
||
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
Assignee | ||
Comment 15•6 years ago
|
||
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!
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 20•6 years ago
|
||
Last push was a rebase-only push. Just trying to make future interdiffs a little more readable.
Comment 21•6 years ago
|
||
mozreview-review |
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 22•6 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 27•6 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 28•6 years ago
|
||
mozreview-review-reply |
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()`.
Assignee | ||
Updated•6 years ago
|
Attachment #8994702 -
Flags: review?(chutten)
Assignee | ||
Comment 29•6 years ago
|
||
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)
Assignee | ||
Updated•6 years ago
|
Attachment #8994701 -
Flags: review?(brjones)
Comment 30•6 years ago
|
||
mozreview-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/#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 31•6 years ago
|
||
mozreview-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-
Comment 32•6 years ago
|
||
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 33•6 years ago
|
||
mozreview-review |
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 34•6 years ago
|
||
mozreview-review-reply |
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 35•6 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 36•6 years ago
|
||
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
Assignee | ||
Comment 37•6 years ago
|
||
Depends on D4590
Assignee | ||
Comment 38•6 years ago
|
||
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
Assignee | ||
Comment 39•6 years ago
|
||
Depends on D4593
Comment 40•6 years ago
|
||
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+
Assignee | ||
Updated•6 years ago
|
Attachment #8994700 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8994701 -
Attachment is obsolete: true
Attachment #8994701 -
Flags: review?(brjones)
Assignee | ||
Updated•6 years ago
|
Attachment #8994702 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8994703 -
Attachment is obsolete: true
Comment 41•6 years ago
|
||
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 42•6 years ago
|
||
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+
Comment 43•6 years ago
|
||
Any reason for this bug not having landed yet? Just noticed it in my phabricator dashboard.
Flags: needinfo?(ksteuber)
Comment 44•6 years ago
|
||
(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
Comment 45•6 years ago
|
||
(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)
Comment 46•6 years ago
|
||
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)
Comment 47•6 years ago
|
||
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
Assignee | ||
Updated•6 years ago
|
Comment 48•6 years ago
|
||
Backed out 4 changesets (bug 1458308) for causing uiAutoPref.js perma failures
push that caused the backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=208679800&revision=4bf34689d4b62ed8299e1239ec9c01a4a6833e38
failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&fromchange=b26a70a0fe8f22ee4a5118c7c563ab98a115e692&selectedJob=208679800&searchStr=windows%2C7%2Cdebug%2Cxpcshell%2Ctests%2Ctest-windows7-32%2Fdebug-xpcshell%2Cx%28x%29
https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&fromchange=b26a70a0fe8f22ee4a5118c7c563ab98a115e692&selectedJob=208679754&searchStr=windows%2C10%2Cx64%2Cdebug%2Cmochitests%2Ctest-windows10-64%2Fdebug-mochitest-chrome-3%2Cm%28c3%29
backout: https://hg.mozilla.org/integration/autoland/rev/f8c4d4c647ae8062fcd224a6baf9ec4a1843f632
Flags: needinfo?(ksteuber)
Comment 50•6 years ago
|
||
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)
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(ksteuber)
Assignee | ||
Comment 51•6 years ago
|
||
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
Assignee | ||
Comment 52•6 years ago
|
||
Assignee | ||
Comment 53•6 years ago
|
||
Assignee | ||
Comment 54•6 years ago
|
||
Assignee | ||
Comment 55•6 years ago
|
||
Assignee | ||
Comment 56•6 years ago
|
||
Assignee | ||
Comment 57•6 years ago
|
||
Depends on D10315
Assignee | ||
Comment 58•6 years ago
|
||
`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
Comment 59•6 years ago
|
||
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
Comment 60•6 years ago
|
||
The try run I checked above was the Windows 10 x64 debug
Comment 61•6 years ago
|
||
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"
Assignee | ||
Comment 62•6 years ago
|
||
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.
Assignee | ||
Comment 63•6 years ago
|
||
Assignee | ||
Comment 64•6 years ago
|
||
Updated•6 years ago
|
Attachment #9022318 -
Attachment is obsolete: true
Assignee | ||
Comment 65•6 years ago
|
||
Comment 66•6 years ago
|
||
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
Comment 67•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/54dd159fd0a6
https://hg.mozilla.org/mozilla-central/rev/dc4048b0f691
https://hg.mozilla.org/mozilla-central/rev/c9ffe7510d09
https://hg.mozilla.org/mozilla-central/rev/c6a1aa25f4fd
https://hg.mozilla.org/mozilla-central/rev/1b57a28b1e90
https://hg.mozilla.org/mozilla-central/rev/663e1f142dd4
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox65:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Comment 68•6 years ago
|
||
Following the STR's provided by Kirk via slack:
- 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).
- Changes to the auto-update setting in about:preferences are propagated to the config file.
- After changing the value from one instance of Firefox, that the new value is properly picked up by another instance of Firefox.
- 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
Updated•6 years ago
|
Updated•4 years ago
|
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.
Description
•