157211 - META: Windows installer for single version of GRE

Status: VERIFIED FIXED

(Core Graveyard :: Installer: GRE, defect)

Description

[Doug Turner (:dougt)]

There are two directories which get pushed to mozilla/dist after the build
process finishes (acually you have to nmake -f makefile.win mre in
mozilla/embedding/config).  

These directories are 

mre
mre_app_support


We need these directories installed to:

program files/mozilla.org/MRE/



Also, the following windows registry key should be added:  

HKEY_LOCAL_MACHINE\Software\Mozilla\MRE

In addition, identical registry subkeys are created using both the major/minor
versions for each version of MRE installed:

HKEY_LOCAL_MACHINE\Software\Mozilla\MRE\1.0
HKEY_LOCAL_MACHINE\Software\Mozilla\MRE\1.1

Each of these keys will contain the following string values which give
additional information about a specific version of the MRE:
 
Name                Default Value
MreHome             <where the MRE was installed>\mre
MreComponentsDir    <where the MRE was installed>\mre\Components




Chak, do you see any problems with this instructions?

Comment 1

[Chak Nanga]

1. We should suggest the default install location to something like:

C:/program files/mozilla.org/MRE1.1/ (note the version number is included here)

2. We need to decide on the MRE version number which gets written out:

HKEY_LOCAL_MACHINE\Software\Mozilla\MRE\1.1 <---should this be 1.1a?

3. I'm wondering if we should leave out copying the "mre_app_support" dir as
part of the MRE installation? "mre_app_support" is for application writers to
include it as part of *their* MRE based embedding application install. I think
we can do without copying this dir as part of MRE install.

Other than that it looks great...thanks for the write up Doug.

Comment 2

[Doug Turner (:dougt)]

yeah.... so there is only ONE directory that we need to push.  Please ingore my
rambling about mre_app_support

Comment 3

[Chak Nanga]

Hi Syd : Who in your team should we be working with to get the MRE installer
going...thanks

Comment 4

[Syd Logan]

You should be working with me.

Comment 5

[Curt Patrick (gone)]

I'm working on this one.

Comment 6

[Curt Patrick (gone)]

Attachment: Installer Patch 1

This does most of the installer stuff, as spec'ed out in
http://blues.mcom.com/users/curt/publish/MRE_Installer/Inst_spec.html except:

- upgrading (which will require some uninstaller support which will follow
later).  
- Command-line only.  Gui is available but it is kludgy still.
- No forced install

So, briefly, this patch supports:

- Recognizes new command-line options
- Maintains an app list
- Knows not to reinstall files if an app has already installed mre

Comment 7

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

Comment on attachment 96232 [details] [diff] [review]
Installer Patch 1

r=jbetak

The only change I was not clear about is the mod in SmartUpdateJars(). What
does it do?

-	 hrResult = pfnXpiInstall(szArchive, "", 0xFFFF);
+	 hrResult = pfnXpiInstall(szArchive, sgProduct.szRegPath, 0xFFFF);

Comment 8

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

Comment on attachment 96232 [details] [diff] [review]
Installer Patch 1

Oh, and it looks this has most likely been changed inadvertently:

and - * Rights Reserved.
- *
- * Contributor(s): 
+ * Rights Reserved.5
  *	Sean Su <[e-mail]>
  */

Comment 9

[Curt Patrick (gone)]

The second parameter to pfnXpiInstall shows up in install scripts as the
variable Install.arguments.  So the line:

+	 hrResult = pfnXpiInstall(szArchive, sgProduct.szRegPath, 0xFFFF);

tells the install scripts what path into the Windows registry we are using. 
Since the user can now change this as a command-line parameter to the installer
there is no way for the scripts to know where the product is register unless we
tell them.

Comment 10

[Curt Patrick (gone)]

Attachment: Uninstaller Patch 1

This has complete command-line functionality.  UI clean-up, including hiding
the UI when appropriate, will follow.

Comment 11

[Syd Logan]

Comment on attachment 97645 [details] [diff] [review]
Uninstaller Patch 1

+      if(siALHead == NULL)
+      {
+	 if((siALHead = NS_GlobalAlloc(sizeof(struct siAppListStruct))) ==
NULL)
+	   exit(1);
+	 lstrcpy(siALHead->szAppID, szBuf);
+	 siALHead->Next = NULL;
+	 siALLast = siALHead;
+      }
+      else
+      {
+	 if((siALTmp = NS_GlobalAlloc(sizeof(struct siAppListStruct))) == NULL)
+	   exit(1);
+	 lstrcpy(siALTmp->szAppID, szBuf);
+	 siALTmp->Next = NULL;
+	 siALLast->Next = siALTmp;
+	 siALLast = siALTmp;
+      }

could be simplified to:


       if((siALTmp = NS_GlobalAlloc(sizeof(struct siAppListStruct))) == NULL)
	  exit(1);	     
       lstrcpy(siALTmp->szAppID, szBuf);
       siALTmp->Next = NULL;
       siALLast = siALTmp;
       if(siALHead == NULL)
	 siALHead = siALTmp;  // empty list, point the head at it

Comment 12

[Curt Patrick (gone)]

Attachment: Uninstaller Patch 2

While I was implementing Syd's last recommendation I realized that I was not
deallocating the link list which I allocated.  So this patch addresses both of
those issues.

Comment 13

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

Curt,

I was going to comment that Syd's code snippet doesn't appear to set
siALLast->Next properly. And was going to suggest

if(siALHead == NULL) 
  siALHead = siALTmp;  // empty list, point the head at it
else 
  siALLast->Next = siALTmp;

which I just noticed in your patch. However, shouldn't be "siALLast = siALTmp;"
precede this if statement? Seems like the current patch will end up setting
siALLast->Next to NULL. 

Comment 14

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

ergo

+      siALLast = siALTmp;
+
+      if(siALHead == NULL)
+        siALHead = siALTmp; //New list, point the head at it.
+      else
+        siALLast->Next = siALTmp;
+

Comment 15

[Curt Patrick (gone)]

So the good news is that I've actually debugged through the code and it is
working correctly.

I just did a search and replace on "siALLast", changing it to "siALPrev" (I
don't think I need to create a new patch for this change, I hope?).  It occurred
to me that "Last" is ambiguous.  I'm not sure if this was confusing you. 
Otherwise, you'll need to explain further what looks wrong to you in the code. 
I'm pretty sure that siALLast, aka siALPrev, is not being set to NULL anywhere,
though.

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 97645 [details] [diff] [review]
Uninstaller Patch 1

This all could stand some documentation, especially
the cryptic use of new registry keys. I'm sure this is
all on some mozilla.org webpage somewhere, but anyone
reading the code needs more clues locally.

>+HRESULT ProcessAppList()

Why does this return HRESULT? the code seems to always
return 0. The main side-effect seems to be to set the variable
ugUninstall.bUninstallFiles (and possibly cleanup some unused
registry keys) so the name is misleading or obscure. Would it
be clearer to return a boolean and set the bUninstallFiles
global where it's called?

>+  typedef struct siAppListStruct siAppList;
>+  struct siAppListStruct
>+  {
>+    char szAppID[MAX_BUF];
>+    siAppList *Next;
>+  };

arg, why is this all in C? The code could be a lot cleaner
if you could write real classes to hide a lot of the memory
management drudgework. live with what we've got I guess.

>+  RegQueryInfoKey(hkHandle, NULL, NULL, NULL, &dwTotalSubKeys, NULL, NULL, &dwTotalValues, NULL, NULL, NULL, NULL);
>+
>+  for(dwIndex = 0; dwIndex < dwTotalSubKeys; dwIndex++)

It appears ProcessAppItem() removes the AppList key if it
removes the last subkey, should you do the same here if
dwTotalSubKeys is 0? Because otherwise you never enter
the for loop where it might get cleaned up.

>+// If an app item is not installed this removes it from the app list.
>+// Returns TRUE if the app item is installed, FALSE if the app is not installed.
>+HRESULT ProcessAppItem(HKEY hkRootKey, LPSTR szAppID)

Use BOOL not HRESULT if you're returning TRUE/FALSE.

>-  if(diUninstall.bShowDialog == TRUE)
>+  if(ugUninstall.bUninstallFiles == TRUE)
>   {
>-    while(GetMessage(&msg, NULL, 0, 0))
>+    if(diUninstall.bShowDialog == TRUE)
>     {

This would be much clearer as a single if with an &&. With
a nested if I'm looking for the other "else" to drop. (As an
added bonus the diff would look simpler because the subsequent
lines don't have the indentation changes.)

Comment 17

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

Curt, it looks like this will result in "siALLast->Next == NULL". I haven't run
it through the debugger yet, meaning that I'm not 100% sure.

+      siALTmp->Next = NULL;
+
+      if(siALHead == NULL)
+        siALHead = siALTmp; //New list, point the head at it.
+      else
+        siALLast->Next = siALTmp;
+
+      siALLast = siALTmp;

siALTmp->Next = NULL
siALLast = siALTmp
=> siALLast->Next == NULL

Comment 18

[Curt Patrick (gone)]

Attachment: Installer Patch 1 using -w diff'ing option

This is a LOT easier to read.

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 97706 [details] [diff] [review]
Installer Patch 1 using -w diff'ing option

>+        if(RetrieveResults == WIZ_OK)
>+        {
>+          if(sgProduct.bInstallFiles)

There doesn't appear to be an else needed, so writing
this as "if (RetrieveResults == WIZ_OK && sgProduct.bInstallFiles)"
would be easier to understand.

>             /* POST_SMARTUPDATE process file manipulation functions */
>             ProcessFileOpsForAll(T_POST_SMARTUPDATE);
>+  
>+            if(sgProduct.bInstallFiles)
>+            {
>             /* PRE_LAUNCHAPP process file manipulation functions */
>             ProcessFileOpsForAll(T_PRE_LAUNCHAPP);

You still do the POST_SMARTUPDATE commands even if bInstallFiles
is false, but don't do the launch-app or other timing stuff?

>+void NeedToInstallFiles(LPSTR szProdDir)
>+{
>+  // MCP - Eventually we should make this check for a representative file 
>+  //       instead of settling for the existence of the folder.  That is why
>+  //       I left this as a function even though it is essentially a one-liner
>+  //       at the moment.
>+
>+  if(FileExists(szProdDir))
>+    sgProduct.bInstallFiles = FALSE;
>+}

The name doesn't seem to match the void return/side-effect.

>+    /* If the previous path is located in the registry, then we need to check to see if the path from
>+     * the registry plus the Sub Path contains the Program Name file.  If it does, then we have the
>      * correct path, so just use it.

So a previous install dir will override one provided on the command line?
That seems wrong, at least for a non-shared installation.

> void ProcessFileOps(DWORD dwTiming, char *szSectionPrefix)
 ...
>+  // This is the only operation we do if we are not installing files
>   ProcessWinReg(dwTiming, szSectionPrefix);

Oh, I see. Maybe this one should be broken out of ProcessFileOps
and then it alone can be outside the installFiles check. Or is the
order critical? Leaving it where it is causes you to create several
seemingly-extra sets of bInstallFiles checks at several levels, for example:

> void ProcessFileOpsForAll(DWORD dwTiming)

>+void LogISShared(void)

Reminder: you mentioned not being sure if this is used
anymore. If not please remove it.

>-        hrResult = pfnXpiInstall(szArchive, "", 0xFFFF);
>+        hrResult = pfnXpiInstall(szArchive, sgProduct.szRegPath, 0xFFFF);

We will probably someday try to install a zippy that's already
using arguments, or more likely one of our zippies will one day
be hosted on some cgi site with a query path (the plugin finder?)
that we interpret as args. Adding an argument name would help,
that is pass in "regpath=<szRegPath>" instead of just <szRegPath>

Just slightly harder to parse in the script, but then your script
can doublecheck that the contents of Install.arguments really
means what it thinks it means.

Comment 20

[Syd Logan]

Comment on attachment 97696 [details] [diff] [review]
Uninstaller Patch 2

r=syd

Comment 21

[Curt Patrick (gone)]

My comments below are preceeded by MCP:

(From update of attachment 97706 [details] [diff] [review])
>+        if(RetrieveResults == WIZ_OK)
>+        {
>+          if(sgProduct.bInstallFiles)

There doesn't appear to be an else needed, so writing
this as "if (RetrieveResults == WIZ_OK && sgProduct.bInstallFiles)"
would be easier to understand.

MCP: The block for bInstall Files doesn't extend all the way through the 
outer block.  Right in the middle of things is a call to ProcessFileOpsForAll()
which needs to happen whether files are installed or not.  This zeroes in
eventually to some POST_SMARTUPDATE "file" activity which ends up doing the
registry settings we to have happen.  These registry settings must happen
whether we are installing files or not.  So I'm *mostly* commenting everything
out if we're not installing files, except for one very important
POST_SMARTUPDATE "FileOp".

>             /* POST_SMARTUPDATE process file manipulation functions */
>             ProcessFileOpsForAll(T_POST_SMARTUPDATE);
>+  
>+            if(sgProduct.bInstallFiles)
>+            {
>             /* PRE_LAUNCHAPP process file manipulation functions */
>             ProcessFileOpsForAll(T_PRE_LAUNCHAPP);

You still do the POST_SMARTUPDATE commands even if bInstallFiles
is false, but don't do the launch-app or other timing stuff?

MCP:  I think my last comment answers this one, too.

>+void NeedToInstallFiles(LPSTR szProdDir)
>+{
>+  // MCP - Eventually we should make this check for a representative file 
>+  //       instead of settling for the existence of the folder.  That is why
>+  //       I left this as a function even though it is essentially a one-liner
>+  //       at the moment.
>+
>+  if(FileExists(szProdDir))
>+    sgProduct.bInstallFiles = FALSE;
>+}

The name doesn't seem to match the void return/side-effect.

MCP: Changed it to SetInstallFilesVar()

>+    /* If the previous path is located in the registry, then we need to check
to see if the path from
>+     * the registry plus the Sub Path contains the Program Name file.  If it
does, then we have the
>      * correct path, so just use it.

So a previous install dir will override one provided on the command line?
That seems wrong, at least for a non-shared installation.

MCP:  For shared installs we definitely we definitely want the command-line
provided path to a *suggestion*.  This command-line option is being added for
shared installs.  Since not-shared installs haven't been supporting this
command-line option anyway, I confess I wasn't thinking of them starting to use
it now.  Even so, it would kind of confuse the logic which non-shared installs
currently use to decide what path to install to.  If the product has already
been installed the installer assumes we are trying to re-install, probably to
fix a corruption or such, so it wants to install to same place.  The more I
think about this, using this command line option only really makes sense if
another app is installing the product.  If multiple apps are doing that it
really should be a shared install.  So I think that I think we should support
the shared app requirement, i.e. leave it as it is.

> void ProcessFileOps(DWORD dwTiming, char *szSectionPrefix)
 ...
>+  // This is the only operation we do if we are not installing files
>   ProcessWinReg(dwTiming, szSectionPrefix);

Oh, I see. Maybe this one should be broken out of ProcessFileOps
and then it alone can be outside the installFiles check. Or is the
order critical? Leaving it where it is causes you to create several
seemingly-extra sets of bInstallFiles checks at several levels, for example:

> void ProcessFileOpsForAll(DWORD dwTiming)

MCP: Yeah.  The code was circuitous enough that I was (and am) pretty hesitant
to try to pull this one piece of functionality out for fear of breaking the
usual isntall in some hidden sort of way.  This seemed like the lowest risk way
of getting the job done, albeit not real pretty.

>+void LogISShared(void)

Reminder: you mentioned not being sure if this is used
anymore. If not please remove it.

MCP: I refreshed my memory on this.  There is nothing essential about this (I
had thought the uninstaller might need to know this, but that turns out not to
be true) but it is working correctly and it doesn't hurt to have the information
logged.  It might be useful for tech support or someone.  So I'm for leaving it in.

>-        hrResult = pfnXpiInstall(szArchive, "", 0xFFFF);
>+        hrResult = pfnXpiInstall(szArchive, sgProduct.szRegPath, 0xFFFF);

We will probably someday try to install a zippy that's already
using arguments, or more likely one of our zippies will one day
be hosted on some cgi site with a query path (the plugin finder?)
that we interpret as args. Adding an argument name would help,
that is pass in "regpath=<szRegPath>" instead of just <szRegPath>

Just slightly harder to parse in the script, but then your script
can doublecheck that the contents of Install.arguments really
means what it thinks it means.

MCP:  How about we open another bug for this one?  This is working and I think
there are bigger fish to fry in the short term.

Comment 22

[Daniel Veditz [:dveditz]]

>> There doesn't appear to be an else needed, so writing
>> this as "if (RetrieveResults == WIZ_OK && sgProduct.bInstallFiles)"
>> would be easier to understand.
>
> MCP: The block for bInstall Files doesn't extend all the way through the 
> outer block.  Right in the middle of things is a call to ProcessFileOpsForAll()
> which needs to happen whether files are installed or not.

In that case it'd be clearer as

  if (RetrieveResults == WIZ_OK)
  {
    if (!sgProduct.bInstallFiles)
        // must do registry updates regardless
        ProcessFileOpsForAll(T_POST_SMARTUPDATE);
    else 
    {  <what you have w/out the extra bInstallFiles checking>

> MCP:  How about we open another bug for this one?  This is working and I think
> there are bigger fish to fry in the short term.

This *will* eventually break things, I just know it--and I remind you of my
track record predicting which innocuous-looking things come back to haunt us.
Will it really get fixed if you file a bug on it? (it'll get relegated to the
unimportant pile I bet.) Is it really worth the effort to file a bug when you
could just fix it? Of course the fact that you're passing in an arg sort of
implies you're *using* an arg, and you have not presented any install script
changes here. Are these patches incomplete?

If it's not used then sure, file a bug and I'll make sure it's fixed when you
present some install script changes that use it.

sr=dveditz

Comment 23

[Henrik Gemal]

and now we're 100% sure that the uninstaller actually removes *everything*
installed by the installer? also all red.db entries? dont want to have the same
messup as with the mozilla installer.

Comment 24

[Daniel Veditz [:dveditz]]

What "messup"--are there bugs filed?

Comment 25

[Curt Patrick (gone)]

Regarding comment #22, the only install scripts that use this and the other
changes modifications are for the new mre installer which is all checked in
already under  the new directory mozilla\xpinstall\packager\win_mre.  We are
tracking getting this into builds in 

I created bug 166634 to track the parameter issue, and made this bug dependent
upon it.  There are several other clean up issues that I have to address before
the mre installer can be called golden and I've added this to my list.  I think
it needs some more design and it is going to require changes to both the engine
and the mre script so I do think it is worth filing a bug rather than holding up
this checkin.

Comment 26

[Curt Patrick (gone)]

Attachment: Uninstaller Patch 3

This patch addresses Dan's issues in comment #16:

I added comments explaining how the app list is maintained.

I renamed ProcessAppList() to CleanupAppList(), added extensive comments at the
beginning and, instead of changing the bInstallFiles variable hear this
function now returns a count of the installed apps from the app list.  The
calling function uses this info to decide whether to uninstall or not.

For the record, the link list is in C because I stole the code from somewhere
else (in the installer code I think) which was in C there propogating a well
established bad coding practice.

Regarding the following:
--------------------------------
>+  RegQueryInfoKey(hkHandle, NULL, NULL, NULL, &dwTotalSubKeys, NULL, NULL,
&dwTotalValues, NULL, NULL, NULL, NULL);
>+
>+  for(dwIndex = 0; dwIndex < dwTotalSubKeys; dwIndex++)

It appears ProcessAppItem() removes the AppList key if it
removes the last subkey, should you do the same here if
dwTotalSubKeys is 0? Because otherwise you never enter
the for loop where it might get cleaned up.
-----------------------------
If the installer/uninstaller are doing their jobs this state should never
happen.  If it does we've got more problems than this one.  I think we need to
identify the bug and fix it if this every occurs.

ProcessAppItem() now returns BOOL.

Combined the nested if statement with the outer if statement using &&.

Comment 27

[Daniel Veditz [:dveditz]]

Comment on attachment 97827 [details] [diff] [review]
Uninstaller Patch 3

Thanks for the changes! sr=dveditz
Carrying over earlier r=syd

Comment 28

[Henrik Gemal]

Dan: the uninstaller bugs in question are:
bug 113593 : uninstall doesn't remove all files
bug 125877 : uninstall doesn't remove all entries from Windows registry

Comment 29

[Curt Patrick (gone)]

Those bugs are specific to the mozilla installer.  The mre uses the same engine,
but those bugs are installer script related so they would not apply to the mre
installer.  None-the-less, we'll need to watch for and deal with the same kind
of issues.

Comment 30

[Curt Patrick (gone)]

I just checked in both patches.  Since there is still plenty of work to do on
the mre installer, though, and since we've already started attaching bugs to
this on I'm going to leave this bug open and call it a metabug...at least until
Jimmy get a new component for tracking mre installer bugs.

Comment 31

[Lummox Thatiam]

The spec at 
http://blues.mcom.com/users/curt/publish/MRE_Installer/Inst_spec.html 
is not accessible outside the Netscape firewall. I believe it should be 
moved to mozilla.org so that outside parties such as myself can read it. 
Thank you.

Comment 32

[Curt Patrick (gone)]

I completely agree.  I had intended to do so already and let it slip off my radar.

Comment 33

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

Attachment: MRE Installer Spec

Curt, since creating publicity waves seems to be my specialty, I took the
liberty of attaching the spec to the bug as a temporary relief measure :-)

Comment 34

[timeless]

# else install to "c:\Program Files\Mozilla\MRE\<version>"

I think that should be something like %programfiles%\mozilla.org\GRE\{version}

points:
1. Don't assert there's a C: drive (my systems usually don't have one)
2. Program Files can be named anything (and is localized for some locales)
3. Aren't we supposed to use vendorname instead of product as the next child
(i.e. mozilla.org) -- note that this matches Comment 0.
4. Didn't MRE just get renamed?

also, the spec has some typos (it's => its), if you want someone to commit a
version of the spec to mozilla.org, I'm willing to do it...

Comment 35

[Marek Z. Jeziorek]

batch: adding topembed per Gecko2 document
http://rocknroll.mcom.com/users/marek/publish/Gecko/Gecko2Tasks.html

Comment 36

[Lummox Thatiam]

marek: would you mind attaching that document to this bug so that those 
of us outside the Netscape firewall can see it? Thank you.

Comment 37

[Curt Patrick (gone)]

I'm going to track list of bugs we need to address to have a minimally
acceptable Windows installer for a single version of MRE.  We can open another
meta-bug to track upgrade issues, until we've released our first version of MRE
upgrade issues are moot.

Comment 38

[Curt Patrick (gone)]

I was using this bug to track essential GRE bugs while the project was getting
underway, but it has outlived its usefulness now that we are using the topembed
keyword.

Comment 39

[Charles Rosendahl]

verf'd