Closed
Bug 949688
Opened 11 years ago
Closed 11 years ago
contacts app does not persist any cookie config settings to file system
Categories
(Firefox OS Graveyard :: Gaia::Contacts, defect)
Tracking
(blocking-b2g:-, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)
People
(Reporter: dbaron, Assigned: jmcf)
References
Details
Attachments
(1 file)
191 bytes,
text/html
|
bkelly
:
review+
arcturus
:
approval-gaia-v1.3+
|
Details |
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•11 years ago
|
||
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
Reporter | ||
Comment 4•11 years ago
|
||
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.
Reporter | ||
Comment 5•11 years ago
|
||
(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.
Reporter | ||
Comment 6•11 years ago
|
||
Is testing with the backout still worthwhile given comment 4?
Flags: needinfo?(dbaron)
Comment 7•11 years ago
|
||
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?
Reporter | ||
Updated•11 years ago
|
Keywords: regression
Comment 8•11 years ago
|
||
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
Comment 9•11 years ago
|
||
That confirms this isn't a regression, which means this isn't a blocker.
blocking-b2g: koi? → -
Comment 10•11 years ago
|
||
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•11 years ago
|
||
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
Assignee | ||
Comment 12•11 years ago
|
||
(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)
Assignee | ||
Updated•11 years ago
|
blocking-b2g: - → 1.3?
Assignee | ||
Comment 13•11 years ago
|
||
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)
Assignee | ||
Comment 14•11 years ago
|
||
Proposed Gaia Patch
Assignee | ||
Updated•11 years ago
|
Severity: normal → major
Comment 15•11 years ago
|
||
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•11 years ago
|
||
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?
Assignee | ||
Comment 17•11 years ago
|
||
(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!
Assignee | ||
Comment 18•11 years ago
|
||
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?
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(fabrice)
Assignee | ||
Updated•11 years ago
|
Attachment #8349328 -
Flags: review?(bkelly)
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → jmcf
Comment 19•11 years ago
|
||
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+
Comment 20•11 years ago
|
||
Probably not a great idea to land this this late. I suggest we bump this to 1.3
blocking-b2g: koi? → 1.3?
Comment 21•11 years ago
|
||
Its not a regression since this bug has always been there. Shall we just ride the trains?
Assignee | ||
Comment 22•11 years ago
|
||
master:
https://github.com/mozilla-b2g/gaia/commit/739b89f6160373b83273feaaad806abf421b8fa0
with Ben's nits addressed.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Target Milestone: --- → 1.3 C1/1.4 S1(20dec)
Comment 23•11 years ago
|
||
triage: this does not block release but please ask for approval to land in v1.3
blocking-b2g: 1.3? → -
Assignee | ||
Comment 24•11 years ago
|
||
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 25•11 years ago
|
||
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+
Comment 26•11 years ago
|
||
Per comment 25 asking for v1.3 branch uplift. John, could you help us with it?. Many thanks!
status-b2g-v1.3:
--- → affected
Flags: needinfo?(jhford)
Comment 27•11 years ago
|
||
[v1.3 cfa2cbf] Merge pull request #14804 from jmcanterafonseca/fix_cookie_contac
Flags: needinfo?(jhford)
Updated•11 years ago
|
status-b2g-v1.3T:
--- → fixed
status-b2g-v1.4:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•