176791 - Support multiple installations of the same app

Status: VERIFIED FIXED

(Core Graveyard :: Installer: GRE, defect)

Description

[Curt Patrick (gone)]

Since we provide for only a single key in the Windows registry for each app that
uses GRE, if the app is installed into two different locations Gre will be aware
of only one of them.  We need to make a unique key for each seperate
installation of each app.

Comment 1

[Chris Nalls]

Marking topembed+ as per topembed triage.

Comment 2

[Curt Patrick (gone)]

Attachment: Patch 1

The logic for the most significant change to the uninstaller is discribed in
http://www.mozilla.org/projects/embedding/GRE_install/win/Uninst_spec.html
in the section titled "Determine whether to physically Uninstal"

Comment 3

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

Comment on attachment 105058 [details] [diff] [review]
Patch 1

r=jbetak

Curt, my apologies for delaying this. I hope I didn't miss anything. 

There is a nits I got busted for once and I felt I should bring it up.

Dan didn't like the fact that there was no upper limit on the while loop here:

http://lxr.mozilla.org/seamonkey/source/xpinstall/wizard/windows/nsinstall/nsin
stall.cpp#968

I decided to cap the interations at 100 and fail if the count needs to go above
that number.

Although it might be a little counterintuitive, I'd suggest the same, That is,
unless you can sneak it by somehow :-)

+	 while(!bDone)
+	 {
+	   if(WinRegNameExists(hRootKey, szKey, szName))
+	   {
+	     GetWinReg(hRootKey, szKey, szName, szBuf, sizeof(szBuf));
+	     if(lstrcmpi(szBuf, sgProduct.szAppPath) == 0)
+	       bDone = TRUE;
+	     else
+	       wsprintf(szName, "%s%02d", szShortName, dwNameIndex++);
+	   }
+	   else
+	     bDone = TRUE;
+	 }

The other thing is the renumeration of PathToExe entries. Having dwIndex and
dwIndex2 looks really confusing to me. 

I'd turn the while loop into a for loop. 

wsprintf(szNextName, "PathToExe%02d", dwIndex--);
for(int i=dwIndex; i <100 && WinRegNameExists(hkRootKey, szKey, szNextName);
i++)

The cap on the loop index is optional, as mentioned earlier.

+	 dwIndex2 = dwIndex--;
+	 wsprintf(szNextName, "PathToExe%02d", dwIndex2++);
+	 while(WinRegNameExists(hkRootKey, szKey, szNextName))
+	 {
+	   GetWinReg(hkRootKey, szKey, szNextName, szBuf, MAX_BUF);
+	   SetWinReg(hkRootKey, szKey, szName, REG_SZ, szBuf, lstrlen(szBuf));
+	   lstrcpy(szName, szNextName);
+	   wsprintf(szNextName, "PathToExe%02d", dwIndex2++);
+	 }	  
+	 DeleteWinRegValue(hkRootKey, szKey, szName);

Comment 4

[Curt Patrick (gone)]

Attachment: Patch 2

Addresses juraj's issues.  Rather than using a for loop for the second issue, I
created a function for replacing a PathToExeXX registry entry with does the
re-enumeration.  I think this came out much cleaner.  Juraj, these changes were
significant enough that I think you should review this again before I pass it
along for sr=.

Comment 5

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

Comment on attachment 105536 [details] [diff] [review]
Patch 2

Curt I'm printing it out to have another look at home, but it looks good so far
:-)

Comment 6

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

Comment on attachment 105536 [details] [diff] [review]
Patch 2

r=jbetak

Comment 7

[Daniel Veditz [:dveditz]]

Comment on attachment 105536 [details] [diff] [review]
Patch 2

>+  DWORD   dwUpperLimit = 100;

This should be a const, and start the name with 'k' so this
doesn't look like a value read from somewhere. Also in the uninstaller.

>-  // The only entries left in the AppList are orphaned entries.  Remove them.
>+  wsprintf(szName, "PathToExe%02d", dwIndex);
>+  while(WinRegNameExists(hkRootKey, szKey, szName)) // Count the entries which can be verified by 

After the loop above it looks like dwIndex is pointing at the gap where
a WinRegName *didn't* exist. Are you trying to clear out any that might be
left? but this will fail at the next gap, and also you're counting these.

I'm confused here.

>+  char szBuf[MAX_BUF];
>+  char szName[MAX_BUF];
>+  char szNextName[MAX_BUF];

How big do you expect these things to be? 4K is insane (cue broken record).

>+BOOL WinRegNameExists(HKEY hkRootKey, LPSTR szKey, LPSTR szName)
>+
>+  ZeroMemory(szBuf, sizeof(szBuf));

ZeroMemory() on a 4K buf (why that big?) is slow overkill. From the way
you use it below szBuf[0] = 0 would do.

sr=dveditz with those changes

Comment 8

[Curt Patrick (gone)]

Checked in.

Comment 9

[Charles Rosendahl]

This is verified - most likely I will need to open some new defects on the
uninstallation aspect of this fix.

Apps are now added in the applist key of the gre with a numeric suffix appended
to the executable path key.