478086 - enumerating drives/disks on Win32 using nsIFile.directoryEntries returns a single string with embedded NULLs in it

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P2)

Description

[Nickolay_Ponomarev]

Windows:
var file = Components.classes["@mozilla.org/file/local;1"].
  createInstance(Components.interfaces.nsILocalFile);
file.initWithPath("\\\\.");
var s = file.directoryEntries.getNext().
  QueryInterface(Components.interfaces.nsIFile).path;

Actual results:
s -> C:\�D:\�F:\���
s.indexOf("\0") -> 3

Expected: s = "C:\", no embedded NULLs.

This is a regression since 3.0 due to bug 455381's checkin http://hg.mozilla.org/mozilla-central/rev/0f1b8ab7cd8e (verified via backout).

Comment 1

[Nickolay_Ponomarev]

Asking for blocking because it's a regression and looks easy to fix.

Comment 2

[Nickolay_Ponomarev]

Attachment: something like this?

Comment 3

[Nickolay_Ponomarev]

Attachment: with xpcshell test [Checkin: Comment 8 & 12]

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #3)
> Created an attachment (id=361869) [details]
> with xpcshell test

This looks like a better solution all together.

Comment 5

[Benjamin Smedberg]

Comment on attachment 361869 [details] [diff] [review]
with xpcshell test
[Checkin: Comment 8 & 12]

I'm not sure we accept public domain code, because of legal issues in countries which don't recognize it. Could you license it under the MIT license to avoid problems? http://www.ibiblio.org/pub/Linux/LICENSES/mit.license (this is the license used for code samples on MDC also)

Comment 6

[Nickolay_Ponomarev]

Thanks for the review.

> I'm not sure we accept public domain code
The only reason I use that boilerplate is that it's short and was okayed by mozilla lawyers (per gerv) [1]. Other testcases in the tree also use it [2]
http://weblogs.mozillazine.org/gerv/archives/2007/04/making_tiny_things_public_doma.html
http://mxr.mozilla.org/mozilla-central/search?string=Any%20copyright%20is%20dedicated%20to%20the%20Public%20Domain.

Comment 7

[Benjamin Smedberg]

Oh in that case, go right ahead.

Comment 8

[Serge Gautherie (:sgautherie)]

Comment on attachment 361869 [details] [diff] [review]
with xpcshell test
[Checkin: Comment 8 & 12]


http://hg.mozilla.org/mozilla-central/rev/3701f0e7fa64

Comment 9

[Daniel Holbert [:dholbert]]

It looks like this patch (or one of the patches that landed with it) might've caused a large Ts regression on Windows -- see bug 480464.

Comment 10

[Benjamin Smedberg]

I would be extraordinarily surprised if this showed up on Talos. As far as I know, we don't actually run this code at all in Firefox, no less in some kind of tight loop.

Comment 11

[Daniel Holbert [:dholbert]]

It turned out that bug 465158 was responsible for the Ts regression -- disregard comment 9.

Comment 12

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/8228cab107a7

Comment 13

[Johnathan Nightingale [:johnath]]

This test failed on mozilla-central with:

NEXT ERROR TEST-UNEXPECTED-FAIL | ../../_tests/xpcshell/xpcom/unit/test_bug478086.js | test failed, see log
../../_tests/xpcshell/xpcom/unit/test_bug478086.js.log:
>>>>>>>
*** test pending
[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsISimpleEnumerator.hasMoreElements]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: ../../_tests/xpcshell/xpcom/unit/test_bug478086.js :: run_test :: line 20"  data: no]
*** FAIL ***

<<<<<<<

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1236108607.1236113233.21575.gz

The only checkin in the window was a font checkin from vlad: http://hg.mozilla.org/mozilla-central/rev/bc2e5908f1e6

If we don't have an obvious regressor or fix, then it seems to me that this test is an intermittent fail, and should be disabled until it can be made more reliable.

Comment 14

[Nickolay_Ponomarev]

The actual regression test needs to run only on windows, so I suggest filing a bug on the issue with Mac and fixing up the test like this:

var isWindows = ("@mozilla.org/windows-registry-key;1" in Components.classes);
  13   if (isWindows) {
  14     root.initWithPath("\\\\.");
- 15   } else {
- 16     root.initWithPath("/");
- 17   }
  18   var drives = root.directoryEntries;
  19   do_check_true(drives.hasMoreElements());
  20   while (drives.hasMoreElements()) {
  21     var newPath = drives.getNext().QueryInterface(nsILocalFile).path;
  22     do_check_eq(newPath.indexOf("\0"), -1);
  23   }
+      }

The tree is closed, and I must get some sleep, so leaving this to someone else.

Comment 15

[Dão Gottwald [:dao]]

In that case, it should be as easy as this:

  13   if (isWindows) {
  14     root.initWithPath("\\\\.");
  15   } else {
+ 16     return; // XXX bug 478086 comment 13
  16     root.initWithPath("/");
  17   }

Comment 16

[Johnathan Nightingale [:johnath]]

With a follow-up bug to fix it properly? I could buy that (though at that point we might as well cite the new bug in the comment, since this one is resolved.)

One of you want to do that?  The tree's closed right now, but we could land this tomorrow when it opens.

Comment 17

[Nickolay_Ponomarev]

> One of you want to do that?  The tree's closed right now, but we could land
> this tomorrow when it opens.

http://hg.mozilla.org/mozilla-central/rev/10d44bf9a01f
bug 481369

Comment 18

[Serge Gautherie (:sgautherie)]

(In reply to comment #17)
> http://hg.mozilla.org/mozilla-central/rev/10d44bf9a01f
> bug 481369

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/70302b37196d