Build: 2001-04-24-09-trunk(MAC) 1. From http://jimbob/trigger3.html, enter in Trigger URL "f_compareversion_addfile.xpi" and click Trigger button 2. Click OK button from Items to Install dialog to add a file 3. From http://jimbob/trigger3.html, enter in Trigger URL "f_compareversion.xpi" and click Trigger button 4. Click OK button from Items to Install dialog to compareversion 5. The install.log should show the expected results 6. Remove "f_compareversion.txt" from the "Program" folder 7. From http://jimbob/trigger3.html, enter in Trigger URL "f_compareversion.xpi" and click Trigger button 8. Click OK button from Items to Install dialog to compareversion RESULT: The install.log shows the same results from step #5 above. compareVersion is returning values as if the installed file is still present in the installed location. EXPECTED RESULT: Compareversion should fail (return -4) because the file is deleted. Linux and Windows behave this way. EXAMPLE of install.log from Windows where the last installation occurs with f_compareversion.txt deleted: ------------------------------------------------------------------------------- http://jimbob/jars/f_compareversion_addfile.xpi -- 04/24/2001 12:26:56 ------------------------------------------------------------------------------- Functional: f_compareversion_addfile ------------------------------------ ** addFile returns = 0 [1/1] Installing: C:\Program Files\Netscape\Netscape 6\f_compareversion.txt Install completed successfully Finished Installation 04/24/2001 12:26:57 ------------------------------------------------------------------------------- http://jimbob/jars/f_compareversion.xpi -- 04/24/2001 12:27:15 ------------------------------------------------------------------------------- ** **** Need to install f_compareversion_addfile.xpi before running this test. **** Functional: f_compareversion ---------------------------- ** CompVers should return -4 = -4 ** CompVers should return -3 = -3 ** CompVers should return -2 = -2 ** CompVers should return -1 = -1 ** CompVers should return 0 = 0 ** CompVers should return 1 = 1 ** CompVers should return 2 = 2 ** CompVers should return 3 = 3 ** CompVers should return 4 = 4 Install completed successfully Finished Installation 04/24/2001 12:27:15 ------------------------------------------------------------------------------- http://jimbob/jars/f_compareversion.xpi -- 04/24/2001 12:27:34 ------------------------------------------------------------------------------- ** **** Need to install f_compareversion_addfile.xpi before running this test. **** Functional: f_compareversion ---------------------------- ** CompVers should return -4 = -4 ** CompVers should return -3 = -4 ** CompVers should return -2 = -4 ** CompVers should return -1 = -4 ** CompVers should return 0 = -4 ** CompVers should return 1 = -4 ** CompVers should return 2 = -4 ** CompVers should return 3 = -4 ** CompVers should return 4 = -4 Install completed successfully Finished Installation 04/24/2001 12:27:34 EXAMPLE of install.log from Mac where the last installation occurs with f_compareversion.txt deleted: ------------------------------------------------------------------------------- http://jimbob/jars/f_compareversion_addfile.xpi -- 04/24/2001 12:26:56 ------------------------------------------------------------------------------- Functional: f_compareversion_addfile ------------------------------------ ** addFile returns = 0 [1/1] Installing: C:\Program Files\Netscape\Netscape 6\f_compareversion.txt Install completed successfully Finished Installation 04/24/2001 12:26:57 ------------------------------------------------------------------------------- http://jimbob/jars/f_compareversion.xpi -- 04/24/2001 12:27:15 ------------------------------------------------------------------------------- ** **** Need to install f_compareversion_addfile.xpi before running this test. **** Functional: f_compareversion ---------------------------- ** CompVers should return -4 = -4 ** CompVers should return -3 = -3 ** CompVers should return -2 = -2 ** CompVers should return -1 = -1 ** CompVers should return 0 = 0 ** CompVers should return 1 = 1 ** CompVers should return 2 = 2 ** CompVers should return 3 = 3 ** CompVers should return 4 = 4 Install completed successfully Finished Installation 04/24/2001 12:27:15 ------------------------------------------------------------------------------- http://jimbob/Jimmylee/jars/f_compareversion.xpi -- 04/24/2001 12:27:34 ------------------------------------------------------------------------------- ** **** Need to install f_compareversion_addfile.xpi before running this test. **** Functional: f_compareversion ---------------------------- ** CompVers should return -4 = -4 ** CompVers should return -3 = -3 ** CompVers should return -2 = -2 ** CompVers should return -1 = -1 ** CompVers should return 0 = 0 ** CompVers should return 1 = 1 ** CompVers should return 2 = 2 ** CompVers should return 3 = 3 ** CompVers should return 4 = 4 Install completed successfully Finished Installation 04/24/2001 12:27:34

Nominating for Beta. I can use Dan's opinion and/or others as to how important this bug is for everybody. I think the risk is low because I do not envision files being deleted in a non-controlled manner. However, there may be other scenarios that I may not have considered. So I'm really nominating this bug to solicit insight as to whether this should be nsbeta1+ or nsbeta1-. Thanks!

reassign to dan

We can live with nsbeta1-.

Actually we *will* want this one if Samir is to implement Update Notification for the Mac.

ADT2 per ADT/XPInstall triage. Samir - Can you add the update notification bug as a dependency.

Over to Dave

InstallTrigger::CompareVersion is not doing the right thing. This code is checking the return value of VR_ValidateComponent. But it is only comparing against REGERR_NOFILE. On the mac we're getting REGERR_NOPATH. The function returns REGERR_OK on success and REGERR_NOFILE, REGERR_NOPATH, and REGERR_BADCHECK on error. I'm inclined to go with checking for REGERR_OK. But the REGERR_BADCHECK is making me second guess. If the CRC check fails, do we want CompareVersion to continue?

It looks like I was chasing after the symptom here. REGERR_NOPATH should not cause failure. It simply indicates that entry in the registry doesn't refer to a file. The problem we're seeing in this bug is when the file was added, no path was saved in the registry. So CompareVersion is doing the right thing, but nsInstallFile::Complete or nsInstall::FinalizeInstall are doing the wrong thing.

This is maddening. When the file exists, VR_ValidateComponent is able to get the path out of the registry and stat the file. When the file does not exist, VR_ValidateComponent is not able to get the path out of the registry and it returns REGERR_NOPATH. It is like some evil gremlin is pulling something out of the registry when we delete the file.

When a file doesn't exist the Mac cannot resolve an alias, but when I look through VR_GetPath and vr_GetPathname it looks like that would return a REGERR_NOFILE -- the same thing VR_Validate should return if we got a real path but the later stat() failed. Oh, I see the problem. That code path only applies if the filetype is REGTYPE_ENTRY_BYTES. If it's REGTYPE_ENTRY_FILE it uses the newer stuff built-in to NR_RegGetEntry(). This *should* be a copy of what the _BYTES stuff did, but there's one crucial difference. Instead of the correct REGERR_NOFILE it returns REGERR_NOFIND -- the worst possible choice because VR_Validate specifically looks for that one. Looking at blame it's my fault. Argh! Don't know what I was thinking, but next time I won't name two errors so much alike. So the fix looks like changing REGERR_NOFIND to REGERR_NOFILE in NR_RegGetEntry in reg.c. Add a comment that this must match nr_GetPathname() in VerReg.c

That's the point in the right direction. Thanks Dan

Comment on attachment 85385 [details] [diff] [review] patch r=ssu once you've made the appropriate tests (before and after the patch)

Comment on attachment 85385 [details] [diff] [review] patch diff -u next time, please. sr=dveditz

on the trunk

Have we added update notification on the Mac, and if so is this still required?

update notification is cross platform, and this two-character fix of my bonehead mistake is safe enough in any case.

please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1" keyword and add the "fixed1.0.1" keyword.

adt1.0.1 (on ADT's behalf) approval for checkin to the 1.0 branch. please checkin to the branch asap, then remove the add the "fixed1.0.1" keyword.

Build: 2002-06-04-08-trunk(MAC) Works like a charm now! Marking Verified for TRUNK. Compareversion now correctly returns -5. Will check BRANCH tomorrow.

Build: 2002-06-05-05-1.0.0(MAC) Fix has not yet been checked into the 1.0.1BRANCH according to http://bonsai.mozilla.org/. And my test does indeed fail. Then again, there is no fixed1.0.1 keyword. Help David :-)

Build: 2002-06-10-05-1.0.0(MAC) Landed on the BRANCH. Behaves as expected now. Adding keyword verified1.0.1.