600098 - update access and modification time on top-level application bundle on Mac OS X after successful update

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Josh Aas]

When Mac OS X 10.5 users update from a ppc/i386 universal binary to an i386/x86_64 universal binary their build will no longer launch, even manually. The problem is that Mac OS X's Launch Services doesn't realize that the app was updated and it incorrectly launches it in x86_64 mode (it doesn't properly read the updated Info.plist file for binary preferences). Since the x86_64 binary is not compatible with 10.5 the application crashes.

Running "touch" on the bundle fixes the problem. We should update the timestamps after any successful update to avoid this problem.

Comment 1

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

Will this also fix bug 314794?

Comment 2

[Josh Aas]

(In reply to comment #1)
> Will this also fix bug 314794?

It might. When we have a reviewed patch we should test.

Comment 3

[Josh Aas]

Attachment: fix v1.0

I have not actually tested this yet but it's probably the right idea and it compiles.

Comment 4

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

I think the following line would be a better place to do this.
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/updater/updater.cpp#1714

Comment 5

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

I'm actually ok with this since it isn't possible to log in the other ifdef. On the other, keeping this type of one-off code in main makes it a little simpler to find / maintain so perhaps before the call to LogFinish would be more appropriate.
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/updater/updater.cpp#1700

Comment 6

[Josh Aas]

The main reasons I chose to put the code where I did were 1) I was more confident that the current working directory would be correct there and 2) easy access to the success status for the update.

Comment 7

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

The working dir isn't changed until LaunchCallbackApp is called and gSucceeded is available but this is ok.

Comment 8

[Josh Aas]

Comment on attachment 478948 [details] [diff] [review]
fix v1.0

I'm not sure how to test this. Rob - can you give me some instructions and also test this yourself when you review?

Comment 9

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

Not sure if I will be able to test this myself. The following are comprehensive instructions for manually applying a mar which should be adequate for testing.
https://wiki.mozilla.org/Software_Update:Manually_Installing_a_MAR_file

You should be able to use the following mar which will add some files
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/test/unit/data/aus-0110_general.mar

Comment 10

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

Also, you could test the directory's time stamp in any of the tests disabled in bug 599477 if nsIProcess was working.

Comment 11

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

Comment on attachment 478948 [details] [diff] [review]
fix v1.0

>diff --git a/toolkit/mozapps/update/updater/updater.cpp b/toolkit/mozapps/update/updater/updater.cpp
>--- a/toolkit/mozapps/update/updater/updater.cpp
>+++ b/toolkit/mozapps/update/updater/updater.cpp
>...
>@@ -1425,20 +1428,39 @@ UpdateThreadFunc(void *param)
>                NS_T("%s/update.mar"), gSourcePath);
> 
>   int rv = gArchiveReader.Open(dataFile);
>   if (rv == OK) {
>     rv = DoUpdate();
>     gArchiveReader.Close();
>   }
> 
>-  if (rv)
>+  if (rv) {
>     LOG(("failed: %d\n", rv));
>-  else
>+  }
>+  else {
>+#ifdef XP_MACOSX
>+    // If the update was successful we need to update the timestamp
>+    // on the top-level Mac OS X bundle directory so that Mac OS X's
>+    // Launch Services picks up any major changes. Here we assume that
>+    // the current working directory is the top-level bundle directory.
>+    char* cwd = getcwd(NULL, 0);
>+    if (cwd) {
>+      if (utimes(cwd, NULL) != 0) {
>+        LOG(("Couldn't set access/modification time on application bundle.\n"));
>+      }
>+      free(cwd);
>+    }
>+    else {
>+      LOG(("Couldn't get current working directory.\n"));
Too vague... please note what the resultant issue is even if it just mentions unable to set the access/modification time on application bundle.

>+    }
>+#endif
>+    
nit: remove the extra spaces

r=me if you've also tested the patch

Comment 12

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

Also, is nsIProcess going to be fixed or <insert other solution> in the works so the tests in bug 599477 can be re-enabled? I can write the tests for this bug after are.

Comment 13

[Josh Aas]

Attachment: fix v1.1

Address review comments.

Thanks for the update testing instructions, I tested and this code seems to work.

Comment 14

[Justin Dolske [:Dolske]]

I bet this fixes bug 504397 and bug 493503 too! Too bad we didn't fix this last year (I'm switching to IE!).

Comment 15

[Josh Aas]

I think there is something not quite right about how I'm testing this. An unpatched build has the same behavior as a patched build - the times for the bundle directory are updated to the current time (access, modify, and change times).

Rob - do you see the same thing?

Comment 16

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #14)
> I bet this fixes bug 504397 

It won't fix that bug, since Josh's patch is only touching the top-level bundle, and the JEP and MRJ are "child" bundles.  And also since there's no JEP yet on 2.0 ;)

> and bug 493503 too!

Probably will fix the Finder icon, but not the Dock icon; I recently did this sort of thing in one of my apps, and the only way I could fix the Dock icon was to remove the app from the Dock and re-add it.

Comment 17

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

(In reply to comment #15)
> I think there is something not quite right about how I'm testing this. An
> unpatched build has the same behavior as a patched build - the times for the
> bundle directory are updated to the current time (access, modify, and change
> times).
> 
> Rob - do you see the same thing?
I don't have a Mac at home so I can't test.

Another thing you can do to test is to get a patched and unpatched updater bundle, download / install a dmg that has a partial update, and perform the steps to manually apply the mar.

You might also be able to test with an empty mar and verify the patched changes the timestamp and the unpatched doesn't.
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/test/unit/data/empty.mar

Comment 18

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

btw: I probably won't be able to test this until Wednesday but comment #17 would be what I would have to do to test this patch.

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Juan/Whimboo, can we get some help testing these fixes. Rob or Josh can help you understand what's needed.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So for testing, does it mean we have to use a PPC box or does a Mac Pro/Mini also work? This is only a problem on OS X 10.5 and 10.6 isn't affected? Would be great to get some more information about the specifics of the single steps.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Oh, and can someone please create a tryserver build with this fix included?

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

This is a problem when using the updated to move from the 20100925 or earlier nightly build to the latest available update on OSX 10.5. PPC not required (nightlies don't work on PPC anyway :))

Comment 23

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

I'm out sick so hopefully Josh can finish this up.

Comment 24

[LegNeato]

Isn't this bug 554997?

Comment 25

[Mike Beltzner [:beltzner, not reading bugmail]]

Yeah, but the effect is a ton worse, here, I think.

Comment 26

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

(In reply to comment #24)
> Isn't this bug 554997?
and bug 554997 is bug 314794 which is this bug as well. ;)

Comment 27

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

I do have a ppc Mac at home with a build env prior to ppc being obsolete and I'll try to verify that the patch does in fact change the timestamp on the bundle. That should be all that is necessary since Josh verified that just touching the bundle is enough.

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Does it mean QA doesn't have to get the hands wet? Thanks Robert! If that's the case please remove the qawanted keyword.

Comment 29

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

Don't know yet

Comment 30

[Mike Beltzner [:beltzner, not reading bugmail]]

No, QA has to get its hands wet. Let's not count on the sick person with a PPC at home that isn't what we're going to ship this towards. Let's let the sick person be sick, and get it done.

Comment 31

[Josh Aas]

I'll have patched try server build ready soon (right now it is changeset  on try server). I'm still working to reproduce on 10.5.

Comment 32

[Josh Aas]

Rob - even if we succeed in successfully patching the updater how are we going to deliver the patched version without causing the problem we're fixing? Users will still be updating with the broken updater they currently have.

Comment 33

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

The patch does the right thing. It turns out that Mac OS X will update the last modified time if files / dirs are added directly under the bundle which is what aus-0110_general.mar does. If you use the empty mar or update using aus-0110_general.mar, touch the bundle to set it to an old dat, and then run it again you will see that the patch is working correctly.

Comment 34

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

(In reply to comment #32)
> Rob - even if we succeed in successfully patching the updater how are we going
> to deliver the patched version without causing the problem we're fixing? Users
> will still be updating with the broken updater they currently have.
I thought that the users are already in a state where they are unable to launch Firefox... is this not true?

Comment 35

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

(should think before he writes)

To update users that don't have the changes that landed in bug 571367 as of yet (e.g. nightly users before bug 571367 and beta users) you could have them update to a build with this change and without the changes from bug 571367 and users that have a build newer than the last update (e.g. they downloaded a dmg) should be fine since the act of installing should change the timestamp... correct? The information in the update url is enough to differentiate the two. Also, the majority of beta users likely have auto update turned on and probably won't even see the update ui due to the following if the update only contains this change.
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/updater/progressui_osx.mm#126

If there are nightly users that updated to the changes from bug 571367 you could offer an improperly formatted update snippet and they will get an error page after 10 failures.

Comment 36

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

(In reply to comment #35)
>...
> If there are nightly users that updated to the changes from bug 571367 you
> could offer an improperly formatted update snippet and they will get an error
> page after 10 failures.
Error page with a download link (Bug 595455)

Comment 37

[Josh Aas]

(In reply to comment #34)
> I thought that the users are already in a state where they are unable to launch
> Firefox... is this not true?

It's not true for everyone yet because we turned off updates to stop this from happening to more people. If we're going to require current trunk nightly users on 10.5 to download a new build or follow the "manual touch" instructions then there is really no point to keeping nightly updates turned off waiting for a patch. Even when we've patched this the existing updater they have won't have the fix.

Aside from nightly users, we have beta 6 and 1.9.2 users on 10.5 to be concerned with because eventually they'll have to upgrade. We have plenty of time to fix the updater on the 1.9.2 branch, but what are we going to do about beta 6 users on 10.5? Require that they download a new build manually or use our "manual touch" fix?

Comment 38

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

(In reply to comment #37)
> (In reply to comment #34)
> > I thought that the users are already in a state where they are unable to launch
> > Firefox... is this not true?
> 
> It's not true for everyone yet because we turned off updates to stop this from
> happening to more people. If we're going to require current trunk nightly users
> on 10.5 to download a new build or follow the "manual touch" instructions then
> there is really no point to keeping nightly updates turned off waiting for a
> patch. Even when we've patched this the existing updater they have won't have
> the fix.
> 
> Aside from nightly users, we have beta 6 and 1.9.2 users on 10.5 to be
> concerned with because eventually they'll have to upgrade. We have plenty of
> time to fix the updater on the 1.9.2 branch, but what are we going to do about
> beta 6 users on 10.5? Require that they download a new build manually or use
> our "manual touch" fix?
See comment #36

Comment 39

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

and comment #35

On Mac OS X and Windows we also support a executing a post update binary that can be included in the update - Bug 562151. It might be enough to just have it perform the touch.

Comment 40

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

I suspect another even simpler workaround would be to add a new file in the bundle directory. Josh, can you confirm that Mac OS X will update the last modified time of the bundle if a file is added to the bundle dir?

Comment 41

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

file or dir that is.

Comment 42

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

(In reply to comment #40)
> I suspect another even simpler workaround would be to add a new file in the
> bundle directory. Josh, can you confirm that Mac OS X will update the last
> modified time of the bundle if a file is added to the bundle dir?
So, this *should* workaround the chicken / egg issue with users not having the fix from this patch.

Comment 43

[Josh Aas]

(In reply to comment #42)
> (In reply to comment #40)
> > I suspect another even simpler workaround would be to add a new file in the
> > bundle directory. Josh, can you confirm that Mac OS X will update the last
> > modified time of the bundle if a file is added to the bundle dir?
> So, this *should* workaround the chicken / egg issue with users not having the
> fix from this patch.

I can confirm that you're correct about this. This is why in my testing I was seeing the proper behavior even with an unpatched updater - I was using a mar file that added  a file in the top-level directory.

Comment 44

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

Bug 366846 tracks a systematic fix to this class of problems.

Comment 45

[Josh Aas]

Attachment: 1.9.2 branch fix v1.1

Comment 46

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

Nick, will you be able to add a file to the top level of the bundle via the update packaging?

Bug 366846 is to say the least somewhat complex to fix and with it being late in the cycle somewhat dangerous. If this is something we want for Firefox 4.0 I can drop other work (I hope that someone else can pick them up) and focus on getting that fixed. The current patch in bug 366846 is not the way I think we should go since it would be done in the Firefox process and I would much prefer to keep the writing to files for update as a separate / isolated process.

Comment 47

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

Josh, can you please comment in bug 599477 regarding what can be done to get those tests running again (preferably by getting nsIProcess working in the new universal build world)? Once these tests are working I can update the tests to test this bug.

Comment 48

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

Rob, I understand re 366846, just wanted to throw that into the mix as a broader comment.

I'll look into packaging an extra file into the complete mar file on build.

Comment 49

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

Thanks Nick, I was just throwing out some additional details in case drivers would like me to make that a priority.

Comment 50

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Also CC'ing Marcia. She is mostly using the PPC box in our lab. So we could have another pair of eyes for tests because I don't have a PPC box.

Comment 51

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

(In reply to comment #50)
> Also CC'ing Marcia. She is mostly using the PPC box in our lab. So we could
> have another pair of eyes for tests because I don't have a PPC box.
per beltzner

(In reply to comment #22)
> This is a problem when using the updated to move from the 20100925 or earlier
> nightly build to the latest available update on OSX 10.5. PPC not required
> (nightlies don't work on PPC anyway :))

so, this should not be tested on a ppc box and as a matter of fact can't be tested on a ppc box without having to hack a test together as I ended up doing.

Comment 52

[juan becerra [:juanb]]

I'm taking a look now. I'll test it using the information in comment #9 and comment #17

Comment 53

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

Rob, could you check I have this straight in my head:
1) adding a file at the top level (next to Contents/) makes OS X read the plist again. This fixes launching the app manually
2) josh's patches here touch the bundle in the updater, achieving the same result
3) bug 600362 is to launch the right arch after an update

We need 1 for 4.0 nightlies and betas because we have builds out there with updaters we can't fix. We'll have to keep doing it forever, because not everyone will update from the old-style universal promptly. Bug 600362 means that we'll crash after the update on 10.5, but at least we'll be launchable manually afterwards. 

To work around 600362, do we need to do some sort of small update that fixes just the updater, then offer 4.0b7 ? Seems doubtful we'd get the majority updated before b7 ships.

For major updates from 1.9.1 & 1.9.2 we can fix this bug and 600362 ahead of time.

(In reply to comment #0)
> Since the x86_64 binary is
> not compatible with 10.5 the application crashes.

Is relaxing this restriction an option ? It would resolve all the launching the wrong arch bugs in a one go.

Comment 54

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

(In reply to comment #53)
> 2) josh's patches here touch the bundle in the updater, achieving the same
> result

rewrite:
2) josh's patches on this bug make the updater set the mtime of the bundle, achieving the same result

Comment 55

[juan becerra [:juanb]]

Ran this on a 10.5 machine using a nightly build from the 26th.

Rob, I've also confirmed that applying the mar creates a dir "mar_test" within Minefield.app (at the same level as Contents) and that the last modified time of the Minefield.app is updated after I applied the mar.

Comment 56

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

Attachment: [checked in] Add force_plist_reload at top level when creating a complete update

Comparing unpacked complete mars (generated with and without this patch):
diff -ru before/ after/
Only in after/: force_plist_reload
diff -ru before/update.manifest after/update.manifest
--- before/update.manifest	2010-09-28 19:20:53.000000000 -0700
+++ after/update.manifest	2010-09-28 19:21:19.000000000 -0700
@@ -93,6 +93,7 @@
 add "Contents/Resources/document.icns"
 add "Contents/Resources/en.lproj/InfoPlist.strings"
 add "Contents/Resources/firefox.icns"
+add "force_plist_reload"
 remove "Contents/MacOS/.autoreg"
 remove "Contents/MacOS/libjsj.dylib"
 remove "Contents/MacOS/libsqlite3.dylib"

A partial from b6 to b7 would also get an add instruction for this new file, but not one from b7 to b8. That's OK if b7 ships with the right fixes in it.

Comment 57

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

(In reply to comment #53)
> Rob, could you check I have this straight in my head:
> 1) adding a file at the top level (next to Contents/) makes OS X read the plist
> again. This fixes launching the app manually
It is my understanding from Josh and a little research that touching the bundle causes the plist to be read again and Josh (comment #43), Juan and I've confirmed that adding a directory alongside Contents changes the last modified time for the bundle.

> 2) josh's patches here touch the bundle in the updater, achieving the same
> result
Correct though on the next update. We could have the updater launch a binary after the update has completed since it reads the updated updater.ini for a relative file to launch after a successful update but that is more complex than just adding the file.

> 3) bug 600362 is to launch the right arch after an update
Correct

> We need 1 for 4.0 nightlies and betas because we have builds out there with
> updaters we can't fix. We'll have to keep doing it forever, because not
> everyone will update from the old-style universal promptly.
Or for these users only allow updates to a mar that contains the fix

> Bug 600362 means
> that we'll crash after the update on 10.5, but at least we'll be launchable
> manually afterwards.
> 
> To work around 600362, do we need to do some sort of small update that fixes
> just the updater, then offer 4.0b7 ? Seems doubtful we'd get the majority
> updated before b7 ships.
I think that is the best course to take. Even if they haven't updated to b7 before it ships they could only be offered an update to the one with the fixes that allows them to update to b7.

> For major updates from 1.9.1 & 1.9.2 we can fix this bug and 600362 ahead of
> time.
Correct

> (In reply to comment #0)
> > Since the x86_64 binary is
> > not compatible with 10.5 the application crashes.
> 
> Is relaxing this restriction an option ? It would resolve all the launching the
> wrong arch bugs in a one go.
No idea so I'll leave this to Josh.

Comment 58

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

(In reply to comment #57)
> > We need 1 for 4.0 nightlies and betas because we have builds out there with
> > updaters we can't fix. We'll have to keep doing it forever, because not
> > everyone will update from the old-style universal promptly.
> Or for these users only allow updates to a mar that contains the fix

Don't think we have that level of control for nightlies, where it's the latest build or nothing. If the cost of reloading Info.plist is just a slightly slower startup after update then we could live with that. It wouldn't affect Talos results.
 
> I think that is the best course to take. Even if they haven't updated to b7
> before it ships they could only be offered an update to the one with the fixes
> that allows them to update to b7.

We'd have to change the buildID or version to make users of b6+updater_fix separately addressable. Looks like changes to Contents/MacOS/application.ini affect the AUS query, so that would be doable.

Comment 59

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

(In reply to comment #58)
> (In reply to comment #57)
> > > We need 1 for 4.0 nightlies and betas because we have builds out there with
> > > updaters we can't fix. We'll have to keep doing it forever, because not
> > > everyone will update from the old-style universal promptly.
> > Or for these users only allow updates to a mar that contains the fix
> 
> Don't think we have that level of control for nightlies, where it's the latest
> build or nothing.
The build id is in the url.

> If the cost of reloading Info.plist is just a slightly slower
> startup after update then we could live with that. It wouldn't affect Talos
> results.
> 
> > I think that is the best course to take. Even if they haven't updated to b7
> > before it ships they could only be offered an update to the one with the fixes
> > that allows them to update to b7.
> 
> We'd have to change the buildID or version to make users of b6+updater_fix
> separately addressable. Looks like changes to Contents/MacOS/application.ini
> affect the AUS query, so that would be doable.
build id would be enough just like we do with nightly builds.

Comment 60

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

(In reply to comment #59)
> The build id is in the url.

Absolutely, but for nightlies all it's used for is determining if a partial should be offered. Nightlies are a many -> latest mapping, releases use specific A -> B mappings.

Comment 61

[Mike Beltzner [:beltzner, not reading bugmail]]

Can we get this patch reviewed so we can re-enable updates? I know we can re-enable some updates now, but I'd rather just get it all done in one shot.

Comment 62

[:Gavin Sharp [email: [e-mail]]]

We'll need the updater fix in the next 1.9.2 update (and 1.9.1 assuming there's another coming?).

Comment 63

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

In the past we have never updated users to a release that is two releases ahead (e.g. from 3.0.x to 3.6.x or in this instance 3.5.x to 4.0.x) though I do think it would be a good thing if we did do this.

Comment 64

[Josh Aas]

pushed access/modification time update code for updater to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/d2b9dae424f6

Comment 65

[[e-mail] (:bhearsum)]

Comment on attachment 479272 [details] [diff] [review]
[checked in] Add force_plist_reload at top level when creating a complete update

Pushed to m-c: changeset:   54876:83455e2c84db

Comment 66

[:Gavin Sharp [email: [e-mail]]]

I believe that makes this FIXED (both patches have landed). We still need to do tests on the fixes before re-enabling updates in bug 600289, though.

Comment 68

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

Will likely be able to update an existing xpcshell test for this

Comment 69

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

Test added in bug 601518

Comment 70

[Josh Aas]

Attachment: 1.9.1 branch fix v1.1

Comment 71

[Josh Aas]

pushed to mozilla-1.9.2

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/bc26273e8147

Comment 72

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

Comment on attachment 491294 [details] [diff] [review]
1.9.1 branch fix v1.1

>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
>...
>@@ -1163,20 +1164,39 @@ UpdateThreadFunc(void *param)
>   NS_tsnprintf(dataFile, MAXPATHLEN, NS_T("%s/update.mar"), gSourcePath);
> 
>   int rv = gArchiveReader.Open(dataFile);
>   if (rv == OK) {
>     rv = DoUpdate();
>     gArchiveReader.Close();
>   }
> 
>-  if (rv)
>+  if (rv) {
>     LOG(("failed: %d\n", rv));
>-  else
>+  }
>+  else {
>+#ifdef XP_MACOSX
>+    // If the update was successful we need to update the timestamp
>+    // on the top-level Mac OS X bundle directory so that Mac OS X's
>+    // Launch Services picks up any major changes. Here we assume that
>+    // the current working directory is the top-level bundle directory.
>+    char* cwd = getcwd(NULL, 0);
>+    if (cwd) {
>+      if (utimes(cwd, NULL) != 0) {
>+        LOG(("Couldn't set access/modification time on application bundle.\n"));
>+      }
>+      free(cwd);
>+    }
>+    else {
>+      LOG(("Couldn't get current working directory for setting "
>+           "access/modification time on application bundle.\n"));
>+    }
>+#endif
>     LOG(("succeeded\n"));
>+  }
nit: add a newline here to be consistent with your other patch.

>   WriteStatusFile(rv);

The difference between this patch and the trunk / 1.9.2 patches are cosmetic from my inspection and didn't really need a review. If that is truly the case then r=me

Comment 73

[Josh Aas]

Attachment: 1.9.1 branch fix v1.2

Comment 74

[Josh Aas]

pushed to mozilla-1.9.1

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/da1f11e5b1e0