600777 - nsUpdateDriver needs to launch updater using the right architecture

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

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

Same issue as bug 600411 / bug 600408 / bug 600362. This is the code that launches the updater when you start Firefox with a pending update.

Comment 1

[Josh Aas]

Attachment: fix v0.8

Saving my work, haven't tested this but it should be most of the work for making it the updater application's responsibility to chdir to the correct working directory based on the command line. This is necessary if we're going to use "posix_spawnp" to launch the updater because processes created with "posix_spawnp" inherit the working directory of the caller and there is no other way to set it. Launch strategies that use "execv" can change the working directory before the "execv" call.

The new updater command line looks like:

Usage: updater update-dir apply-to-dir [wait-pid [callback-working-dir callback-path args...]]

Comment 2

[Josh Aas]

Attachment: fix v1.0

Haven't tested this yet, but it compiles and I think it is pretty close to what we want. Need to look carefully for problems this might cause with heavily special-cased platforms like WINCE.

Comment 3

[(not currently active) Ted Mielczarek]

WINCE is no longer actively supported, so it's probably okay if you break it.

Comment 4

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

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

You will need to update runUpdate in head_update.js.in as well
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/test/unit/head_update.js.in#157

that should be changed to var args = [updatesDirPath, cwdPath];

I would like the following tests reenabled for Mac OS X if possible
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/test/unit/test_0110_general.js#90
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/test/unit/test_0111_general.js#108
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/test/unit/test_0112_general.js#90


>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
>@@ -1461,81 +1461,79 @@ UpdateThreadFunc(void *param)
> 
>   LOG(("calling QuitProgressUI\n"));
>   QuitProgressUI();
> }
> 
> int NS_main(int argc, NS_tchar **argv)
> {
>   InitProgressUI(&argc, &argv);
>+
>   // The updater command line consists of the directory path containing the
>-  // updater.mar file to process followed by the PID of the calling process.
>-  // The updater will wait on the parent process to exit if the PID is non-
>-  // zero.  This is leveraged on platforms such as Windows where it is
>-  // necessary for the parent process to exit before its executable image may
>-  // be altered.
>-
>-#ifndef WINCE
>-  if (argc < 2) {
>-    fprintf(stderr, "Usage: updater <dir-path> [parent-pid [working-dir callback args...]]\n");
>+  // updater.mar file to process followed by the directory to apply the update
>+  // from. Next is an option argument of the PID, if this is non-zero we'll
>+  // wait for the process to exit before applying the update. Last is an
>+  // optional callback binary path plus arguments.
nit: please change this as follows:
To process an update the updater command line must at a minimum have the directory path containing the updater.mar file to process as the first argument and the directory to apply the update to as the second argument. When the updater is launched by another process the PID of the parent process should be provided in the optional third argument and the updater will wait on the parent process to exit if the value is non-zero and the process is present. This is necessary due to not being able to update files that are in use on Windows. The optional fourth argument is the callback's working directory and the optional fifth argument is the callback path. The callback is the application to launch after  updating and it will be launched when these arguments are provided whether the update was successful or not. All remaining arguments are optional and are passed to the callback when it is launched.

>+  if (argc < 3) {
>+    fprintf(stderr, "Usage: updater update-dir apply-to-dir [wait-pid [callback-working-dir callback-path args...]]\n");
>     return 1;
>   }
>-#else
>-  if (argc < 4) {
>-    fprintf(stderr, "Usage: updater <dir-path> parent-pid <working-dir> [callback args...]]\n");
>+
>+  // The directory containing the update information.
>+  gSourcePath = argv[1];
please put this back where it was previously after the pid block

>+
>+  // Change current directory to the directory where we need to apply the update.
>+  if (NS_tchdir(argv[2]) != 0) {
>     return 1;
>   }
Please #ifndef WINCE the above block... we don't include the WinCE shunt's version of _wchdir and instead use gDestDir and the full paths on WinCE.

>diff --git a/toolkit/xre/nsUpdateDriver.cpp b/toolkit/xre/nsUpdateDriver.cpp
>--- a/toolkit/xre/nsUpdateDriver.cpp
>+++ b/toolkit/xre/nsUpdateDriver.cpp
>...
>@@ -451,71 +440,58 @@ ApplyUpdate(nsIFile *greDir, nsIFile *up
>   // we pass "0" which is then ignored by the updater.
> #if defined(USE_EXECV)
>   NS_NAMED_LITERAL_CSTRING(pid, "0");
> #else
>   nsCAutoString pid;
>   pid.AppendInt((PRInt32) getpid());
> #endif
> 
>-  int argc = appArgc + 4;
>+  int argc = appArgc + 5; // Initially set to the maximum.
nit: Initially set to the minimum.

r=me with the above addressed

Comment 5

[Josh Aas]

Attachment: fix v1.1

Comment 6

[Josh Aas]

pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/861afa477aba

Comment 8

[Rail Aliiev [:rail]]

Releng uses updater binary from command line to verify if generated updates apply well. In bug 617188 we hit a failure which was caused by updater command line parameter changes introduced in this bug.

In the future, it would be great if you CC [e-mail] for significant updater changes. Thanks in advance!