Bugzilla

(In reply to nihal from comment #25)
> (In reply to :Gijs (he/him) from comment #24)
> > (In reply to nihal from comment #21)
> > > This problem has been bothering me for some time.
> > 
> > Out of interest, do you also see bug 922719 or should we mark that as works-for-me?
> > 
> > (In reply to nihal from comment #23)
> > > I just removed the creation code from `GetUnixXDGUserDirectory` and it doesn't appear to break anything. If Desktop doesn't exist, it will just open the home directory instead.
> > 
> > This would potentially re-break bug 1737926 in that downloads will just fail if the default is set to a directory that doesn't exist, depending on the system in question. It might also (depending on e.g. the filepicker code in this example) cause issues with other consumers of the directory service that look up the desktop dir and assume it then exists, e.g. https://searchfox.org/mozilla-central/rev/abf6758ed833c203f84703aa2e3e3d317571b1e9/toolkit/content/contentAreaUtils.js#698 .
> > 
> 
> True, but in these cases we can just check whether the directory exists and create it

Except we can't always do that, when we fetch the directory in the content process, as that is sandboxed. This _might_ happen in https://searchfox.org/mozilla-central/source/dom/html/HTMLInputElement.cpp#357 , but I'm not 100% sure (would need to work out if that code is running in the parent process or child - I _think_ child, as callers at e.g. https://searchfox.org/mozilla-central/rev/abf6758ed833c203f84703aa2e3e3d317571b1e9/dom/html/HTMLInputElement.cpp#545 seem to live in the child, given that they have direct element references to `input`s in the page). So in that case we'd probably need to fall back to the home dir. I think that's fine, but worth bearing in mind. In fact, it may be preferable to fall back to $HOME anyway.

> when we actually need to save something to it (in Javascript). If I understand correctly, the download failing would also happen if the user sets their download directory and then deletes it. If we create the directory when it is actually needed, this would solve that problem to.

This is what bug 1737926 did for the download case. We would now have to audit and do this for all the relevant consumers of the `"Desk"` directory service, the relevant `#define` ( https://searchfox.org/mozilla-central/search?q=symbol:M_b890687da901ef14&redirect=false ), and any of the C++ consumers that end up calling the getter you identified through other call paths (I think there aren't any but it's possible I missed something, there's... more indirection than I would consider ideal, if I were designing this from scratch...).

> So I think the best solution is the following: remove the existence check and creation code from `GetUnixXDGUserDirectory`, and instead perform the existence check and creation right before the directory is opened in the file picker. There are not that many areas where this happens, so it seems pretty easy to do. Does this sound reasonable to you?

It sounds reasonable. It's more work than the other alternative you outlined, but probably more "correct" longer term.

The other option would be to fall back silently to just $HOME in the `GetUnixXDGUserDirectory` code, rather than pushing that responsibility to the consumers. That would be a smaller patch and would also solve the issue (assuming you're not running some... peculiar... kind of unix env where $HOME doesn't exist?!).

(FWIW, I'm also still curious about what the fate of bug 922719 should be :-) )

(In reply to nihal from comment #25)
> (In reply to :Gijs (he/him) from comment #24)
> > (In reply to nihal from comment #21)
> > > This problem has been bothering me for some time.
> > 
> > Out of interest, do you also see bug 922719 or should we mark that as works-for-me?
> > 
> > (In reply to nihal from comment #23)
> > > I just removed the creation code from `GetUnixXDGUserDirectory` and it doesn't appear to break anything. If Desktop doesn't exist, it will just open the home directory instead.
> > 
> > This would potentially re-break bug 1737926 in that downloads will just fail if the default is set to a directory that doesn't exist, depending on the system in question. It might also (depending on e.g. the filepicker code in this example) cause issues with other consumers of the directory service that look up the desktop dir and assume it then exists, e.g. https://searchfox.org/mozilla-central/rev/abf6758ed833c203f84703aa2e3e3d317571b1e9/toolkit/content/contentAreaUtils.js#698 .
> > 
> 
> True, but in these cases we can just check whether the directory exists and create it

Except we can't always do that, when we fetch the directory in the content process, as that is sandboxed. This _might_ happen in https://searchfox.org/mozilla-central/source/dom/html/HTMLInputElement.cpp#357 , but I'm not 100% sure (would need to work out if that code is running in the parent process or child - I _think_ child, as callers at e.g. https://searchfox.org/mozilla-central/rev/abf6758ed833c203f84703aa2e3e3d317571b1e9/dom/html/HTMLInputElement.cpp#545 seem to live in the child, given that they have direct element references to `input`s in the page). So in that case we'd probably need to fall back to the home dir. I think that's fine, but worth bearing in mind.

In fact, it may be preferable to fall back to $HOME anyway.

> when we actually need to save something to it (in Javascript). If I understand correctly, the download failing would also happen if the user sets their download directory and then deletes it. If we create the directory when it is actually needed, this would solve that problem to.

This is what bug 1737926 did for the download case. We would now have to audit and do this for all the relevant consumers of the `"Desk"` directory service, the relevant `#define` ( https://searchfox.org/mozilla-central/search?q=symbol:M_b890687da901ef14&redirect=false ), and any of the C++ consumers that end up calling the getter you identified through other call paths (I think there aren't any but it's possible I missed something, there's... more indirection than I would consider ideal, if I were designing this from scratch...).

> So I think the best solution is the following: remove the existence check and creation code from `GetUnixXDGUserDirectory`, and instead perform the existence check and creation right before the directory is opened in the file picker. There are not that many areas where this happens, so it seems pretty easy to do. Does this sound reasonable to you?

It sounds reasonable. It's more work than the other alternative you outlined, but probably more "correct" longer term.

The other option would be to fall back silently to just $HOME in the `GetUnixXDGUserDirectory` code, rather than pushing that responsibility to the consumers. That would be a smaller patch and would also solve the issue (assuming you're not running some... peculiar... kind of unix env where $HOME doesn't exist?!).

(FWIW, I'm also still curious about what the fate of bug 922719 should be :-) )