Closed
Bug 1458314
Opened 6 years ago
Closed 6 years ago
Move the update directory to a common installation specific location
Categories
(Toolkit :: Application Update, enhancement, P1)
Tracking
()
RESOLVED
FIXED
mozilla65
Tracking | Status | |
---|---|---|
firefox65 | --- | fixed |
People
(Reporter: bytesized, Assigned: bytesized)
References
(Blocks 3 open bugs)
Details
Attachments
(2 files, 2 obsolete files)
46 bytes,
text/x-phabricator-request
|
Details | Review | |
114.06 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → ksteuber
Assignee | ||
Comment 1•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=909673c255e9ed1cd91519001f8d569c322a3401
Assignee | ||
Comment 2•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=655f1d23f6988068d16d5e38eae13677aa75e72a
Assignee | ||
Comment 3•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b40ff4725e75d4735d1e52143f5b2fea8d2986a
Assignee | ||
Comment 4•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9244d6e38fd2ee34432044e684f9f189adeee694
Assignee | ||
Comment 5•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a220bfbfc8668dc24e8711765e3a9f6ed517e5f
Assignee | ||
Comment 6•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=221114779e1d58b5b52c3e83d3a431305b892f66
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=da39b5846bf78e56f525e1534171edf7749a8ad6
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8985713 -
Flags: review?(robert.strong.bugs)
Comment 11•6 years ago
|
||
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 12•6 years ago
|
||
mozreview-review |
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?
Attachment #8985713 -
Flags: review?(robert.strong.bugs)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 15•6 years ago
|
||
mozreview-review |
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.
Attachment #8985713 -
Flags: review?(robert.strong.bugs) → review-
Comment 16•6 years ago
|
||
(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
Comment hidden (mozreview-request) |
Assignee | ||
Comment 18•6 years ago
|
||
Last review update was only a rebase. This should make the next interdiff more readable.
Comment 19•6 years ago
|
||
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
Updated•6 years ago
|
Summary: Move the update directory to an installation specific location → Move the update directory to a common installation specific location
Comment 20•6 years ago
|
||
It looks like all of the changes weren't picked up https://reviewboard.mozilla.org/r/251256/diff/5-6/
Assignee | ||
Comment 21•6 years ago
|
||
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.
Comment 22•6 years ago
|
||
Doh! Sorry about that
Assignee | ||
Comment 23•6 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 24•6 years ago
|
||
I have addressed the review comments, but I want to address Comment 19 before I post the new patch.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 27•6 years ago
|
||
Hmm. It seems that the changes I made are causing the test added by this patch to fail. Investigating...
Comment hidden (mozreview-request) |
Assignee | ||
Comment 29•6 years ago
|
||
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.
Comment 30•6 years ago
|
||
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
Assignee | ||
Comment 31•6 years ago
|
||
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.
Assignee | ||
Comment 32•6 years ago
|
||
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 33•6 years ago
|
||
mozreview-review |
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
Attachment #8985713 -
Flags: review?(robert.strong.bugs) → review-
Comment hidden (mozreview-request) |
Comment 35•6 years ago
|
||
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
Assignee | ||
Comment 36•6 years ago
|
||
That makes sense. I'll make that change.
Assignee | ||
Comment 37•6 years ago
|
||
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
Assignee | ||
Comment 38•6 years ago
|
||
Comment on attachment 8985713 [details] Bug 1458314 - Move the update directory to an installation specific location Migrating to Phabricator now that mozreview is offline...
Attachment #8985713 -
Flags: review?(robert.strong.bugs)
Assignee | ||
Comment 39•6 years ago
|
||
Robert- I cannot request a review from you. I am guessing that you do not have a Phabricator account?
Flags: needinfo?(robert.strong.bugs)
Comment 41•6 years ago
|
||
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.
Comment 42•6 years ago
|
||
Here's a modification of CreateWorldWritableAppUpdateDir that creates the directory with default (inherited) permissions and then adds the builtin Users group.
Assignee | ||
Updated•6 years ago
|
Attachment #8985713 -
Attachment is obsolete: true
Assignee | ||
Comment 43•6 years ago
|
||
Comment on attachment 9004065 [details]
Adding permissions to update directory
The new patch adds permissions for Administrators and SYSTEM.
Attachment #9004065 -
Attachment is obsolete: true
Comment 44•6 years ago
|
||
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.
Attachment #9003871 -
Flags: review+
Assignee | ||
Comment 45•6 years ago
|
||
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.
Assignee | ||
Comment 46•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cde95de13821f2d5c36900452261d576fdbbb873
Assignee | ||
Comment 47•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f318050f9845708af3f335b8ada00e145406314e
Assignee | ||
Comment 48•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bad59505b6cd242d90015e41e8ae58665678ffd8
Comment 49•6 years ago
|
||
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.
Assignee | ||
Comment 50•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b850e502e02ce47dde30fd763e167421b4e36ae6
Assignee | ||
Comment 51•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b0528c187e2248c0fddeaf85c2eb45574ec8ed9
Comment 52•6 years ago
|
||
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
Assignee | ||
Comment 53•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3033db3d022b5865087a07f5e4baabd06fd8c439
Assignee | ||
Comment 54•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=212d8fd1613175133e8d93557faa6732746a7c40
Comment 55•6 years ago
|
||
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.
Assignee | ||
Comment 56•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad51449deb81cdf475aec8a9b8ac801f937f1899
Assignee | ||
Comment 57•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7124016696adb8211ec49194ec915702c8c5541c
Assignee | ||
Comment 58•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=eed0530445df66c52fb74273683fa0d6a034f264
Assignee | ||
Comment 59•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca8de1c151ef773692fce2e98247241b31c71d3c
Assignee | ||
Comment 60•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=363c7ed5939e8bfea56bf2a07588ad8fda1f50d5
Assignee | ||
Comment 61•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e55b03cd97da8e5e61f71fef0f79e36681f6ef2a
Assignee | ||
Comment 62•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d811b35f2287b2a297522665c41309dbe59dc4c3
Assignee | ||
Comment 63•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd9df22b779e30d813865e0b9dc6da621f5d0458
Assignee | ||
Comment 64•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=04832f08900ee9473f7a413af576343b75c530dd
Assignee | ||
Comment 65•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d46632cd9986512f2468257d5051f2f203dd1cc
Assignee | ||
Comment 66•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d54c36e1b0224871fce8da8a819766954054a499
Assignee | ||
Comment 67•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed4e47b1d142f85eaefd0a572cfac0d4e4ad966f
Assignee | ||
Comment 68•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a3891562cdc27a269451cd6d74e4a0efd7a0786
Assignee | ||
Comment 69•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6835cfad46386858b2c93b913ceaa606394c4bb
Assignee | ||
Comment 70•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=47da705d265133335c9cc06d67627224aea17229
Comment 71•6 years ago
|
||
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.
Attachment #9003871 -
Flags: review+
Assignee | ||
Comment 72•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0082412afb778a30424b65dc780961dffb0791e
Assignee | ||
Comment 73•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ddc1d8870d0bfd95f32ecb107425da9bcf3f2ef5
Assignee | ||
Comment 74•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e033ec5e26db28701b16e56b083e8cb1150375f
Assignee | ||
Comment 75•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8313fa35def6c1889dff9017a335ce75d16b11f6
Assignee | ||
Comment 76•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=009a13e701b350750a415850bc77105df684a484
Updated•6 years ago
|
Priority: -- → P1
Comment 77•6 years ago
|
||
Assignee | ||
Comment 78•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=54f70982a3d639b5b50ad7b0d3e68769fe69dd1b
Assignee | ||
Comment 79•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=21dd6bd4d919addefdd6711fca0d0bbf41441d35
Comment 80•6 years ago
|
||
Pushed by ksteuber@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3d22697d9c23 Move the update directory to an installation specific location r=rstrong
Comment 81•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3d22697d9c23
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox65:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Comment 82•6 years ago
|
||
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?
Flags: needinfo?(ksteuber)
Assignee | ||
Comment 83•6 years ago
|
||
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.
Flags: needinfo?(ksteuber)
Comment 84•6 years ago
|
||
Filed bug 1501792 (Thunderbird) and bug 1501790 (SeaMonkey) to port the changes.
You need to log in
before you can comment on or make changes to this bug.
Description
•