529464 - Investigate using different steps to improve update reliability

Status: RESOLVED WONTFIX

(Toolkit :: Application Update, defect)

Description

[Justin Dolske [:Dolske]]

Currently I suspect we're spending a lot of time *copying* to backup files. This should be a lot faster if we just rename files, and fiddle with some ordering to keep everything safe.

EG, currently we do:

Execute:
1) Copy foo to foo.backup
2) Delete foo
3) Create patched foo
4) Copy bar to bar.backup
5) Delete bar
6) Create patched bar
...
Finish:
7) Delete backups or do rollback (by copying!).

The new way would  be to:

Execute-Part1:
1) Create foo.patched
2) Create bar.patched
...
Execute-Part22:
3) Rename foo to foo.backup
4) Rename foo.patched to foo
5) Rename bar to bar.backup
6) Rename bar.patched to bar
...
Finish:
7) Delete any .patched files, or rollback (by renaming)

In addition to this being faster, it should be safer. Execute-Part1, where most of the work is done, is now perfectly failsafe. We can "rollback" (more of a cleanup) by just deleting *.patched, and even crashes / forced quits are safe because we have not modified anything. Execute-Part2 -- like the current Execute phase -- is not immune to crashes / forced quits, but it's safer because there's a smaller window of exposure and there's less work being done.

Comment 1

[Justin Dolske [:Dolske]]

Attachment: Patch v.1 (WIP)

This was itching at me today, so I went ahead and did it. Not quite finished, doesn't pass tests yet, but it's a good rough cut.

Comment 2

[Robert Strong (they/them - no direct email)]

dolske, thanks for doing this! I believe this is also a major step towards getting bug 466778 fixed as well so adding dependency.

Comment 3

[Justin Dolske [:Dolske]]

Attachment: Patch v.2 (WIP)

Cleaned up most stuff, now passes tests on OS X and Win32 (didn't try anywhere else). I think I've found a couple of bugs with the existing code, so I must be getting close to finished. ;-)

Comment 4

[Robert Strong (they/them - no direct email)]

From skimming the comments in the patch I believe the restoring of the existing perms of the file that is being updated / replaced might be what is confusing? Might also be the code for trying to fix perms that Seth added a while back as well. Neither of those are bugs though they are a tad confusing at first.

Comment 5

[Justin Dolske [:Dolske]]

Attachment: Patch v.3 (WIP)

Passes tests on OS X and Win7.

One change I made for simplifying the file permission stuff is to always use the permissions set in the MAR file. This allows us to correct permissions in the future if needed (eg, see bug 535090). I've temporarily hacked one of the tests to not fail, since the conditions it expects are different.

Comment 7

[Justin Dolske [:Dolske]]

Comment on attachment 419725 [details] [diff] [review]
Patch v.3 (WIP)

Flagging for an initial review, there are still a few things to address (see comments at top of patch and "XXX" comments inline), but they're fairly minor and specific.

I went through the patch over the weekend to get things back in my head, and didn't see anything to change that wasn't already marked.

Comment 8

[Robert Strong (they/them - no direct email)]

Comment on attachment 419725 [details] [diff] [review]
Patch v.3 (WIP)

>diff --git a/toolkit/mozapps/update/src/updater/updater.cpp b/toolkit/mozapps/update/src/updater/updater.cpp
>--- a/toolkit/mozapps/update/src/updater/updater.cpp
>+++ b/toolkit/mozapps/update/src/updater/updater.cpp
>@@ -1,8 +1,24 @@
>+// TODO Things to make tests for:
>+// * create preexisting .moz-backup / .moz-temp / .moz-patch files, to make sure we can remove them.
Filed bug 563784

>+// * more checks to ensure permissions are corrent through various cases?
I don't know of any additional permission cases that should be tested and there are tests that verify existing permissions are kept and new files get the permission set in the mar file.

>+// * umask checks?
What scenario are you thinking of that isn't covered by the existing tests?

>+// * ADD /path/doesnt/exist/newfile
Please remove... there is a test that adds new files to a non-existent path

>+// * action on /path/is/readonly/uhoh
Filed 563785

>+//
>+// XXX other things
>+// * should rename_file enforce that spath and dpath files be in the same dir?
I don't think so since the new file is the existing file name with a suffix defined in this code

>+// * can a manifest say "remove foo, add foo"? that would make rollback hard.
It can and we do but I believe the rollback works fine.

>+// * might want to set file modes at end of EXECUTE, so we could add/patch file that we want to end up as read-only.
>+//
>+// XXX Notes
>+// * In Action base class, mManifestTarget is old mFile, mFile is old mDestFile
nit: instead of 'is old' please change to 'was' or 'used to be' if you want to keep them. At first this comment made me think it was copying mFile to mManifestTarget, etc.

>+// * In PatchFile class, mManifestPatch is the old mPatchFile
nit: same here

I believe these notes were just for keeping track of questions and aren't going to be checked in.

>diff --git a/toolkit/mozapps/update/test/unit/test_0111_general.js b/toolkit/mozapps/update/test/unit/test_0111_general.js
>--- a/toolkit/mozapps/update/test/unit/test_0111_general.js
>+++ b/toolkit/mozapps/update/test/unit/test_0111_general.js
>@@ -142,17 +142,18 @@
> 
>       // Skip these tests on Windows (includes WinCE) and OS/2 since their
>       // implementaions of chmod doesn't really set permissions.
>       if (!IS_WIN && !IS_OS2 && f.originalPerms) {
>         testFile.permissions = f.originalPerms;
>         // Store the actual permissions on the file for reference later after
>         // setting the permissions.
>         if (!f.comparePerms)
>-          f.comparePerms = testFile.permissions;
>+f.comparePerms = 0644; // XXX MAR says 644 for its contents, we're now using it.
>+//          f.comparePerms = testFile.permissions;
This scares me... the mar creation scripts for nightly builds wasn't setting the permissions for new files correctly. Options are changing the patch to set the permissions as they were set previously or test the nightly and release mar creation scripts to verify that they are doing the right thing in relation to this change.

Comment 9

[Robert Strong (they/them - no direct email)]

Comment on attachment 419725 [details] [diff] [review]
Patch v.3 (WIP)

>-static int copy_file(const NS_tchar *spath, const NS_tchar *dpath)
>+static int rename_file(const NS_tchar *spath, const NS_tchar *dpath)
> {
>-  int rv = ensure_parent_dir(dpath);
>+  int rv;
>+#ifdef XP_WIN
>+  // We shouldn't ever actually be replacing or copying, but allow anyway for robustness.
>+  rv = !::MoveFileExW(spath, dpath, (MOVEFILE_REPLACE_EXISTING | MOVEFILE_COPY_ALLOWED));
I think I'd like to keep this compiling on WinCE for now.

As discussed in person since files in use can be renamed this should try to open the file exclusively first and if that fails bail out... if it succeeds it should close the file before continuing.

Comment 10

[Robert Strong (they/them - no direct email)]

Comment on attachment 419725 [details] [diff] [review]
Patch v.3 (WIP)

Minusing per comment #8 and comment #9... I'll hopefully find time to go through the rest next week but I am reviewed out for this week.

Comment 11

[Robert Strong (they/them - no direct email)]

renaming of files was added when implementing bug 466778 so changing summary to cover the remaining work that has been done in these patches.

Comment 12

[Justin Dolske [:Dolske]]

Pretty sure this is no longer useful, especially with ehsan's apply-in-background work going on. Too bad, I enjoyed writing this patch. :)