1025560 - Flush preferences before Gecko process is killed

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect, P5)

Description

[Bill Appledorf]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release)
Build ID: 20140506152807

Steps to reproduce:

1. On an SM-T320 (Samsung TabPro 8.4 tablet) select Settings, Privacy, Tracking, "Tell sites I do not want to be tracked."

2. Close Firefox.

3. Restart Firefox.

4. Select Privacy, Settings, Tracking. Note that the option selected has reverted to "Do not tell sites anything about my tracking preferences."


Actual results:

Setting reverted to "Do not tell sites anything about my tracking preferences."


Expected results:

Setting should have remained "Tell sites I do not want to be tracked."

Comment 1

[Aaron Train [:aaronmt]]

While we aren't in possession of an SM-T320, I tried to reproduce this on a variety of devices and was unsuccessful in doing so. On each device, with Firefox 30, Gecko honoured the preference set.

How are you closing and restarting Firefox?

Do you have any possible mobile add-ons interfering with the settings?

Comment 2

[Bill Appledorf]

To close Firefox:

1. Press what I believe is the "Apps" button. This button opens an area at the bottom of the screen in which running apps are shown.

2. Swipe the Firefox representation down and off the screen. Firefox closes.

----
To reopen Firefox, either:

1. Touch a link in an app such as Facebook. Firefox opens.
2. Touch the Firefox icon on the homepage. Firefox opens.
3. Touch a Firefox bookmark permanently displayed on the homepage. Firefox opens.

----
I have no visibility into the internals of my device. Samsung/Android apparently runs tons of garbage I have no way of identifying in the background. I run a bare-bones tablet with no apps downloaded by me running in the background. I do use Facebook, I am embarrassed to admit, but the behavior I reported
occurs when Firefox is the only foreground process running on my device.

Comment 3

[Bill Appledorf]

My use of AdBlock Plus is such second nature that I forgot to indicate that I have it installed on my SM-T320. 

This add-on actually includes no-tracking functionality, which could be interfering with configuring Firefox's do-not-track option, given that both are operating in the same ballpark. 

I regret having neglected to include this piece of information in my description. I hope this helps.

Comment 4

[Aaron Train [:aaronmt]]

Thanks for the additional information. One thing that might be of interest to test is wether disabling Ad-Block in the Add-ons Manager in Firefox has any affect on toggling the browser settings. Process of elimination.

The preference in question here is 'Allow some non-intrusive advertising' and wether that has any affect on our Do Not Track browser settings.

Comment 5

[Richard Newman [:rnewman]]

Sounds like prefs might not be being being flushed before the process is killed.

Comment 6

[Aaron Train [:aaronmt]]

(In reply to Richard Newman [:rnewman] from comment #5)
> Sounds like prefs might not be being being flushed before the process is
> killed.

Maybe it's worth a custom build for the reporter with something akin to bug 1016161

Comment 8

[Richard Newman [:rnewman]]

Easy, btw: just like Bug 1016161.

Comment 9

[Brian Nicholson (:bnicholson)]

Note that we already flush the prefs on onPause: http://mxr.mozilla.org/mozilla-central/source/widget/android/nsAppShell.cpp?rev=b4224a22edac#342. Opening the apps tray should always trigger an onPause since it moves Fennec to the background. 

Bill, just as a sanity check, can you try disabling all of your add-ons and then performing your steps again to see if anything changes? If that doesn't help, I can try giving you a custom APK to help debug this (such as one that shows a toast on onPause to ensure the callbacks are being triggered).

(In reply to Richard Newman [:rnewman] from comment #8)
> Easy, btw: just like Bug 1016161.

The fix in 1016161 seems wrong -- we don't want to litter our code with pref flushes everywhere. Instead, we should make sure that whatever "hard quit" path we're going through triggers the correct events before performing the quit. More than just prefs can be lost on an unclean quit; there's also the user's session, the disk cache, etc. These all react to the application-background message by performing a synchronous write.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Brian Nicholson (:bnicholson) from comment #9)

> (In reply to Richard Newman [:rnewman] from comment #8)
> > Easy, btw: just like Bug 1016161.
> 
> The fix in 1016161 seems wrong -- we don't want to litter our code with pref
> flushes everywhere. Instead, we should make sure that whatever "hard quit"
> path we're going through triggers the correct events before performing the
> quit. More than just prefs can be lost on an unclean quit; there's also the
> user's session, the disk cache, etc. These all react to the
> application-background message by performing a synchronous write.

So you're suggesting we refactor the code so that the code in the "application-background" handler is also fired when force-quitting before a restart?

Comment 11

[Richard Newman [:rnewman]]

(In reply to Brian Nicholson (:bnicholson) from comment #9)

> The fix in 1016161 seems wrong -- we don't want to litter our code with pref
> flushes everywhere.

Wrong, but I considered it worthwhile in that case.

If you switch language and then Firefox crashes before that pref is flushed -- e.g., when a crash resuming BrowserApp or loading a pref fragment -- sadness ensues. You have to figure out that you need to switch language twice to get things back to functioning, and in the mean time weird stuff happens, like error pages appearing in English even though the rest of your browser is in Farsi.

(If you have a magic way to fire application-backgrounded when we crash, then I'd love to hear it :P)


But I pointed to that bug merely to help along the implementation point that you just made -- we should make sure we flush preferences as part of our pre-death phase, and that's the line of code that will make it happen. No point forcing you to wander around MDN unnecessarily!

Comment 12

[Bill Appledorf]

Brian:

The only add-on I am using on my tablet is AdBlock Plus. 
I disabled it then ran the steps. 
The setting still does not stick.

Comment 13

[Brian Nicholson (:bnicholson)]

(In reply to Mark Finkle (:mfinkle) from comment #10)
> So you're suggesting we refactor the code so that the code in the
> "application-background" handler is also fired when force-quitting before a
> restart?

In general, yes, I think that's the right thing to do (see also: https://bugzilla.mozilla.org/show_bug.cgi?id=996590#c10).

I'm not sure whether that fix would be relevant to this bug, though, since the OP is closing Fennec through the apps tray. That should first trigger an onPause, and we're already sending application-background events in onPause, so something else funky is going on here.

(In reply to Richard Newman [:rnewman] from comment #11)
> If you switch language and then Firefox crashes before that pref is flushed

Yeah, if we're talking about crashes, not quits/Android OOM kills, that's a different story. The only way to prevent data loss for crashes would be performing synchronous writes everywhere, which is obviously not an option.

Since the consequence of bug 1016161 is a broken UX, I guess it's reasonable to have that special case that ensures the pref gets written in case of crashes immediately after changing the pref (though getting in such a state is hopefully very rare).

Comment 14

[Brian Nicholson (:bnicholson)]

Hi Bill, can you try installing this Firefox build on your phone as a quick test? When you install this, it will appear on your phone as a separate app named "Fennec brian", and it will not touch your existing Firefox profile.

Two simple things I'd like you try:
1) After installing and opening it, push the home button on your phone. Confirm that the message "GeckoApplication onPause" appears.
2) Open it again, then push the app switcher button as if you were going to swipe away Firefox. Confirm that the message "GeckoApplication onPause" appears again.

Build here: http://people.mozilla.org/~bnicholson/fennec_builds/pause-toast.apk

Thanks!

Comment 15

[Bill Appledorf]

Hi Brian,

Very interesting result. 
In step 1, the app behaved as advertised. 
In step 2, the message did not appear.

To confirm that I performed the steps you requested:
In step 1, I pushed the home button. (Fine.)
In step 2, I pushed the app switcher button. (Not fine!)

I hope this helps.

Bill

Comment 16

[Brian Nicholson (:bnicholson)]

That's what I was afraid of -- other people [1][2] have also indicated that onPause is not triggered for the app switcher on some devices.

That's rather unfortunate, because we need *some* kind of trigger before we're killed to prevent loss of prefs, the disk cache, and the user session, and it seems like we aren't left with many options. I can look into using onWindowFocusChanged in addition to or instead of onPause/onResume.

[1] http://stackoverflow.com/questions/4620001/how-to-detect-if-application-switcher-is-visible
[2] http://stackoverflow.com/questions/9226027/why-onpause-is-not-called-in-following-situation

Comment 17

[Brian Nicholson (:bnicholson)]

onWindowFocusChanged isn't a good option either since it's called too frequently (just from showing a menu or dialog, for example). We could somewhat mitigate this issue by flushing prefs immediately just for the settings UI, but that still doesn't fix any other outgoing prefs, the disk cache, or the session.

Bill, as a workaround, my suggestion is that you hit the home button before killing Fennec (or any app) from the app switcher. I can't confirm that it's actually a bug since this behavior is all undocumented, but your device seems to be an outlier.

Comment 18

[Bill Appledorf]

OK, Brian. Sounds doable.
Do you guys report their bug to Samsung, or do I?
Thank you.

Bill

Comment 19

[Bill Appledorf]

Actually, could the SM-T320 be generating another event besides the one that standard Android devices do when the switch-app button is pressed?
The OS is 4.4.2.

Comment 20

[Bill Appledorf]

Two more possibilities occurred to me:

1. Does Samsung send an "I am killing you" message on app close?
2. Can fennec flush preferences when they are set?

I apologize for butting in as an end user with superficial, if any, understanding of the issue.

Comment 21

[Richard Newman [:rnewman]]

(In reply to Bill Appledorf from comment #20)

> 2. Can fennec flush preferences when they are set?

We try not to do that, because prefs are written frequently by all parts of Gecko. Writing to disk every time -- particularly if we also flush your open tabs etc., which is also in scope here -- would affect your experience.

Comment 22

[Brad Lassey [:blassey] (use needinfo?)]

filter on [mass-p5]

Comment 23

[Teodora Vermesan (:TeoVermesan)]

Crashing Firefox doesn't keep the state of "Show search suggestions" option from Settings.
Steps to reproduce:
1. Go to Settings -> Customize -> Search -> check "Show search suggestions" option
2. Install crash-me add-on: http://people.mozilla.org/~tmielczarek/crashme/
3. Go to Menu -> Crash me!
4. Restart Firefox and go to Settings -> Customize -> Search
=> "Show search suggestions" option is unchecked

This is reproducible on Firefox for Android 37.0a1 (2015-01-06), Firefox for Android 36.0a2 (2015-01-06), Firefox for Android 35 Beta 10. It is not reproducible on Firefox for Android 34.0.1 build 2.

Comment 24

[Richard Newman [:rnewman]]

Something we could do here to mitigate: flush preferences when we leave GeckoPreferences. That'll cover the common user-facing prefs, and won't interfere with in-browser activity. Thoughts, Brian?

Comment 25

[Brian Nicholson (:bnicholson)]

Makes sense to me. It's unclear whether the reporter here is even leaving GeckoPreferences before killing the app, but it can't hurt.

Comment 26

[Richard Newman [:rnewman]]

Un-duped for Bug 1118818 Comment 9.

Comment 27

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.