949688 - contacts app does not persist any cookie config settings to file system

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Contacts, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The contacts app doesn't remember the "sort by last name" setting.

Steps to reproduce:
 1. start a fresh phone and import contacts from gmail
 2. open contacts app
 3. look at sort of contacts
 4. click the gear icon in the corner, and check the "sort by last name" setting
 5. look at sort of contacts
 6. reboot phone (hold down power button and then tap "Restart")
 7. open contacts app
 8. look at sort of contacts

Expected results:
 3. contacts sorted by first name
 5. contacts sorted by last name
 8. contacts sorted by last name

Actual results:
 3. --as expected--
 5. --as expected--
 8. contacts sorted by first name

Observed in a self-built build of v1.2 on hamachi, gaia 096722a9e2510ecdfe45ba7382d7d50826b82feb and gecko c1d0cff58112df46ce11be216fbadac96ecced82

See also bug 841082.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

I touched this code in bug 921685.  Perhaps I introduced a regression.  Can you try reverting that change to see if it resolves the problem.

Also, is this only when you reboot or also just from an app kill?

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Ni? to request testing with bug 921685 backed out.

Comment 3

[Jason Smith [:jsmith]]

QA Wanted - can we check if this reproduces on 1.1?

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Incidentally, I saw the bug many times on 1.1 -- I toggled that setting many times, and it never stuck.  But I never did a 1.1 test starting from a clean profile.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Ben Kelly [:bkelly] from comment #1)
> Also, is this only when you reboot or also just from an app kill?

App kill is not sufficient; it really does require a reboot.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Is testing with the backout still worthwhile given comment 4?

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Hmm, probably not.

Its strange since we immediately save the setting in a cookie when the control is clicked in the settings view.  Somehow the cookie is not getting set or its getting erased when the reboot happens.  Does setting the cookie not flush to the filesystem somehow?

Comment 8

[Nikolai Khristoforov]

This bug reproduces on the latest 1.1 MOZ build. After restarting the phone, the contacts are sorted by their first name instead of their last name.

Device: Buri 1.1 MOZ
BuildID: 20131212041201
Gaia: 6ff3a607f873320d00cb036fa76117f6fadd010f
Gecko: bdac595a4e46
Version: 18.0
Firmware Version: V1.2_20131115

Comment 9

[Jason Smith [:jsmith]]

That confirms this isn't a regression, which means this isn't a blocker.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

I don't have time to fully investigate at the moment, but I adb shell'd into my device and looked at the cookie.sqlite files in my profile.  Setting the sorting option in contacts does not modify the cookie database.

In contrast, opening the email app does modify the cookie database files.

I suspect this is due to contacts not following standard cookie format of "prop=value" and instead just jamming some JSON in there.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Updating the description to indicate this will effect all cookie configuration items.  In particular, as of 1.3 we are storing a config related to migrating FB contacts to DataStore.

Jose, it looks like the cookie is ignored across reboots.  How bad is it that we will try to re-run the migration after every boot?  Should fixing this be a 1.3 blocker?

Comment 12

[Jose Manuel Cantera]

(In reply to Ben Kelly [:bkelly] from comment #11)
> Updating the description to indicate this will effect all cookie
> configuration items.  In particular, as of 1.3 we are storing a config
> related to migrating FB contacts to DataStore.
> 
> Jose, it looks like the cookie is ignored across reboots.  How bad is it
> that we will try to re-run the migration after every boot?  Should fixing
> this be a 1.3 blocker?

Yes, we will try to re-run the migration every time contacts app starts. That's not a big issue but unconvenient. Wordt for the user is not remembering the order by. So, in essence I believe this is a blocker for v1.3. 

Ben, Please let us know if you have time to fix it. Otherwise I will assign it to German, or myself. 

thansk!

Comment 13

[Jose Manuel Cantera]

The provided Gaia patch makes the cookie value a valid value. 

However, I can confirm that there is a Gecko issue in persisting cookies, because even with a simple cookie like 'key1=value1' it is not persisted (if you reboot or kill B2G as described on the bug STR).  

We need more help here, Fabrice, any idea?

Comment 14

[Jose Manuel Cantera]

Attachment: 14804.html

Proposed Gaia Patch

Comment 15

[Francisco Jordano [:arcturus] [:francisco]]

FYI,

we were using cookies for the ordering since we realised that were faster than the localStorage (and indexedDB of course) for the initial contact order to make the initial list load fast.

I guess that for the fb migration we won't need to be that fast, but anyway, I'm still shocked that our beloved cookies are not there :(

http://tanniscuisine.es/wp-content/uploads/2013/03/Triky-el-famoso-Monstruo-de-la_54363259499_54028874188_960_639.jpg

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Jose, can you look at what email is doing?  Their cookie seems to get persisted correctly.  Looking at the code here:

  https://github.com/mozilla-b2g/gaia/blob/master/apps/email/js/html_cache.js#L39

It appears they set an expiration.  Is that required for some reason?

Comment 17

[Jose Manuel Cantera]

(In reply to Ben Kelly [:bkelly] from comment #16)
> Jose, can you look at what email is doing?  Their cookie seems to get
> persisted correctly.  Looking at the code here:
> 
>  
> https://github.com/mozilla-b2g/gaia/blob/master/apps/email/js/html_cache.
> js#L39
> 
> It appears they set an expiration.  Is that required for some reason?

ag yes, if we don't set the expiration time the cookie will only be valid for the current session and it seems "session" in this case is the B2G process session. I'm gonna fix it right now!

Comment 18

[Jose Manuel Cantera]

noming to koi. The fix is pretty simple and I think the device should not go to streets with such a funny bug.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8349328 [details]
14804.html

Looks good! Verified it worked across a reboot.  Just a couple nits in GH.  Thanks!  r=me

Comment 20

[John Hammink]

Probably not a great idea to land this this late.   I suggest we bump this to 1.3

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Its not a regression since this bug has always been there.  Shall we just ride the trains?

Comment 22

[Jose Manuel Cantera]

master: 

https://github.com/mozilla-b2g/gaia/commit/739b89f6160373b83273feaaad806abf421b8fa0

with Ben's nits addressed.

Comment 23

[Joe Cheng [:jcheng] (please needinfo)]

triage: this does not block release but please ask for approval to land in v1.3

Comment 24

[Jose Manuel Cantera]

Comment on attachment 8349328 [details]
14804.html

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined: Very High, the user will lose her configuration for the app
[Testing completed]: Yes
[Risk to taking this patch] (and alternatives if risky): Very low
[String changes made]:

Comment 25

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8349328 [details]
14804.html

definitely a+ the patch is non risky and fix a totally broken user experience.

Thanks guys!

Comment 26

[Noemí Freire (:noemi)]

Per comment 25 asking for v1.3 branch uplift. John, could you help us with it?. Many thanks!

Comment 27

[John Ford [:jhford] CET/CEST Berlin Time]

[v1.3 cfa2cbf] Merge pull request #14804 from jmcanterafonseca/fix_cookie_contac