77443 - compareVersion returns false info when file removed on Mac only

Status: VERIFIED FIXED

(Core Graveyard :: Installer: XPInstall Engine, defect)

Description

[Jimmy Lee]

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

Comment 1

[Jimmy Lee]

Attachment: Install script for f_compareversion_addfile.xpi

Comment 2

[Jimmy Lee]

Attachment: Install script for f_compareversion.xpi

Comment 3

[Jimmy Lee]

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!

Comment 4

[Syd Logan]

reassign to dan

Comment 5

[Jimmy Lee]

We can live with nsbeta1-.

Comment 6

[Daniel Veditz [:dveditz]]

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

Comment 7

[Jaime Rodriguez, Jr.]

ADT2 per ADT/XPInstall triage.

Samir - Can you add the update notification bug as a dependency.

Comment 8

[Syd Logan]

Over to Dave

Comment 9

[dprice (gone)]

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?

Comment 10

[dprice (gone)]

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. 

Comment 11

[dprice (gone)]

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.  

Comment 12

[Daniel Veditz [:dveditz]]

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

Comment 13

[dprice (gone)]

Attachment: patch

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

Comment 14

[Sean Su]

Comment on attachment 85385 [details] [diff] [review]
patch

r=ssu  once you've made the appropriate tests (before and after the patch)

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 85385 [details] [diff] [review]
patch

diff -u next time, please.

sr=dveditz

Comment 16

[dprice (gone)]

on the trunk

Comment 17

[scottputterman]

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

Comment 18

[Daniel Veditz [:dveditz]]

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

Comment 19

[Judson Valeski]

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

Comment 20

[Jaime Rodriguez, Jr.]

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.

Comment 21

[Jimmy Lee]

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.

Comment 22

[Jimmy Lee]

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 :-)

Comment 23

[Jimmy Lee]

Build: 2002-06-10-05-1.0.0(MAC)

Landed on the BRANCH.  Behaves as expected now.  Adding keyword verified1.0.1.