99670 - Install.addDirectory() doesn't report errors most of the time

Status: VERIFIED FIXED

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

Description

[Daniel Veditz [:dveditz]]

addDirectory() only reports the status of the last processed file, any errors
encountered prior to that are ignored.

Comment 1

[Daniel Veditz [:dveditz]]

Attachment: patch that returns addDirectory errors, and doesn't leak nsVoidArray

Comment 2

[Daniel Veditz [:dveditz]]

I recommend this for the branch, it could be the cause of many mysterious
install failures we see. On the other hand, given CRC checking and the fact that
we've lasted this long broken maybe 0.9.5 is a better choice.

The patch fixes a couple potential nsVoidArray leaks (in error cases) and
returns the first error detected while attempting to add files. It does not
attempt to address bug 8983, script authors should cancelInstall() after
detecting an error here to avoid that problem.

Comment 3

[Cindy Roberts]

There aren't any comments on this bug since the 14th of
Sept.  Can QA regess against the Netscape commercial builds and determine if
this is still a valid bug?  If so, and we can get fixes/reviews in the next day
or two, please mark as nsbranch+ which will get this on the PDT radar.  Also,
can someone comment in the bug how serious you think this is?  PDT is only
accepting "stop ship" bugs such as data loss and loss of major functionality.

Comment 4

[Jaime Rodriguez, Jr.]

0.9.5 is a better time in IMHO. nsbranch- unless Syd can convince me to take
this one, and get the needed reviews.

Comment 5

[Syd Logan]

putting on the 0.9.5 schedule, assiging to dprice to apply patch, unit test, and
land on trunk.

Comment 6

[Jimmy Lee]

Build:  2001-09-25-05-0.9.4(WIN),  2001-09-25-03-0.9.4(MAC), 
2001-09-25-04-0.9.4(LINUX)

Catching up here.  Error is not being reporting during addDirectory when install
file is not the last file being added.

I tried to simulate this scenario by creating a directory which a file attempts
to replace thus creating an error during addDirectory().

The test I used is http://jimbob/jars/f_adddirfile_permissions.xpi  This test
adds files and directories with special attention to file permissions.

1. Install this package
2. From the "Program" directory, find the subdirectory, perms
3. Remove "smrtupdt3.txt"
4. Create a directory there named "smrtupdt3.txt"
5. Install the package again (http://jimbob/jars/f_adddirfile_permissions.xpi)

RESULT:
From Mac, the file smrtupdt3.txt, replaces the subdirectory smrtupdt3.txt. 
Should this happen?

For Linux, -215 errors are returned due to the file permissions.  This makes
sense.  If I make all files and directories writable, then the file
smrtupdt3.txt replaces the subdirectory smrtupdt3.txt.  Again, does this make sense?

For Windows NT, the situation is much different.  The "smrtupdt3.txt"
subdirectory appears to stay intact.  A new folder, smrtupdt3.old, is created. 
Also, a file smrtupdt3.new is created as well.  All other files are replaced as
expected.  The install.log indicates that I need to restart.  Exit browser and
attempt to restart.  Error message that a currect session of browser is already
running.  xpicleanup.dat is present in the "Program" directory.  xpicleanup.exe
is still running.  Each time user attempts to launch the browser, a new
xpicleanup.exe process is spawned.  So, several xpiclean.exe processes are
running.  Browser can only be started up by killing all these processes and
removing xpicleanup.dat.  The files and directories are unchanged from perms.

For Windows 98, the result is similar to Windows NT except that xpicleanup.dat
is not created and no xpicleanup.exe process is spawned.  The files and
directories under perms looks exactly the same as Windows NT.

Comment 7

[Daniel Veditz [:dveditz]]

Wow... not good. Nice catch Jimmy, especially the windows one. And yet, you were
still unable to demonstrate the initial addDirectory() problem because of these
other things.

I'll post later when I've found a better way to expose the problem. I did notice
an explicit test for a *file* in the way of a new *directory* -- the opposite of
what I told you to test before. Will dig deeper.

Comment 8

[dprice (gone)]

To test this on win 2K, I downloaded and modified 
http://jimbob/jars/a_adddir_5_string_ver.xpi  I added 3 more files to the .xpi
in the thefolder folder.  I did the install and verified that all 4 files
appeared in the right place.  I made two of the files read only, then reran the
test.  It appears to be working properly.  I'll run it through linux and mac in
the morning.  Here's the install log.

-------------------------------------------------------------------------------
file:///D:/tmp/werk/add_directory_text.xpi  --  10/09/2001 02:16:59
-------------------------------------------------------------------------------

     Acceptance: adddir_5_string_ver
     -------------------------------

     [1/4]	Installing: Y:\mozilla\dist\WIN32_D.OBJ\bin\subdir\dir.diff
     [2/4]	Replacing: Y:\mozilla\dist\WIN32_D.OBJ\bin\subdir\smrtupdt.txt
     [3/4]	Installing: Y:\mozilla\dist\WIN32_D.OBJ\bin\subdir\dir.diff2
     [4/4]	Installing: Y:\mozilla\dist\WIN32_D.OBJ\bin\subdir\handle.gif

     Install completed successfully
     Finished Installation  10/09/2001 02:16:59

-------------------------------------------------------------------------------
file:///D:/tmp/werk/add_directory_text.xpi  --  10/09/2001 02:19:41
-------------------------------------------------------------------------------

     Acceptance: adddir_5_string_ver
     -------------------------------

     ** ERROR (-215): Replacing: Y:\mozilla\dist\WIN32_D.OBJ\bin\subdir\smrtupdt.txt

     Install cancelled by script
     Finished Installation  10/09/2001 02:19:41

    

Comment 9

[dprice (gone)]

Attachment: New patch

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 70617 [details] [diff] [review]
New patch

I'll take that as r=dprice for my patch since that actual code changes are the
same as the outdated patch.

Comment 11

[Scott MacGregor]

Comment on attachment 70617 [details] [diff] [review]
New patch

sr=mscott

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 70617 [details] [diff] [review]
New patch

a=dbaron for 0.9.9

Comment 13

[dprice (gone)]

Checked in

Comment 14

[Jimmy Lee]

Build: 2002-03-08-05-trunk(WIN), 2002-03-08-08-trunk(MAC), 
Directory2002-03-08-10-trunk(LINUX)

Looks good to me.  I tested this similar to how dprice describes.  The test I
ran on 2001-09-25 10:20 is different since it is about scheduling a replacement
versus permissions to replace.

Marking Verified!