167606 - Allow multiple MRE installer instances

Status: VERIFIED FIXED

(Core Graveyard :: Installer: GRE, defect, P1)

Description

[[e-mail] (away - not reading bugmail)]

Per offline discussion with Curt, we should be able to handle multiple installer
instances. I'm going to attach a preliminary patch, which has been bitrotting
for about two weeks on my PC.

I need to merge it with Curt's latest patches, polish it up a bit and get it
reviewed.

Comment 1

[[e-mail] (away - not reading bugmail)]

Attachment: functional patch - includes multiple instances, GRE app progress bar update and some of Curt's changes for bug 163689, needs a merge and some clean up work

Comment 2

[Chak Nanga]

Hi Juraj : Can you elaborate a bit more on the need for multiple MRE
instances...thanks

Comment 3

[[e-mail] (away - not reading bugmail)]

Chak, I believe that in one conceivable scenario the user would invoke two MRE-
based app installations. Each installer will attempt to spawn an independent 
MRE installation instance. They'll be competing for the first MRE instance and 
only one app will get registered properly. Also, they could require different 
versions of MRE, in which case one app might not even run.

Comment 4

[Jimmy Lee]

Reassigning QA Contact.

Comment 5

[[e-mail] (away - not reading bugmail)]

Attachment: cleaned-up patch v1

Curt's patches have landed, let's get this moving

Comment 6

[[e-mail] (away - not reading bugmail)]

Attachment: screenshot - unique temp directory for each instance

Comment 7

[[e-mail] (away - not reading bugmail)]

Attachment: screenshot - multiple installer instances

Comment 8

[[e-mail] (away - not reading bugmail)]

Attachment: patch v1

Curt, same applies here - this should be r=/sr= worthy. I'd appreciate if you
had a minute to look at this.

Comment 9

[Curt Patrick (gone)]

r=curt

As we talked about, there might be some concerns with extremely edge cases where
the the same installer gets launched twice--say, for example--two different apps
that both use mre are being installed at the same time--but I really can't think
of any realistic scnenerios where that would be likely either to happen nor to
cause problems.  That being the case, I'm for this simple solution until we get
hard feedback that we need to get fancier.  If we do, your suggestion to put
something in the config.ini to allow specifiying the instance name makes a lot
of sense.  We can fall back to that plan if it ever becomes evident that there
is a need for something more sophisticated.

Comment 10

[[e-mail] (away - not reading bugmail)]

Curt, thanks for looking at this. I agree with your comments; I can't see any
obvious issues with change either.

cc'ing Dan with a sr= request

Comment 11

[Curt Patrick (gone)]

Regarding Chak's question in comment #2, the fundamental reason for this bug is
that mre requires that app installers install mre by launching the mre
installer.  There will certainly be cases where the app installer is a mozilla
based installer.  Presently, if the app's mozilla based installer tried to
launch the mre installer the mre installer would see an instance of the mozilla
installer running and do a noop.  Not very productive!  Juraj's patch will fix that.

Juraj, I just got to thinking:  Before this patch gets checked in I'd like you
to do some testing with the mfcembed installer and the mre installer running
together.  You don't necassarily have to go to the extent of actually packaging
up the mre installer in the mfcembed installer (although that is on our list:  I
just created bug #168361 to track that) but you could do a little testing
launching them both in gui and then clicking the install buttons simultaneously.
 I think this would simulate a good test of the worse case scenario.

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 98923 [details] [diff] [review]
patch v1

>+char      WIZ_TEMP_DIR[MAX_BUF] = "ns_temp";

WIZ_TEMP_DIR looks like a #define name following prevailing style,
you're just asking for trouble using it. An initial 'g' is often
used to indicate global storage in Mozilla code, for example
gWizTempDir.

a 4k buffer to hold ns_tempXXXX is insane. Surely your memory
mgmt skills aren't so sloppy you need a 4000 byte safety zone.

>-  /* Allow only one instance of nsinstall to run.
>-   * Detect a previous instance of nsinstall, bring it to the 
>-   * foreground, and quit current instance */
>-  if(FindWindow("NSExtracting", "Extracting...") != NULL)
>-    return(1);

Running two copies of the *same* installer at the same time
is still bad. We need to add an equivalent check in setup,
perhaps protecting against using the same destination directory
or some key value name in the config.it

>+  /*  Allow multiple installer instances with the 
>+      provision that each instance is guaranteed 
>+      to have its own unique setup directory 

If we do this then we'll have to do a better job making
sure ns_temp goes away at the end of the install. Currently
it sticks around, and pretty soon people will be overrun
by ns_temp directories. Find out from ssu why he didn't just
pass TRUE to DirectoryRemove(), that should handle the problem.

>+  if(FindWindow("NSExtracting", "Extracting...") != NULL ||
>+     FindWindow(CLASS_NAME_SETUP_DLG, NULL) != NULL) 

Why the FindWindow checks? you should create a fresh directory
regardless. Trying to re-use ns_temp this way doesn't solve
the dir-growth problem and isn't that safe.

>+    char szTempPath[MAX_BUF];

Another 4k buffer.

>+      if (0 < instanceNumber) 

This is more readable as "if (instanceNumber > 0)"

>+      instanceNumber++;
>+    } 
>+    while (FileExists(szTempPath) != FALSE);

Any max cap before you give up?

>RCS file: /cvsroot/mozilla/xpinstall/wizard/windows/setup/extra.c,v

>+  /*  check for multiple installer instances; each 
>+      instance requires a unique temp directory
>+   */

Could you walk me through here. Are "installer instances"
referring to muliple nsinstall or multiple setup.exe?

>+  if(FindWindow(CLASS_NAME_SETUP_DLG, NULL) != NULL &&
>+     strstr(szSetupDir, szTempDir) != NULL)

You seem to mean strncmp() rather than strstr(). Would you really
be satisfied to find szTempDir starting at the 5th character?

Why the window check? You may not find another setup.exe, but
that doesn't mean another copy of nsinstall isn't currently
using ns_temp. It would be safer if you're not already in a
temp directory to do the directory name clash avoidance you
do in nsinstall.

Comment 13

[[e-mail] (away - not reading bugmail)]

Dan,

thanks for taking time to look over this. I appreciate your comments and will
try to come up with a timely follow-up.

Just a very brief comment from my side: yes I do realize that this approach is
potentially inherently more risky than the other option I was considering: i.e.
we could leave the code the way it is and allow the instance name to be
configurable. I haven't looked at it in much detail, but we could keep the
current instance name for Mozilla installers and use a different one for MRE
installers.

I will address your comments pint by point tomorrow.

Comment 14

[[e-mail] (away - not reading bugmail)]

As discussed offline, I made an attempt to make the window class (aka instance
name) a command-line parameter. Halfway into the implementation I realized that
the window class name is also being used in the dialog resource file:

http://lxr.mozilla.org/seamonkey/source/xpinstall/wizard/windows/setuprsc/setuprsc.rc#199

I'd suggest that we allow multiple installer instances when a specific
commanand-line parameter ("mi" for "multiple instances") is used. This would
help prevent the kind of user errors that have made this roadblock necessary and
open the gates enough to resolve the issues we are facing when calling the GRE
installer from other xpinstall-based installations (MFCembed, Mozilla).

Comment 15

[[e-mail] (away - not reading bugmail)]

Attachment: revised patch - this could be the best compromise yet

Comment 16

[[e-mail] (away - not reading bugmail)]

I have implemented several of Dan's comments. Some of the omissions were caused
on my incomplete understanding of the xpinstall functionality and architecture.
Others (like the use of 4096B buffers) resulted from following a precedent set
in the file.

I'd be more than happy to continue improving the patch quality, but having
thought about it for almost a month, I'm quite confident that however we choose
to resolve this issue, it's going to be a compromise.

I've implemented the idea outlined in comment 14. Sean has confirmed that we
don't want users to start multiple xpinstall sessions and it would take quite a
bit of work to make that safe enough. Controlling this via a command line switch
should guarantee that the average user will never run into that situation. The
"-mi" option should be used when an external xpinstall wishes to call another
xpinstall to perform some tasks in depends on (say install GRE). This puts the
burden of preventing target directory and resource clashes on the external
xpinstall.

I believe that the biggest unresolved issue is the ns_temp directory clean-up.
In the tests I've performed ns_temp directories get routinely cleaned up.
However Sean confirmed that a client install might keep a temp directory around
to transmit a log file to a remote server. The xpinstall process typically
terminates before the transmission is over and leaving this log file behind.
Again, I was not able to see this in my tests, but I tried to minimize the
"growth rate" of ns_temp directories by reusing existing ones. We are already
doing this - currently xpinstall always defaults to (and reuses) ns_temp,
whether it exists or not. 

 

Comment 17

[Curt Patrick (gone)]

Comment on attachment 101795 [details] [diff] [review]
revised patch - this could be the best compromise yet

I agree that this is the right approach.  

The accumulation of temp folders does not seem like a big deal to me.  Looks
like it could only happen when the /mi option is used, which won't be often for
any user.  And the size of temp folders which might fail to be cleaned up is
VERY small.

One little nit.  With the cryptic command-line options to date a leading 'm'
has meant "mode".  Maybe this option should be "/mmi" for "mode
multi-instance".  Or just make it less cryptic.

r=curt

Comment 18

[[e-mail] (away - not reading bugmail)]

Attachment: changed command-line option "mi" to "mmi" as suggested by Curt

Comment 19

[[e-mail] (away - not reading bugmail)]

Comment on attachment 101869 [details] [diff] [review]
changed command-line option "mi" to "mmi" as suggested by Curt

carrying over r=curt

Curt, thanks for reviewing this (again) and for the cathing the inconsistency.
Let's hope Dan will give an sr= this time. I'll send him a direct email him
tomorrow.

Comment 20

[[e-mail] (away - not reading bugmail)]

we really need this ASAP, setting prio to P1

Comment 21

[Daniel Veditz [:dveditz]]

Comment on attachment 101869 [details] [diff] [review]
changed command-line option "mi" to "mmi" as suggested by Curt

>+char      gszWizTempDir[MAX_BUF] = "ns_temp";

Please just change this buffer size to 20 or so and make me happy. This isn't a
path like the other places using MAX_BUF.

>+BOOL      gbAllowMultipleInstalls;

You don't set this to false anywhere I can see, yet you seem to expect that
value in places. Please initialize this.

>RCS file: /cvsroot/mozilla/xpinstall/wizard/windows/setup/setup.c,v
>+BOOL            gbAllowMultipleInstalls;

Needs to be initialized here, too.

Comment 22

[[e-mail] (away - not reading bugmail)]

Attachment: smaller buffer for gszWizTempDir, proper initialization of gbAllowMultipleInstalls

Comment 23

[[e-mail] (away - not reading bugmail)]

Comment on attachment 102288 [details] [diff] [review]
smaller buffer for gszWizTempDir, proper initialization of gbAllowMultipleInstalls

carrying over r=curt

Comment 24

[Daniel Veditz [:dveditz]]

Comment on attachment 102288 [details] [diff] [review]
smaller buffer for gszWizTempDir, proper initialization of gbAllowMultipleInstalls

sr=dveditz

Comment 25

[[e-mail] (away - not reading bugmail)]

closing this at last - thanks everyone!

Comment 26

[Curt Patrick (gone)]

Note to QA:  This can be tested directly from the command-line, but the real
test will be bug #168361, when we use the MfcEmbed installer to launch the GRE
installer using this command-line option.

Comment 27

[Charles Rosendahl]

I've currently verified that running the GRE Installer from three different
instances simultaneously (one GUI based, two mode silent) successfully installs
the GRE without any race conditions tweaking the registry or directory structure.

The only item out of the ordinary here was that it generated duplicate entries
in the install log files, but this did not seem to affect the functionality of
the uninstaller.

I will wait until I build some test apps or mfcembed integrates the GRE
Installer before actually marking this bug verified.

Comment 28

[Charles Rosendahl]

As far as I can tell, this bug is fixed.  Should I run into any strange race
conditions later, I will file a new bug against it.  For now, the installer,
when in /mmi mode, seems to handle several running instances of the GRE and
mfcembed installers without any problems.