239804 - Cannot remove a directory using nsIFile.remove if directoryEntries was retrieved before

Status: VERIFIED DUPLICATE of bug 291177

(Core :: XPCOM, defect)

Description

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

I have a portion of JS code, relying on an nsIFile |dir| representing a
directory and trying to determine if the removal of the dir has to be done
recursively or not. This is of course dependant on the fact the dir is empty or
not. I find if the dir is empty or not checking
|dir.directoryEntries.hasMoreElements()|. But, according to nsLocalFileWin.cpp,
that call opens the directory and closes it only when the enumerator is
destroyed (that's pretty normal). So when I call a |dir.remove(true)|, the files
inside the dir are removed, but the removal of the directory itself fails,
WindowsXP refusing to remove a directory with an open file descriptor on it...

Deleting the |dir.directoryEntries| does not really help because there is little
delay between the call and the real deletion of the enumerator. In the meantime,
I am trying to remove the directory, and it fails.

If there is no way to wait until the enumerator deletion is really done, we
could easily solve the problem

a) with a very simple interface nsDirEnumerator could implement, allowing to
close the file descriptor carried by the enumerator.

b) or with a new isEmptyDirectory() method on nsIFile

Thoughts?

Comment 1

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

fixing title, sorry for spam

Comment 2

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: this should solve the problem...

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #2)
> Created an attachment (id=145550)
> this should solve the problem...

nsIFile is frozen, you can't add anything to it. You can't even add anything at
the end, because nsILocalFile inherits from it and is also frozen.

Comment 4

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

(In reply to comment #3)

> nsIFile is frozen, you can't add anything to it. You can't even add anything at
> the end, because nsILocalFile inherits from it and is also frozen.

Damn!!! I missed that. Thanks Christian.
But my fix works well. Speaking of it, it's more a workaround than a real fix
since the original problem still stands. I'll work on a better solution.

Comment 5

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Comment on attachment 145550 [details] [diff] [review]
this should solve the problem...

nsIFile is frozen :-/

Comment 6

[Boris Zbarsky [:bzbarsky]]

Why does recursive vs non-recursive deletion depend on whether the dir is empty,
exactly?

That said, this seems like an OS issue to me; one we can maybe work around by
having the enumerator close the file desc after each access, but past that....

Comment 7

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

bz: nsLocalFile::Remove does not depend on it, of course. My code does. It first
checks if the dir is empty or not. If it's not empty, the user is prompted with
a choice "remove dir only or dir with contents". Sorry if I explained badly.

Comment 8

[timeless]

you should be able to loop until hasmoreelements returns false. the code while
not perfect will close its handle when that happens.

"invalid" as filed.

i may or may not write an xpcshell testcase showing this.

Comment 9

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

(In reply to comment #8)
> you should be able to loop until hasmoreelements returns false. the code while
> not perfect will close its handle when that happens.

Tried it. Does not work because even when hadMoreElements() becomes false, the
file descriptor is not closed by nsDirEnumerator. That's prolly the bug.

Comment 10

[Jerome Lacoste]

Daniel, I am not sure how your code is written, but here's my advice:

What about making the loop over the elements in a specific block of code?
If the enumerator is discarded, the destructor should be called, and the handle
released (at least according to
http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsLocalFileWin.cpp#383).

Comment 11

[timeless]

[e-mail]: your advice doesn't make sense because the realm is JS
not C++.

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

surely objects created from JS will also be released at some point?

is it possible to force a GC?

Comment 13

[Brendan Eich [:brendan]]

Whoever designed nsIFile's directoryEntries attribute, please repeat after me:
Finalization shall not be used to close file descriptors or release other
external resources; an explicit method call is mandatory.

Someone was ass-u-ming ref-counting in all programming language bindings that
might use this XPCOM interface implementation.  Naughty.  What's worse is that
the interface used, nsISimpleEnumerator, shouldn't have any such explicit
method.  And nsIFile.idl is frozen (sigh).

So how about this workaround, temporary I hope: in your JS code, set dir to null
when you're done, and force a GC by reloading about:blank in a hidden frame? 
Lousy, but unless someone has a better idea, that is the only way to force a GC.
 If about:blank reloads without going through the unload-document path in the
DOM that runs a GC from a timer, then you'll have to use a real document.

Doug, darin, shaver: ideas on how to fix this without breaking binary
compatibility?  Could we return an nsISimpleEnumerator but make the enumerator
implement a new interface with a mandatory close() method?  Call this new
interface nsIDirectoryEntryEnumerator or nsIDirectoryEntries, maybe.

/be

Comment 14

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

(In reply to comment #9)

> Tried it. Does not work because even when hadMoreElements() becomes false, the
> file descriptor is not closed by nsDirEnumerator. That's prolly the bug.

Hmmm. Seems I had a bug in my code. Enumerating all entries closes the
descriptor correctly and I can then remove the directory. Here's my code for
those who wanna see it:

      var localFile = GetLocalFileFromURLSpec(url);
      var dirEntries = localFile.directoryEntries;
      var removeAll = false;
      if (dirEntries.hasMoreElements())
      {
        while (dirEntries.hasMoreElements())
          var junk = dirEntries.getNext();

        if (promptService.confirm(window, "Remove directory : alert",
                                  "This directory is not empty. Do you want to
delete its contents too?"))
          removeAll = true;
      }
      localFile.remove(removeAll);

But the cause of the bug still stands: I should not have to enumerate all
directory's entries to be able to remove it...

Comment 15

[Doug Turner (:dougt)]

what is the cost of having assignment to null forced gc?

is this really the only place where we depend on finalization to clean up
resources?  about about a nsIObject which has a explict finalize method (along
with the other base class type stuff)?  Cleaner than loading about:blank into a
hidden window, but still pretty bad.

Comment 16

[Brendan Eich [:brendan]]

GC is a global process, it costs too much to do on every assignment to null.  It
is not a nominally local operation such as nsIQueryInterface::Release in XPCOM.

If you know of other XPCOM implementations that assume they're called from C++
or another ref-counting XPCOM-using language, file bugs.  That there might be
others is always a possibility, but it doesn't make depending on prompt release
to free external resources a good idea.

/be

Comment 17

[Doug Turner (:dougt)]

another possibly bad idea: can the reflection system provide a "free this object
now" method?  

Comment 18

[Brendan Eich [:brendan]]

Why not expose Release to JS?  C'mon, people will lose fingers that way. 
"Close" is not "Finalize".  The two should not be coupled, in general.  Adding a
close method on a new interface suffices, uses XPCOM as designed, and does not
lead to dangling pointer crashes (generated from JS, yet).  Let's do the right
thing.

/be

Comment 19

[Darin Fisher]

I like the idea of a new interface for directory enumeration. 
nsISimpleEnumerator anyways leaves much to be desired, like the fact that
getNext does not return a nsQIResult (hence forcing consumers to QI-to-nsIFile
code).  

nsIFileEnumerator (or whatever we call it) sounds like an excellent opportunity
to simplify directory enumeration code in the tree.

FWIW, I like nsIFileEnumerator as the name because it parallels
nsIStringEnumerator, etc.

That said, perhaps we should also make sure that "Close()" is called whenever
HasMore() returns false.

Comment 20

[Doug Turner (:dougt)]

I was trying to not have another interface.  exposing release for situation
seams like a better idea than opening some window opening to trigger gc.  they
are all bad ideas, i suppose. 

lets go with what darin says.  

Comment 21

[Darin Fisher]

Whoops, I forgot about this bug when talking to Ben about this very same
problem.  He went and filed bug 291177 for the same problem and attached a patch
that implements the solution I proposed here.  Marking this bug as a duplicate
for simplicity sake.

*** This bug has been marked as a duplicate of 291177 ***

Comment 22

[timeless]

tsk