In order for the Background Update Agent to work, it needs to be able to access the update directory. This is a problem in the current configuration, because the update directory is in a user's home directory. Not only does the Background Update Agent not know which user's home directory to look in, but it may not have permissions to access any a user's home directory. An additional benefit of this change should be that if an update download is in progress, and then the user logs out and another user logs in, the new user should be able to resume the download started by the old user. It seems that the proper location for this directory in Windows is the %ProgramData% directory, which is intended for storing non-user-specific application data.

This adds a file stat on every startup which I think it is best to avoid. We had something similar in the past which you can find at the link below that avoids this. https://dxr.mozilla.org/mozilla-esr24/source/toolkit/mozapps/update/nsUpdateServiceStub.js#149

Comment on attachment 8985713 [details] Bug 1458314 - Move the update directory to an installation specific location https://reviewboard.mozilla.org/r/251256/#review263552 Discussed on irc and the suggested approach is agreed upon in general so removing r?

Comment on attachment 8985713 [details] Bug 1458314 - Move the update directory to an installation specific location https://reviewboard.mozilla.org/r/251256/#review268046 ::: toolkit/mozapps/update/nsUpdateServiceStub.js:46 (Diff revision 5) > let statusFile = getUpdateDirNoCreate([DIR_UPDATES, "0"]); > statusFile.append(FILE_UPDATE_STATUS); > + > + // We may need to migrate update data > + if (AppConstants.platform == "win" && > + !Services.prefs.getBoolPref(PREF_UPDATE_DIR_MIGRATED, false)) { This will only migrate once so clients using the same profile with multiple installations won't have the other installations migrated. I think this is rare enough that this is ok since the majority of clients won't have multiple installs but please add a comment to this affect or change this to migrate based on the ID used for the directory name. ::: toolkit/mozapps/update/nsUpdateServiceStub.js:78 (Diff revision 5) > + let destRootDir = FileUtils.getDir(KEY_UPDROOT, [], true); > + > + sourceRootDir.normalize(); > + destRootDir.normalize(); > + if (sourceRootDir.equals(destRootDir)) { > + LOG("UpdateService:_migrateUpdateDirectory - Abort: No migration " + Please change UpdateService to UpdateServiceStub in all of the LOG messages. ::: toolkit/mozapps/update/nsUpdateServiceStub.js:104 (Diff revision 5) > + LOG("UpdateService:_migrateUpdateDirectory - Abort: No migration " + > + "necessary. Nothing to migrate."); > + return; > + } > + > + if (destStatusFile.exists()) { Why not just check if destRootDir exists? Also, the status file won't exist after migration and an update is applied. If you go this route please move this and |if (!sourceRootDir.exists()) {| to just after setting these variables. ::: toolkit/mozapps/update/tests/data/shared.js:46 (Diff revision 5) > const NS_GRE_DIR = "GreD"; > const NS_GRE_BIN_DIR = "GreBinD"; > const NS_XPCOM_CURRENT_PROCESS_DIR = "XCurProcD"; > const XRE_EXECUTABLE_FILE = "XREExeF"; > const XRE_UPDATE_ROOT_DIR = "UpdRootD"; > +const XRE_OLD_UPDATE_ROOT_DIR = "OldUpdRootD"; nit: please move this above XRE_UPDATE_ROOT_DIR to manintain sorting. If you feel like moving NS_GRE_BIN_DIR to fix that sorting at the same time please do so. Thanks! ::: toolkit/mozapps/update/tests/unit_aus_update/updateDirectoryMigrate.js:133 (Diff revision 5) > MSG_SHOULD_EQUAL); > > + let oldDir = getOldUpdatesRootDir(); > + let newDir = getUpdatesRootDir(); > + if (oldDir.path != newDir.path) { > + Assert.ok(!oldDir.exists(), Is it possible to perform this check in the test itself? ::: toolkit/mozapps/update/tests/unit_aus_update/xpcshell.ini:31 (Diff revision 5) > [downloadResumeForSameAppVersion.js] > [downloadCompleteAfterPartialFailure.js] > [uiSilentPref.js] > [uiUnsupportedAlreadyNotified.js] > [uiAutoPref.js] > +[updateDirectoryMigrate.js] This should be a Windows only test... right? ::: toolkit/xre/nsXREDirProvider.h:82 (Diff revision 5) > + * If aGetOldLocation is true, this function will return the location of > + * the update directory before it was moved from the user profile directory > + * to a per-installation directory. This functionality is only meant to be > + * used for migration of the update directory to the new location. > */ > - nsresult GetUpdateRootDir(nsIFile* *aResult); > + nsresult GetUpdateRootDir(nsIFile** aResult, bool aGetOldLocation = false); This should probably throw NS_ERROR_NOT_IMPLEMENTED for plaforms other than Windows when aGetOldLocation is true.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #15) > ::: > toolkit/mozapps/update/tests/unit_aus_update/updateDirectoryMigrate.js:133 > (Diff revision 5) > > MSG_SHOULD_EQUAL); > > > > + let oldDir = getOldUpdatesRootDir(); > > + let newDir = getUpdatesRootDir(); > > + if (oldDir.path != newDir.path) { > > + Assert.ok(!oldDir.exists(), > > Is it possible to perform this check in the test itself? I thought this was in a different file... please ignore and I dropped the issue in reviewboard

Last review update was only a rebase. This should make the next interdiff more readable.

The removal of the directory on install and uninstall will need to be done. https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/windows/nsis/common.nsh#3231

It looks like all of the changes weren't picked up https://reviewboard.mozilla.org/r/251256/diff/5-6/

The only changes that should be visible in that interdiff are changes that are part of the rebase. Which is why I did a rebase-only push. So that the next interdiff will no contain changes that I did not make.

Doh! Sorry about that

Comment on attachment 8985713 [details] Bug 1458314 - Move the update directory to an installation specific location https://reviewboard.mozilla.org/r/251256/#review268046 > Why not just check if destRootDir exists? Also, the status file won't exist after migration and an update is applied. > > If you go this route please move this and > |if (!sourceRootDir.exists()) {| > > to just after setting these variables. Makes sense. I will do that, but I am keeping the `if (destRootDir.exists())` check after the `if (sourceRootDir.equals(destRootDir))` because I do not want `sourceRootDir.remove(true);` to be called in that case.

I have addressed the review comments, but I want to address Comment 19 before I post the new patch.

Hmm. It seems that the changes I made are causing the test added by this patch to fail. Investigating...

Ok, I figured out the problem. A call ends up being made to `nsUpdateService.getUpdateDirCreate()` early in Firefox's startup. This was causing the new check `destRootDir.exists()` to *always* return `true`, causing the migration to abort (thinking that migration had already occurred). I changed the `destRootDir.exists()` check to a check for whether `destRootDir` contains any files. I think that this is the best option available.

Have you tested write access to this directory with multiple Windows accounts? One example of what I am referring to: https://stackoverflow.com/questions/22107812/privileges-owner-issue-when-writing-in-c-programdata

I just tried it with two Windows accounts and it seemed to work fine on both. I suspect that the exact problem you are describing may actually be what led me to use FOLDERID_PublicDocuments instead of FOLDERID_ProgramData. I had some weird permissions issues that seemed to be causing things to work incorrectly. Then when I checked the default permissions of that directory, I realized that regular users do not have write permission for it. I have now looked at the *Advanced* security window and see that the permissions are actually more complicated, as described in the stackoverflow link that you posted. It looks like potentially we could go back to using FOLDERID_ProgramData and make sure that when the directory is created, it is given the expected permissions. But frankly I don't see a reason to. It seems like using FOLDERID_PublicDocuments sidesteps the entire issue.

Hmm. Actually I can think of one reason to move it back. Windows 10 users that manually turned Libraries back on as well as all Windows 7 users will probably start to see this in their Documents Library. Maybe its worth moving it back and setting the ACL properly. I'll start working on that.

Comment on attachment 8985713 [details] Bug 1458314 - Move the update directory to an installation specific location https://reviewboard.mozilla.org/r/251256/#review269554 Just a minor nit to be included with the next patch ::: toolkit/mozapps/update/tests/unit_aus_update/xpcshell.ini:32 (Diff revision 9) > [downloadCompleteAfterPartialFailure.js] > [uiSilentPref.js] > [uiUnsupportedAlreadyNotified.js] > [uiAutoPref.js] > +[updateDirectoryMigrate.js] > +skip-if = os != 'win' # Update directory migration happens on Windows only Remove the comment after # and add something along the lines of the following below the skip-if reason = Update directory migration is currently Windows only

I think it would be better to go with the computer's Users group since it Authenticated Users and Interactive since that is what is required to launch the Maintenance Service. https://dxr.mozilla.org/mozilla-central/source/toolkit/components/maintenanceservice/serviceinstall.cpp#648

That makes sense. I'll make that change.

This change applies to Windows only. Firefox will need to migrate the directory from the old location to the new location. This will be done only once by setting the pref `app.update.migrated.updateDir2` to true once migration has completed. Note: The pref name app.update.migrated.updateDir has already been used, thus the '2' suffix. It can be found in ESR24. This patch re-adds constants used for obtaining the common application data directory in Windows. These constants were removed in Bug 1449686 and have been returned to the codebase with their original values. MozReview-Commit-ID: AFVMlAcT2LG

Comment on attachment 8985713 [details] Bug 1458314 - Move the update directory to an installation specific location Migrating to Phabricator now that mozreview is offline...

Robert- I cannot request a review from you. I am guessing that you do not have a Phabricator account?

Created

A potential issue: When creating a directory with security attributes, it does not automatically inherit permissions from its parent directory. One effect of giving permission only to the Users group is that LocalSystem cannot access it (though LocalService can as it is like a logged on user), so the updater would have to juggle LocalSystem and impersonated LocalService to touch Program Files. This may be desirable as it prevents us from unintentionally accessing ProgramData\Mozilla as LocalSystem, but there may be other unwanted side effects and it would add some complication to the updater.

Here's a modification of CreateWorldWritableAppUpdateDir that creates the directory with default (inherited) permissions and then adds the builtin Users group.

Comment on attachment 9004065 [details] Adding permissions to update directory The new patch adds permissions for Administrators and SYSTEM.

Comment on attachment 9003871 [details] Bug 1458314 - Move the update directory to an installation specific location Robert Strong [:rstrong] (use needinfo to contact me) has approved the revision.

This patch seems to be about ready to merge. However, because it implements particularly large changes, I am going to rebase and a try run before merging.

I verified after the staging phase on try that the BUILTIN\Users group only has Read access on the update.status file. BUILTIN\Administrators and NT AUTHORITY\SYSTEM both have full control.

The output on try of cacls when run against C:\ProgramData\Mozilla NT AUTHORITY\SYSTEM:(OI)(CI)(ID)F BUILTIN\Administrators:(OI)(CI)(ID)F CREATOR OWNER:(OI)(CI)(IO)(ID)F BUILTIN\Users:(OI)(CI)(ID)R BUILTIN\Users:(CI)(ID)(special access:) FILE_WRITE_DATA FILE_APPEND_DATA FILE_WRITE_EA FILE_WRITE_ATTRIBUTES

I ran a try build where it intentionally failed during test setup and logged whether the C:\ProgramData\Mozilla directory existed and it already existed for every update test. I then changed the directory used by app update to C:\ProgramData\Mozilla2 and all of the tests passed. https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac77e0fe20ffe6a042384ced0fe1732461230ade&selectedJob=197007412 So, this appears to be an anomaly with the try systems but it is possible that there are systems in the wild that already have a C:\ProgramData\Mozilla directory with these permissions. I've also had issues in the past and learned that the build systems don't always reboot and apply a fresh image for every run so it is possible that we are just hitting systems that had the directory created incorrectly during try runs though I don't know what the current policy is for this. Kirk, let's discuss what the best path forward for this might be next week.

Comment on attachment 9003871 [details] Bug 1458314 - Move the update directory to an installation specific location Robert Strong [:rstrong] (use needinfo to contact me) has requested changes to the revision.

Pushed by ksteuber@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3d22697d9c23 Move the update directory to an installation specific location r=rstrong

Kirk, I'd like to port this to Thunderbird. Can we directly copy https://hg.mozilla.org/mozilla-central/rev/3d22697d9c23a23087190225aa201a44bc1be130#l1.12 or should we use a different updatedir name?

To be honest, I don't know a lot about how Thunderbird or Thunderbird update works. I see no particular reason that it couldn't use the same update directory name. Firefox includes a hash of its installation path in the update directory name, so it shouldn't cause a conflict or anything. Although the directory name that it creates in the installer is hardcoded to "Mozilla", the function that generates the update directory path uses the vendor name for that directory (which should also be "Mozilla"). I believe that Thunderbird also has the vendor name "Mozilla", so that shouldn't be a problem. It seems to me that you should be fine using the same update directory.

Filed bug 1501792 (Thunderbird) and bug 1501790 (SeaMonkey) to port the changes.