291177 - Need Interface for Enumerating Directories

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Ben Goodger (use ben at mozilla dot org for email)]

nsIFile has a |directoryEntries| attribute which returns an object implementing
nsISimpleEnumerator. This object opens the directory being enumerated when it is
initialized (when the attribute is read) with PR_OpenDir. The directory is only
closed (with PR_CloseDir) when the object's dtor fires. 

This is unacceptable from JavaScript, where object lifetime often (and usually)
exceeds its use... the effect here is that once you enumerate a directory using
the |directoryEntries| attribute, the directory enumerator object now holds it
open and you cannot do anything with it such as say, delete it. This makes
cleaning a directory file by file from JavaScript impossible. 

Solution:

Create an interface called nsIDirectoryEnumerator, similar to
nsISimpleEnumerator except with: 
- convenient |nextFile| attribute which returns an object QI'ed to nsIFile,
since that's usually what you want (returns null when there are no more,
eliminating the need for hasMoreElements()) 
- |close()| method which can be called at any time which closes the directory
releasing the system resource so that other things can be done to it.

Comment 1

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch

Comment 2

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: oops, put interface in correct set of XPIDLSRCs.

Comment 3

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 181307 [details] [diff] [review]
oops, put interface in correct set of XPIDLSRCs. 

oops, how about I implement this for MacOS X/Linux too!

Comment 4

[Darin Fisher]

nsLocalFileOS2 will need an implementation too.  Don't worry about
nsLocalFileMac... it isn't used.

Comment 5

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

isn't this a duplicate of bug 290840? that'd certainly fix the problem described
in the first two paragraphs of comment 0.

Comment 6

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

and then there's bug 239804...

Comment 7

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more...

Comment 8

[Ben Goodger (use ben at mozilla dot org for email)]

But if you wish to stop enumerating once you locate an item, there is no means
available to you to close the enumerator other than enumerating all the way to
the end, which could be costly. Darin filed 290840 yesterday after I described
the original problem to him, and we have discussed this interface. 

Comment 9

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch for windows, linux, osx and os2 (os2 untested)

Comment 10

[Darin Fisher]

*** Bug 239804 has been marked as a duplicate of this bug. ***

Comment 11

[Darin Fisher]

*** Bug 290840 has been marked as a duplicate of this bug. ***

Comment 12

[timeless]

it does not make sense to close an enumerator. "stopEnumerating()", "stop()", or
"closeDirectory()"

+   * @return NS_OK if the call succeeded and the directory was closed.
+   *         NS_ERROR_FAILURE if the directory close failed.

really?
                 PRStatus status = PR_CloseDir(mDir);
                 NS_ASSERTION(status == PR_SUCCESS, "close failed");
+                mDir = nsnull;
sure doesn't look like it returns error failure.

supposing it does return failure, whose responsibility is it to try to _finish_
the resource? will calling _finish_ again do something useful?

Comment 13

[Darin Fisher]

I think Ben originally had closeDirectory and I encouraged him to just call it
close.  I like that name because it is short.  Closing the directory enumerator
makes sense to me, so I'm fine with "close" as the method name.  It's like
closing a stream, except that this stream consists of elements which are file
objects.

Comment 14

[Darin Fisher]

Comment on attachment 181328 [details] [diff] [review]
patch for windows, linux, osx and os2 (os2 untested)

>Index: xpcom/io/nsIDirectoryEnumerator.idl

>+ * The Original Code is the File Enumerator Interface.

nit: s/File/Directory/ ;-)


>+  /**
>+   * Closes the directory enumerated, releasing the system resource.

nit: "... the directory [being] enumerated..."


>+   * @return NS_OK if the call succeeded and the directory was closed.
>+   *         NS_ERROR_FAILURE if the directory close failed.
>+   */
>+  void close();

nit: this function returns "void"... you should use @throws to
describe the NS_ERROR_FAILURE exception.

  @throws NS_ERROR_FAILURE if the directory could not be closed.

NOTE: you should indicate that it is safe to call close many times
(just as it is safe to close a stream many times).


>Index: xpcom/io/nsLocalFileOS2.cpp

>+        NS_IMETHOD GetNextFile(nsIFile **result)
>+        {
>+            *result = nsnull;
>+            PRBool hasMore = PR_FALSE;
>+            HasMoreElements(&hasMore);
>+            if (hasMore)
>+            {
>+                nsCOMPtr<nsISupports> supp;
>+                GetNext(getter_AddRefs(supp));
>+                nsCOMPtr<nsIFile> nextFile(do_QueryInterface(supp));
>+                NS_IF_ADDREF(*result = nextFile);
>+            }
>+            return NS_OK;
>+        }

nit: I think you might want to just use mNext directly after
calling HasMoreElements.  There doesn't seem to be much point
to calling GetNext and then QueryInterface.


>+        NS_IMETHOD Close()
>         {
>+            if (mDir)
>             {
>                 PRStatus status = PR_CloseDir(mDir);
>                 NS_ASSERTION(status == PR_SUCCESS, "close failed");
>+                mDir = nsnull;
>             }
>+            return NS_OK;
>+        }

Yeah, what timeless said.  We should probably propogate this
error as we claimed we would ;-)


>Index: xpcom/io/nsLocalFileOSX.cpp

>+        NS_IMETHOD GetNextFile(nsIFile **result)
>+        {
>+          *result = nsnull;
>+          PRBool hasMore = PR_FALSE;
>+          HasMoreElements(&hasMore);
>+          if (hasMore) {
>+            nsCOMPtr<nsISupports> supp;
>+            GetNext(getter_AddRefs(supp));
>+            nsCOMPtr<nsIFile> nextFile(do_QueryInterface(supp));
>+            NS_IF_ADDREF(*result = nextFile);
>+          }
>+          return NS_OK;
>+        }

Same deal.  I think you should just use mNext directly
after calling HasMoreElements.


>Index: xpcom/io/nsLocalFileUnix.cpp

> NS_IMETHODIMP
> nsDirEnumeratorUnix::HasMoreElements(PRBool *result)
> {
>     *result = mDir && mEntry;
>+    if (!*result)
>+        Close();
>     return NS_OK;
> }
...
>+NS_IMETHODIMP
>+nsDirEnumeratorUnix::GetNextFile(nsIFile **result)
>+{
>+    *result = nsnull;
>+    PRBool hasMore = PR_FALSE;
>+    HasMoreElements(&hasMore);
>+    if (hasMore) {
>+        nsCOMPtr<nsISupports> supp;
>+        GetNext(getter_AddRefs(supp));
>+        nsCOMPtr<nsIFile> nextFile(do_QueryInterface(supp));
>+        NS_IF_ADDREF(*result = nextFile);
>+    }
>+    return NS_OK;
>+}

Ah, so this guy implements the iterating in GetNext instead
of in HasMoreElements.	OK, interesting.  How about implementing
GetNext in terms of GetNextFile instead of the other way around?
That way you would avoid the QueryInterface call in GetNextFile.


>Index: xpcom/io/nsLocalFileWin.cpp

>+        NS_IMETHOD GetNextFile(nsIFile **result)
>+        {
>+            *result = nsnull;
>+            PRBool hasMore = PR_FALSE;
>+            HasMoreElements(&hasMore);
>+            if (hasMore)
>+            {
>+                nsCOMPtr<nsISupports> supp;
>+                GetNext(getter_AddRefs(supp));
>+                nsCOMPtr<nsIFile> nextFile(do_QueryInterface(supp));
>+                NS_IF_ADDREF(*result = nextFile);
>+            }
>+            return NS_OK;
>+        }

This guy is like OS/2 and OSX.	GetNextFile should just
access mNext directly after calling HasMoreElements.

Comment 15

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: (attaching for testing only)

attaching so I can test on my linux and mac machines...

Comment 16

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch - tested mac, windows, linux

Comment 17

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch

Comment 18

[Darin Fisher]

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

>Index: xpcom/io/nsLocalFileUnix.cpp

> NS_IMETHODIMP
> nsDirEnumeratorUnix::GetNext(nsISupports **_retval)
> {
>+    nsCOMPtr<nsIFile> file;
>+    nsresult rv = GetNextFile(getter_AddRefs(file));
>+    if (NS_FAILED(rv))
>         return rv;
>+    return CallQueryInterface(file, _retval);

nit: QueryInterface here is not needed.  nsIFile "is a" nsISupports.

just do:

  NS_ADDREF(*_retval = file);
  return NS_OK;


>+nsDirEnumeratorUnix::GetNextFile(nsIFile **_retval)
...
>+    nsLocalFile* file = new nsLocalFile();
>+    if (!file)
>+        return NS_ERROR_OUT_OF_MEMORY;
>+
>+    if (NS_FAILED(rv = file->InitWithNativePath(mParentPath)) ||
>+        NS_FAILED(rv = file->AppendNative(nsDependentCString(mEntry->d_name)))) {
>+        return rv;

So, the return here is a memory leak, no?  It looks like that
leak existed before, but can you fix it while your here?
|delete file;| before return rv would probably be good.


The rest looks good.  r=darin with those two changes.

Comment 19

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch to be checked in

- fixes leak
- no qi, NS_IF_ADDREF since GetNextFile can return null.

Comment 20

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

Comment on attachment 181438 [details] [diff] [review]
patch to be checked in

+   * @throws NS_OK if the call succeeded and the directory was closed.

well, NS_OK is not really thrown :-) it's just normal return, in JS...

Comment 21

[Darin Fisher]

agreed.  it's wrong to say throws NS_OK ;-)

Comment 22

[Brendan Eich [:brendan]]

Comment on attachment 181438 [details] [diff] [review]
patch to be checked in

a=me for 1.8b2 with the "throws NS_OK" fixed to say "returns normally" or
something like that.

/be

Comment 23

[Ben Goodger (use ben at mozilla dot org for email)]

landed 1.8b2/1.1a