1486311 - GetDirectoryEntries() should be passed a nsIDirectoryEnumerator

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[:aceman]

In https://hg.mozilla.org/mozilla-central/rev/062e4138bfde of bug 1484496 it was changed that a nsIDirectoryEnumerator is to be passed to nsIFile->GetDirectoryEntries().

It seems this was missed at least at https://searchfox.org/mozilla-central/rev/7df3c5baab8a65a6c2cfc6e8a504b9d3fe3184c3/xpcom/io/nsLocalFileWin.cpp#2047.

Found by Thunderbird tests, see bug 1485820 comment 102.

Comment 1

[:aceman]

Attachment: 1486311.patch

These are the potentially left over occurrences. Compiled on Linux locally.
Please push to c-c and m-c try and see if the change makes sense at all places.

Comment 2

[Jorg K (CEST = GMT+2)]

This fixes the test crash we see on Windows in Thunderbird. I haven't done M-C try pushes in ages, so I hope Kris can do that.

Comment 3

[:aceman]

Thanks for testing.
I wonder what is the assert/crash good for, if the tests pass fine in opt builds. Is the problem real and are the opt builds silently not working?

Comment 4

[Kris Maglione [:kmag]]

(In reply to :aceman from comment #3)
> I wonder what is the assert/crash good for, if the tests pass fine in opt
> builds. Is the problem real and are the opt builds silently not working?

It's mostly a sanity check. In this particular case, it doesn't matter much, since we just happen to have two nsISimpleEnumerator vtable entries in this class. It matters in the case of bad casts and missing QueryInterface entries, though.

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 9004102 [details] [diff] [review]
1486311.patch

Review of attachment 9004102 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/io/nsIFile.idl
@@ +349,5 @@
>      [binaryname(DirectoryEntriesImpl)]
>      readonly attribute nsIDirectoryEnumerator directoryEntries;
>  
>      %{C++
> +/* XXX: still needed for anything?

Please remove this comment. Feel free to just remove the function, though.

Comment 6

[Jorg K (CEST = GMT+2)]

Thanks Kris. Aceman is on holidays and will return in a week. Could you please make this adjustment and get it landed. Thanks in advance. If not, I can do it.

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: 1486311.patch

Addressed review comment. Carrying forward Kris' r+.
(Author Aceman on vacation.)

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d40ab2ee6023
convert remaining nsIFile::GetDirectoryEntries() callers to pass in a nsIDirectoryEnumerator. r=kmag

Comment 9

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/d40ab2ee6023

Comment 10

[[e-mail]]

Comment on attachment 9004640 [details] [diff] [review]
1486311.patch

>     [binaryname(DirectoryEntriesImpl)]
>     readonly attribute nsIDirectoryEnumerator directoryEntries;
> 
>     %{C++
>-    nsresult GetDirectoryEntries(nsISimpleEnumerator** aOut)
>-    {
>-      nsCOMPtr<nsIDirectoryEnumerator> dirEnum;
>-      nsresult rv = GetDirectoryEntries(getter_AddRefs(dirEnum));
>-      dirEnum.forget(aOut);
>-      return rv;
>-    };
>-
>     nsresult GetDirectoryEntries(nsIDirectoryEnumerator** aOut)
>     {
>       return GetDirectoryEntriesImpl(aOut);
>     };
>     %}
Now that the function isn't being overloaded any more you can remove all of the gubbins (keeping just the second line of course).