470946 - PostUpdateWin WinPostProcess should launch synchronously

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Mook (songbird dead account)]

Attachment: 1.9.0 branch patch

Currently (until bug 397964 gets in) we still have a chance at running a helper process post-app-update.  Unfortunately, this runs asynchronously so there's an inherent race between trying to do anything in that process, and launching the app.

This is a request to make that blocking instead, so that things can be done there reliably.  Attaching a 1.9.0 branch patch because that's what I have handy at the moment, and not requesting r? because this probably won't land on that branch :)

If this feels like something that would be useful, I can make a mozilla-central patch instead (but that will happen after some sort of confirmation that this would actually have a chance at being accepted).

Comment 1

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

Do you have any reason to believe things aren't done reliably? The process that is launched updates registry keys and updates the uninstall log and I see no reason to make it run synchronously.

Comment 2

[Mook (songbird dead account)]

Sure; we're making our helper delete / rename the executable.  Which, on Windows, can't be done while it's in use :)  (This is Songbird, not Firefox)

Comment 3

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

That definitely was never an intended use for the update post process. I would be more inclined to make it an optional entry in the updater.ini that specifies the process should run synchronously.

Comment 4

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

I am also not sure how bug 397964 comes into play since that process is launched by the app itself. Were you planning on having it kill the process before the rename or something to that affect?

btw: all of the apps have been updated to use the updater.ini and bug 397964 will be fixed after the the next Seamonkey, Sunbird, and Thunderbird releases.

Comment 5

[Mook (songbird dead account)]

Oh, heh, I guess I misread that one then, sorry.  This  is the stuff being launched from the updater, http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/src/updater/updater.cpp?rev=3fdd6aaa25f1&mark=1109#1057

Comment 6

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

I believe that an optional entry in the updater.ini that specifies the process should run synchronously should work for you and I would be fine with such and addition.

Comment 7

[Mook (songbird dead account)]

Yep, and I'll try to come up with a patch for mozilla-central :)

Comment 8

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

btw: another alternative you could likely use is provide updates with a file remove for the original file and a file add for the new file.

Comment 9

[Mook (songbird dead account)]

Attachment: read from updater.ini

This is made against mozilla-central and uses an ExeAsync in updater.ini instead.

(I have no idea why, but we went with some strange setup where the same update is fed to all partner distributions, so while making those changes happen in the mar would make sense, it doesn't work.)

Comment 10

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

Comment on attachment 354520 [details] [diff] [review]
read from updater.ini

Instead of changing WinLaunchChild use the same approach in bug 459615 where CreateProcessW is used instead of WinLaunchChild. I can land the patch in bug 459615 on mozilla-central next week if it would simplify things for this patch.

Comment 11

[Mook (songbird dead account)]

Attachment: assume bug 459615 has been applied

yeah, that does make it easier (at least, means I have to worry about fewer things to change).  And by the magic of a DVCS, I can apply it locally and still have diffs that make sense!

So, please do land bug 459615 some time soon :)

(There were conflicts in nsWindowsRestart.cpp applying attachment 346692 [details] [diff] [review], but that's not relevant here)

Comment 12

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

Comment on attachment 354723 [details] [diff] [review]
assume bug 459615 has been applied

Please make the default false and the value in updater.ini should be optional with it defaulting to false.

Comment 13

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

Comment on attachment 354723 [details] [diff] [review]
assume bug 459615 has been applied

bah... I read this wrong. I'll take a longer look at it a little later

Comment 14

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

Comment on attachment 354723 [details] [diff] [review]
assume bug 459615 has been applied

>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
>@@ -1099,16 +1105,20 @@
>   int len = wcslen(exearg) + wcslen(dummyArg);
>   WCHAR *cmdline = (WCHAR *) malloc((len + 1) * sizeof(WCHAR));
>   if (!cmdline)
>     return;
> 
>   wcscpy(cmdline, dummyArg);
>   wcscat(cmdline, exearg);
> 
>+  if (!_wcsnicmp(exeasync, L"false", 6) || !_wcsnicmp(exeasync, L"0", 2)) {
>+    async = PR_FALSE;
>+  }
nit: the file's prevailing style doesn't use curly braces unless necessary

>   // We want to launch the post update helper app to update the Windows
>   // registry even if there is a failure with removing the uninstall.update
>   // file or copying the update.log file.
>   NS_tremove(dlogFile);
>   CopyFile(slogFile, dlogFile, FALSE);
> 
>   STARTUPINFOW si = {sizeof(si), 0};
>   PROCESS_INFORMATION pi = {0};
>@@ -1121,18 +1131,22 @@
>                            0,     // No special process creation flags
>                            NULL,  // inherit my environment
>                            NULL,  // use my current directory
>                            &si,
>                            &pi);
>   free(cmdline);
> 
>   if (ok) {
>-    CloseHandle(pi.hProcess);
>-    CloseHandle(pi.hThread);
>+    if (async) {
>+      CloseHandle(pi.hProcess);
>+      CloseHandle(pi.hThread);
>+    } else {
>+      WaitForSingleObject(pi.hProcess, INFINITE);
I believe that Windows will close open handles when the updater process exits but I'd prefer it if CloseHandle was called when the wait succeeds.

Comment 15

[Mook (songbird dead account)]

Attachment: address review comment 14

Comment 16

[Mook (songbird dead account)]

Thanks for the review!

Double checked on the try server for the heck of it, the kreeger builds on http://tinderbox.mozilla.org/showbuilds.cgi?tree=MozillaTry&maxdate=1232140680&mindate=1232128800&legend=0&norules=1

Comment 17

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

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