992185 - Download location preference field is empty on reload

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[Manish Goregaokar [:manishearth]]

See bug 738796 comment 82:

> Under General -> Downloads -> “Save files to” option selected, I see the next issue: if I selected any other folder: “/home/your_name/Folder_Name” appears. After reloading the tab, nothing appears.

So by default, the download location field shows "Downloads". However, when you change the directory to anything other than Downloads, the field shows the full path (probably not desired). When the preferences page is reloaded, the field is just empty.

Comment 1

[Manish Goregaokar [:manishearth]]

Alright, I was able to fix the full path thing.

However, the emptyness issue is strange. I tracked it down to the `value` of the <preference> element being null on page load. This is strange, since I do see a value for the pref about:config.

When I looked at about:config, the pref was being stored as a string instead of as a "file": http://i.stack.imgur.com/DnTnL.png (chaîne is French for "string"). This might have something to do with the null value.

Comment 2

[Manish Goregaokar [:manishearth]]

Attachment: Patch 1

Figured it out.

The downloads field now loads correctly. It also no longer shows the cramped, full path, and only shows the folder name.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8402128 [details] [diff] [review]
Patch 1

>diff --git a/browser/components/preferences/in-content/main.js b/browser/components/preferences/in-content/main.js

>   _getDisplayNameOfFile: function (aFolder)

>-    return aFolder ? aFolder.path : "";
>+    return aFolder ? aFolder.leafName : "";

Is this really wanted? Not sure that showing only the leafName makes sense, makes it hard to see what you picked. Regardless, should probably be done separately from your bugfix. 

>diff --git a/browser/components/preferences/in-content/main.xul b/browser/components/preferences/in-content/main.xul

>+    <preference id="browser.download.folderList"
>+                name="browser.download.folderList"
>+                type="int"/>
>     <preference id="browser.download.dir"
>                 name="browser.download.dir"
>                 type="file"
>                 onchange="gMainPane.displayDownloadDirPref();"/>
>-    <preference id="browser.download.folderList"
>-                name="browser.download.folderList"
>-                type="int"/>

Why does this fix the weirdness?

Comment 4

[Manish Goregaokar [:manishearth]]

(In reply to :Gavin Sharp (email [e-mail]) from comment #3)

> Is this really wanted? Not sure that showing only the leafName makes sense,
> makes it hard to see what you picked. Regardless, should probably be done
> separately from your bugfix. 

Well, this was mentioned in Camelia's comment on the main bug. But yeah, this can be filed separately.


> Why does this fix the weirdness?

I'm not completely sure. It works, though.

As far as I can tell, when the tags are in the opposite order, and the onsyncfrompreference for forderList here[1] is called, the folderList preference has been loaded, but the dir preference has not, and calling the value in the method doesn't work.


 [1]: http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/main.xul#173

Comment 5

[Manish Goregaokar [:manishearth]]

Filed bug 992456 for the full path vs leaf name issue.

Comment 6

[Manish Goregaokar [:manishearth]]

Attachment: Patch 2 - split into two bugs

Comment 7

[:Gavin Sharp [email: [e-mail]]]

As I understand things, the preferences should both be initialized before any onsyncfrompreference handlers are called. Is that not happening?

Comment 8

[Manish Goregaokar [:manishearth]]

As far as I can tell, it's not happening. Put a debugger; at the top of `displayDownloadDirPref()` in main.js and have a look at `document.getElementById('browser.download.dir').value` both with and without the patch. The <preference> element has a null value when the patch isn't applied. I'm not sure if that's by design -- it *shouldn't* be, but I'm not familiar with the code behind preferences. We can file a separate bug about this if necessary.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to Manish Goregaokar [:manishearth] from comment #8)
> As far as I can tell, it's not happening. Put a debugger; at the top of
> `displayDownloadDirPref()` in main.js and have a look at
> `document.getElementById('browser.download.dir').value`

What does the call stack to displayDownloadDirPref look like when the error case occurs? I.e. which of the two displayDownloadDirPref calls in mail.xul are the problem, onchange or onsyncfrompreference?

Comment 10

[Manish Goregaokar [:manishearth]]

It seems to be onsyncfrompreference

Call stack:

gMainPane.displayDownloadDirPref
anonymous (preferences.xml:1)
setElementValue (preferences.xml:403)
updateElements (preferences.xml:524)
_setValue (preferences.xml:168)
preference_XBL_constructor (preferences.xml:118)

setElementValue contains:

> var event = document.createEvent("Events");
> event.initEvent("syncfrompreference", true, true);
> var f = new Function ("event", aElement.getAttribute("onsyncfrompreference"));
> rv = f.call(aElement, event);

near line 403. 

Also, the contents of updateElements:

> var elements = document.getElementsByAttribute("preference", this.id);
> for (var i = 0; i < elements.length; ++i) 
>  this.setElementValue(elements[i]);


So basically, the moment a <preference> object is constructed, all form controls on the page linked with that pref get instantiated too (either by copying the preference over, or by firing onsyncfrompreference). In this case, the latter is happening.


I guess this means that referring to preferences by id in an onsyncfrompreferences handler is not good, we should just load them via Services.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Cool, thanks for digging deeper - that makes sense. Do you want to adjust the patch to avoid using the pref element, then, and maybe add a clarifying comment?

Comment 12

[:Gavin Sharp [email: [e-mail]]]

(I'm surprised we don't run into this issue more often. Maybe we've just been lucky?)

Comment 13

[Manish Goregaokar [:manishearth]]

I'm surprised we haven't run into this either, since the trick of referencing a <preference> element is used in almost all the onsyncfrompreferences, and it's rather convenient over using get*Pref.

I plan on adding a note to the MDN page that explicitly mentions this behavior of onsyncfrompreference.

I'm not sure if we should fix this by replacing it with a direct call to getComplexValue(), since the rest of the code seems to rely on precise ordering of <preference> elements anyway. I'll upload a patch for this case, though.

I think we should just swap the order for now, and then try and investigate ways of fixing onsyncfrompreference's strangeness in a separate bug. I haven't thought much about it yet, but there doesn't seem to be a straightforward alternate behavior for it, so this might require some discussion.

Comment 14

[Manish Goregaokar [:manishearth]]

Attachment: Alternate patch that does away with referencing the preference element

Here's the alternate patch, choose the one you like more :)

Comment 15

[Manish Goregaokar [:manishearth]]

Attachment: Alternate patch that does away with referencing the preference element

Whoops, forgot to qref.

Comment 16

[Manish Goregaokar [:manishearth]]

I've left a note on the MDN page here: https://developer.mozilla.org/en-US/docs/Mozilla/Preferences/Preferences_system/New_attributes#onsyncfrompreference.2Fonsynctopreference

Let me know if it can be improved :)

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8402140 [details] [diff] [review]
Patch 2 - split into two bugs

(clearing review request since Gavin has been working closer with you on this)

Comment 18

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8402140 [details] [diff] [review]
Patch 2 - split into two bugs

I changed my mind, let's use this for now. This seems like a huge footgun but I'm not sure how to solve it offhand. Perhaps file a bug about this general problem (to audit for other such situations, find a global solution, etc.)?

Comment 19

[Manish Goregaokar [:manishearth]]

Alright, thanks.

I'll file a detailed bug in a while. A fix might require some radical changes to the internals, though.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0820dbc1ab42

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0820dbc1ab42

Comment 22

[Camelia Badau [:cbadau], Desktop Test Engineering]

Verified fixed on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.7.5 using Nightly 31.0a1 (buildID: 20140428030203).