386760 - Add directory removal and ability to downgrade to application update

Status: RESOLVED FIXED

(Toolkit :: Application Update, enhancement)

Description

[(not reading, please use [e-mail] instead)]

directory removal with software update

For the transition from talkback to crashpad, rob strong wanted to add a directory to removed-files.in.

This won't work (currently) for two reasons:

1)  software update doesn't remove directories (only files).  see 

http://lxr.mozilla.org/seamonkey/source/toolkit/mozapps/update/src/updater/updater.cpp#371

2)  our tools (http://lxr.mozilla.org/mozilla/source/tools/update-packaging/common.sh#82) strip out directories when creating the update from removed-files.in

See bug #300136 for some discussion about why we don't support removing of directories.

I think this discussion has come up in bugzilla or email before, but I can't find it.

So for now, this is a tracker bug for this enhancement.

Comment 1

[Benjamin Smedberg]

dmills and I need this for partner builds also.

Comment 2

[(not reading, please use [e-mail] instead)]

benjamin / dmills:  rob needs something very specific for the 2.0.0.x -> 3.0 update.  We think we could handle it in a low risk way, by adding some logic to updater.cpp, after the update to do this:

"if [e-mail]/components is empty remove it"

then

"if [e-mail]" is empty remove it"

This will make it so when we upgrade from 2.0.0.5 -> 2.0.0.6, we would not remove talkback, but if the 3.0 upgrade removes all the files in those directores, we would remove them.

I'll let rob strong elaborate on why we need to remove talkback.

If you guys need to be able to remove other directories, or non empty directories, will probably need a better solution.

And, the solution has to land in 2.0.0.x so that we'll have it for the 3.0 upgrade.

So thoughts about a generic solution:

1) we'll need ti stop filtering directories out in common.sh

2) audit removed-files.in make sure we aren't removing any directories that are ok to remove on install, but not on update

3) either add a directive to remove directories or add a directive to add an empty directories.

Comment 3

[(not reading, please use [e-mail] instead)]

cc mscott, as tbird might need this as well.

Comment 4

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

We need to remove the talkback dirs during software update so the EM won't try to process the dir on startup when the user doesn't have rights to remove the dir.

There is the chicken / egg problem here as well in that the previous updater that is launched won't have the changes so we are going to need something in a 2.0.0.x release. For 2.0.0.x perhaps a one-off where it checks if the dir is empty and if so removes it? Also, we may want to limit this to just the dirs we are concerned with for the branch. One solution for the chicken and egg problem is to have the updater launch the new updater with a command line switch to perform post processing.

Comment 5

[Dan Mills [:thunder]]

Just chatted with Seth about this--

Benjamin and I need the ability to do (basically) am "rm -rf" on a directory, because we'll be placing all distribution modifications into a separate directory and need the ability to easily return to a "vanilla" Firefox (from any distribution).

Since we'll be moving these changes into their own directory for Firefox 3, we don't need the ability to remove directories and their contents (in one go) in the 2.x updater.

However, since we may be updating the 2.x partner distributions to 3.x, we may need the ability to remove single empty directories, somewhat like Seth described in comment #2.  In the 2->3 partner migration, I think we may generate mars for each distribution that move files from the various extensions they are currently in to the new distribution customizations directory.  So we may not want to leave those empty extension directories behind (right Rob?).

Comment 6

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

(In reply to comment #5)
>...<snip>
> they are currently in to the new distribution customizations directory.  So we
> may not want to leave those empty extension directories behind (right Rob?).
Right... we need those to be cleaned up when the app is updated.

The same may be true during an in place upgrade when running the installer... we should talk about what the requirements are for this.

Comment 7

[Mike Connor [:mconnor]]

Clearly not making M7, moving out...

Comment 8

[Mike Connor [:mconnor]]

moving out bugs that don't need to block b1

Comment 9

[Mike Connor [:mconnor]]

Benjamin, Dan, is this still required?

Comment 10

[Dan Mills [:thunder]]

Yes, this one is needed for repatriation.  Otherwise we'll have to create one-off MARs to do that.

Comment 11

[Mike Connor [:mconnor]]

Would really like to see this happen, but not going to block on it.

Comment 12

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

Attachment: patch rev1

Adds support for removing empty directories. I'm a tad hesitant about adding support for removing directories that are not empty.

Comment 13

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

Attachment: removed-files cleanup

Not required for the main patch... if the main patch is approved I'll add a new Firefox bug for cleaning up the removed-files.in file.

Comment 14

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

Comment on attachment 498568 [details] [diff] [review]
patch rev1

btw: this will be very easy to test but I am holding off on submitting the tests until after the review of the main patch since I am going to have to generate new test mar files.

Comment 15

[Benjamin Smedberg]

Why are we doing this now? We don't need this for removing the extra start menu directory, right? Can I just ignore this request until we're done with blockers?

Comment 16

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

(In reply to comment #15)
> Why are we doing this now? We don't need this for removing the extra start menu
> directory, right? Can I just ignore this request until we're done with
> blockers?
Definitely, I wanted to get it out of my queue, don't plan on landing this until after we branch, and am hoping you'll review when you have some spare time.

Comment 17

[J. Paul Reed [:preed]]

FWI[I]W, there's a patch over at http://bugzilla.songbirdnest.com/show_bug.cgi?id=13383, which does both rmdir (which is what you've implemented here) and rmrfdir (which, as the name implies, recursively deletes everything).

That was on my list of patches to upstream, since I left Songbird last week. We've been shipping it for a few months.

Feel free to reuse any useful bits of it.

Comment 18

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

Thanks! I'm sure it will be useful.

Comment 19

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

Attachment: patch in progress

Almost done... this supports removing directories recursively and removes the files using the existing RemoveFile so it supports files in use. It also uses a new manifest so it is backwards compatible.

Comment 20

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

Attachment: removed-files cleanup in progress (moved over to bug 649607)

Comment 21

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

Attachment: patch in progress

Nick, requesting review from you for the changes to the mar generation shell scripts. By adding both the current manifest format and a new manifest format the mar files will still work with clients with or without these changes though clients without will obviously not have the directories removed.

Comment 22

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 517376 [details] [diff] [review]
patch in progress

>diff --git a/tools/update-packaging/common.sh b/tools/update-packaging/common.sh

>+append_remove_instructionsv1() {
>+append_remove_instructionsv2() {

Lots of shared code here, are you anticipating they'll diverge at some point later ? Could we move the redirection into a single append_remove_instructions() call, and write to $updatemanifestv1 and $updatemanifestv2 on consecutive lines ? Not sure if the variable for redirection is expanded when the code is sourced or executed. Alternatively we could use a version parameter and if blocks.

>diff --git a/tools/update-packaging/make_incremental_update.sh b/tools/update-packaging/make_incremental_update.sh
...
>+updatemanifestv1="$workdir/update.manifest"
>+updatemanifestv2="$workdir/updatev2.manifest"
>+archivefiles="update.manifest optional.manifest"

I think you want 
 archivefiles="update.manifest updatev2.manifest"
there. Otherwise looks OK for nightlies and the the completes for releases. Do you know about the code that does the release partials ? That's at make_incremental_updates.py.

Comment 23

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

Attachment: packaging patch v2

Comment 24

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

Comment on attachment 517646 [details] [diff] [review]
packaging patch v2

Nick, I'm not sure what if anything else is current or actually used in the test directory... do I need to go through those files as well? Is there any info on how to run those tests if they do?

Comment 25

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

Attachment: patch

Comment 26

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

Attachment: test patch in progress

Comment 27

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

Attachment: test patch

Dave, since you are more familiar with the updater tests would you mind reviewing this test patch if you have time?

Comment 28

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

Comment on attachment 519091 [details] [diff] [review]
patch

Hey dolske, I am going to use the new manifest format needed for this bug to implement app update downgrades which is planned for Firefox next. Sorry if there are any stupid mistakes in the patch... I haven't looked over the changes thoroughly due to being on PTO the last couple of days.

Comment 29

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

Comment on attachment 517646 [details] [diff] [review]
packaging patch v2

Talked with Nick via email and I'm going to include additional functionality in this patch for bug 635161 to push the review request out a few days and to lessen the number of reviews he has to do.

The test patch is still good to go so leaving request though I will be submitting another patch with a few more tests for the new functionality.

Talked with dolske and he knows that a new patch is on the way. He said he might do a once over on the existing patch so leaving request.

Comment 30

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 517646 [details] [diff] [review]
packaging patch v2

One thing I noticed about this is that make_incremental_updates.py (which is used to create partials for releases) writes the new commands to the current manifest. The shell scripts (which are used for all completes, and nightly partials) write two manifests. 

If we want to backport these changes to an older branch we'd have to use the existing code to generate the partials. Users on the latest would then have the new updater and we could use the new code to generate the next set of partials. Similarly, we have to use the existing code if we decide to generate partials for a major update and haven't backported to older branches (not that we've ever done this).

That would be OK for us, but was the manifest difference deliberate ?

Comment 31

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

(In reply to comment #30)
> Comment on attachment 517646 [details] [diff] [review]
> packaging patch v2
> 
> One thing I noticed about this is that make_incremental_updates.py (which is
> used to create partials for releases) writes the new commands to the current
> manifest. The shell scripts (which are used for all completes, and nightly
> partials) write two manifests. 
I realized that as well and have it fixed locally.

> If we want to backport these changes to an older branch we'd have to use the
> existing code to generate the partials. Users on the latest would then have the
> new updater and we could use the new code to generate the next set of partials.
> Similarly, we have to use the existing code if we decide to generate partials
> for a major update and haven't backported to older branches (not that we've
> ever done this).
I'm not planning on backporting the client patches and even though the packaging changes won't adversely affect older branches I don't think we should. The client will do the *right thing* if the mar wasn't generated with these changes.

> That would be OK for us, but was the manifest difference deliberate ?
The update.manifest will be backwards compatible and the updatev2.manifest will only be used by clients with the changes.

Comment 32

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

Attachment: packaging patch v3

Still need to update the packaging tests so not quite ready

Comment 33

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

Attachment: packaging patch v4

Nick, besides this bug I've also added the packaging changes for bug 642765 since I don't want to have to introduce a 3rd manifest format. This also makes it so everything also works on Windows including the update-packaging shell script tests.

This also makes it so removed-files should seldom if ever have to be used after older clients are no longer served updates by removing files first during a complete update using the precomplete file if it exists in the root of the installation. I think it is best to not support removing anything under extensions or distribution when using the precomplete instructions.

When there is a channel change the remove-cc instruction from the precomplete file and the add-cc instruction from the update will be acted upon. By having both if the location of channel-prefs.js changes this will handle it correctly.

I've added a bunch of console logging in make_full_update.sh and make_incremental_update.sh to make it easier to see what is going on. Let me know if you'd like me to do the same for make_incremental_updates.py and I'll do so in a followup bug.

I've made the manifest instructions consistent between make_incremental_update.sh and make_incremental_updates.py so they output everything in the same order in case you are wondering about why I made some of the changes in these two files. I much prefer that the update instructions used for release builds is in the same order as the update instructions in the nightly builds. This is also necessary to make the update-packaging shell script tests pass.

Since make_incremental_updates.py excluded removed-files from the partial update I made make_incremental_update.sh also exclude removed-files. The full update still includes it. I think it would be best to include removed-files in all of the mars without an add instruction but that should be done in a different bug.

Comment 34

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

Attachment: packaging patch v5

Forgot that --time-style isn't always available so I added detection for Msys or Cygwin... rest is the same.

Comment 35

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

Comment on attachment 522627 [details] [diff] [review]
packaging patch v5

>diff --git a/tools/update-packaging/test/diffmar.sh b/tools/update-packaging/test/diffmar.sh
>--- a/tools/update-packaging/test/diffmar.sh
>+++ b/tools/update-packaging/test/diffmar.sh
>@@ -2,34 +2,44 @@
>...
>-diff -r "$fromdir" "$todir"
>\ No newline at end of file
>+diff -u8 "$fromdir/updatev2.manifest" "$todir/updatev2.manifest"
The above line comparing the updatev2.manifest snuck when I was testing and has been removed locally.

Comment 36

[Dave Townsend [:mossop]]

Comment on attachment 520173 [details] [diff] [review]
test patch

Brain hurty but looks good I think

Comment 37

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

Attachment: packaging patch v6

Forgot to quote the paths in createprecomplete.py

Comment 38

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

Attachment: main patch in progress

Comment 39

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

Attachment: Additional tests in progress.

Large patch in part due to new mars to support downgrade tests

Comment 40

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

Attachment: patch - test binary files

Comment 41

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

Attachment: Additional tests in progress

Comment 42

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

Attachment: main patch in progress

Comment 43

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

Comment on attachment 523427 [details] [diff] [review]
packaging patch v6

Asking khuey to review the makefile changes and the python script.

Comment 44

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 523427 [details] [diff] [review]
packaging patch v6

Rob, I've had a look through this (up to the tests) and it it looks fine so far. I need to finish off and do some spot testing before signing off, which should be doable in the next day or so.

I was curious about the reverse sorting used on several file lists. Does that get us something nice like promoting xul.dll to early in the processing ? Also wondered if you know if that (or anything here) affects dolske's fix to the smoothness of the progress bar as the update is applied (at least until silent updates come along).

Comment 45

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

(In reply to comment #44)
> Comment on attachment 523427 [details] [diff] [review]
> packaging patch v6
> 
> Rob, I've had a look through this (up to the tests) and it it looks fine so
> far. I need to finish off and do some spot testing before signing off, which
> should be doable in the next day or so.
> 
> I was curious about the reverse sorting used on several file lists. Does that
> get us something nice like promoting xul.dll to early in the processing ?
It makes it so files are always removed before directories and even when it doesn't need to be reversed sorted I did so anyway mainly for consistency. It should at the very least put xul earlier in the queue if not at the beginning of the queue of files.

> Also
> wondered if you know if that (or anything here) affects dolske's fix to the
> smoothness of the progress bar as the update is applied (at least until silent
> updates come along).
It uses the same logic so it shouldn't. There is a pre-existing bug with the Finish phase not updating in part due to all of the files listed in removed-files not existing which the directory removal will make better.

btw: even with silent updates the ui will still be needed for the case where the user doesn't have rights to install services and there is at least one other case where it might be displayed which I haven't come up with a decent solution for as of yet.

Comment 46

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

Attachment: patch - test binary files

Comment 47

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

Attachment: Additional tests

Comment 48

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

Attachment: main patch

I haven't re-verified that everything works on Mac / Linux again yet but it won't take much to fix things up if there is a problem

Comment 49

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

Attachment: main patch

Comment 50

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

Comment on attachment 524372 [details] [diff] [review]
main patch

I've been doing a bunch of testing and found a stupid bug with the current patch... I'll resubmit after I get it fixed.

Comment 51

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

Attachment: main patch

The updater wasn't dealing with the callback correctly so I rewrote how the callback gets updated which I think is a better solution than the one in bug 635161. I need to go through the code a bit mainly for wince (bah!) before asking for review again.

I've also tested downgrades successfully by repackaging the Firefox 4.0 complete mar and applying it to my own build.

Comment 52

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 523427 [details] [diff] [review]
packaging patch v6

First off, I appreciate your patience with this code. I'd forgotten how horrible the shell is. Mostly I have suggestions rather than problems, except for the testing comments at the bottom. If I'm barking up the wrong tree then throw me a bone or something.

>diff --git a/config/createprecomplete.py b/config/createprecomplete.py
...
>+    rel_file_path_set = set()
...
>+                rel_file_path_set.add(rel_path_file)
....
>+    rel_file_path_list = list(rel_file_path_set)
>+    rel_file_path_list.sort(reverse=True)

I suspect you could just cast this is as a list straight away, and use append instead of add, but it's not a big deal.

>diff --git a/tools/update-packaging/common.sh b/tools/update-packaging/common.sh
> append_remove_instructions() {
>+            if [ $(echo "$f" | grep -c '^../') = 1 ]; then
>+              if [ $(echo "$f" | grep -c '^../../') = 1 ]; then

You should escape the '.'s here too (the sed that follows has that). Guess I can't complain about the assignment operator here rather than the '-eq' comparison, given its already all through this code, ahem.

>diff --git a/tools/update-packaging/make_full_update.sh b/tools/update-packaging/make_full_update.sh
>+notice "Adding file add on channel change instructions to file 'updatev2.manifest'"

Needs a little clarifying, how about
 notice "Adding file ADD instructions for channel change to file 'updatev2.manifest'"

>diff --git a/tools/update-packaging/make_incremental_update.sh b/tools/update-packaging/make_incremental_update.sh
>+    num_removes=`expr $num_removes + 1`

'(( num_removes++ ))' is a more compact pattern used elsewhere in this code.

>+  # removed-files is excluded by make_incremental_updates.py so it is excluded
>+  # here for consistency.
>+  if [ "$f" = "removed-files" ]; then
>+    continue 1
>+  fi
>+  if [ "$f" = "Contents/MacOS/removed-files" ]; then
>+    continue 1
>+  fi

Could do this once by comparing `basename $f` to 'removed-files'. 

Whatever you want to do with removed-files (in both complete & partial, or out as here) is fine by me.

>diff --git a/tools/update-packaging/make_incremental_updates.py b/tools/update-packaging/make_incremental_updates.py
>-        self.manifest=[]
>+        self.manifestv1=[]
>+        self.manifestv2=[]

There's a comment above this context that still refers to manifest.

>+                # Windows uses backslashes so replace with forward slashes
>+                line=os.path.join(prefix,line).replace("\\", "/")

Should this comment read something like 'Python on windows uses \\ in paths, updater manifests should use / on all platforms' ?

> def create_partial_patch(from_dir_path, to_dir_path, patch_filename, shas, patch_info, forced_updates):
>+    # Require that the precomplete file is included in the to complete update
>+    if "precomplete" not in list(to_file_set):

Should be able to just write 'if "precomplete" not in to_file_set:'

>diff --git a/tools/update-packaging/test/common.sh b/tools/update-packaging/test/common.sh
> list_files() {
...
>   find . -type f \
>     | sed 's/\.\/\(.*\)/\1/' \
>     | sort > "$workdir/temp-filelist"

Missing -r on the sort here, so it differs from the main copy of this file by more than the exclusions & workdir.

>diff --git a/tools/update-packaging/test/diffmar.sh b/tools/update-packaging/test/diffmar.sh
...
>+diff -r "$fromdir" "$todir"

I think changing this to '-ru' is useful. It helped me to grok better some ordering issues in the tests:

diffing ref.mar and test.mar
diff -ru /tmp/diffmar/0/update.manifest /tmp/diffmar/1/update.manifest
--- /tmp/diffmar/0/update.manifest      2011-04-08 01:21:52.000000000 -0700
+++ /tmp/diffmar/1/update.manifest      2011-04-08 01:21:52.000000000 -0700
@@ -1,17 +1,17 @@
-add "searchplugins/diff/diff-patch-larger-than-file.txt"
-add "precomplete"
 add "{foodir/readme.txt"
 add "{foodir/diff-patch-larger-than-file.txt"
+add "searchplugins/diff/diff-patch-larger-than-file.txt"
+add "precomplete"
...(some more differences)

Did you see this too ? I had called buildrefmars.sh in a patched checkout, then runtests.sh; on Linux. Adding the -r to the sort in list_files() didn't help me, when I would have expected that it would. Looks like a difference in manifest ordering between a partial generated by the python and shell scripts.

>diff --git a/tools/update-packaging/test/runtests.sh b/tools/update-packaging/test/runtests.sh

AFAICT this is running the same test twice now. Lets remove the second call unless there's some reason for it.

So that's all r+ with changes on checkin, if you can convince me the tests is either something wrong my end, or not a big deal, or ____________.

Comment 53

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

Attachment: packaging patch v7

(In reply to comment #52)
> Comment on attachment 523427 [details] [diff] [review]
> packaging patch v6
> 
> First off, I appreciate your patience with this code. I'd forgotten how
> horrible the shell is. Mostly I have suggestions rather than problems, except
> for the testing comments at the bottom. If I'm barking up the wrong tree then
> throw me a bone or something.
> 
> >diff --git a/config/createprecomplete.py b/config/createprecomplete.py
> ...
> >+    rel_file_path_set = set()
> ...
> >+                rel_file_path_set.add(rel_path_file)
> ....
> >+    rel_file_path_list = list(rel_file_path_set)
> >+    rel_file_path_list.sort(reverse=True)
> 
> I suspect you could just cast this is as a list straight away, and use append
> instead of add, but it's not a big deal.
> 
> >diff --git a/tools/update-packaging/common.sh b/tools/update-packaging/common.sh
> > append_remove_instructions() {
> >+            if [ $(echo "$f" | grep -c '^../') = 1 ]; then
> >+              if [ $(echo "$f" | grep -c '^../../') = 1 ]; then
> 
> You should escape the '.'s here too (the sed that follows has that).
Done

> Guess I
> can't complain about the assignment operator here rather than the '-eq'
> comparison, given its already all through this code, ahem.
I'll look at fixing this when looking at only including removed-files in the mar and not laying it down on the client.

> >diff --git a/tools/update-packaging/make_full_update.sh b/tools/update-packaging/make_full_update.sh
> >+notice "Adding file add on channel change instructions to file 'updatev2.manifest'"
> 
> Needs a little clarifying, how about
>  notice "Adding file ADD instructions for channel change to file
> 'updatev2.manifest'"
Done

> >diff --git a/tools/update-packaging/make_incremental_update.sh b/tools/update-packaging/make_incremental_update.sh
> >+    num_removes=`expr $num_removes + 1`
> 
> '(( num_removes++ ))' is a more compact pattern used elsewhere in this code.
Done

> >+  # removed-files is excluded by make_incremental_updates.py so it is excluded
> >+  # here for consistency.
> >+  if [ "$f" = "removed-files" ]; then
> >+    continue 1
> >+  fi
> >+  if [ "$f" = "Contents/MacOS/removed-files" ]; then
> >+    continue 1
> >+  fi
> 
> Could do this once by comparing `basename $f` to 'removed-files'. 
Done

> Whatever you want to do with removed-files (in both complete & partial, or out
> as here) is fine by me.
Will do when there's some breathing room

> >diff --git a/tools/update-packaging/make_incremental_updates.py b/tools/update-packaging/make_incremental_updates.py
> >-        self.manifest=[]
> >+        self.manifestv1=[]
> >+        self.manifestv2=[]
> 
> There's a comment above this context that still refers to manifest.
changed / added (I'm going to rely on the code and blame to explain the differences between the versions)
manifestv1 = set of manifest version 1 patch instructions
manifestv2 = set of manifest version 2 patch instructions

> >+                # Windows uses backslashes so replace with forward slashes
> >+                line=os.path.join(prefix,line).replace("\\", "/")
> 
> Should this comment read something like 'Python on windows uses \\ in paths,
> updater manifests should use / on all platforms' ?
Changed it to
# Python on windows uses \ for path separators and the update
# manifests expects / for path separators on all platforms.

> > def create_partial_patch(from_dir_path, to_dir_path, patch_filename, shas, patch_info, forced_updates):
> >+    # Require that the precomplete file is included in the to complete update
> >+    if "precomplete" not in list(to_file_set):
> 
> Should be able to just write 'if "precomplete" not in to_file_set:'
Done

> >diff --git a/tools/update-packaging/test/common.sh b/tools/update-packaging/test/common.sh
> > list_files() {
> ...
> >   find . -type f \
> >     | sed 's/\.\/\(.*\)/\1/' \
> >     | sort > "$workdir/temp-filelist"
> 
> Missing -r on the sort here, so it differs from the main copy of this file by
> more than the exclusions & workdir.
Done

> >diff --git a/tools/update-packaging/test/diffmar.sh b/tools/update-packaging/test/diffmar.sh
> ...
> >+diff -r "$fromdir" "$todir"
> 
> I think changing this to '-ru' is useful. It helped me to grok better some
> ordering issues in the tests:
Done

> diffing ref.mar and test.mar
> diff -ru /tmp/diffmar/0/update.manifest /tmp/diffmar/1/update.manifest
> --- /tmp/diffmar/0/update.manifest      2011-04-08 01:21:52.000000000 -0700
> +++ /tmp/diffmar/1/update.manifest      2011-04-08 01:21:52.000000000 -0700
> @@ -1,17 +1,17 @@
> -add "searchplugins/diff/diff-patch-larger-than-file.txt"
> -add "precomplete"
>  add "{foodir/readme.txt"
>  add "{foodir/diff-patch-larger-than-file.txt"
> +add "searchplugins/diff/diff-patch-larger-than-file.txt"
> +add "precomplete"
> ...(some more differences)
> 
> Did you see this too ? I had called buildrefmars.sh in a patched checkout, then
> runtests.sh; on Linux. Adding the -r to the sort in list_files() didn't help
> me, when I would have expected that it would. Looks like a difference in
> manifest ordering between a partial generated by the python and shell scripts.
I hadn't seen that on Windows and iirc on Mac either so it might be something weird going on with sorting on Linux... I'll look into it.

> >diff --git a/tools/update-packaging/test/runtests.sh b/tools/update-packaging/test/runtests.sh
> 
> AFAICT this is running the same test twice now. Lets remove the second call
> unless there's some reason for it.
Done... I noticed that was the case previously as well and I'm still not sure why it did so.

> So that's all r+ with changes on checkin, if you can convince me the tests is
> either something wrong my end, or not a big deal, or ____________.
The difference won't hurt anything on the client since the files are still removed prior to the file's directories but I'd like to get this consistent across all platforms and will look into it over the weekend.

Comment 54

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 524709 [details] [diff] [review]
packaging patch v7

>diff --git a/config/createprecomplete.py b/config/createprecomplete.py
>+if len(sys.argv) == 1:
>+  generate_precomplete()

if __name__ == "__main__":
    generate_precomplete()

>diff --git a/toolkit/mozapps/installer/packager.mk b/toolkit/mozapps/installer/packager.mk
>--- a/toolkit/mozapps/installer/packager.mk
>+++ b/toolkit/mozapps/installer/packager.mk
>@@ -414,20 +417,21 @@ PACK_OMNIJAR	= \
>   printf "manifest components/binary.manifest\n" > chrome.manifest
> UNPACK_OMNIJAR	= \
>   $(OPTIMIZE_JARS_CMD) --deoptimize $(_ABS_DIST)/jarlog/ ./ ./ && \
>   unzip -o omni.jar && \
>   rm -f components/binary.manifest && \
>   sed -e 's/^\#binary-component/binary-component/' components/components.manifest > components.manifest && \
>   mv components.manifest components
> 
>-MAKE_PACKAGE	= (cd $(STAGEPATH)$(MOZ_PKG_DIR)$(_BINPATH) && $(PACK_OMNIJAR)) && $(INNER_MAKE_PACKAGE)
>+MAKE_PACKAGE	= (cd $(STAGEPATH)$(MOZ_PKG_DIR)$(_BINPATH) && $(PACK_OMNIJAR)) && \
>+	              (cd $(STAGEPATH)$(MOZ_PKG_DIR)$(_BINPATH) && $(CREATE_PRECOMPLETE)) && $(INNER_MAKE_PACKAGE)

My shell-foo is rusty, but can't you just (cd $(STAGEPATH)$(MOZ_PKG_DIR)$(_BINPATH) && $(PACK_OMNIJAR) && $(CREATE_PRECOMPLETE))?  Or does PACK_OMNIJAR do bizarre things.

I didn't look at anything ending in .sh here.

Comment 55

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

(In reply to comment #54)
> Comment on attachment 524709 [details] [diff] [review]
> packaging patch v7
> 
> >diff --git a/config/createprecomplete.py b/config/createprecomplete.py
> >+if len(sys.argv) == 1:
> >+  generate_precomplete()
> 
> if __name__ == "__main__":
>     generate_precomplete()
Done

> >diff --git a/toolkit/mozapps/installer/packager.mk b/toolkit/mozapps/installer/packager.mk
> >--- a/toolkit/mozapps/installer/packager.mk
> >+++ b/toolkit/mozapps/installer/packager.mk
> >@@ -414,20 +417,21 @@ PACK_OMNIJAR	= \
> >   printf "manifest components/binary.manifest\n" > chrome.manifest
> > UNPACK_OMNIJAR	= \
> >   $(OPTIMIZE_JARS_CMD) --deoptimize $(_ABS_DIST)/jarlog/ ./ ./ && \
> >   unzip -o omni.jar && \
> >   rm -f components/binary.manifest && \
> >   sed -e 's/^\#binary-component/binary-component/' components/components.manifest > components.manifest && \
> >   mv components.manifest components
> > 
> >-MAKE_PACKAGE	= (cd $(STAGEPATH)$(MOZ_PKG_DIR)$(_BINPATH) && $(PACK_OMNIJAR)) && $(INNER_MAKE_PACKAGE)
> >+MAKE_PACKAGE	= (cd $(STAGEPATH)$(MOZ_PKG_DIR)$(_BINPATH) && $(PACK_OMNIJAR)) && \
> >+	              (cd $(STAGEPATH)$(MOZ_PKG_DIR)$(_BINPATH) && $(CREATE_PRECOMPLETE)) && $(INNER_MAKE_PACKAGE)
> 
> My shell-foo is rusty, but can't you just (cd
> $(STAGEPATH)$(MOZ_PKG_DIR)$(_BINPATH) && $(PACK_OMNIJAR) &&
> $(CREATE_PRECOMPLETE))?  Or does PACK_OMNIJAR do bizarre things.
I wanted to keep them entirely independent so changes in the future to one won't affect the other.

Comment 56

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

Comment on attachment 524370 [details] [diff] [review]
Additional tests

># HG changeset patch
># Parent 9428661aa27c7fff49f9aabd04f3dc2d23ae9466
>...
>diff --git a/toolkit/mozapps/update/test/unit/test_0120_channelChange_complete.js b/toolkit/mozapps/update/test/unit/test_0120_channelChange_complete.js
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/mozapps/update/test/unit/test_0120_channelChange_complete.js
>@@ -0,0 +1,291 @@
>+/* Any copyright is dedicated to the Public Domain.
>+ * http://creativecommons.org/publicdomain/zero/1.0/
>+ */
>+
>+/* Channel change complete MAR file patch apply test */
>+
>+const TEST_ID = "0120";
>+// All we care about is that the last modified time has changed so that Mac OS
>+// X Launch Services invalidates its cache so the test allows up to one minute
>+// difference in the last modified time.
>+const MAX_TIME_DIFFERENCE = 60000;
>+
>+// The files are in the same order as they are applied from the mar
>+const TEST_FILES = [
>+{
>+  description      : "Added by update.manifest (add-cc)",
>+  fileName         : "channel-prefs.js",
>+  relPathDir       : "a/b/defaults/pref/",
>+  originalContents : "ToBeReplacedWithFromComplete\n",
>+  compareContents  : "FromComplete\n",
>+  originalFile     : null,
>+  compareFile      : null,
>+  originalPerms    : 0767,
>+  comparePerms     : 0767
comparePerms should be 0644 and I've changed this locally.

Comment 57

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

Talked with Nick about the shell tests and we've agreed to go with them as is where they pass on Windows / Mac and the manifest diff fails on Linux due to differences between shell sort and python sort for some chars.

I also changed the use of uname in diffmars.sh as follows since uname -o is not available on Mac.
# On Windows, creation time can be off by a second or more between the files in
# the fromdir and todir due to them being extracted synchronously so use
# time-style and exclude seconds from the creation time.
lsargs="-algR"
unamestr=`uname`
if [ ! "$unamestr" = 'Darwin' ]; then
  unamestr=`uname -o`
  if [ "$unamestr" = 'Msys' -o "$unamestr" = "Cygwin" ]; then
     lsargs="-algR --time-style=+%Y-%m-%d-%H:%M"
  fi
fi

Comment 58

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 524709 [details] [diff] [review]
packaging patch v7

Looks good to me.

Comment 59

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

Attachment: packaging patch updated to comments

I found one more case where basename removed-files should be used.

Carrying forward nthomas and khuey r+

Comment 60

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

Attachment: Additional tests

Added tests for the bug I found in comment #51 and fixed the comparePerms value mentioned in comment #56

Comment 61

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

Comment on attachment 524997 [details] [diff] [review]
Additional tests

bah... forgot to refresh

Comment 62

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

Attachment: main patch

This solves the problem with finding the callback we discussed last week.

Comment 63

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

Attachment: patch - test binary files

Had to update the mar files for the new tests.

btw: I am planning on just making the building of the mar generation tools the default and generate the mars on the fly sometime in the future.

Comment 64

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

Attachment: Additional tests

Added tests for the bug found in comment #51 and fixed the comparePerms value mentioned in comment #56

Comment 65

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

Note: Though it isn't required for this bug I filed bug 648945 to build update-packaging by default for Linux, Mac, and Windows to make it easier to extend the tests in the future.

Comment 66

[Justin Dolske [:Dolske]]

So... Much... Patch... This is taking longer than expected, but I'm most of the way through it. Will finish it up tomorrow.

Comment 67

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

Morphing summary of bug since it would have been a pita to add directory removal and downgrade capabilities in more than one bug since they both need a new manifest to support the actions required.

Comment 68

[Sheila Mooney]

Is there ANY way we could get this done today?

Comment 69

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

It depends on the changes dolske requests when he finishes the review which he said he would finish today in comment #66 but yes, there is a definite chance this will be done today.

Comment 70

[Justin Dolske [:Dolske]]

Comment on attachment 525024 [details] [diff] [review]
main patch

>+++ b/toolkit/mozapps/update/updater/updater.cpp

metacomment1: Hmm, probably should have does this as 3 patches. 1 for the wchar changes, 1 for the new remove actions, and 1 channel changing stuff. Oh well!

metacomment2: We should probably just nuke the WinCE support, especially if it's causing pain to keep. Or at least #error the places where support would need added in the future.

metacomment3: Did you check for any compiler warnings on the 3 tier-1 platforms?


>+ *  precomplete
>+ *  -----------
>+ *  method   = "remove" | "rmdir" | "remove-cc"

How about calling this cleanup.manifest?


>+# define S_ISDIR(s) (((s) & _S_IFMT) == _S_IFDIR)
>+# define S_ISREG(s) (((s) & _S_IFMT) == _S_IFREG)

Ugh, Windows, Y U NO POSIX?


>+# define fchmod(a,b)

Just remove this (instead of moving it)? Seems unused, and seems like it could cause a bad surprise in the future...


>-// _wrename is used instead of MoveFileW to avoid the link tracking service.
...
>+# define NS_trename _wrename // _wrename is used to avoid the link tracking service.

Hmm, are all preprocessers ok with comments on the same line?


>+#define BACKUP_EXT NS_T(".moz-backup")

Since you're cleaning up, worth sticking all the constant extensions/dirnames together, even if they're unused on some platforms?


>-get_wide_path(const char *path)
>+get_full_path(const NS_tchar *relpath)
> {
...
>-  if (wcscmp(wpath, gFFData.cFileName) == 0) {
>-    wcscat(c, CALLBACK_BACKUP_EXT);
>-    c += wcslen(CALLBACK_BACKUP_EXT);
>-  }

Gack. Thanks for moving this to PatchFile::Execute. Seems much more sane there.


>+static NS_tchar*
>+get_valid_path(NS_tchar **line, bool isdir = false, bool wincepath = true)
>+{
...
>+}

TODO, will review this in next comment.


>+static NS_tchar*
>+get_quoted_path(const NS_tchar *path)
>+{
...
>+}

TODO, will review this in next comment.


>@@ -573,16 +683,30 @@ static int rename_file(const NS_tchar *s
>   if (NS_taccess(spath, F_OK)) {
>     LOG(("rename_file: source file doesn't exist: " LOG_S "\n", spath));
>     return READ_ERROR;
>   }
> 
>+  struct stat spathInfo;
>+  rv = NS_tstat(spath, &spathInfo);

Get rid of the preceeding NS_taccess() check, since you're doing a full stat here anyway. (Ie, do the access()-equivalent check yourself)


>+  if (rv) {
>+    LOG(("rename_file: failed to read file status info: " LOG_S ", " \
>+         "err: %d\n", spath, errno));
>+    return READ_ERROR;
>+  }
>+
>+  if (!S_ISREG(spathInfo.st_mode)) {
>+    LOG(("rename_file: path present, but not a file: " LOG_S ", err: %d\n",
>+         spath, errno));
>+    return UNEXPECTED_ERROR;
>+  }

This is a pattern in a few places, worth a follow to look at making a helper function to roll all this up?


> static int backup_discard(const NS_tchar *path)
> {
>   NS_tchar backup[MAXPATHLEN];
>   NS_tsnprintf(backup, sizeof(backup)/sizeof(backup[0]),
>                NS_T("%s" BACKUP_EXT), path);
> 
>   // Nothing to discard
>   if (NS_taccess(backup, F_OK)) {
>-    LOG(("backup_discard: backup file doesn't exist: " LOG_S "\n", backup));
>     return OK;
>   }

Was this generating noise? ISTR this isn't a normal condition (we expect a backup file, it's just not fatal if it somehow goes awol), so keeping the log would be nice.


>+RemoveFile::Parse(NS_tchar *line)
> {
>   // format "<deadfile>"
> 
>-  mFile = mstrtok(kQuote, &line);
>+  mFile = get_valid_path(&line);
>   if (!mFile)
>     return PARSE_ERROR;

Why pass the quoted path into get_valid_path? Seems like the quotes are part of the parsing, and should be removed here instead of in get_valid_path.

(Similarly for the other ::Parse methods now doing this)


> int
> RemoveFile::Prepare()
> {
>-  LOG(("PREPARE REMOVE %s\n", mFile));
>-
>   // We expect the file to exist if we are to remove it.
>-  int rv = NS_taccess(mDestFile, F_OK);
>+  int rv = NS_taccess(mFile, F_OK);
>   if (rv) {
>-    LOG(("file does not exist; skipping\n"));

Yay, this always made so much noise in logs. :)

Maybe just add a comment here, or tweak the one above, to note that a missing file is fine and normal?


>+  LOG(("PREPARE REMOVEFILE " LOG_S "\n", mFile));
>+
>+  // Make sure that we're actually a file...
>+  struct stat fileInfo;
>+  rv = NS_tstat(mFile, &fileInfo);
>+  if (rv) {
>+    LOG(("failed to read file status info: " LOG_S ", err: %d\n", mFile,
>+         errno));
>+    return READ_ERROR;
>+  }
>+
>+  if (!S_ISREG(fileInfo.st_mode)) {
>+    LOG(("path present, but not a file: " LOG_S "\n", mFile));
>+    return UNEXPECTED_ERROR;
>+  }

Hmm... I was a little paranoid here that the additional checks here could make us fail when we didn't before (eg, if some old thing we're always trying to remove was actually a directory), but I think we would have failed before anyway, just a little farther along -- when creating the backup. So that should be fine.


>+  NS_tchar *slash = (NS_tchar *) NS_tstrrchr(mFile, NS_T('/'));
>   if (slash) {
>     *slash = NS_T('\0');
>-    rv = NS_taccess(mDestFile, W_OK);
>+    rv = NS_taccess(mFile, W_OK);
>     *slash = NS_T('/');
>   } else {
>     rv = NS_taccess(NS_T("."), W_OK);
>   }

Shouldn't the |else| should use gDestPath now? Better yet, just hoist this up to a 1-time "can we write to the app dir" call. (Followup?)



>+int
>+RemoveDir::Prepare()
>+{

The (undocumented?) assumption here is that the specified directory is empty. Seems like that should be explicitly checked (opendir/readdir).

Are manifests allowed to do sequences like these (assume foo/a and foo/b exist):

remove foo/a
remove foo/b
rmdir foo/

or how about

remove foo/a
remove foo/b
rmdir foo/


Are there cases where we really want things to fail if rmdir has unexpected files? (IE, could we just always use rmrfdir instead?)


>+int
>+RemoveDir::Execute()
>+{

It's odd that the Execute() step doesn't actually do anything.

Should it rename the dir to dir.mozbackup, and then have Finish() actually nuke it (or un-rename if we're rolling back)?


>+RemoveDir::Finish(int status)
>+{
...
>+    if (NS_trmdir(mDir)) {
>+      LOG(("non-fatal error removing directory: " LOG_S "/, rv: %d, err: %d\n",
>+           mDir, rv, errno));

I wonder if failures here really should be non-fatal... (I'm thinking of things like how components/ used to work, specifically). But that's a moot concern if we add a check for an empty directory in ::Prepare. At least, I can't think of any cases where we do something special/interesting based solely on a directory existing.


> int
> AddFile::Execute()
> {
...
>+#ifdef XP_WIN
>+  int n = (int)NS_tstrlen(mFile) + 1;
>+  char *sourcefile = (char *) malloc(n * sizeof(char));
>+  if (!sourcefile)
>+    return MEM_ERROR;
...wince case...
>+  if (!WideCharToMultiByte(CP_UTF8, 0, mFile, n, sourcefile, n, NULL, NULL)) {
>+    LOG(("error converting wchar to utf8: %d\n", GetLastError()));
>+    return MEM_ERROR;
>+  }

This isn't right. If you're converting wide strings to utf8, you'll need a larger output buffer to account for potential UTF8 overhead. This works for the trivial case (ascii strings), but the worst case is, what, 6 bytes for every input character? Or just MAXPATHLEN?

Also, free |sourcefile| when you're done with it?

(Can we make gArchiveReader just take NS_tchar strings to avoid the conversion? Oh, I bet not. Sigh.)


> class PatchFile : public Action
...
>+  virtual int Prepare(); // // should check for patch file and for checksum here

oops oops!


> int
> PatchFile::Prepare()
> {
...

Shouldn't there be a WINCE case as in AddFile::Prepare? [Or not, if we just get rid of all the WINCE cruft. ;)]

Maybe a helper since these two things are doing the same work (also given the correctness/leak issues from AddFile::Prepare up above.)?



>   if (argc > callbackIndex) {
>-    // FindFirstFileW is used to get the callback's filename for comparison
>-    // with the callback's patch since it will return the correct case and the
>-    // long name instead of the 8.3 format name.
>-    HANDLE hFindFile;
>-    hFindFile = FindFirstFileW(argv[callbackIndex], &gFFData);
>-    if (hFindFile == INVALID_HANDLE_VALUE) {
>+#ifndef WINCE
>+    // If the callback executable is specified it must exist for a successful
>+    // update.
>+    NS_tchar callbackLongPath[MAXPATHLEN];
>+    if (!GetLongPathNameW(argv[callbackIndex], callbackLongPath,
>+                          sizeof(callbackLongPath)/sizeof(callbackLongPath[0]))) {
>       LOG(("NS_main: unable to find callback file: " LOG_S "\n", argv[callbackIndex]));
>       LogFinish();
>       WriteStatusFile(WRITE_ERROR);
>       EXIT_WHEN_ELEVATED(elevatedLockFilePath, updateLockFileHandle, 1);
>       LaunchCallbackApp(argv[4], argc - callbackIndex, argv + callbackIndex);
>       return 1;
>     }
>-    FindClose(hFindFile);
>-
>-    // Make a copy of the callback executable.
>-    NS_tsnprintf(callbackBackupPath,
>-                 sizeof(callbackBackupPath)/sizeof(callbackBackupPath[0]),
>+
>+    NS_tchar applyDirLongPath[MAXPATHLEN];
>+    if (!GetLongPathNameW(argv[2], applyDirLongPath,
>+                          sizeof(applyDirLongPath)/sizeof(applyDirLongPath[0]))) {
>+      LOG(("NS_main: unable to find apply to dir: " LOG_S "\n", argv[2]));
>+      LogFinish();
>+      WriteStatusFile(WRITE_ERROR);
>+      EXIT_WHEN_ELEVATED(elevatedLockFilePath, updateLockFileHandle, 1);
>+      LaunchCallbackApp(argv[4], argc - callbackIndex, argv + callbackIndex);
>+      return 1;
>+    }
>+
>+    int len = NS_tstrlen(applyDirLongPath);
>+    NS_tchar *s = callbackLongPath;
>+#else
>+    // Paths on Windows CE are already long paths.
>+    int len = NS_tstrlen(argv[2]);
>+    NS_tchar *s = argv[callbackIndex];
>+#endif
>+
>+    NS_tchar *d = gCallbackRelPath;
>+    // advance to the apply to directory and advance past the trailing backslash
>+    // if present.
>+    s += len;
>+    if (*s == NS_T('\\'))
>+      ++s;
>+
>+    // Copy the string and replace backslashes with forward slashes along the
>+    // way.
>+    do {
>+      if (*s == NS_T('\\'))
>+        *d = NS_T('/');
>+      else
>+        *d = *s;
>+      ++s;
>+      ++d;
>+    } while (*s);
>+    *d = NS_T('\0');
>+    ++d;
>+
>+    // Make a copy of the callback executable so it can be read when patching.
>+    NS_tsnprintf(gCallbackBackupPath,
>+                 sizeof(gCallbackBackupPath)/sizeof(gCallbackBackupPath[0]),
>                  NS_T("%s" CALLBACK_BACKUP_EXT), argv[callbackIndex]);
>-    NS_tremove(callbackBackupPath);
>-    CopyFileW(argv[callbackIndex], callbackBackupPath, FALSE);
>+    NS_tremove(gCallbackBackupPath);
>+    CopyFileW(argv[callbackIndex], gCallbackBackupPath, false);
> 
>     // Since the process may be signaled as exited by WaitForSingleObject before
>     // the release of the executable image try to lock the main executable file
>     // multiple times before giving up.
>     int retries = 5;
>     do {
>       // By opening a file handle to the callback executable, the OS will prevent
>-      // launching the process while it is being updated. 
>+      // launching the process while it is being updated.
>       callbackFile = CreateFileW(argv[callbackIndex],
> #ifdef WINCE
>                                  GENERIC_WRITE,
>                                  FILE_SHARE_READ | FILE_SHARE_WRITE,
> #else
>                                  DELETE | GENERIC_WRITE,
>-                                 0, // no sharing!
>+                                 // allow delete, rename, and write
>+                                 FILE_SHARE_DELETE | FILE_SHARE_WRITE,
> #endif
>                                  NULL, OPEN_EXISTING, 0, NULL);

TODO, will review this chunk in next comment.


>+int add_dir_entries(const NS_tchar *dirpath, ActionList *list)
>+{ ... }
>+getmanifestcontents(const NS_tchar *manifest)
>+{ ... }
>+int AddPreCompleteActions(ActionList *list, bool channelchange)
>+{ ... }

TODO, will review these in next comment.

(nit: GetManifestContents().)


>+  bool firstpass = true;

Suggestion: isFirstAction, or keep a actionCount and check actionCount == 1. First pass makes me think of reading though the manifest twice...


>+  bool iscomplete = false;

isComplete or is_complete?


>+    // Placeholder so the update manifest version doesn't have to be revisioned.

What changes do you see coming? Because this isn't really true, if a new action is invented, an old updater will fail parsing the manifest.


>+    // The type will be the the update type (e.g. complete or partial) and if
>+    // present MUST be the first entry in the update manifest. The type will be
>+    // used for supporting downgrades via app update.

This comment would be more useful up at the top of the file with the manifest BNF.


>+    if (firstpass && NS_tstrcmp(token, NS_T("type")) == 0) {
...
>+    }

|else| here to fail if we don't recognize the manifest type?


>+    else if (NS_tstrcmp(token, NS_T("rmrfdir")) == 0) { // rmdir recursive
...
>+      if (reldirpath[NS_tstrlen(reldirpath) - 1] != NS_T('/'))
>+        return PARSE_ERROR;

Is there any reason to support "rmrfdir /"? Seems like that could be dangerous if it mistakenly made it into a manifest. Could just add an extra check to see if there's at least one other character (that's not . or /) in the path...


>+    else if (NS_tstrcmp(token, NS_T("add-cc")) == 0) { // Add if channel change
>+      // The channel should only be changed when the user requests a channel
>+      // change to avoid overwriting the update channel when testing RC's and it
>+      // can only be changed using a complete update. If the user requested a
>+      // channel change and this isn't a complete let the partial fail and the
>+      // existing error recovery handle the failure to apply the update.
>+      if (!channelchange || !iscomplete)
>+        continue;
>+
>+      action = new AddFile();
>+    }

I don't understand this. Shouldn't we just add a check to explicitly fail here immediately? EG, |if (channelchange && !iscomplete) return OMGFAIL;|

Although, consider the case of, somehow, an undeletable "changechannel" file getting added to the appdir. I'd be wary of failing to apply the partial lest they be unable to upgrade, but I guess we should already then fallback to downloading a complete update and trying that.

Side nit: pick one of changechannel / channelchange and stick with it everywhere.]

Comment 71

[Ed Morley [:emorley]]

(In reply to comment #70)
> metacomment2: We should probably just nuke the WinCE support

If it helps tips the balance, I've already removed WinCE support from spidermonkey (bug 647389), am about to remove it from the js/src build system (bug 648866) and as such am going to need to remove it from the main build system too given |make check-sync-dirs|. I also was intending on doing bug 473687 soon, which removes support for WinCE from widget too.

In the words of :shaver in bug 647389 comment 4:
> If/when Windows Phone 7 publishes an NDK for the platform, and if/when we
> decide to port to it, people should add whatever support is required by that
> new environment.  In the meantime we should strip it out, IMO.

Comment 72

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

(In reply to comment #70)
> Comment on attachment 525024 [details] [diff] [review]
> main patch
> 
> >+++ b/toolkit/mozapps/update/updater/updater.cpp
> 
> metacomment1: Hmm, probably should have does this as 3 patches. 1 for the wchar
> changes, 1 for the new remove actions, and 1 channel changing stuff. Oh well!
> 
> metacomment2: We should probably just nuke the WinCE support, especially if
> it's causing pain to keep. Or at least #error the places where support would
> need added in the future.
Done

> metacomment3: Did you check for any compiler warnings on the 3 tier-1
> platforms?
Mac and Windows only so far

> 
> >+ *  precomplete
> >+ *  -----------
> >+ *  method   = "remove" | "rmdir" | "remove-cc"
> 
> How about calling this cleanup.manifest?
Since it will be on the file system and Windows already uses .manifest I don't want it to have a .manifest extension.
This file is only used for complete updates and it's actions are performed prior to a complete update. I have no problem with renaming it but I think cleanup.manifest or cleanup are worse names.

> >+# define fchmod(a,b)
> 
> Just remove this (instead of moving it)? Seems unused, and seems like it could
> cause a bad surprise in the future...
Done

> >-// _wrename is used instead of MoveFileW to avoid the link tracking service.
> ...
> >+# define NS_trename _wrename // _wrename is used to avoid the link tracking service.
> 
> Hmm, are all preprocessers ok with comments on the same line?
Put it on the previous line

> 
> >+#define BACKUP_EXT NS_T(".moz-backup")
> 
> Since you're cleaning up, worth sticking all the constant extensions/dirnames
> together, even if they're unused on some platforms?
I thought about that and plan on making use of several on all platforms. I'll do that when I do so.

> 
> >@@ -573,16 +683,30 @@ static int rename_file(const NS_tchar *s
> >   if (NS_taccess(spath, F_OK)) {
> >     LOG(("rename_file: source file doesn't exist: " LOG_S "\n", spath));
> >     return READ_ERROR;
> >   }
> > 
> >+  struct stat spathInfo;
> >+  rv = NS_tstat(spath, &spathInfo);
> 
> Get rid of the preceeding NS_taccess() check, since you're doing a full stat
> here anyway. (Ie, do the access()-equivalent check yourself)
> 
> 
> >+  if (rv) {
> >+    LOG(("rename_file: failed to read file status info: " LOG_S ", " \
> >+         "err: %d\n", spath, errno));
> >+    return READ_ERROR;
> >+  }
> >+
> >+  if (!S_ISREG(spathInfo.st_mode)) {
> >+    LOG(("rename_file: path present, but not a file: " LOG_S ", err: %d\n",
> >+         spath, errno));
> >+    return UNEXPECTED_ERROR;
> >+  }
> 
> This is a pattern in a few places, worth a follow to look at making a helper
> function to roll all this up?
Sure... I'll file after this is finished

> > static int backup_discard(const NS_tchar *path)
> > {
> >   NS_tchar backup[MAXPATHLEN];
> >   NS_tsnprintf(backup, sizeof(backup)/sizeof(backup[0]),
> >                NS_T("%s" BACKUP_EXT), path);
> > 
> >   // Nothing to discard
> >   if (NS_taccess(backup, F_OK)) {
> >-    LOG(("backup_discard: backup file doesn't exist: " LOG_S "\n", backup));
> >     return OK;
> >   }
> 
> Was this generating noise? ISTR this isn't a normal condition (we expect a
> backup file, it's just not fatal if it somehow goes awol), so keeping the log
> would be nice.
I recall that it was and I'll double check.

> >+RemoveFile::Parse(NS_tchar *line)
> > {
> >   // format "<deadfile>"
> > 
> >-  mFile = mstrtok(kQuote, &line);
> >+  mFile = get_valid_path(&line);
> >   if (!mFile)
> >     return PARSE_ERROR;
> 
> Why pass the quoted path into get_valid_path? Seems like the quotes are part of
> the parsing, and should be removed here instead of in get_valid_path.
> 
> (Similarly for the other ::Parse methods now doing this)
I could split them up and have two calls for each or just rename get_valid_path to something more palatable which is what I'd prefer.

> > int
> > RemoveFile::Prepare()
> > {
> >-  LOG(("PREPARE REMOVE %s\n", mFile));
> >-
> >   // We expect the file to exist if we are to remove it.
> >-  int rv = NS_taccess(mDestFile, F_OK);
> >+  int rv = NS_taccess(mFile, F_OK);
> >   if (rv) {
> >-    LOG(("file does not exist; skipping\n"));
> 
> Yay, this always made so much noise in logs. :)
> 
> Maybe just add a comment here, or tweak the one above, to note that a missing
> file is fine and normal?
Done... changed to
// Skip the file if it already doesn't exist.

> 
> >+  LOG(("PREPARE REMOVEFILE " LOG_S "\n", mFile));
> >+
> >+  // Make sure that we're actually a file...
> >+  struct stat fileInfo;
> >+  rv = NS_tstat(mFile, &fileInfo);
> >+  if (rv) {
> >+    LOG(("failed to read file status info: " LOG_S ", err: %d\n", mFile,
> >+         errno));
> >+    return READ_ERROR;
> >+  }
> >+
> >+  if (!S_ISREG(fileInfo.st_mode)) {
> >+    LOG(("path present, but not a file: " LOG_S "\n", mFile));
> >+    return UNEXPECTED_ERROR;
> >+  }
> 
> Hmm... I was a little paranoid here that the additional checks here could make
> us fail when we didn't before (eg, if some old thing we're always trying to
> remove was actually a directory), but I think we would have failed before
> anyway, just a little farther along -- when creating the backup. So that should
> be fine.
> 
> 
> >+  NS_tchar *slash = (NS_tchar *) NS_tstrrchr(mFile, NS_T('/'));
> >   if (slash) {
> >     *slash = NS_T('\0');
> >-    rv = NS_taccess(mDestFile, W_OK);
> >+    rv = NS_taccess(mFile, W_OK);
> >     *slash = NS_T('/');
> >   } else {
> >     rv = NS_taccess(NS_T("."), W_OK);
> >   }
> 
> Shouldn't the |else| should use gDestPath now? Better yet, just hoist this up
> to a 1-time "can we write to the app dir" call. (Followup?)
I'll file a followup after this bug is finished.

> 
> 
> 
> >+int
> >+RemoveDir::Prepare()
> >+{
> 
> The (undocumented?) assumption here is that the specified directory is empty.
> Seems like that should be explicitly checked (opendir/readdir).
When I add the new instructions to removed-files.in I plan on adding documentation for this there.
I'm not sure what additional value the check would provide since RemoveDir is never fatal and should only remove the dir if it is empty. rmrfdir requires that the files in the directory are successfully removed prior to the attempt to remove the empty dir.

> Are manifests allowed to do sequences like these (assume foo/a and foo/b
> exist):
> 
> remove foo/a
> remove foo/b
> rmdir foo/
> 
> or how about
> 
> remove foo/a
> remove foo/b
> rmdir foo/
I believe the two examples above are the same.

Those are valid. The mar generation scripts makes sure the order for these actions is correct.

In the Additional tests patch there is the following sequence
+remove "a/b/1/10/10text0"
<skip a bunch of lines>
+rmdir "a/b/1/10/"
+rmdir "a/b/1/"

> Are there cases where we really want things to fail if rmdir has unexpected
> files? (IE, could we just always use rmrfdir instead?)
Not that I know of. IMO we should lean hard towards only removing files we know about and requiring the dir to be empty to remove it. For example, if a 3rd party installs a binary in one of our dirs on Windows, it is in use during update, and we try to remove it the update will fail.

> 
> >+int
> >+RemoveDir::Execute()
> >+{
> 
> It's odd that the Execute() step doesn't actually do anything.
> 
> Should it rename the dir to dir.mozbackup, and then have Finish() actually nuke
> it (or un-rename if we're rolling back)?
No because of files in use. Though it is strange I think doing any extra work here would be for the sake of doing extra work which isn't all that valuable.

> 
> >+RemoveDir::Finish(int status)
> >+{
> ...
> >+    if (NS_trmdir(mDir)) {
> >+      LOG(("non-fatal error removing directory: " LOG_S "/, rv: %d, err: %d\n",
> >+           mDir, rv, errno));
> 
> I wonder if failures here really should be non-fatal... (I'm thinking of things
> like how components/ used to work, specifically). But that's a moot concern if
> we add a check for an empty directory in ::Prepare. At least, I can't think of
> any cases where we do something special/interesting based solely on a directory
> existing.
It is by no means a requirement for the dir to be empty in Prepare. For example, the following would fail for that case
remove foo/a
remove foo/b
rmdir foo/

Comment 73

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

(In reply to comment #70)
> Comment on attachment 525024 [details] [diff] [review]
> main patch
>...
> 
> > int
> > AddFile::Execute()
> > {
> ...
> >+#ifdef XP_WIN
> >+  int n = (int)NS_tstrlen(mFile) + 1;
> >+  char *sourcefile = (char *) malloc(n * sizeof(char));
> >+  if (!sourcefile)
> >+    return MEM_ERROR;
> ...wince case...
> >+  if (!WideCharToMultiByte(CP_UTF8, 0, mFile, n, sourcefile, n, NULL, NULL)) {
> >+    LOG(("error converting wchar to utf8: %d\n", GetLastError()));
> >+    return MEM_ERROR;
> >+  }
> 
> This isn't right. If you're converting wide strings to utf8, you'll need a
> larger output buffer to account for potential UTF8 overhead. This works for the
> trivial case (ascii strings), but the worst case is, what, 6 bytes for every
> input character? Or just MAXPATHLEN?
Used MAXPATHLEN

> Also, free |sourcefile| when you're done with it?
Removed sourcefile

> 
> (Can we make gArchiveReader just take NS_tchar strings to avoid the conversion?
> Oh, I bet not. Sigh.)
I'd like to but not right now

> 
> > class PatchFile : public Action
> ...
> >+  virtual int Prepare(); // // should check for patch file and for checksum here
> 
> oops oops!
Fixed

> 
> 
> > int
> > PatchFile::Prepare()
> > {
> ...
> 
> Shouldn't there be a WINCE case as in AddFile::Prepare? [Or not, if we just get
> rid of all the WINCE cruft. ;)]
Removed WINCE

> Maybe a helper since these two things are doing the same work (also given the
> correctness/leak issues from AddFile::Prepare up above.)?
I'm a bit thickheaded... can I get some more deets?

> >+int add_dir_entries(const NS_tchar *dirpath, ActionList *list)
> >+{ ... }
> >+getmanifestcontents(const NS_tchar *manifest)
> >+{ ... }
> >+int AddPreCompleteActions(ActionList *list, bool channelchange)
> >+{ ... }
> 
> TODO, will review these in next comment.
> 
> (nit: GetManifestContents().)
Done

> 
> >+  bool firstpass = true;
> 
> Suggestion: isFirstAction, or keep a actionCount and check actionCount == 1.
> First pass makes me think of reading though the manifest twice...
> 
> >+  bool iscomplete = false;
> 
> isComplete or is_complete?
Changed to isFirstAction... also changed is iscomplete to isComplete and channelchange to isChannelChange

> 
> >+    // Placeholder so the update manifest version doesn't have to be revisioned.
> 
> What changes do you see coming? Because this isn't really true, if a new action
> is invented, an old updater will fail parsing the manifest.
Old comment... removed

Comment 74

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

(In reply to comment #70)
> Comment on attachment 525024 [details] [diff] [review]
> main patch
>...

> 
> >+    // The type will be the the update type (e.g. complete or partial) and if
> >+    // present MUST be the first entry in the update manifest. The type will be
> >+    // used for supporting downgrades via app update.
> 
> This comment would be more useful up at the top of the file with the manifest
> BNF.
Done and added a couple of more... I'll add the rest in another bug.

> 
> >+    if (firstpass && NS_tstrcmp(token, NS_T("type")) == 0) {
> ...
> >+    }
> 
> |else| here to fail if we don't recognize the manifest type?
> 
> 
> >+    else if (NS_tstrcmp(token, NS_T("rmrfdir")) == 0) { // rmdir recursive
> ...
> >+      if (reldirpath[NS_tstrlen(reldirpath) - 1] != NS_T('/'))
> >+        return PARSE_ERROR;
> 
> Is there any reason to support "rmrfdir /"? Seems like that could be dangerous
> if it mistakenly made it into a manifest. Could just add an extra check to see
> if there's at least one other character (that's not . or /) in the path...
We already only support relative paths that don't start with /.

> >+    else if (NS_tstrcmp(token, NS_T("add-cc")) == 0) { // Add if channel change
> >+      // The channel should only be changed when the user requests a channel
> >+      // change to avoid overwriting the update channel when testing RC's and it
> >+      // can only be changed using a complete update. If the user requested a
> >+      // channel change and this isn't a complete let the partial fail and the
> >+      // existing error recovery handle the failure to apply the update.
> >+      if (!channelchange || !iscomplete)
> >+        continue;
> >+
> >+      action = new AddFile();
> >+    }
> 
> I don't understand this. Shouldn't we just add a check to explicitly fail here
> immediately? EG, |if (channelchange && !iscomplete) return OMGFAIL;|
Done with a slightly different approach. Partials shouldn't ever have an add-cc instruction unless someone hacks one in so I don't see this adding much value or causing much harm.

> Although, consider the case of, somehow, an undeletable "changechannel" file
> getting added to the appdir. I'd be wary of failing to apply the partial lest
> they be unable to upgrade, but I guess we should already then fallback to
> downloading a complete update and trying that.
> 
> Side nit: pick one of changechannel / channelchange and stick with it
> everywhere.]
Changed to channelchange.

Comment 75

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

Attachment: updated to current comments

Comment 76

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

Attachment: Additional tests (changed due to review comments on main patch)

Comment 77

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

Attachment: patch - wince removals outside of updater.cpp

Comment 78

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

(In reply to comment #73)
>...
> > > int
> > > PatchFile::Prepare()
> > > {
> > ...
> > 
> > Shouldn't there be a WINCE case as in AddFile::Prepare? [Or not, if we just get
> > rid of all the WINCE cruft. ;)]
> Removed WINCE
> 
> > Maybe a helper since these two things are doing the same work (also given the
> > correctness/leak issues from AddFile::Prepare up above.)?
> I'm a bit thickheaded... can I get some more deets?
The blind man picked up the hammer and saw.

Let's do that in a followup.

Comment 79

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

Attachment: 1. main patch updated to comments

bah... forgot to add free.

Comment 80

[Justin Dolske [:Dolske]]

Comment on attachment 525024 [details] [diff] [review]
main patch

Remaining bits of review... Will look at interdiff with new patch next!


>+static NS_tchar*
>+get_valid_path(NS_tchar **line, bool isdir = false, bool wincepath = true)
>+{

|wincepath| is unused.

Also, ah, I guess this is more of a check_path() function since it doesn't really do anything.


>+static NS_tchar*
>+get_quoted_path(const NS_tchar *path)
>+{

Seems like this function could be simpler with just an snprintf, or just a single strcpy and poke the quotes/nulls into the right spots. No big deal, though.


>   if (argc > callbackIndex) {
...

Ugh, all the string code here makes me sad. :( But it looks correct.


>       callbackFile = CreateFileW(argv[callbackIndex],
>                                  DELETE | GENERIC_WRITE,
>-                                 0, // no sharing!
>+                                 // allow delete, rename, and write
>+                                 FILE_SHARE_DELETE | FILE_SHARE_WRITE,
>                                  NULL, OPEN_EXISTING, 0, NULL);

...but not reading, right? Add a comment?


>+#ifdef XP_WIN
>+int add_dir_entries(const NS_tchar *dirpath, ActionList *list)
>+{
...
>+    FindClose(hFindFile);
>+    {
>+      // Add the directory to be removed to the ActionList.
>+      NS_tchar *quotedpath = get_quoted_path(dirpath);
>+      if (!quotedpath)
>+        return PARSE_ERROR;
>+
>+      Action *action = new RemoveDir();
>+      rv = action->Parse(quotedpath);
>+      if (rv)
>+        LOG(("add_dir_entries Parse error on close: " LOG_S ", err: %d\n", quotedpath, rv));
>+      else
>+        list->Append(action);
>+    }

Any particular reason this chunk is in a block? Doesn't seem like it needs to be...


>+#else
>+int add_dir_entries(const NS_tchar *dirpath, ActionList *list)
>+{
...
>+    switch (ftsdirEntry->fts_info) {
>+      // Filesystem objects that shouldn't be in the application's directories
>+      case FTS_SL:
>+      case FTS_SLNONE:
>+      case FTS_DEFAULT:
>+        LOG(("add_dir_entries: found a non-standard file: " LOG_S "\n",
>+             ftsdirEntry->fts_path));
>+        // Fall through and try to remove as a file

Maybe treat FTS_DEFAULT as an error? This seems to actually be for unknown file types, or perhaps other kinds of files that would be quite bizarre to have show up here (say, a block device?).

Though, TBH, if weird stuff shows up in the app dir, we might as well try deleting it. So I guess this is fine.


>+      case FTS_DP:
...
>+          //        LOG(("FTS_DP quotedpath = %s\n", quotedpath));

Remove?


>+      default:
>+        rv = OK;
>+        break;
>+    }

For completeness, should note the known-values that are hitting the |default| case...

FTS_D   -- I assume you get these, and are deliberately ignoring them.
FTS_DC  -- Hmm. This should actually be an error. Cyclical paths are bad ju-ju.
FTS_DOT -- shouldn't get due to search options 


>+static NS_tchar*
>+getmanifestcontents(const NS_tchar *manifest)
>+{

You loves you some 1 and 2 letter variable names here, eh? :)


>+  NS_tchar *wrb = (NS_tchar *) malloc((ms.st_size + 1) * sizeof(NS_tchar));

Add null check for if malloc fails?


>+int AddPreCompleteActions(ActionList *list, bool channelchange)
>+{
...
>+  if (rb == NULL) {
>+    LOG(("AddPreCompleteActions: error getting contents of precomplete " \
>+         "manifest\n"));
>+    channelchange = false;
>+    // Applications aren't required to have a precomplete manifest yet.
>+    return OK;
>+  }

Aroo? |channelchange| isn't a reference here, so this isn't doing anything.

Comment 81

[Justin Dolske [:Dolske]]

Comment on attachment 525243 [details] [diff] [review]
patch - wince removals outside of updater.cpp

Adios, Au revoir, Auf Wiedersehn, Good Night!

Comment 82

[Justin Dolske [:Dolske]]

(In reply to comment #74)

> > Is there any reason to support "rmrfdir /"? Seems like that could be dangerous
> > if it mistakenly made it into a manifest. Could just add an extra check to see
> > if there's at least one other character (that's not . or /) in the path...
> We already only support relative paths that don't start with /.

How about if "rmdir ./" slipped in? Worth worrying about?

Comment 83

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

I don't think it is worth worrying about at this time.

Comment 84

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

(In reply to comment #80)
> Comment on attachment 525024 [details] [diff] [review]
> main patch
> 
> Remaining bits of review... Will look at interdiff with new patch next!
> 
> 
> >+static NS_tchar*
> >+get_valid_path(NS_tchar **line, bool isdir = false, bool wincepath = true)
> >+{
> 
> |wincepath| is unused.
Done in last patch

> Also, ah, I guess this is more of a check_path() function since it doesn't
> really do anything.
It does strip the trailing forward slash from dir paths and returns the path so it should at least be get_*.

> >       callbackFile = CreateFileW(argv[callbackIndex],
> >                                  DELETE | GENERIC_WRITE,
> >-                                 0, // no sharing!
> >+                                 // allow delete, rename, and write
> >+                                 FILE_SHARE_DELETE | FILE_SHARE_WRITE,
> >                                  NULL, OPEN_EXISTING, 0, NULL);
> 
> ...but not reading, right? Add a comment?
Added it to the existing comment above the call.

> 
> >+#ifdef XP_WIN
> >+int add_dir_entries(const NS_tchar *dirpath, ActionList *list)
> >+{
> ...
> >+    FindClose(hFindFile);
> >+    {
> >+      // Add the directory to be removed to the ActionList.
> >+      NS_tchar *quotedpath = get_quoted_path(dirpath);
> >+      if (!quotedpath)
> >+        return PARSE_ERROR;
> >+
> >+      Action *action = new RemoveDir();
> >+      rv = action->Parse(quotedpath);
> >+      if (rv)
> >+        LOG(("add_dir_entries Parse error on close: " LOG_S ", err: %d\n", quotedpath, rv));
> >+      else
> >+        list->Append(action);
> >+    }
> 
> Any particular reason this chunk is in a block? Doesn't seem like it needs to
> be...
I could rename but Action *action is used elsewhere so I just went with this for simplicity.

> >+#else
> >+int add_dir_entries(const NS_tchar *dirpath, ActionList *list)
> >+{
> ...
> >+    switch (ftsdirEntry->fts_info) {
> >+      // Filesystem objects that shouldn't be in the application's directories
> >+      case FTS_SL:
> >+      case FTS_SLNONE:
> >+      case FTS_DEFAULT:
> >+        LOG(("add_dir_entries: found a non-standard file: " LOG_S "\n",
> >+             ftsdirEntry->fts_path));
> >+        // Fall through and try to remove as a file
> 
> Maybe treat FTS_DEFAULT as an error? This seems to actually be for unknown file
> types, or perhaps other kinds of files that would be quite bizarre to have show
> up here (say, a block device?).
> 
> Though, TBH, if weird stuff shows up in the app dir, we might as well try
> deleting it. So I guess this is fine.
Songbird has been treating this case as a file for a while now so I feel safer with copying this part.

> 
> >+      case FTS_DP:
> ...
> >+          //        LOG(("FTS_DP quotedpath = %s\n", quotedpath));
> 
> Remove?
Done

> 
> >+      default:
> >+        rv = OK;
> >+        break;
> >+    }
> 
> For completeness, should note the known-values that are hitting the |default|
> case...
> 
> FTS_D   -- I assume you get these, and are deliberately ignoring them.
> FTS_DC  -- Hmm. This should actually be an error. Cyclical paths are bad ju-ju.
> FTS_DOT -- shouldn't get due to search options 
Added comment about FTS_D and made FTS_DC an error.

> 
> >+static NS_tchar*
> >+getmanifestcontents(const NS_tchar *manifest)
> >+{
> 
> You loves you some 1 and 2 letter variable names here, eh? :)
Not particularly, just didn't feel like changing the names from where that code was originally.

> >+  NS_tchar *wrb = (NS_tchar *) malloc((ms.st_size + 1) * sizeof(NS_tchar));
> 
> Add null check for if malloc fails?
Done

> 
> >+int AddPreCompleteActions(ActionList *list, bool channelchange)
> >+{
> ...
> >+  if (rb == NULL) {
> >+    LOG(("AddPreCompleteActions: error getting contents of precomplete " \
> >+         "manifest\n"));
> >+    channelchange = false;
> >+    // Applications aren't required to have a precomplete manifest yet.
> >+    return OK;
> >+  }
> 
> Aroo? |channelchange| isn't a reference here, so this isn't doing anything.
Fixed

Comment 85

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

Attachment: main patch updated to comments

Comment 86

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

Attachment: Additional tests patch w/ test helper wince code removed

Comment 87

[Justin Dolske [:Dolske]]

Comment on attachment 525287 [details] [diff] [review]
main patch updated to comments

> int
> PatchFile::Prepare()
> {
...
>+#ifdef XP_WIN
>+  char *sourcefile = (char *) malloc(MAXPATHLEN);
>+  if (!sourcefile)
>+    return MEM_ERROR;

Could just use an array on the stack? [Ditto for the other ::Prepare doing this]

Comment 88

[Justin Dolske [:Dolske]]

Comment on attachment 525025 [details] [diff] [review]
patch - test binary files

rs=me.

Comment 89

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

Attachment: main patch updated to comments

(In reply to comment #87)
> Comment on attachment 525287 [details] [diff] [review]
> main patch updated to comments
>...
> Could just use an array on the stack? [Ditto for the other ::Prepare doing
> this]
Done

Comment 90

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

Attachment: Followup test only fix to make log parsing happy

Test only fix pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/a174b86200d6

Comment 91

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

Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/4ceb3de77444
http://hg.mozilla.org/mozilla-central/rev/860d857e899d
http://hg.mozilla.org/mozilla-central/rev/8d0f07c4f8b7
http://hg.mozilla.org/mozilla-central/rev/47ceb2f1aac9
http://hg.mozilla.org/mozilla-central/rev/f4f18866ee3a
http://hg.mozilla.org/mozilla-central/rev/35e27b34c7c5

Probably good to have litmus tests for this or bug 644517.

Comment 92

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

Comment on attachment 525289 [details] [diff] [review]
Additional tests patch w/ test helper wince code removed

received rs=dolske on this patch.

Comment 93

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

I suck... besides the following changeset mentioned in comment #90
http://hg.mozilla.org/mozilla-central/rev/a174b86200d6

I had to land two additional test fixes
http://hg.mozilla.org/mozilla-central/rev/e5bf084b2375
http://hg.mozilla.org/mozilla-central/rev/bfce65c217d4

Comment 94

[Rail Aliiev [:rail]]

The patch broke l10n repacks as well, see the attached log

Comment 95

[Rail Aliiev [:rail]]

Attachment: l10n repacks log

Comment 96

[Johnathan Nightingale [:johnath]]

I've filed bug 649322 to disable OSX64 opt nightly updates as a precaution, since the builds have been permaorange with "precomplete file is missing" since this bug landed.

Comment 97

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

Attachment: patch to fix Mac orange

This has r=khuey

Comment 98

[Rail Aliiev [:rail]]

rstrong, how about l10n repacks? I don't think that attachment 525499 [details] [diff] [review] fixes them...

Comment 99

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

See bug 649430

Comment 100

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

Comment on attachment 525499 [details] [diff] [review]
patch to fix Mac orange

Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/b1592bb02a93

Comment 101

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

Comment on attachment 525499 [details] [diff] [review]
patch to fix Mac orange

Pushed to mozilla-central and OS X64 opt is now green
http://hg.mozilla.org/mozilla-central/rev/b1592bb02a93

Pushed to mozilla-aurora (got a=LegNeato in person)
http://hg.mozilla.org/mozilla-aurora/rev/155da7ebad86

Comment 102

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

The tests should suffice for this.