Closed Bug 997332 Opened 6 years ago Closed 6 years ago

Changing prefs in about:config often doesn't get saved

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 31
Tracking Status
fennec + ---

People

(Reporter: kats, Assigned: capella)

Details

(Whiteboard: [good-first-bug][lang=js])

Attachments

(1 file, 1 obsolete file)

Nexus 4, running recent m-c code:
1. Start fennec and load a page (https://people.mozilla.org/~kgupta/griddiv.html)
2. Observe that no tile borders (green lines overlayed on the content) are painted
3. Open a new tab and go to about:config
4. Find the layers.draw-tile-borders pref and toggle it to true
5. Observe that now there are green lines overlayed on everything
6. Close the about:config tab. Observe there are green lines on the other tab content
7. Use the Android app switcher and swipe away fennec to close it
8. Repeat step 1

Expected:
Tile borders pref is still set to true and tile borders are still visible

Actual:
pref is reset back to false! :(
Version: unspecified → Trunk
tracking-fennec: --- → ?
It seems that this is somewhat intermittent. Sometimes the prefs get saved and sometimes they don't. I suspect we only flush to disk at particular times and it's possible to exit fennec without flushing them to disk. For example, if you replace step 7 with "adb install -r fennec.apk" then you can consistently reproduce this. I think it might be good to flush to disk when we unload the about:config tab, if that's possible.
can someone mentor this bug?
tracking-fennec: ? → +
Whiteboard: [good-first-bug][lang=js]
We could call Services.prefs.savePrefFile(null) in AboutConfig.uninit() (which is already triggered on page unload) ... but I don't think that helps when swiping close via app switcher
Note that I do close the about:config tab as part of the STR, so the unload handler on the page should be running. Of course it would it would be nice to not have to do that either.
Attached patch bug997332.diff (obsolete) — Splinter Review
So currently, pref changes are saved when Fennec moves to background by tapping the device menu before app-swiping close, but not when we finish about:config by tab-swipe close.

The main confusing STR that I can repro is where a user will:
   1) Make a pref change via:
        -Add new pref,
        -Reset-to-default by: button click, or,
        -Change-value by:
           toggle button click
           integer up/down arrow click
           integer/string value manual keying

   2) Manually close the about:config tab

   3) Install new version while Fennec still active.

This patch will catch these situations .. we basically savePrefFile() whenever changes are made and about:config tab is closed.

If there's another trappable way to force Fennec closed that avoids flushing prefs, I'll be happy to address that :-)
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attachment #8409372 - Flags: review?(wjohnston)
Attached patch bug997332.diffSplinter Review
meh, (working too hard again)

The patch is even simpler, as Preferences already avoids duplicate writes
   http://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/Preferences.cpp?rev=e2c046c72d6c&mark=885-886#880

So we can just always do the call in uninit()
Attachment #8409372 - Attachment is obsolete: true
Attachment #8409372 - Flags: review?(wjohnston)
Attachment #8409385 - Flags: review?(wjohnston)
Comment on attachment 8409385 [details] [diff] [review]
bug997332.diff

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

I think we'd be well within our rights to do writes whenever prefs change in about:config. But this seems like a simpler fix. Lets see what the masses think :)
Attachment #8409385 - Flags: review?(wjohnston) → review+
Yah, I thought about that too but figured I'd be over-ruled on review to avoid (potential) extraneous updates ... "damned if yah do!"  :-D

https://hg.mozilla.org/integration/fx-team/rev/7bd04d2f2984
https://hg.mozilla.org/mozilla-central/rev/7bd04d2f2984
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
You need to log in before you can comment on or make changes to this bug.