42433 - If directory already exists, File.dirCreate returns -230

Status: VERIFIED FIXED

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

Description

[David Epstein]

build 2000-06-13-09-M17. also see 41982.
1. Go to http://jimbob/trigger3.html
2. Select a_fileop_dircreate from the acceptance menu.
3. Press Trigger, then OK.
4. Trigger a_fileop_dircreate again.
Result: Get -230 error.
Notes: As expected, it does not remove directory contents. And unix and dos 
don't allow a directory to be recreated (we get an error msg). The issue raised 
in this bug is whether it's inconvenient for the script writer to have to check 
for the existence of the directory before creating it.

Comment 1

[David Epstein]

changed QA contact to depstein

Comment 2

[Daniel Veditz [:dveditz]]

Yes, we could certainly live with the way things are currently designed (which 
mimics the platform) but it will make for *much* simpler scripts if script 
authors didn't have to check for directory existence first or reset error to 
ignore errors.

This is not one we'll get to in a while, but it would be nice I think. CC'ing 
Sean in case he has a different perspective as an experienced install author

Comment 3

[Sean Su]

I agree with Dan.  It is much easier if the script writer didn't have to check 
for the existance of the dir ahead of time.

This can actually be a one line fix in the NativeFileOpDirCreatePrepare() 
function to return nsInstall::SUCCESS regardless if the directory already exists 
or not, instead of the -230 error if it already exists.

Comment 4

[Jimmy Lee]

Updating QA Contact.

Comment 5

[dprice (gone)]

Attachment: fix that always returns success

Comment 6

[dprice (gone)]

I've made a patch that always returns sucess.  I notice we never pay attention
to the result of mTarget->Create()  Is that a bad thing?

Comment 7

[Daniel Veditz [:dveditz]]

You're right, expecting that Create() always succeeds is pretty lame. As long as
you're changing this code please fix it.

I think you'll find nsInstallFileOpItem is rife with such problems... feel free
to fix it up ;-)

In the future please give more context in your patches, the default -3 is not
adequate. "cvs diff -u10" would be a good minimum, some reviewers want -20 for
non-trivial changes.

Comment 8

[dprice (gone)]

Attachment: patch also checks return value of mTarget->Create()

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 47937 [details] [diff] [review]
patch also checks return value of mTarget->Create()

>-  PRInt32 ret = nsInstall::ALREADY_EXISTS;
>+  PRInt32 ret;
>   PRBool  flagExists;
> 
>-  mAction = nsInstallFileOpItem::ACTION_FAILED;
>-
>   mTarget->Exists(&flagExists);
>   if (!flagExists)
>+    ret = mTarget->Create(1, 0755);
>+ 
>+  if (ret == NS_OK) 

Return value from an nsIFile is going to be an nsresult -- use the NS_SUCCEEDED/NS_FAILED macros.
And you really should use an nsresult for it rather than re-using a PRInt32.

You no longer have an initial value for ret, so what if the target *does* exist? What if it exists
and happens to be a file that's in the way? Now that already existing is OK, we have to make
sure it's the right kind of thing.

There's no check that mTarget->Exists() returns success, so you can't trust the potentially
undefined flagExists variable.

Comment 10

[dprice (gone)]

A quick clarification.  If mTarget exists, and it is a directory, then we're
okay to return nsInstall::SUCCESS  If mTarget is a file, then we want to fail
and return nsInstall::IS_FILE?

Comment 11

[Daniel Veditz [:dveditz]]

Sure, IS_FILE sounds appropriate.

Comment 12

[dprice (gone)]

Attachment: Patch v3.0

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 49681 [details] [diff] [review]
Patch v3.0

>+  else
>+  {

You could also get here if mTarget->Exists() fails, not just if it exists.
Presumably that would lead to mTarget->IsFile() failing as well so it's
not so bad to ignore that possibility, but

>+    rv = mTarget->IsFile(&flagIsFile);
>+    if (NS_SUCCEEDED(rv) && (flagIsFile))
>+    {
>+      mAction = nsInstallFileOpItem::ACTION_FAILED;
>+      ret = nsInstall::IS_FILE;
>+    }

you can't ignore the possibility that mTarget->IsFile() will fail. If it
does you will return nsInstall::SUCCESS and mAction will be set to
ACTION_SUCCESS

Since you're in nsInstallFileOpItem methods you don't need to
specify nsInstallFileOpItem:: to use ACTION_FAILED and ACTION_SUCCESS.
The rest of the file does, though, so it's up to you if you want to
strike a blow for readability or for consistency ;-)

Comment 14

[dprice (gone)]

Attachment: newer patch

Comment 15

[dprice (gone)]

Thanks for the feedback Dan.  Here's a patch that should cover everything.  

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 51771 [details] [diff] [review]
newer patch

looks good, r=dveditz

Comment 17

[Scott MacGregor]

Comment on attachment 51771 [details] [diff] [review]
newer patch

sr=mscott

Comment 18

[dprice (gone)]

check in to the trunk

Comment 19

[Jimmy Lee]

Build: 2001-10-31-03-trunk(WIN), 2001-10-31-08-trunk(MAC),
2001-10-31-08-trunk(LINUX)

Looks good on all platforms.  Marking Verified.