Closed
Bug 997570
Opened 10 years ago
Closed 10 years ago
Onsyncfrompreference sensitive to order of preference elements
Categories
(Toolkit :: Preferences, defect)
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: manishearth, Assigned: manishearth)
References
Details
Attachments
(2 files, 6 obsolete files)
5.57 KB,
patch
|
MattN
:
review+
MattN
:
feedback+
|
Details | Diff | Splinter Review |
4.91 KB,
patch
|
MattN
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•10 years ago
|
||
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)
Comment 2•10 years ago
|
||
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)
Assignee | ||
Comment 3•10 years ago
|
||
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.
Assignee | ||
Comment 4•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Comment 5•10 years ago
|
||
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+
Assignee | ||
Comment 6•10 years ago
|
||
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)
Assignee | ||
Comment 7•10 years ago
|
||
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)
Comment 8•10 years ago
|
||
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)
Assignee | ||
Comment 9•10 years ago
|
||
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)
Assignee | ||
Comment 10•10 years ago
|
||
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 11•10 years ago
|
||
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 12•10 years ago
|
||
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+
Assignee | ||
Comment 13•10 years ago
|
||
New tests, will update the main patch in a moment.
Attachment #8413266 -
Attachment is obsolete: true
Attachment #8413273 -
Flags: feedback?(MattN+bmo)
Assignee | ||
Comment 14•10 years ago
|
||
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)
Assignee | ||
Comment 15•10 years ago
|
||
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 16•10 years ago
|
||
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 17•10 years ago
|
||
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 18•10 years ago
|
||
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+
Assignee | ||
Comment 19•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8415834 -
Flags: review?(MattN+bmo) → review+
Assignee | ||
Comment 20•10 years ago
|
||
Marking as checkin-needed (both patches), unless there are some other issues.
Keywords: checkin-needed
Assignee | ||
Comment 21•10 years ago
|
||
Attachment #8415834 -
Attachment is obsolete: true
Attachment #8416218 -
Flags: review?(MattN+bmo)
Assignee | ||
Updated•10 years ago
|
Attachment #8413273 -
Attachment description: Tests +nits → Part 2 (test)
Updated•10 years ago
|
Attachment #8416218 -
Flags: review?(MattN+bmo) → review+
Comment 22•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/93914bec7e29 https://hg.mozilla.org/integration/fx-team/rev/d6e9c178000c
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Comment 23•10 years ago
|
||
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.
Description
•