Open Bug 1136620 Opened 6 years ago Updated 4 years ago

-profile option does not create a new directory on linux if the directory does not exist

Categories

(Toolkit :: Startup and Profile System, defect)

All
Linux
defect
Not set
normal

Tracking

()

People

(Reporter: hiro, Unassigned)

References

Details

Attachments

(1 file)

From realpath's man:

   GNU extensions
       If the call fails with either EACCES or ENOENT and resolved_path is not NULL, then the prefix of path that is not readable or does not exist is returned in resolved_path.

<snip>
The
       resolved_path == NULL feature, not standardized in POSIX.1-2001, but standardized in POSIX.1-2008, allows this design problem to be avoided.


So we can use resolved_path if errno is ENOENT.
Attachment #8569071 - Flags: review?(benjamin)
Comment on attachment 8569071 [details] [diff] [review]
realpath_fix.patch

I'm not convinced that this is the behavior we want. I expected Firefox to fail to launch (return a nonzero exit code) if we use -profile with a path that doesn't exist. Unless there is a compelling reason to auto-create the directory, I'd prefer to make that explicitly illegal.
Flags: needinfo?(hiikezoe)
(And this has nothing to do with XPCOM)
Component: XPCOM → Startup and Profile System
Product: Core → Toolkit
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> Comment on attachment 8569071 [details] [diff] [review]
> realpath_fix.patch
> 
> I'm not convinced that this is the behavior we want. I expected Firefox to
> fail to launch (return a nonzero exit code) if we use -profile with a path
> that doesn't exist. Unless there is a compelling reason to auto-create the
> directory, I'd prefer to make that explicitly illegal.

FWIW, on Windows and OS X, we do what the proposed patch would add to Linux, too. I know that quite a few people use it to create throwaway-profiles like so: firefox -profile /tmp/somerandomname.
(In reply to Till Schneidereit [:till] from comment #4)
> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> > Comment on attachment 8569071 [details] [diff] [review]
> > realpath_fix.patch
> > 
> > I'm not convinced that this is the behavior we want. I expected Firefox to
> > fail to launch (return a nonzero exit code) if we use -profile with a path
> > that doesn't exist. Unless there is a compelling reason to auto-create the
> > directory, I'd prefer to make that explicitly illegal.
> 
> FWIW, on Windows and OS X, we do what the proposed patch would add to Linux,
> too. I know that quite a few people use it to create throwaway-profiles like
> so: firefox -profile /tmp/somerandomname.

Also worth noting that we have stuff in-tree that depends on this:

http://mxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/mach_commands.py#940
So, which behavior should firefox take in case of -profile option?

a) create the directory if not exist
b) just abort

Current implementation is inconsistency between Linux and others. I will provide a patch to fix the inconsistency.

I know some companies use -profile option for daily use on Windows, and they rely on a). If firefox take b), they will need a script which creates directory on first launch of firefox.
Flags: needinfo?(hiikezoe)
Comment on attachment 8569071 [details] [diff] [review]
realpath_fix.patch

ok, given that mach depends on this behavior, I guess we should support using -profile for a directory that doesn't yet exist.

What happens if the path is relative? I feel like this will still fail:

firefox -profile ../foo-notpresent

Because realpath isn't making the path absolute. The realpath docs say: "If the call fails with either EACCES or ENOENT and resolved_path is not NULL, then the prefix of path that is not readable or does not exist is returned in resolved_path. " Which is either confusing or will produce incorrect results in this case.
Flags: needinfo?(hiikezoe)
That works.

From the man of realpath.

realpath()  expands  all  symbolic  links and resolves references to /./, /../ and extra '/' characters in the null-terminated string named by path to produce a canonicalized absolute pathname.
Flags: needinfo?(hiikezoe)
The man page is talking about what happens when realpath succeeds. But this patch deals with the case where realpath fails (with ENOENT). I was quoting the manpage for that case in comment 7.
Flags: needinfo?(hiikezoe)
I am sorry for my misunderstanding. Actually I did try relative path against non-existent directory with atachment #8569071 when I commented comment #8. And now I tried it again for confirmation, it worked!
Flags: needinfo?(hiikezoe)
I still don't understand *why* it works. Can you explain what happens to the buffers in question when we call realpath() on a path that doesn't exist and it returns ENOENT? Is that behavior well-defined or accidental? Is it a POSIX behavior or Linux-only, and will this cause problems on the BSDs or even perhaps on mac?

I feel like the man page is saying that the behavior is not what we actually want, which is why I'm concerned about compatibility and correctness here.
Flags: needinfo?(hiikezoe)
Comment on attachment 8569071 [details] [diff] [review]
realpath_fix.patch

Ah, the behavior is GNU extension, will not work on BSD.
Flags: needinfo?(hiikezoe)
Attachment #8569071 - Flags: review?(benjamin)
Any progress?
You need to log in before you can comment on or make changes to this bug.