277367 - 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

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[timeless]

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

[timeless]

Attachment: testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

Confirming.  The api certainly implies that the file will be created...

Comment 3

[Ben]

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?

Comment 4

[Boris Zbarsky [:bzbarsky]]

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

[Ben]

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> 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

[Ben]

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!

Comment 8

[Boris Zbarsky [:bzbarsky]]

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

[Ben]

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"?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Normally MODE_CREATE would create the file itself, so I think you want to create all of the above.

Comment 11

[Ben]

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

nsString path;
file->GetPath(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(); 
    file->InitWithPath(path); 
    try {
        nsIFile.Create(DIRECTORY_TYPE, perm);
    } catch (NS_ERROR_FILE_ALREADY_EXISTS) {

    }
    
    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?

Comment 12

[Boris Zbarsky [:bzbarsky]]

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

[Ben]

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?

Comment 14

[Boris Zbarsky [:bzbarsky]]

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

[Ben]

Attachment: 277367-create-dir.patch

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

[Ben]

Attachment: attachment.js

A working version of the test case previously posted on this bug.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8876542 [details] [diff] [review]
277367-create-dir.patch

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

[Ben]

Attachment: 277367-create-dir.patch

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!

Comment 19

[Boris Zbarsky [:bzbarsky]]

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

[Ben]

Attachment: 277367-create-dir.patch

How's this?

Comment 21

[Boris Zbarsky [:bzbarsky]]

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

[Ben]

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?

Comment 23

[Boris Zbarsky [:bzbarsky]]

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

[Ben]

Attachment: 277367-create-dir.patch

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.

Comment 25

[Boris Zbarsky [:bzbarsky]]

> 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

[Ben]

Attachment: 277367-create-dir-v2.patch

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

[Ben]

Comment on attachment 8878223 [details] [diff] [review]
277367-create-dir-v2.patch

>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);

Comment 28

[Boris Zbarsky [:bzbarsky]]

>+        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;
  mOpenParams.localFile->GetParent(getter_AddRefs(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.

Comment 29

[Boris Zbarsky [:bzbarsky]]

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 <[e-mail]>" what you want, or something else?

Comment 30

[Ben]

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

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/17af61e163bd
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

Comment 32

[Boris Zbarsky [:bzbarsky]]

Ben, thank you for sticking with this!

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/17af61e163bd