PostUpdateWin WinPostProcess should launch synchronously

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Toolkit
Application Update
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Mook (songbird dead account), Assigned: Mook (songbird dead account))

Tracking

unspecified
mozilla1.9.2a1
x86
Windows XP
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

10 years ago
Created attachment 354307 [details] [diff] [review]
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).
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.
(Assignee)

Comment 2

10 years ago
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)
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.
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.
(Assignee)

Comment 5

10 years ago
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
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.
(Assignee)

Comment 7

10 years ago
Yep, and I'll try to come up with a patch for mozilla-central :)
Assignee: nobody → mook
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.
(Assignee)

Comment 9

10 years ago
Created attachment 354520 [details] [diff] [review]
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.)
Attachment #354307 - Attachment is obsolete: true
Attachment #354520 - Flags: review?(robert.bugzilla)
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.
Attachment #354520 - Flags: review?(robert.bugzilla) → review-
(Assignee)

Comment 11

10 years ago
Created attachment 354723 [details] [diff] [review]
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)
Attachment #354520 - Attachment is obsolete: true
Attachment #354723 - Flags: review?(robert.bugzilla)
Attachment #354723 - Flags: review?(robert.bugzilla) → review-
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 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
Attachment #354723 - Flags: review- → review?(robert.bugzilla)
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.
Attachment #354723 - Flags: review?(robert.bugzilla) → review-
(Assignee)

Comment 15

10 years ago
Created attachment 356084 [details] [diff] [review]
address review comment 14
Attachment #354723 - Attachment is obsolete: true
Attachment #356084 - Flags: review?(robert.bugzilla)
Attachment #356084 - Flags: review?(robert.bugzilla) → review+
(Assignee)

Comment 16

10 years ago
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
Blocks: 357052
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: checkin-needed
Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/2acc60edfb2f
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.