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

RESOLVED FIXED in mozilla1.9.2a1

Status

()

defect
P2
normal
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: asqueella, Assigned: asqueella)

Tracking

(Blocks 1 bug, {regression, testcase})

Trunk
mozilla1.9.2a1
x86
Windows XP
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
in-testsuite +

Firefox Tracking Flags

(status1.9.1 .3-fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

10 years ago
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).
Assignee

Comment 1

10 years ago
Asking for blocking because it's a regression and looks easy to fix.
Blocks: 455381
Flags: blocking1.9.1?
Assignee

Comment 2

10 years ago
Posted patch something like this? (obsolete) — Splinter Review
Assignee

Comment 3

10 years ago
Attachment #361866 - Attachment is obsolete: true
Attachment #361869 - Flags: review?
Assignee

Updated

10 years ago
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 5

10 years ago
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+
Assignee

Comment 6

10 years ago
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

10 years ago
Oh in that case, go right ahead.
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
Last Resolved: 10 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

Comment 10

10 years ago
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.
Assignee

Comment 14

10 years ago
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.
Assignee

Updated

10 years ago
Blocks: 481369
Assignee

Comment 17

10 years ago
> 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.