Closed Bug 478086 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: XPCOM, defect, P2)

x86
Windows XP
defect

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1
Tracking Status
status1.9.1 --- .3-fixed

People

(Reporter: asqueella, Assigned: asqueella)

References

(Blocks 1 open bug)

Details

(Keywords: regression, testcase)

Attachments

(1 file, 1 obsolete file)

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).
Asking for blocking because it's a regression and looks easy to fix.
Blocks: 455381
Flags: blocking1.9.1?
Attached patch something like this? (obsolete) — Splinter Review
Attachment #361866 - Attachment is obsolete: true
Attachment #361869 - Flags: review?
Attachment #361869 - Flags: review? → review?(benjamin)
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
(In reply to comment #3)
> Created an attachment (id=361869) [details]
> with xpcshell test

This looks like a better solution all together.
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)
Attachment #361869 - Flags: review?(benjamin) → review+
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.
Oh in that case, go right ahead.
Keywords: checkin-needed
Comment on attachment 361869 [details] [diff] [review]
with xpcshell test
[Checkin: Comment 8 & 12]


http://hg.mozilla.org/mozilla-central/rev/3701f0e7fa64
Attachment #361869 - Attachment description: with xpcshell test → with xpcshell test [Checkin: Comment 8]
Assignee: nobody → asqueella
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs 1.9.1 landing]
Target Milestone: --- → mozilla1.9.2a1
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.
Depends on: 480464
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.
It turned out that bug 465158 was responsible for the Ts regression -- disregard comment 9.
No longer depends on: 480464
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.
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.
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   }
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.
Blocks: 481369
> 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
Attachment #361869 - Attachment description: with xpcshell test [Checkin: Comment 8] → with xpcshell test [Checkin: Comment 8 & 12]
Whiteboard: [c-n: comment 17 to m-1.9.1]
You need to log in before you can comment on or make changes to this bug.