434327 - opening "main" preferences pane creates ~/Desktop even when not set to download directory

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[Pouya K]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9) Gecko/2008051807 Firefox/3.0
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9) Gecko/2008051807 Firefox/3.0

When opening the preferences dialogue and going to the main pane, a ~/Desktop directory is created. I also tested with the "mozilla-firefox-bin-3.0_rc1" in gentoo, which is the version that is released by mozilla. The problem persisted. 

Reproducible: Always

Steps to Reproduce:
1. Open preferences 
2. Goto "Main" pane
Actual Results:  
~/Desktop created

Expected Results:  
There should be no ~/Desktop

Comment 1

[Jan Darmochwal]

I can confirm this with a recent trunk build.

Comment 2

[Jan Darmochwal]

This is strange:
Yesterday, this happened every time I tried. Now, I'm unable to reproduce it at all.

Comment 3

[Pouya K]

Are you switching to the "Main" preference pane? For me it only occurs on that pane (and not the other ones, e.g. Security or Advanced). 

Comment 4

[Jan Darmochwal]

Yes, this happened when switching to the "Main" pane. It only happened the first time I switched to the "Main" pane:
  * Edit -> Preferences
  * go to "Main" pane, if not already there (creates ~/Desktop)
  * rm -r ~/Desktop
  * go to other pane
  * go to "Main" pane (does not create ~/Desktop)

That was yesterday. Today I have tried to investigate the bug, but I can't even reproduce it any more.

Comment 5

[Pouya K]

weird... I can still consistently produce it. 

Comment 6

[Warner Young]

I'm seeing this with the recent trunk nightlies in Windows.

Comment 7

[Ken Bloom]

Could someone with permission to edit things mark #482662 and #479191 as duplicates of this bug?

Also, this is Debian bug #487842

Comment 10

[Pouya K]

Note: this same behavior occurs for me when switching to the "Attachments" pane in Thunderbird 3.0 Beta 3: Bug #511271

Comment 11

[Kevin Brosnan]

To avoid this happening  create a text file ~/.config/user-dirs.dirs and in it place 

XDG_DESKTOP_DIR="/home/linux_user/"

Where linux_user is your user name.

Other XDG_*_DIR settings
XDG_DOWNLOAD_DIR="/home/linux_user/downloads"
XDG_TEMPLATES_DIR="/home/linux_user/"
XDG_PUBLICSHARE_DIR="/home/linux_user/"
XDG_DOCUMENTS_DIR="/home/linux_user/docs"
XDG_MUSIC_DIR="/home/linux_user/music"
XDG_PICTURES_DIR="/home/linux_user/pics"
XDG_VIDEOS_DIR="/home/linux_user/vids"

References http://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html and http://support.mozilla.com/tiki-view_forum_thread.php?locale=en-US&comments_parentId=428359&forumId=1

Comment 13

[Zack Weinberg (:zwol)]

It seems to me that we should only create the default download directory if and when we are actually going to save a file into it.

Comment 14

[WADA:World Anti-bad-Duping Agency]

As written in "Unable to save or download files"(an MozillaZine Knowledge Base document), download location, download location selection, is affeced by next setting.  
> http://kb.mozillazine.org/Unable_to_save_or_download_files#Reset_download_folder
>   * browser.download.dir
>   * browser.download.downloadDir
>   * browser.download.folderList
>   * browser.download.lastDir
>   * browser.download.useDownloadDir
See next document for each setting.
> https://developer.mozilla.org/en/Download_Manager_preferences
> Note: browser.download.downloadDir is not seen in this document and about:config)

Sufficiantly confusing, but roughly next;
> browser.download.useDownloadDir=false : Always Ask
> browser.download.useDownloadDir=true  : Save file to
>   - browser.download.folderList=0 :
>       indicates the Desktop; (Default of Fx 3.0 or earlier, and Tb) 
>   - browser.download.folderList=1 :
>       indicates the systems default downloads location; (Default of Fx 3.5 or later, and Seamonkey) 
>   - browser.download.folderList=2 :
>       indicates a custom (see: browser.download.dir) folder.
>   If existent folder is selected via UI at "Save file to",
>      browser.download.folderList is changed to 2, and, pointer to selected
>      folder is saved in browser.download.dir.         
>   Once browser.download.folderList=2 is set, it wont't be changed via UI. 

Above is easily be observed by reset of all five prefs.js settings via about:config/Config Editor, change download setting at Preference/Options UI, and check above five settings via about:config/Config Editor.

If browser.download.folderList=0 is set, a ~/Desktop directory may be created.
Next is probably easiest recovery procedure when a ~/Desktop directory was created.
  Select a directory via UI(force folderList=2), if you want to use
  "Save file to"(useDownloadDir=true), and delete created ~/Desktop.
Another workaround is;
  browser.download.folderList=1, if system's download locationsetting can be
  used. If system's download directory setting can not be used, set it to one
  you want, like comment #11.
  (if Mac OS X, it seems to depend on OS version and environment.)

(off-topic)
Because one of next is used as fall back directory when specified directry(for example, browser.download.lastDir) is not found(or no write permission, ....),
  (a) browser.download.dir (currently, this is probably used), 
  (b) most recently used directory,
  (c) current directory(working directory),
browser.download.dir shouldn't be set to directory on USB memory or removable drive. If you want to set directory on network file server in browser.download.dir, please take care for network error or server down.

Comment 18

[[:Aleksej]]

It is being created on start-up now: bug 922719.  Maybe that's because of Reset Firefox?

Comment 19

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S4. However, the bug has 6 duplicates.
:mak, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 20

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

The last needinfo from me was triggered in error by recent activity on the bug. I'm clearing the needinfo since this is a very old bug and I don't know if it's still relevant.

Comment 21

[nihal]

This problem has been bothering me for some time. I finally decided to figure out what was happening and I think I found the root cause. I'd be happy to send a patch if I could get some guidance on what an appropriate solution is.

First, using breakpoints, I found that the Desktop folder is created on this function call:
https://searchfox.org/mozilla-central/rev/f24503d2f5d8b7e4341be8bc20d94a04a6b223d6/browser/components/preferences/main.js#3057

let desktopDir = await this._getDownloadsFolder("Desktop");

This is used if the user manually sets the system Desktop directory as their Download directory. If it is set, then a locale-specific name is used for the Desktop directory
_getDownloadsFolder in turn calls Services.dirsvc.get("Desk", Ci.nsIFile);. This seems to call the C++ function

https://searchfox.org/mozilla-central/rev/f24503d2f5d8b7e4341be8bc20d94a04a6b223d6/xpcom/io/nsDirectoryService.cpp#439

rv = GetSpecialSystemDirectory(Unix_XDG_Desktop, getter_AddRefs(localFile));

On Linux, this eventually calls the function GetUnixXDGUserDirectory which runs the following line if the folder doesn't exist:
https://searchfox.org/mozilla-central/rev/f24503d2f5d8b7e4341be8bc20d94a04a6b223d6/xpcom/io/SpecialSystemDirectory.cpp#428

rv = file->Create(nsIFile::DIRECTORY_TYPE, 0755);

There seem to be two things that could be considered causes of this problem

1. the function GetSpecialSystemDirectory that purports to just get the path to a directory should not also create it
2. _getDownloadsFolder should not be using a function that creates a directory to get the name

In the latter case, I see 2 solutions:

• Modify the called C++ to do the right thing. This could be either adding a flag which indicates whether the call to GetSpecialSystemDirectory should make the directory or making new functions entirely that don't create the directory
• Modify the Javascript to set config values: when the user changes the download directory, we check if it is the system Desktop or Downloads directory. If it is, then we set browser.download.folderList to 0 or 1, respectively. If we do that, then when browser.download.folderList is set to 2, it is unnecessary to check whether the path is a system default path. If user modifies it manually (outside of preferences), they probably know what they're doing and don't need the full path of the folder hidden from them.

I prefer the latter because it is less invasive.

Comment 22

[nihal]

Actually it looks like the Javascript already modifies browser.download.folderList when a new Downloads directory is selected, so I'm confused why calling this._getDownloadsFolder("Desktop") is necessary at all in _getSystemDownloadFolderDetails, since if browser.download.folderList is 2, the full path should be displayed.

Comment 23

[nihal]

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. And of course ~/Desktop no longer gets created when about:preferences is opened.

Comment 24

[:Gijs (he/him)]

(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 .

(In reply to nihal from comment #21)

In the latter case, I see 2 solutions:

• Modify the called C++ to do the right thing. This could be either adding a flag which indicates whether the call to GetSpecialSystemDirectory should make the directory

Adding flags in this case would mean passing a bool through a bunch of callstack, which means changing a bunch of functions, including ones that are generic (i.e. the directory service), where you would then have to implement support for the flag across all the x-platform directory getters (tedious and error-prone, as some may want different defaults than others), or the flag would be oddly specific (generic "getFile" method with optional bool "aDontCreateDesktopDir"), or the flag would not do what it says on the tin (opt in/out of creating a directory) for the vast majority of directories. None of those seem appealing.

or making new functions entirely that don't create the directory

This seems similarly tedious for similar reasoning... We could add an alias to the dir service (e.g. DeskNoCreate) and then implement that, I guess?

• Modify the Javascript to set config values: when the user changes the download directory, we check if it is the system Desktop or Downloads directory. If it is, then we set browser.download.folderList to 0 or 1, respectively. If we do that, then when browser.download.folderList is set to 2, it is unnecessary to check whether the path is a system default path.

AIUI if we do this we would still create the desktop directory whenever the user changes the value here, right? I guess that is better than the status quo...

Comment 25

[nihal]

(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 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.

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?

Comment 26

[:Gijs (he/him)]

(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 inputs 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 :-) )

Comment 27

[nihal]

OK, I agree that $HOME is a more reasonable fallback on Linux, I'll upload a patch that fixes this soon.

I still think it makes more sense for the directories to be created lazily when necessary, but from what you said, it seems that sandboxing might make it more harder than I thought. Maybe this is a patch for another day :)

With regard to bug 922719, I cannot reproduce it.

Comment 28

[nihal]

Attachment: Bug 434327 - Fallback to $HOME in absence of $HOME/Desktop r?#settings-reviewers

There are many Linux systems where $HOME/Desktop doesn't exist, so falling
back to $HOME, which is treated as the "default" by other programs anyway,
makes more sense.

This prevents the creation of $HOME/Desktop every time about:preferences
is opened.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b6260907be11
Fallback to $HOME in absence of $HOME/Desktop r=settings-reviewers,Gijs

Comment 30

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/b6260907be11