600362 - solve problem in which updater can launch incompatible binary after update

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Josh Aas]

We need to solve the problem in which the updater can launch an incompatible binary after an update. This happens when we update from ppc/i386 to i386/x86_64 on Mac OS X 10.5 since Mac OS X prefers x86_64 over i386 and our x86_64 binary is incompatible with 10.5. The bundle prefs aren't taken into account because the updater launches the binary directly instead of the app bundle.

Comment 1

[Josh Aas]

Attachment: approach #1: prefer the current arch v1.0

I think we have two options for fixing this. We could launch the app bundle or we can play it safe and prefer the architecture for the updater process. The app bundle is probably a better idea since the update could have changed compatibility with the current arch.

This patch implements the approach where we prefer the current architecture. I haven't tested it yet and I prefer the bundle approach but since I already wrote it, here it is.

Comment 2

[Josh Aas]

We want to launch the bundle from the updater so that we don't have to know the architecture to launch from within the updater. That is hard to know, especially if you want this to work for more than just Firefox. We don't want to duplicate the selection logic based on the OS, available binaries, and the contents of the app's Info.plist which specifies compatibility.

The problem with launching the bundle rather than the binary is that, believe it or not, we can't pass command line arguments if we launch the bundle (I have yet to find a way, anyway, that works on 10.5). So if we don't need to pass command line arguments to the application from the updater, then this is easy. If we do, I don't know what to do. Rob - do you know if the updater application is ever invoked with arguments specified to be passed on to the callback app? Do we have to support that, at least on Mac OS X?

This is only a problem for the updater's launching code because only the updater has to deal with the possibility that the preferred architecture has changed. App-initiated restart code can assume that it wants the current architecture and specify that via "posix_spawnp" attribute.

Comment 3

[Josh Aas]

BTW, my approach #1 where we prefer the updater's architecture won't work. That would, for example, launch i386 Firefox on 10.6 instead of x86_64 after an upgrade since the updater would be i386. I've marked that patch obsolete.

Comment 4

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

Arguments are definitely passed back to the callback app... both need them. :(

I wonder which would be more ugly... recreating the selection logic and sharing it between both the app and the updater so they both know which to launch or reading the command line via env vars or a file? If the selection logic were implemented would nsIProcess also be able to use it and thereby allow the tests for nsIProcess and rely on nsIProcess to be re-enabled?

Comment 5

[Josh Aas]

Attachment: approach #2: per-OS plus fallback v1.0

This should work for the updater. It specifies a preferred architecture per-OS but if that doesn't work it lets the OS select a fallback (just based on the binary choices, not Info.plist, but as a fallback option it plays out pretty nicely).

Comment 6

[Josh Aas]

Comment on attachment 479323 [details] [diff] [review]
approach #2: per-OS plus fallback v1.0

Requesting review but note that I haven't tested that this does anything other than compile on 10.6.

Comment 7

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

Josh, I've asked you and others several times what is going to be done to get the tests that were disabled in Bug 571367 (Bug 599477 filed as a followup for these tests being disabled) but I haven't as of yet to receive an answer (I once again brought them up in comment #4 in this bug). Before I take any time to review this I need an answer regarding what the plan is to get these tests re-enabled.

Comment 8

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

Comment on attachment 479323 [details] [diff] [review]
approach #2: per-OS plus fallback v1.0

minusing until comment #7 is addressed or at the very least a plan is provided to address it.

Comment 9

[Josh Aas]

I don't know what to do about the tests that were disabled in bug 571367 yet, just that we need to do something about it. That is my next task.

Comment 10

[Josh Aas]

Comment on attachment 479323 [details] [diff] [review]
approach #2: per-OS plus fallback v1.0

This patch has a problem - the argv argument to posix_spawnp should be a NULL-terminated array. This is done rather than pass an argc parameter.

Comment 11

[Josh Aas]

Attachment: approach #2: per-OS plus fallback v1.1

Fixes argument count bug.

Comment 12

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

(In reply to comment #9)
> I don't know what to do about the tests that were disabled in bug 571367 yet,
> just that we need to do something about it. That is my next task.
Thanks!

Though I will definitely look over the final patch for this bug it will probably be better to get someone that is more familiar with Mac and universal binaries than I am to review this. Perhaps the person that reviews the patch in bug 600411 since the code appears to be similar.

Comment 13

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

My understanding is that this needs to block the next minor updates before 2.0, because otherwise those builds will crash on the 2.0 update.

Comment 14

[LegNeato]

(In reply to comment #2)
> We want to launch the bundle from the updater so that we don't have to know the
> architecture to launch from within the updater. That is hard to know,
> especially if you want this to work for more than just Firefox. We don't want
> to duplicate the selection logic based on the OS, available binaries, and the
> contents of the app's Info.plist which specifies compatibility.
> 
> The problem with launching the bundle rather than the binary is that, believe
> it or not, we can't pass command line arguments if we launch the bundle (I have
> yet to find a way, anyway, that works on 10.5). So if we don't need to pass
> command line arguments to the application from the updater, then this is easy.
> If we do, I don't know what to do. Rob - do you know if the updater application
> is ever invoked with arguments specified to be passed on to the callback app?
> Do we have to support that, at least on Mac OS X?

What about "open [/path/to/bundle] --args [program args]"?

Comment 15

[LegNeato]

Also, to my untrained eye it looks like LSOpenApplication takes params:


Parameters

inAppParams

    A LSApplicationParameters structure specifying the application to launch and its launch parameters. This parameter cannot be NULL.

(http://developer.apple.com/library/mac/#documentation/Carbon/Reference/LaunchServicesReference/Reference/reference.html)

Comment 16

[Josh Aas]

(In reply to comment #14)

> What about "open [/path/to/bundle] --args [program args]"?

Good to know - I didn't know "open" had an "--args" option!

Comment 17

[Josh Aas]

(In reply to comment #15)
> Also, to my untrained eye it looks like LSOpenApplication takes params:

I saw that but it's not clear that that functionality is actually implemented by Mac OS X. The docs say it isn't in 10.4, where the API was introduced, but they don't mention 10.5. I've been meaning to write a test app and see if it actually does work. In any case, nice detective work :)

Comment 18

[LegNeato]

From http://developer.euro.apple.com/library/mac/#releasenotes/Carbon/RN-LaunchServices/index.html:

"The LSApplicationParameters structure introduced in Mac OS X 10.4 contains an argv field designed for passing arguments to an application's main function. This API feature was disabled in the 10.4 release but is implemented in Leopard. If a new application process is created as a result of calling to LSOpenApplication, LSOpenItemsWithRole or LSOpenURLsWithRole , the elements of the argv field are passed to the application's main function."

So I think we are golden, woohoo!

Comment 19

[Josh Aas]

Attachment: approach #2: per-OS plus fallback v1.2

Fix memory leak.

Comment 20

[Josh Aas]

Attachment: approach #3: use LSOpenApplication v1.0

I haven't tested this yet but it should be pretty close.

Comment 21

[Axel Hecht [:Pike]]

open doesn't work for me, fwiw.

host-4-98:~ axelhecht$ open /Applications/Minefield.app --args -P namaroka&
[5] 40876
[4]   Exit 1                  open /Applications/Minefield.app -args -P namaroka
host-4-98:~ axelhecht$ open: unrecognized option `--args'
Usage: open [-e] [-t] [-f] [-W] [-n] [-g] [-h] [-b <bundle identifier>] [-a <application>] [filenames]
Help: Open opens files from a shell.
      By default, opens each file using the default application for that file.  
      If the file is in the form of a URL, the file will be opened as a URL.
Options: 
      -a                Opens with the specified application.
      -b                Opens with the specified application bundle identifier.
      -e                Opens with TextEdit.
      -t                Opens with default text editor.
      -f                Reads input from standard input and opens with TextEdit.
      -W, --wait-apps   Blocks until the used applications are closed (even if they were already running).
      -n, --new         Open a new instance of the application even if one is already running.
      -g, --background  Does not bring the application to the foreground.
      -h, --header      Searches header file locations for headers matching the given filenames, and opens them.

[5]+  Exit 1                  open /Applications/Minefield.app --args -P namaroka

Comment 22

[Josh Aas]

Attachment: approach #2: per-OS plus fallback v1.3

I think we should go with the per-os solution for now and move to LSOpenApplication later. The per-os solution is simpler and will get the job done. I need to do some more testing with LSOpenApplication before we can take a patch.

The updated patch removes a variable-sized stack allocation.

Comment 23

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

The env will need to be passed to the updater from the app and back to the app from the updater otherwise there will be bugs similar to bug 601145.

Comment 24

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

Comment on attachment 480087 [details] [diff] [review]
approach #2: per-OS plus fallback v1.3

Please update the patch per comment #23 and I'll review it after I am finished with bug 600777

Comment 25

[Josh Aas]

Attachment: approach #2: per-OS plus fallback v1.4

Pass along environment to the callback application.

Comment 26

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

Comment on attachment 480275 [details] [diff] [review]
approach #2: per-OS plus fallback v1.4

>diff --git a/toolkit/mozapps/update/updater/launchchild_osx.mm b/toolkit/mozapps/update/updater/launchchild_osx.mm
>--- a/toolkit/mozapps/update/updater/launchchild_osx.mm
>+++ b/toolkit/mozapps/update/updater/launchchild_osx.mm
>@@ -32,64 +32,108 @@
>...
> void LaunchChild(int argc, char **argv)
> {
>-  int i;
>-  NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init];
>-  NSTask *child = [[[NSTask alloc] init] autorelease];
>-  NSMutableArray *args = [[[NSMutableArray alloc] init] autorelease];
>+  // We prefer CPU_TYPE_X86_64 on 10.6 and CPU_TYPE_X86 on 10.5,
>+  // if that isn't possible we let the OS pick the next best 
>+  // thing (CPU_TYPE_ANY).
>+  cpu_type_t cpu_types[CPU_ATTR_COUNT];
>+  if (OnSnowLeopardOrLater()) {
>+    cpu_types[0] = CPU_TYPE_X86_64;
>+  }
>+  else {
>+    cpu_types[0] = CPU_TYPE_X86;
>+  }
>+  cpu_types[1] = CPU_TYPE_ANY;
Please also handle ppc - though we are moving away from ppc it has been accounted for in the other patches and as soon as those other components no longer handle ppc then the ppc case can be removed from app update.

with that r=me

Comment 27

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

Josh pointed out on irc that CPU_TYPE_ANY will handle ppc so all should be fine. Would be nice if the other patches were consistent with using CPU_TYPE_ANY for ppc but iirc Josh said that caused a compilation error for him.

Comment 28

[Josh Aas]

The other patches are consistent, but they do something different. They are preferring the current arch. The updater code is not preferring the current arch, it is preffering architectures based on OS since the updater may have changed the preferred arch.

Comment 29

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

Makes sense and thanks.

Comment 30

[Josh Aas]

pushed to mozilla-central

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

Comment 31

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

Josh, can you put together the 1.9.1 and 1.9.2 patches for this?

Comment 32

[Josh Aas]

Do we really need a 1.9.1 patch? We'd only need a patch for things that can do updates that add x86_64 as an architecture. Can someone update to 2.0.0 directly from 1.9.1, without going to 1.9.2 first?

Comment 33

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

Yes. For example, 1.9.0 users are being offered updates to 1.9.2.

Comment 34

[Josh Aas]

I recommend pushing this back to blocking1.9.2.14+. The backport is not trivial because "posix_spawnp" and friends don't exist on Mac OS X 10.4 and I'd just be starting to write all-new code today. Better to wait a cycle and not rush this since getting it wrong could be a major problem.

Comment 35

[Josh Aas]

Attachment: [1.9.2] fix v1.0

This should do the trick but I haven't tested it yet. It does compile. This same approach should work for the 1.9.1 branch.

Comment 36

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

I'm finishing up backporting a few tests before I review this.

Comment 37

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

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

I just landed the tests, etc. on mozilla-1.9.2 and I'll try to get to this review this week

Comment 38

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

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

You need to verify if arch -i386 overrides setting as ppc via sysctlbyname on 10.5. If it does I think you can just specify ppc and then i386.

I've landed a bunch of new tests for the updater on 1.9.2... please verify they all pass on 10.5, etc.
make -C objdir/toolkit/mozapps/update/test xpcshell-tests

Minusing until the above is done. I'd like a second set of eyes on this so asking bsmedberg for review as well.

Comment 39

[Josh Aas]

Attachment: [1.9.2] fix v1.1

Add '-ppc' to arch command line when running in PPC mode.

All tests pass on an Intel machine running Mac OS X 10.5.8.

Comment 40

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

Comment on attachment 498905 [details] [diff] [review]
[1.9.2] fix v1.1

Since this will be landing on 1.9.2 I'd like a second set of eyes on this.

Did you check if using arch overrode sysctlbyname and if so, did it?

Comment 41

[Josh Aas]

(In reply to comment #40)

> Did you check if using arch overrode sysctlbyname and if so, did it?

I did not check that, I just played it safe by leaving sysctlbyname and adding PPC to the arch command line.

Comment 42

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

Comment on attachment 498905 [details] [diff] [review]
[1.9.2] fix v1.1

We'll likely need this in the upcoming security release

Comment 43

[Daniel Veditz [:dveditz]]

Comment on attachment 498905 [details] [diff] [review]
[1.9.2] fix v1.1

Approved for 1.9.2.14 and 1.9.1.17, a=dveditz for release-drivers

Comment 44

[Daniel Veditz [:dveditz]]

What happens to 10.4 users who get a minor update to the next 3.5.x/3.6.x after this? Will the right thing happen for PPC users as well as i386 users?

Comment 45

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

(In reply to comment #44)
> What happens to 10.4 users who get a minor update to the next 3.5.x/3.6.x after
> this?
The code is for all practical purposes the same for 10.4 users as it was before this patch.

> Will the right thing happen for PPC users as well as i386 users?
Yes. Also, bug 552924 landed so we can distinguish between i386 and ppc users so we only offer an update to systems running i386.

Comment 46

[Josh Aas]

pushed to mozilla-1.9.2

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

Comment 47

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

Al, can you verify updates using 3.6 nightly builds on Mac OS X 10.5.x specifically regarding relaunching Firefox after the update? Would be a good thing to verify that an update to Firefox 4.0 relaunches Firefox correctly as well. I'll put together a test update snippet for doing this and post it in the bug.

Comment 48

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

Attachment: Mac update snippet to update to 4.0b10pre

Comment 49

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

Al, with a Mac OS X 10.5 system you add a new string pref named app.update.url.override with a value of https://bug600362.bugzilla.mozilla.org/attachment.cgi?id=503528 to test updating to Firefox 4.0

Comment 50

[Josh Aas]

pushed to mozilla-1.9.1

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

Comment 51

[Al Billings [:abillings - ex-MoCo]]

I downloaded the nightly 1.9.2 build from 1/16 (Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2.14pre) Gecko/20110116 Namoroka/3.6.14pre) and did a normal update. It restarted fine and says it is today's 1.9.2 build (Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2.14pre) Gecko/20110118 Namoroka/3.6.14pre).

I also used last night's 1.9.2.14pre build and the override that Rob suggested to his update snippet. Check for updates showed the nightly Firefox 4 beta 10pre build as available. I downloaded it and Firefox relaunched with the 1/13 4b10pre build specified (Mozilla/5.0 (Macintosh; Intel Mac OS X 10.5; rv:2.0b10pre) Gecko/20110113 Firefox/4.0b10pre).

I then downloaded the 1.9.1 nightly build from 1/15/2011 (Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.17pre) Gecko/20110115 Shiretoko/3.5.17pre) and did a normal nightly update to last night's build (Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.17pre) Gecko/20110118 Shiretoko/3.5.17pre).

I took last night's 1.9.1 nightly, adding the override to Rob's update snipped and it updated to the 1/13 build (Mozilla/5.0 (Macintosh; Intel Mac OS X 10.5; rv:2.0b10pre) Gecko/20110113 Firefox/4.0b10pre).

In all instances, the browser ran normally after updates.

This was on a OS X 10.5.8 Intel box.