29415 - XPInstall does not chmod read-only files so it can delete them

Status: VERIFIED FIXED

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

Description

[Daniel Veditz [:dveditz]]

When xpinstall finds a file it should delete (whether to replace or simply to
clean up) it schedules it for later attempts if the delete fails on the
assumption that the file might be in use.  In fact, the file might simply be
marked read-only and we should first chmod it to writable and try again.  On
Unix we should perhaps check the ownership of the file to see if we'll ever be
able to do anything to the file.

There's no point in scheduling actions for later that we cannot ever perform.

Comment 1

[Jimmy Lee]

Ccing Paul and David.

Comment 2

[Cathleen]

beta2...  setting to M15

Comment 3

[Cathleen]

set to M17
reassign to sgehani
bug meeting 3/20

Comment 4

[Daniel Veditz [:dveditz]]

Resetting target field for missed milestones

Comment 5

[Daniel Veditz [:dveditz]]

Reassigning, Don has other similar bugs to this

Comment 6

[Daniel Veditz [:dveditz]]

Moz 0.9 tasks

Comment 7

[dbragg]

p3 tasks

Comment 8

[dbragg]

Attachment: Patch in DeleteFileNowOrShedule

Comment 9

[dbragg]

Posted patch.  Samir brought up a good point in the review.  I'm returning
nsInstall::READ_ONLY instead of ACCESS_DENIED so that we don't try to schedule
this file for deletion and never actually delete it.  I did this based on the
original comment in the bug but Samir pointed out that it may be the case where
another process has set the attributes on the file and prevents us from changing
them.  In this case we should return ACCESS_DENIED so that when the other
process releases or resets the attributes we'll be able to delete it.

Please advise.

Comment 10

[Daniel Veditz [:dveditz]]

No, that's too late... We need to know in the Prepare() step whether this is 
going to fail or not so we can return a useful error message to the script at a 
time when the script can deal with it.

The Prepare step should check permissions, if read-only change them right then, 
and if they can't be changed return an error ACCESS_DENIED right there. If we 
wanted to be nice we could store the fact the permissions were changed just in 
case of an Abort() so we can put it back.

What's the bug number on SetPermissions not working on the Mac? This one will 
depend on that one because when we do OS X mac permissions come into play.

Comment 11

[dbragg]

So what do you want to do with this Dan?  It was left in the air after our
discussion a week or so ago when we saw how thorny this is if you want to try to
catch it in the Prepare phases.  Do you want me to add it to all the Prepare
phases that need it, (as we discussed it's more than one) or do we want to just
go with this patch?

Comment 12

[dbragg]

After discussing this somewhat sticky issue with Dan we came up with the
following patch.

Comment 13

[dbragg]

Attachment: New patch in ExtractFileFromJar.

Comment 14

[Samir Gehani]

Patch looks good.  Can you include the rationale?

Comment 15

[Daniel Veditz [:dveditz]]

Rather than try to cover all the bases where we might be able to chmod a 
read-only file (and detect the cases where we can't early enough to do any 
good) we decided that it's safer to simply respect the user's read-only 
settings and detect them at the ::Prepare() step.

As an enhancement we could do similar detection in the File object methods, but 
in the meanwhile we're going to expose File.isWritable() to let script authors 
deal with it on their own when using the lower-level file commands.

Comment 16

[Scott MacGregor]

sr=mscott

Comment 17

[dbragg]

Fix checked in.

Comment 18

[Jimmy Lee]

Build: 2001-04-09-11-trunk(WIN), 2001-04-09-09-trunk(MAC), 
2001-04-09-09-trunk(LINUX)

When file is read-only, we now correctly return error -215 (READ_ONLY).

Marking Verified!