Last Comment Bug 600098 - update access and modification time on top-level application bundle on Mac OS X after successful update
: update access and modification time on top-level application bundle on Mac OS...
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Application Update (show other bugs)
: Trunk
: All Mac OS X
: -- normal with 1 vote (vote)
: mozilla2.0b7
Assigned To: Josh Aas
:
Mentors:
: 314794 (view as bug list)
Depends on: 601518
Blocks: 493503 314794 504397 554997 571367 600289
  Show dependency treegraph
 
Reported: 2010-09-27 18:14 PDT by Josh Aas
Modified: 2010-11-18 13:34 PST (History)
17 users (show)
robert.strong.bugs: in‑testsuite+
robert.strong.bugs: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta7+
.13+
.13-fixed
.16+
.16-fixed


Attachments
fix v1.0 (1.68 KB, patch)
2010-09-27 18:43 PDT, Josh Aas
robert.strong.bugs: review+
Details | Diff | Splinter Review
fix v1.1 (1.75 KB, patch)
2010-09-27 20:38 PDT, Josh Aas
no flags Details | Diff | Splinter Review
1.9.2 branch fix v1.1 (1.79 KB, patch)
2010-09-28 14:06 PDT, Josh Aas
robert.strong.bugs: review+
christian: approval1.9.2.13+
Details | Diff | Splinter Review
[checked in] Add force_plist_reload at top level when creating a complete update (661 bytes, patch)
2010-09-28 19:28 PDT, Nick Thomas [:nthomas]
bhearsum: review+
Details | Diff | Splinter Review
1.9.1 branch fix v1.1 (1.67 KB, patch)
2010-11-17 12:56 PST, Josh Aas
robert.strong.bugs: review+
Details | Diff | Splinter Review
1.9.1 branch fix v1.2 (1.67 KB, patch)
2010-11-17 19:45 PST, Josh Aas
christian: approval1.9.1.16+
Details | Diff | Splinter Review

Description Josh Aas 2010-09-27 18:14:23 PDT
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 [:rstrong] (use needinfo to contact me) 2010-09-27 18:24:40 PDT
Will this also fix bug 314794?
Comment 2 Josh Aas 2010-09-27 18:39:35 PDT
(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 2010-09-27 18:43:11 PDT
Created attachment 478948 [details] [diff] [review]
fix v1.0

I have not actually tested this yet but it's probably the right idea and it compiles.
Comment 4 Robert Strong [:rstrong] (use needinfo to contact me) 2010-09-27 18:45:18 PDT
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 [:rstrong] (use needinfo to contact me) 2010-09-27 18:55:22 PDT
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 2010-09-27 19:04:42 PDT
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 [:rstrong] (use needinfo to contact me) 2010-09-27 19:15:24 PDT
The working dir isn't changed until LaunchCallbackApp is called and gSucceeded is available but this is ok.
Comment 8 Josh Aas 2010-09-27 19:36:00 PDT
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 [:rstrong] (use needinfo to contact me) 2010-09-27 19:48:15 PDT
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 [:rstrong] (use needinfo to contact me) 2010-09-27 19:54:33 PDT
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 [:rstrong] (use needinfo to contact me) 2010-09-27 20:20:08 PDT
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 [:rstrong] (use needinfo to contact me) 2010-09-27 20:22:45 PDT
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 2010-09-27 20:38:22 PDT
Created attachment 478963 [details] [diff] [review]
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] 2010-09-27 21:08:35 PDT
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 2010-09-27 21:36:54 PDT
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) 2010-09-27 21:38:38 PDT
(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 [:rstrong] (use needinfo to contact me) 2010-09-27 21:47:09 PDT
(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 [:rstrong] (use needinfo to contact me) 2010-09-28 00:14:54 PDT
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] 2010-09-28 09:59:12 PDT
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) 2010-09-28 10:11:37 PDT
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) 2010-09-28 10:13:07 PDT
Oh, and can someone please create a tryserver build with this fix included?
Comment 22 Mike Beltzner [:beltzner, not reading bugmail] 2010-09-28 10:14:04 PDT
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 [:rstrong] (use needinfo to contact me) 2010-09-28 10:33:08 PDT
I'm out sick so hopefully Josh can finish this up.
Comment 24 christian 2010-09-28 10:38:37 PDT
Isn't this bug 554997?
Comment 25 Mike Beltzner [:beltzner, not reading bugmail] 2010-09-28 10:40:38 PDT
Yeah, but the effect is a ton worse, here, I think.
Comment 26 Robert Strong [:rstrong] (use needinfo to contact me) 2010-09-28 10:48:48 PDT
(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 [:rstrong] (use needinfo to contact me) 2010-09-28 11:11:39 PDT
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) 2010-09-28 11:22:40 PDT
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 [:rstrong] (use needinfo to contact me) 2010-09-28 11:24:07 PDT
Don't know yet
Comment 30 Mike Beltzner [:beltzner, not reading bugmail] 2010-09-28 11:25:49 PDT
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 2010-09-28 11:48:53 PDT
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 2010-09-28 11:54:37 PDT
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 [:rstrong] (use needinfo to contact me) 2010-09-28 11:55:32 PDT
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 [:rstrong] (use needinfo to contact me) 2010-09-28 11:57:37 PDT
(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 [:rstrong] (use needinfo to contact me) 2010-09-28 12:15:20 PDT
(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 [:rstrong] (use needinfo to contact me) 2010-09-28 12:17:14 PDT
(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 2010-09-28 12:20:23 PDT
(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 [:rstrong] (use needinfo to contact me) 2010-09-28 12:22:09 PDT
(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 [:rstrong] (use needinfo to contact me) 2010-09-28 12:32:32 PDT
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 [:rstrong] (use needinfo to contact me) 2010-09-28 13:22:33 PDT
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 [:rstrong] (use needinfo to contact me) 2010-09-28 13:23:04 PDT
file or dir that is.
Comment 42 Robert Strong [:rstrong] (use needinfo to contact me) 2010-09-28 13:51:17 PDT
(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 2010-09-28 13:57:20 PDT
(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] 2010-09-28 14:01:54 PDT
Bug 366846 tracks a systematic fix to this class of problems.
Comment 45 Josh Aas 2010-09-28 14:06:58 PDT
Created attachment 479160 [details] [diff] [review]
1.9.2 branch fix v1.1
Comment 46 Robert Strong [:rstrong] (use needinfo to contact me) 2010-09-28 14:13:26 PDT
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 [:rstrong] (use needinfo to contact me) 2010-09-28 14:32:22 PDT
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] 2010-09-28 15:15:22 PDT
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 [:rstrong] (use needinfo to contact me) 2010-09-28 15:17:09 PDT
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) 2010-09-28 16:05:31 PDT
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 [:rstrong] (use needinfo to contact me) 2010-09-28 16:07:59 PDT
(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] 2010-09-28 16:26:08 PDT
I'm taking a look now. I'll test it using the information in comment #9 and comment #17
Comment 53 Nick Thomas [:nthomas] 2010-09-28 18:27:25 PDT
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] 2010-09-28 18:29:49 PDT
(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] 2010-09-28 18:48:02 PDT
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] 2010-09-28 19:28:47 PDT
Created attachment 479272 [details] [diff] [review]
[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 [:rstrong] (use needinfo to contact me) 2010-09-28 19:45:16 PDT
(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] 2010-09-28 20:07:12 PDT
(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 [:rstrong] (use needinfo to contact me) 2010-09-28 21:41:03 PDT
(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] 2010-09-28 21:52:02 PDT
(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] 2010-09-29 13:11:24 PDT
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: gavin@gavinsharp.com] 2010-09-29 13:32:37 PDT
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 [:rstrong] (use needinfo to contact me) 2010-09-29 13:36:21 PDT
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 2010-09-29 19:58:25 PDT
pushed access/modification time update code for updater to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/d2b9dae424f6
Comment 65 Ben Hearsum (:bhearsum) 2010-10-01 09:35:30 PDT
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: gavin@gavinsharp.com] 2010-10-01 14:12:02 PDT
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 67 Robert Strong [:rstrong] (use needinfo to contact me) 2010-10-02 11:03:49 PDT
*** Bug 314794 has been marked as a duplicate of this bug. ***
Comment 68 Robert Strong [:rstrong] (use needinfo to contact me) 2010-10-02 11:04:58 PDT
Will likely be able to update an existing xpcshell test for this
Comment 69 Robert Strong [:rstrong] (use needinfo to contact me) 2010-10-19 21:47:44 PDT
Test added in bug 601518
Comment 70 Josh Aas 2010-11-17 12:56:53 PST
Created attachment 491294 [details] [diff] [review]
1.9.1 branch fix v1.1
Comment 71 Josh Aas 2010-11-17 13:32:59 PST
pushed to mozilla-1.9.2

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/bc26273e8147
Comment 72 Robert Strong [:rstrong] (use needinfo to contact me) 2010-11-17 16:10:35 PST
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 2010-11-17 19:45:52 PST
Created attachment 491441 [details] [diff] [review]
1.9.1 branch fix v1.2
Comment 74 Josh Aas 2010-11-18 13:34:16 PST
pushed to mozilla-1.9.1

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

Note You need to log in before you can comment on or make changes to this bug.