bugzilla.mozilla.org will be intermittently unavailable on Saturday, March 24th, from 16:00 until 20:00 UTC.

nsIFileOutputStream.init(file_in_adirectory_that_does_not_exist, MODE_WRONLY /* 0x2*/ | MODE_CREATE /* 0x8*/ | MODE_TRUNCATE /* 0x20*/, 0644, 0) should either create the directory or throw something more sensible than NS_ERROR_FILE_NOT_FOUND

RESOLVED FIXED in Firefox 56



13 years ago
8 months ago


(Reporter: timeless, Unassigned)


Windows XP

Firefox Tracking Flags

(firefox56 fixed)


(Whiteboard: [necko-would-take][good first bug])


(4 attachments, 3 obsolete attachments)



13 years ago
There's no documentation on nsIFileOutputStream about this case, and getting
FILE_NOT_FOUND is confusing for a function call where i think* i'm telling the
api to really really really create the file.

As it happens, the api i'm using takes a string, and therefore has no
nsILocalFile, if i created such a file first, then i'd be wasting cycles on an
object i can't even use.

A caller could call nsILocalFile.parent.create(nsIFile.DIRECTORY_TYPE), but why
should the caller have to spend cycles getting an object that really doesn't
interest it?

alternatively, the caller could do nsILocalFile.create(nsIFile.NORMAL_FILE_TYPE)
but this is silly as the first thing the caller then does is ask netwerk to ask
the system to destroy the file and create a new one. and on VMS this creates a
new version of the file.

If the preceding arguments are insufficient for making nsIFileOutputStream
create the directories, i'd like to argue that the error be changed. I was
tearing out hair trying to figure out why a write operation was giving an error
about *file not found*. Even an access denied error would be better.

Comment 1

13 years ago
Created attachment 170522 [details]
Confirming.  The api certainly implies that the file will be created...
Ever confirmed: true


12 years ago
Assignee: darin → nobody
QA Contact: benc → networking
Whiteboard: [necko-would-take][good first bug]

Comment 3

10 months ago
hi, i'm a newbie and i want to get started working on this. I've found the nslFileStreams.idl with nsIFileOutputStream in my local firefox build (netwerk/base), but i'm not sure where the actual implementation for init() is or how to use the testcase attachment. can someone point me in the right direction?
Ben, thank you for looking at this!

I believe the testcase is meant as input to the "xpcshell" executable you get when you compile Firefox locally.  It will appear in the dist/bin subdirectory of the directory where the Firefox build is placed (the objdir).  If you download the testcase to a file names "test.js", you should be able to run it as "xpcshell test.js".

As far as where the implementation of init() is, it's in netwerk/base/nsFileStreams.cpp.  See http://searchfox.org/mozilla-central/rev/b318c7dca7392bd16c0b11929f55b1be133f0b31/netwerk/base/nsFileStreams.cpp#732

I hope that helps, and please feel free to ask if you have any other questions!

Comment 5

10 months ago
I couldn't actually find the xpcshell executable in my firefox build (I built it the way it says in https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Windows_Prerequisites), and I don't even have a mozilla-central/dist/ subdirectory). 

How do I get the xpcshell executable? I tried to use './mach xpcshell-test testcase' from 'https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests', but no tests are actually run.
> and I don't even have a mozilla-central/dist/ subdirectory

The "dist" subdirectory is inside the objdir.  You can run "mach environment" to find out what the objdir is; it's the last thing "mach environment" outputs (the "config topobjdir") directory.

That's where the "firefox.exe" executable is, in dist/bin.  I'd expect an xpcshell.exe executable in the same location.  Is it not there?

Comment 7

10 months ago
You were right, thanks. I found xpcshell.exe, but running it against the testcase fails. When I run 'xpcshell.exe testcase':
    * build() doesn't work. Don't know what it is or where it came from, so I commented it out and continued running it.
    * According to https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsILocalFile, after uncommenting build(), wouldn't line 100 (`file.initWithPath(temp)`) and anything that uses nsILocalFile be deprecated? Trying to run it fails with the exception:
nsILocalFile.initWithPath failed: [Exception... "Component returned failure code: 0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH) [nsILocalFile.initWithPath]"  nsresult: "0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH)"  location: "JS frame :: testcase :: <TOP_LEVEL> :: line 112"  data: no]

    Since I thought nsILocalFile wouldn't work, I replaced all instances of nsILocalFile with nsIFile (which also has the same function signature for initWithPath() as nsILocalFile), but got the same error with all nsILocalFile replaced with nsIFile in the output. I've found that the error, NS_ERROR_FILE_UNRECOGNIZED_PATH just means that I didn't give it an absolute path, so the issues are coming from line 81 (the creation of the env variable), but I'm not sure how to fix that. "c:\\temp" and any of its variations seem like absolute paths to me.

    What should I do to make this testcase work, or is it meant to break like that? As I understand, the testcase should fail at least after line 134 (which is the creation of nsIFileOutputStream & init()). 

Thanks for your time!
nsIFile.initWithPath still exists and should be called in this case.

What string ends up in "temp" and hence passed to initWithPath there?

Most simply, you could set USE_ENVIRONMENT to false to always take the USER_SPECIFIED_TEMP_DIR codepath in the testcase, then set it to something that works.

Comment 9

10 months ago
So just for clarification, if the path "c:/temp/w3e/randfile1" doesn't exist, should I just be creating the directories "temp/" and "w3e/", or do I also need to create "randfile1"?
Normally MODE_CREATE would create the file itself, so I think you want to create all of the above.

Comment 11

10 months ago
So I'm trying to do this to make the files:

nsString path;
for (PRInt32 offset = path.Find("/", offset, -1); offset != kNotFound; offset = offset + 1){
    // cut off the path to the current file
    path.Replace(offset, 1, '\0'); // err: ambiguous 

    // Make a new file where we've chopped off part of the path
    nsIFile* file = new nsIFile(); 
    try {
        nsIFile.Create(DIRECTORY_TYPE, perm);

    delete file;
    path.Replace(offset, 1, '\\'); // put it back when we're done

I got the function signatures and constants from https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIFile, but nsIFile is just the abstract class, the compiler doesn't recognize my instantiation for nsIFile or the exceptions. Is this what I should be referencing for the class (the functions in dist/include/nsIFile.h are slightly different than the ones on the site)? Is there some kind of thing where I can find what classes use this interface? What's the right way to create a file?
Instead of munging the file path manually, you can use GetParent() on the nsIFile object to get an nsIFile for the parent directory.  Then you can either check that it exists or try to create it as needed.

> Is there some kind of thing where I can find what classes use this interface?

Sure.  http://searchfox.org/mozilla-central/search?q=public%20nsIFile&case=true but then you have to look for the ones that are not similar-looking classnames or example code.  In practice, that's just nsILocalFile.  So then http://searchfox.org/mozilla-central/search?q=public%20nsILocalFile&case=true which finds two implementations: one for unix (including Mac) and one for Windows.

Comment 13

10 months ago
Hey, so I think I've fixed the bug. I'm just a bit confused on how to submit the patch. Do I just attach the file I edited here, like it says in Step 4 here (https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction#What_skills_do_I_need) or try and figure out how to use MozReview? 

I appreciate the help. Also, if I have "c:/temp/1234/abcde", I make temp/ and temp/1234/ subdirectories and abcde a file. If I have "c:/temp/1234/abcde/" instead, should I treat this differently i.e make temp/ temp/1234/ and temp/1234/abcde/ so that all three are folders?
Just attaching a patch is fine; you don't have to use mozreview.  That said, you'd want to attach the diff, not the entire file.  You should be able to create a diff using "hg diff"; the exact arguments depend on whether you've made local commits or whether all your work is just present as local uncommitted changes to the source.

> If I have "c:/temp/1234/abcde/" instead, should I treat this differently i.e make temp/ temp/1234/ and temp/1234/abcde/

I'm pretty sure that under the hood nsLocalFile normalizes away trailing '/'.  It certainly does that on Mac.

In any case, the creation of the "abcde" bit should presumably be handled by the existing code; this bug is just about creating "temp" and "1234".

Comment 15

10 months ago
Created attachment 8876542 [details] [diff] [review]

Added a check for whether the file existed, used nsIFile's Create() function to make the path, removed the extra folder made by it.

Comment 16

10 months ago
Created attachment 8876547 [details]

A working version of the test case previously posted on this bug.
Comment on attachment 8876542 [details] [diff] [review]

Thank you for the patch!

Two issues, and a few questions.  First the issues:

1) The creation should only happen if ioFlags contains PR_CREATE_FILE, I would think.

2) Please use nsIFile constants for the first arg for Create().  In this case, using nsIFile::DIRECTORY_TYPE (1) there is a bit odd; I would have expected nsIFile::NORMAL_FILE_TYPE (0).  It doesn't matter with the patch as written, of course, given that Remove() call.

Now the questions:

1)  Why do we need to Remove the file?  We're presumably going to just recreate it anyway.  If we use NORMAL_FILE_TYPE we should be good after this creation.

2)  Why do we need to pre-check whether the file exists?  Just trying to create should be fine; if it already exists we'll just fail to do that, but we're ignoring failures from the Create() call anyway.

3)  Do we really want to do this up front even in the deferred-open case?  Or should this creation bit move into nsFileStreamBase::DoOpen?

Skipping the Remove and Exists bits would make this code less racy, in general....

Comment 18

9 months ago
Created attachment 8876957 [details] [diff] [review]

Ahhhhh you're absolutely right. That's just me being dumb. I had an itchy feeling about doing the Create() and Remove() calls, I figured it was a pretty weird thing to do. 

The reason I didn't use the actual constant was because it kept telling me that it was undeclared, wasn't aware that it was part of the nsIFile namespace. I have a version now that hopefully addresses those problems.

Thanks so much!
Attachment #8876542 - Attachment is obsolete: true
Thanks!  I did a try push with that patch at https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c627268e6450a857d710d975166941771ea4368

The red jobs are failing because Create() is marked as "warn if the result value is not used".  Since we are purposefully ignoring it here, we want to do something like this:

   Unused << mOpenParams.localFile->Create(nsIFile::NORMAL_FILE_TYPE, mOpenParams.perm);

with a comment explaining why it's OK (and even desirable) to ignore the return value from Create.

Comment 20

9 months ago
Created attachment 8877214 [details] [diff] [review]

How's this?
Attachment #8876957 - Attachment is obsolete: true
One other problem, which is visible on the orange Windows "X" jobs on that try push: the modules/libjar/zipwriter/test/unit/test_zippermissions.js xpcshell test is failing with this patch.  I just tested locally, on Mac, and it fails there too.

The relevant stack from the log on those jobs looks like this:

22:11:40     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
22:11:40     INFO -  NS_ERROR_FILE_ACCESS_DENIED: Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIFileOutputStream.init]
22:11:40     INFO -  run_test@c:/slave/test/build/tests/xpcshell/tests/modules/libjar/zipwriter/test/unit/test_zippermissions.js:44:5
22:11:40     INFO -  _execute_test@c:\slave\test\build\tests\xpcshell\head.js:544:7
22:11:40     INFO -  @-e:1:1

and matches what I see locally.

Comment 22

9 months ago
So I've looked at this for awhile, and I can't figure out why that little bit of code is breaking that test in particular. I looked through the test code, and it seems a bit like the testcase. I tried moving around the bit of code I wrote before assertions in DoOpen(), MaybeOpen() and back in Init(). Create() never throws NS_ERROR_FILE_ACCESS_DENIED either, and neither do any of the macros in Init(), DoOpen(), MaybeOpen() afaik. Removing it in any of those functions completely resolves the error as expected. I imagine that the issue has to do with how the permissions work (because it's a test for opening a file with certain permissions to see how some other program reacts). 

I have to admit I'm pretty lost here. Can I get some guidance as to where to go from here?
If you have the time and inclination to try to figure out the test failure, I'd probably start with checking which exact init() call fails.  That is, what the value of "TESTS[i].permission" is for the failing test.

At a possible guess, we're creating the file without write permissions, so the attempt to open it throws.  If that's what's going on, the fact that we can normally open a writable stream to a new file created without write permissions is ... odd.

Anyway, if this is what's happening, the right fix, insofar as such a thing makes sense here, would in fact be to Remove() the file and document why we're doing it.

Comment 24

9 months ago
Created attachment 8878029 [details] [diff] [review]

All right, fourth time's the charm.

It turns out that what was happening was that the entire path was being created with the same permissions as the permission passed in (it would keep breaking on permission 0256). So, what I ended up doing was what I had submitted the first time, but with the permission 0644 for the entire directory path, deleted the last file created, and let the existing code do the rest. 

So far, it seems like it passes test_zippermissions.js and the attached testcase without any errors. Please let me know if there's anything else wrong, and thanks for all the help. This took me way too long to figure out.
Attachment #8877214 - Attachment is obsolete: true
> it would keep breaking on permission 0256

So "user can write but not read".  OK, that sounds plausible as a thing to fail a later open() on, if we try to stat it. :)

Instead of doing the create-and-remove thing, would it make sense to just get the parent and create that as DIRECTORY_TYPE and 0644?  That would avoid problems with umask that lead to unwritable parents causing the Remove to fail or other weirdness like that....

Comment 26

9 months ago
Created attachment 8878223 [details] [diff] [review]

Here we go. I rewrote it to not include the create-remove shenanigans. Hopefully this works. 

I had kept the create-remove sort of stuff since I figured that I'd have to do more with variables and checking stuff. I imagine it would take a bit more unnecessary work to do it that way though and this way does make more sense.

Comment 27

9 months ago
Comment on attachment 8878223 [details] [diff] [review]

>diff --git a/netwerk/base/nsFileStreams.cpp b/netwerk/base/nsFileStreams.cpp
>--- a/netwerk/base/nsFileStreams.cpp
>+++ b/netwerk/base/nsFileStreams.cpp
>@@ -320,16 +320,25 @@ nsFileStreamBase::DoOpen()
>     MOZ_ASSERT(mState == eDeferredOpen || mState == eUnitialized ||
>                mState == eClosed);
>     NS_ASSERTION(!mFD, "Already have a file descriptor!");
>     NS_ASSERTION(mOpenParams.localFile, "Must have a file to open");
>     PRFileDesc* fd;
>     nsresult rv;
>+    if (mOpenParams.ioFlags & PR_CREATE_FILE){
>+        nsIFile *parent = nullptr;
>+        mOpenParams.localFile->GetParent(&parent);
>+        // Result doesn't need to be checked. If the file's parent path does not exist, make it. If it does exist, do nothing.
>+        if (parent != nullptr)
>+            Unused >> parent->Create(nsIFile::DIRECTORY_TYPE, 0644);
>+    }  
> #ifdef XP_WIN
>     if (mBehaviorFlags & nsIFileInputStream::SHARE_DELETE) {
>       nsCOMPtr<nsILocalFileWin> file = do_QueryInterface(mOpenParams.localFile);
>       MOZ_ASSERT(file);
>       rv = file->OpenNSPRFileDescShareDelete(mOpenParams.ioFlags,
>                                              mOpenParams.perm,
>                                              &fd);
>+        nsIFile *parent = nullptr;
>+        mOpenParams.localFile->GetParent(&parent);

This actually leaks.  nsIFile objects are reference-counted.  The getter increments the reference count, but the caller is expected to decrement it when done.

You want something like this:

  nsCOMPtr<nsIFile> parent;

and then the nsCOMPtr destructor will drop the refcount when it goes out of scope.

>+        if (parent != nullptr)

The normal style for this code is to just do "if (parent)".

I'll send off a try run of this patch with those two modifications, pluse the Unused comment 27 points out.
With those modifications, tests look good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4025033d6e45641f53b76f82a1dc15746411cb3 -- I believe all those remaining failures are known intermittents.

Ben, how would you like your name/email to appear in the changeset?  Is "Ben <bchau3440@gmail.com>" what you want, or something else?

Comment 30

9 months ago
Sounds good, I don't mind too much. Thanks again.

Comment 31

9 months ago
Pushed by bzbarsky@mozilla.com:
Trying to open a file in MODE_CREATE via an XPCOM file stream in a directory that does not exist should crete the directory too, not just the file.  r=bzbarsky
Ben, thank you for sticking with this!

Comment 33

9 months ago
Last Resolved: 9 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56


8 months ago
See Also: → bug 1385816
You need to log in before you can comment on or make changes to this bug.