Closed Bug 997570 Opened 10 years ago Closed 10 years ago

Onsyncfrompreference sensitive to order of preference elements

Categories

(Toolkit :: Preferences, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: manishearth, Assigned: manishearth)

References

Details

Attachments

(2 files, 6 obsolete files)

While investigating bug 992185 I found out that `onsyncfrompreference` is implemented a bit strangely. When the `value` field of a <preference> element is loaded, all the `onsyncfrompreference` handlers for the associated form widgets are triggered. (Traceback: element constructor calls[1] `_setValue()` which calls[2] `updateElements()` which calls `setElementValue()` on each associated element[3], which triggers the `onsyncfrompreference` for each[4])

Now, a common (efficient) practice in preference code is to load preference values by taking the `value` from the associated <preference> element (obtained via `getElementById`), as is done here[5]

However, if this method is used, we can have a case where the onsyncfrompreference handler tries to load the value of an uninitialized <preference> object. This simply depends on the order of the <preference> objects in the <preferences> area, each one has its constructor called in sequence, and until the constructor is called, the `value` of the element will be null.

This is rather off, the order of non-display elements like <preference> should have no effect whatsoever in behavior, yet we have cases like bug 992185 where just swapping two adjacent elements solved the bug. I'm rather surprised that this hasn't broken anything else on preferences, since the method of using `value`s of other <preference> elements is rather widespread, including in `onsyncfrompreference` calls.



One possible fix for this is to not touch any form widgets from the <preference> constructor, instead we do it as part of the <preferences> initialization. 

 [1]: http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/preferences.xml#129
 [2]: http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/preferences.xml#183
 [3]: http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/preferences.xml#537
 [4]: http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/preferences.xml#411
 [5]: http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/main.js#286
After a preliminary glance, the proposed fix from comment 0 is a one line change:

`value` -> `_value` here[1], maybe with some additional validation. Currently that line triggers a getter that propagates to onsyncfrompreference.

Gavin, do you agree with the proposed fix? This *will* result in a change of behavior, though it hopefully won't break anything.


 [1]: http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/preferences.xml#42
Flags: needinfo?(gavin.sharp)
Not sure I understand why that observer is related - that's the code that updates the pref dialog if a pref changes while it is open, as I understand it?

> One possible fix for this is to not touch any form widgets from the <preference>
> constructor, instead we do it as part of the <preferences> initialization. 

This sounds right, if it's feasible - I don't understand all of the constraints.
Flags: needinfo?(gavin.sharp)
Update: The <constructor> of a <preferences> element is called *before* that of its containing <preference> elements. So the easy way out here doesn't work, I'll have to have the parent element keep track of all of its children.
Attached patch Test solution (obsolete) — Splinter Review
Alright, the problem seems to be fixed. Try it out on a pre-bug 992185 system (basically, the order of the two  download preference elements in in-content/main.xul should be reversed, see the patch)

Note that the download folder display loads, regardless of the order of preference elements.

There *is* a debugger call in the current patch (I'll remove it if this gets f+). Open up the browser toolbox, and check that currentDirPref.value is non null at the breakpoint even on swapping the preference elements.


What this does is that it directly stores the value without calling any setters in the <preference> constructor. Every time a <preference> element is constructed, the parent element checks if there are any remaining un-constructed contained preference elements and fires updateElements() (the method which propagates preference values to the form widgets, directly or via onsyncfrompreference) if there aren't.

It doesn't seem to break any functionality.
Attachment #8409512 - Flags: feedback?(gavin.sharp)
Status: NEW → ASSIGNED
Comment on attachment 8409512 [details] [diff] [review]
Test solution

The general approach seems sound, though I have not looked into whether this might break any other invariants. Someone else closer to the preferences code should probably review - perhaps MattN, or jaws, or Neil.

>diff --git a/toolkit/content/widgets/preferences.xml b/toolkit/content/widgets/preferences.xml

>   <binding id="preferences">
>     <implementation implements="nsIObserver">
>+      <method name="_constructAfterChildren">
>+      <body>
>+      <![CDATA[
>+      var elements = this.getElementsByTagName("preference");
>+      for (element of elements){
>+        if (!element._constructed) {
>+        	return;
>+        }
>+        element.updateElements();
>+      }
>+      for (element of elements){
>+        element.updateElements();
>+      }

Looping twice over "elements" is not optimal, I assume you'll fix that before requesting review.

>-          this._setValue(preference ? preference.value 
>-                                    : this.valueFromPreferences, false);
>+          this._value = preference ? preference.value : this.valueFromPreferences;
>         }
>         else
>-          this._setValue(this.valueFromPreferences, false);
>+          this._value = this.valueFromPreferences;
>+        this.parentNode._constructAfterChildren();

AFAICT this should be "this.preferences._constructAfterChildren()". As I understand it it's already guaranteed that this.preferences is non-null (and preferences-less <preference> elements are already broken).

I don't have much confidence in our test coverage for this stuff - are there any tests for preference panes in general? We should at least write some to cover this change...
Attachment #8409512 - Flags: feedback?(gavin.sharp) → feedback+
Attached patch Final patch (obsolete) — Splinter Review
Fixed nits. Matt, could you have a look at this? If the exact issue is unclear feel free to ping me in IRC.

Testcase: Revert this patch[1]  and see if bug 992185 still exists.

 [1]: https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=992185&attachment=8402140
Attachment #8409512 - Attachment is obsolete: true
Attachment #8412265 - Flags: review?(MattN+bmo)
Attached patch prefs.patch (obsolete) — Splinter Review
Oops, forgot to remove the debugger call.
Attachment #8412265 - Attachment is obsolete: true
Attachment #8412265 - Flags: review?(MattN+bmo)
Attachment #8412268 - Flags: review?(MattN+bmo)
Hey Manish, this seems complicated enough that it would be good to have a test for it. Is that feasible or is there a race condition? It's going to take me a bit to understand everything that's going on and the added benefit of a test is that it clearly demonstrates the problem so I don't have to try and parse English to construct the problem scenario in my head.
Flags: needinfo?(manishearth)
Yes, creating a test shouldn't be hard. I just have to create a simple <preference> pane with a couple of <preference> objects and linked input fields, then add some cross-referencing onsyncfrompreference logic to each.


Basically, create a function like this:

function checkPrefs() {
 let prefs = ["testpref.pref1", "testpref.pref2", "testpref.pref3"];
 for (pref of prefs) {
  is (Services.prefs.getIntPref(pref), document.getElementById(pref), "Values have been initialized");
 }
 return 1; // We don't really care what value the actual field gets, just that all <preference> elements are initialized during onsyncfrompreference
}

Now we attach this as an onsyncfrompreference to all the input fields in the document.

There's no race condition, this is all synchronous code (I'm not too sure of this, but it seems to be -- XBL constructors at least are synchronous). Just that it was synchronous in the wrong order.


I'll try this later then, a tad busy at the moment (I'll clear the needinfo when I have a working test)
Attached patch Tests (obsolete) — Splinter Review
Here's the test. With the patch, all 9 tests should pass. Without it, 3/9 should fail.

It could have been simpler, with just two <preference> elements and an onsyncfrompreference on a control associated with the first one referring to the value of the second one, but I decided to keep three so that it can check for race conditions as well (the code is synchronous, so this shouldn't happen, but it's good to have a test)
Attachment #8413266 - Flags: review?(MattN+bmo)
Flags: needinfo?(manishearth)
Comment on attachment 8412268 [details] [diff] [review]
prefs.patch

Review of attachment 8412268 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/widgets/preferences.xml
@@ +32,5 @@
>      <implementation implements="nsIObserver">
> +      <method name="_constructAfterChildren">
> +      <body>
> +      <![CDATA[
> +      var elements = this.getElementsByTagName("preference");

Add a comment explaining what this method is about.

@@ +35,5 @@
> +      <![CDATA[
> +      var elements = this.getElementsByTagName("preference");
> +      for (element of elements) {
> +        if (!element._constructed) {
> +        	return;

Nit: bad indentation

@@ +38,5 @@
> +        if (!element._constructed) {
> +        	return;
> +        }
> +      }
> +      for (element of elements){ 

Nit: trailing whitespace

@@ +142,5 @@
>                if (parentPrefs[l].localName == "preference")
>                  preference = parentPrefs[l];
>              }
>            }
> +          this._value = preference ? preference.value : this.valueFromPreferences;

Could you explain in a comment why this shouldn't use _setValue anymore?

Without knowing this code, I find it weird that passing |false| for the aUpdate argument of _setValue leads to calling this.updateElements(); as that seems to contradict the argument. If you can think of a better name for that argument, please rename it or fix the method to update when it should (which I believe would mean you wouldn't need to change this).
Attachment #8412268 - Flags: review?(MattN+bmo) → feedback+
Comment on attachment 8413266 [details] [diff] [review]
Tests

Review of attachment 8413266 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/tests/chrome/test_preferences_onsyncfrompreference.xul
@@ +1,2 @@
> +<?xml version="1.0"?>
> +<?xml-stylesheet href="chrome://global/skin" type="text/css"?>

Missing license header

@@ +22,5 @@
> +      SpecialPowers.setIntPref(pref, 1);
> +    }
> +
> +    var counter = 0;
> +    var prefWindow = openDialog("window_preferences_onsyncfrompreference.xul", "", "", onSync);

Nit: use var instead of let in new files

@@ +24,5 @@
> +
> +    var counter = 0;
> +    var prefWindow = openDialog("window_preferences_onsyncfrompreference.xul", "", "", onSync);
> +
> +    SimpleTest.registerCleanupFunction(() => {

I'm not seeing where the dialog gets closed. Please put that in the cleanup function

@@ +35,5 @@
> +    function onSync() {
> +      for (let pref of PREFS) {
> +        // The `value` field of each <preference> element should be initialized by now.
> +
> +        is(SpecialPowers.getIntPref(pref), prefWindow.document.getElementById(pref).value, 

Nit: trailing whitespace

::: toolkit/content/tests/chrome/window_preferences_onsyncfrompreference.xul
@@ +1,4 @@
> +<?xml version="1.0"?>
> +<?xml-stylesheet href="chrome://global/skin" type="text/css"?>
> +<!--
> +  XUL Widget Test for preferences window with onsyncfrompreference

missing license header. An updated description here of the problem being solved would be good.
Attachment #8413266 - Flags: review?(MattN+bmo) → feedback+
Attached patch Part 2 (test)Splinter Review
New tests, will update the main patch in a moment.
Attachment #8413266 - Attachment is obsolete: true
Attachment #8413273 - Flags: feedback?(MattN+bmo)
Attached patch Main patch +nits (obsolete) — Splinter Review
I added some comments (and fixed the nits). The aUpdate parameter has to do with instantApply (The if block here[1] is the only difference between the two branches, since fireChangedEvent eventually[2] calls updateElements). However, with my change _setValue is only being used once in the document, so the aUpdate parameter becomes unnecessary, and I removed it.


 [1]: http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/preferences.xml#177
 [2]: http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/preferences.xml#546
Attachment #8412268 - Attachment is obsolete: true
Attachment #8413276 - Flags: review?(MattN+bmo)
Pushed to try (single build, only mochi/marionette), to check if this breaks any other tests. I ran a couple of tests and tried using the preferences with this, but it's best to be sure.

https://tbpl.mozilla.org/?tree=Try&rev=6634769cb60e
Comment on attachment 8413273 [details] [diff] [review]
Part 2 (test)

Review of attachment 8413273 [details] [diff] [review]:
-----------------------------------------------------------------

The test looks fine but it doesn't fail for me on OS X without the main patch. Does it fail for you?
Attachment #8413273 - Flags: feedback?(MattN+bmo) → feedback+
Comment on attachment 8413273 [details] [diff] [review]
Part 2 (test)

Sorry, I realized I accidentally re-applied the patch so that's why it was passing.
Attachment #8413273 - Flags: review+
Comment on attachment 8413276 [details] [diff] [review]
Main patch +nits

Review of attachment 8413276 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/widgets/preferences.xml
@@ -173,3 @@
>          <body>
>          <![CDATA[
> -          if (aUpdate && this.value !== aValue) {

Why did you remove the "this.value !== aValue" test? I would assume it was there for good reason.
Attachment #8413276 - Flags: feedback+
Attached patch Main patch (obsolete) — Splinter Review
Hm, didn't need to remove that; fixed.

Should I fold these patches before asking for a final review? (if so, are they ready for a final review?)
Attachment #8413276 - Attachment is obsolete: true
Attachment #8413276 - Flags: review?(MattN+bmo)
Attachment #8415834 - Flags: review?(MattN+bmo)
Attachment #8415834 - Flags: review?(MattN+bmo) → review+
Marking as checkin-needed (both patches), unless there are some other issues.
Keywords: checkin-needed
Attachment #8415834 - Attachment is obsolete: true
Attachment #8416218 - Flags: review?(MattN+bmo)
Attachment #8413273 - Attachment description: Tests +nits → Part 2 (test)
Attachment #8416218 - Flags: review?(MattN+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/93914bec7e29
https://hg.mozilla.org/mozilla-central/rev/d6e9c178000c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.