Closed Bug 949688 Opened 6 years ago Closed 6 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
major

Tracking

(blocking-b2g:-, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)

RESOLVED FIXED
1.3 C1/1.4 S1(20dec)
blocking-b2g -
Tracking Status
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: dbaron, Assigned: jmcf)

References

Details

Attachments

(1 file)

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.
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?
blocking-b2g: --- → koi?
Keywords: regression
Ni? to request testing with bug 921685 backed out.
Flags: needinfo?(dbaron)
QA Wanted - can we check if this reproduces on 1.1?
Keywords: qawanted
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.
(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.
Is testing with the backout still worthwhile given comment 4?
Flags: needinfo?(dbaron)
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?
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
Keywords: qawanted
QA Contact: nkhristoforov
That confirms this isn't a regression, which means this isn't a blocker.
blocking-b2g: koi? → -
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.
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?
Flags: needinfo?(jmcf)
Summary: contacts app doesn't remember "sort by last name" setting → contacts app does not persist any cookie config settings to file system
(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!
Flags: needinfo?(jmcf)
blocking-b2g: - → 1.3?
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?
Flags: needinfo?(fabrice)
Attached file 14804.html
Proposed Gaia Patch
Severity: normal → major
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
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?
(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!
noming to koi. The fix is pretty simple and I think the device should not go to streets with such a funny bug.
blocking-b2g: 1.3? → koi?
Flags: needinfo?(fabrice)
Attachment #8349328 - Flags: review?(bkelly)
Assignee: nobody → jmcf
Comment on attachment 8349328 [details]
14804.html

Looks good! Verified it worked across a reboot.  Just a couple nits in GH.  Thanks!  r=me
Attachment #8349328 - Flags: review?(bkelly) → review+
Probably not a great idea to land this this late.   I suggest we bump this to 1.3
blocking-b2g: koi? → 1.3?
Its not a regression since this bug has always been there.  Shall we just ride the trains?
master: 

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

with Ben's nits addressed.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.3 C1/1.4 S1(20dec)
triage: this does not block release but please ask for approval to land in v1.3
blocking-b2g: 1.3? → -
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]:
Attachment #8349328 - Flags: approval-gaia-v1.3?(francisco.jordano)
Comment on attachment 8349328 [details]
14804.html

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

Thanks guys!
Attachment #8349328 - Flags: approval-gaia-v1.3?(francisco.jordano) → approval-gaia-v1.3+
Per comment 25 asking for v1.3 branch uplift. John, could you help us with it?. Many thanks!
Flags: needinfo?(jhford)
[v1.3 cfa2cbf] Merge pull request #14804 from jmcanterafonseca/fix_cookie_contac
Flags: needinfo?(jhford)
Duplicate of this bug: 958942
You need to log in before you can comment on or make changes to this bug.