Closed
Bug 992120
Opened 11 years ago
Closed 11 years ago
[Tarako][Contact] Pressing "Home" button will discard the contact info when creating new contact/editing an existed contact
Categories
(Firefox OS Graveyard :: Gaia::Contacts, defect)
Tracking
(blocking-b2g:1.3T+, b2g-v1.3T verified)
People
(Reporter: bli, Assigned: sergi, NeedInfo)
References
Details
(Keywords: dataloss, Whiteboard: OOM[sprd294848][partner-blocker][tarako_only])
Attachments
(3 files, 2 obsolete files)
Environment:
----------------------------------------------------
Gaia 32e15ec78cb245576a382468790d8f65461a5b3d
Gecko https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/e20c64a0ccd3
BuildID 20140403164001
Version 28.1
ro.build.version.incremental=eng.cltbld.20140403.201900
ro.build.date=Thu Apr 3 20:19:07 EDT 2014
Step to reproduce:
---------------------------------------------------
1. Launch Contact
2. Tap on "+" button on the top right corner
--> Goto the edit page
(Or Tap on a contact--> Goto the contact info page--> Tap on the 'edit' button on the top right corner--> Goto the edit page )
3. Press home button
4. Launch Contact again
Actual result
----------------------------------------------------
It shows the contact list.
And the contact info is discarded.
Reporter | ||
Updated•11 years ago
|
Summary: [Tarako][Contact] Pressing "Home" button will discard the contact info when creating new contact/adding to an existing contact from call log → [Tarako][Contact] Pressing "Home" button will discard the contact info when creating new contact/adding
Reporter | ||
Updated•11 years ago
|
Summary: [Tarako][Contact] Pressing "Home" button will discard the contact info when creating new contact/adding → [Tarako][Contact] Pressing "Home" button will discard the contact info when creating new contact/editing an existed contact
Reporter | ||
Updated•11 years ago
|
blocking-b2g: --- → 1.3T?
Comment 1•11 years ago
|
||
seem like the memory is low and contacts app got killed, in such low memory device it can be expected.
session restore is another topic which is not going to be in time for tarako. let's not block on this.
blocking-b2g: 1.3T? → -
Updated•11 years ago
|
Whiteboard: OOM
Comment 2•11 years ago
|
||
Binquing - Thanks for your work on this case. I'm however seeing a slightly different STR:
1.) Launch Contact
2.) Hit the "="
3.) Start entering a name, and some data to the fields
4.) hit the home button
*5.) Swipe back and forth between the screens (added this step)
6.) hit contact again
Actual
Apparently contact is killed and then launched again - the data you started to enter is gone
Expected
Contact should stay alive during the homescreen swipes, thus preserving the data
We should be keeping at least one app alive in the foreground.
Joe - Gonna have to push back here. We have data loss.
We need to test other similar scenarios around this (creating email or calendar event, pushing home, going back to event) so ni-ing me.
And renomming. Sorry, Joe :)
blocking-b2g: - → 1.3T?
Flags: needinfo?(jhammink)
Whiteboard: OOM
Updated•11 years ago
|
Whiteboard: OOM
Comment 3•11 years ago
|
||
my argument would be that it could also happen on a buri where if a user is entering names and some data into contacts fields and hit home button and start playing with browser or some other apps and triggers low memory killer to kill background apps, the next time when the user enters the contacts app, data lost can be observed
yes it's not a great user experience if the user has to type in all the fields again but given that the user has quit the app by pressing home, i think having session restore is nice to have
and my commercial android phone behaves the same..once i press home i have to start over
Comment 4•11 years ago
|
||
(In reply to Joe Cheng [:jcheng] from comment #3)
> my argument would be that it could also happen on a buri where if a user is
> entering names and some data into contacts fields and hit home button and
> start playing with browser or some other apps and triggers low memory killer
> to kill background apps, the next time when the user enters the contacts
> app, data lost can be observed
Well right, but that's different than this bug. In this bug, the contacts app is getting OOMed killed immediately after going to the background. That's far too aggressive - you should always be able to retain the most recently used app, even on Tarako. The problems John cites are valid - this is going to cause data loss to a user too quickly here, as the contacts app can die too quickly if it goes to the background (e.g. home button, incoming call, alarm firing), so the user could lose partially edited information.
>
> yes it's not a great user experience if the user has to type in all the
> fields again but given that the user has quit the app by pressing home, i
> think having session restore is nice to have
That would destroy the entire point of what the card view is intending to do. You should be able to retain most recently app at a minimum.
>
> and my commercial android phone behaves the same..once i press home i have
> to start over
That doesn't happen on my Android phone - my Android phone maintains multiple background processes after you move them to the background. If I return to a recently used app, then session state should be resumed if it's in the background.
Comment 5•11 years ago
|
||
:jsmith I've consulted :fabrice and implementing session restore might be the only way to resolve this issue. At the moment, doing that in a qualitative manner might risk the timeline here so we may have to live with this although its not a great user experience.
Marvin, from product stand point do you have any thoughts here ? Curious how does android behave in this scenario on the exact hardware.
Flags: needinfo?(mkhoo)
Comment 6•11 years ago
|
||
(In reply to bhavana bajaj [:bajaj] from comment #5)
> :jsmith I've consulted :fabrice and implementing session restore might be
> the only way to resolve this issue. At the moment, doing that in a
> qualitative manner might risk the timeline here so we may have to live with
> this although its not a great user experience.
>
> Marvin, from product stand point do you have any thoughts here ? Curious how
> does android behave in this scenario on the exact hardware.
FWIW - If Android behaves the same here, then I have no concerns with not blocking on this.
Comment 7•11 years ago
|
||
triage: per discussion with Product, let's not block with this
blocking-b2g: 1.3T? → backlog
Updated•11 years ago
|
Whiteboard: OOM → OOM[sprd294848]
Comment 8•11 years ago
|
||
Hi Bhavana / Jsmith
yes, Tarako / Android behave the same here.
STR:
1. Create a new contact
2. input name and field, but not saving,
3. press Home, open browser,
4. input www.cnn.com
5. browse any of article,
6. long press home, Tap Contact once Recent app shown.
7. Contact app goes back to glance UI instead of creating contact.
Flags: needinfo?(mkhoo)
Comment 9•11 years ago
|
||
btw, once you press home on STR steps 3, the half create contact will be gone. so, sorry for late and i expect we don't need to block this.
Updated•11 years ago
|
blocking-b2g: backlog → 1.3T?
Comment 11•11 years ago
|
||
Thomas, could you find someone to review my patch?
Flags: needinfo?(jesse.ji) → needinfo?(ttsai)
Comment 12•11 years ago
|
||
Need owner review this patch, it's spreadtrum block issue.
Whiteboard: OOM[sprd294848] → OOM[sprd294848][partner-blocker]
Updated•11 years ago
|
Flags: needinfo?(gal)
Flags: needinfo?(fabrice)
Updated•11 years ago
|
Attachment #8413573 -
Flags: review?(francisco.jordano)
Comment 13•11 years ago
|
||
Comment on attachment 8413573 [details] [diff] [review]
save_contact_draft.patch
Review of attachment 8413573 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks a lot for the contribution.
As mentioned before this is a complicated problem, not just related to contacts, but to anything using web activities.
First, IMHO, this shouldn't block any release, second, will need input from UX and product, since if we add this code, we will making appear the unfinished contacts when we create a new one, don't know if that's the desired flow.
Also will be a problem if we try to run the activity for updating a contact (yes there is one), that opens the form flow as well.
A part from that, did the code review and left some comments, the main two things are:
- Use of localStorage is banned in gaia, since it's synchronous and will block the UI.
- The code in contacts.js should be on a separate file and lazyloaded since this code path won't be used quite often and it's in one of the files that is part of the critical path of the app.
Thanks!
::: apps/communications/contacts/js/contacts.js
@@ +105,4 @@
> });
> break;
> default:
> + var draft = window.localStorage.getItem('draft');
We don't use localStorage cause it's sync and will cause the app to block until you recover the data.
For storing information we all use indexedDB
@@ +109,5 @@
> + if (!draft) {
> + showApp();
> + return;
> + }
> + initForm(function onInitForm() {
This piece of code won't be used that often and it's in one of the critical files of the app.
I would recommend to separate this to a different file and preload it when necessary.
@@ +129,5 @@
> + contactsForm.render(currentContact, goToForm);
> + showApp();
> + }, function onError() {
> + delete draft.id;
> + contactsForm.render(draft, goToForm);
|contactsForm.render(draft, goToForm);
showApp();|
Is repeated 3 times, sure we can extract it somehow, specially if we move this to an independent file.
::: apps/communications/contacts/js/views/form.js
@@ +184,5 @@
> });
> };
>
> + var cancelEdit = function cancelEdit() {
> + window.localStorage.removeItem('draft');
Don't use localStorage for saving the info
@@ +299,4 @@
> params = params || {};
>
> givenName.value = params.givenName || '';
> + familyName.value = params.familyName || params.lastName || '';
You are changing the logic here, please leave it as it was
Attachment #8413573 -
Flags: review?(francisco.jordano) → review-
Comment 14•11 years ago
|
||
As indicated above, in addition to the UX issues this patch will slow down launching the app because it eagerly loads code and because it uses localStorage. Can we get a patch that fixes these two issues? As for the UX problem we can take this on 1.3T only if the partner really wants this fix and we don't like the UX behavior.
Flags: needinfo?(gal)
Comment 15•11 years ago
|
||
(In reply to Andreas Gal :gal from comment #14)
> As indicated above, in addition to the UX issues this patch will slow down
> launching the app because it eagerly loads code and because it uses
> localStorage. Can we get a patch that fixes these two issues? As for the UX
> problem we can take this on 1.3T only if the partner really wants this fix
> and we don't like the UX behavior.
We can improve this patch and we really want to fix it.
Updated•11 years ago
|
Flags: needinfo?(fabrice)
Comment 16•11 years ago
|
||
Please keep in mind this comment:
Also will be a problem if we try to run the activity for updating a contact (yes there is one), that opens the form flow as well.
We can definitely land this on 1.3T. It might need more work for trunk but we don't have to block on that.
Comment 17•11 years ago
|
||
Jesse, i wonder if you have updated patch ?Thanks
Flags: needinfo?(jesse.ji)
Comment 18•11 years ago
|
||
Jesse will.
Comment 19•11 years ago
|
||
(In reply to Francisco Jordano [:arcturus] (on PTO till 19th May) from comment #13)
> Comment on attachment 8413573 [details] [diff] [review]
> save_contact_draft.patch
>
> Review of attachment 8413573 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Thanks a lot for the contribution.
>
> As mentioned before this is a complicated problem, not just related to
> contacts, but to anything using web activities.
>
> First, IMHO, this shouldn't block any release, second, will need input from
> UX and product, since if we add this code, we will making appear the
> unfinished contacts when we create a new one, don't know if that's the
> desired flow.
>
> Also will be a problem if we try to run the activity for updating a contact
> (yes there is one), that opens the form flow as well.
>
> A part from that, did the code review and left some comments, the main two
> things are:
>
> - Use of localStorage is banned in gaia, since it's synchronous and will
> block the UI.
> - The code in contacts.js should be on a separate file and lazyloaded since
> this code path won't be used quite often and it's in one of the files that
> is part of the critical path of the app.
>
> Thanks!
>
> ::: apps/communications/contacts/js/contacts.js
> @@ +105,4 @@
> > });
> > break;
> > default:
> > + var draft = window.localStorage.getItem('draft');
>
> We don't use localStorage cause it's sync and will cause the app to block
> until you recover the data.
>
> For storing information we all use indexedDB
>
> @@ +109,5 @@
> > + if (!draft) {
> > + showApp();
> > + return;
> > + }
> > + initForm(function onInitForm() {
>
> This piece of code won't be used that often and it's in one of the critical
> files of the app.
>
> I would recommend to separate this to a different file and preload it when
> necessary.
>
> @@ +129,5 @@
> > + contactsForm.render(currentContact, goToForm);
> > + showApp();
> > + }, function onError() {
> > + delete draft.id;
> > + contactsForm.render(draft, goToForm);
>
> |contactsForm.render(draft, goToForm);
> showApp();|
>
> Is repeated 3 times, sure we can extract it somehow, specially if we move
> this to an independent file.
>
> ::: apps/communications/contacts/js/views/form.js
> @@ +184,5 @@
> > });
> > };
> >
> > + var cancelEdit = function cancelEdit() {
> > + window.localStorage.removeItem('draft');
>
> Don't use localStorage for saving the info
>
> @@ +299,4 @@
> > params = params || {};
> >
> > givenName.value = params.givenName || '';
> > + familyName.value = params.familyName || params.lastName || '';
>
> You are changing the logic here, please leave it as it was
Well,
1. localStorage.getItem() is a high performance call, I'm surprised that you team forbid the use of localStorage though it's a w3c standard. IndexedDB is not a good alternative due to its low performance.
2. I didn't add much code in contacts, so using lazy load is not necessary. Abusing lazy load may cause
bad performance instead.
3. In my opinion, updating a contact from webactivity also need a draft. if most of u insist on not handling activity case, I will update the patch later.
Flags: needinfo?(jesse.ji)
Comment 20•11 years ago
|
||
localStorage is synchronous. An IPC message is sent to the parent process and the child process stops and waits for the result. This has very bad results for startup latency. You can easily regress startup by 100+ms with a single localStorage access. IndexedDb is async and you can fire off a request and continue loading and processing and then react to the result once it arrives.
Actually, it's even worse. You can get ~100ms on-the-average regressions with localStorage, but worst case scenarios of sync I/O can be multi-seconds. You really want to avoid them like the plague.
Comment 22•11 years ago
|
||
NI to retest when a patch lands.
Comment 23•11 years ago
|
||
1.Changing from localStorage to indexedDB.
2.Still simply saving draft in all cases.
3.No gaia unit test support, need your team's test cases.
Attachment #8413573 -
Attachment is obsolete: true
Attachment #8416855 -
Flags: review?(francisco.jordano)
Flags: needinfo?(ttsai) → needinfo?(james.zhang)
Updated•11 years ago
|
Flags: needinfo?(james.zhang) → needinfo?(fabrice)
Updated•11 years ago
|
Whiteboard: OOM[sprd294848][partner-blocker] → OOM[sprd294848][partner-blocker][tarako_only]
Updated•11 years ago
|
Attachment #8416855 -
Flags: review?(francisco.jordano) → review?(jmcf)
Updated•11 years ago
|
Flags: needinfo?(fabrice)
Comment 25•11 years ago
|
||
this has been waiting for a review for over a couple of days now!
Comment 26•11 years ago
|
||
This is going into 1.3T only and trunk and 1.4 already have a solution for this. I will review this to unblock it.
Comment 27•11 years ago
|
||
Comment on attachment 8416855 [details] [diff] [review]
save_contact_draft.patch
Review of attachment 8416855 [details] [diff] [review]:
-----------------------------------------------------------------
I am ok with this landing with a review from fabrice, this is for 1.3T and will not land anywhere else. Fabrice, can you please take a look as well and then stamp it?
::: apps/communications/contacts/js/contacts_draft.js
@@ +3,5 @@
> +var contacts = window.contacts || {};
> +
> +contacts.Draft = (function() {
> +
> + var DB_NAME = 'contactdraft';
isn't asyncStorage much easier for this?
@@ +14,5 @@
> + console.error(e.message);
> + }
> +
> + function query(dbName, storeName, func, onsuccess, data, onerror) {
> + var indexedDB = window.indexedDB || window.webkitIndexedDB ||
we won't need most of these forms
@@ +34,5 @@
> + }
> + };
> +
> + request.onupgradeneeded = function(event) {
> + console.log('Jesse - Upgrading db');
please remove
@@ +41,5 @@
> + db.createObjectStore(storeName, {
> + keyPath: 'id'
> + });
> + }
> + console.log('Jesse - Upgrading db done');
please remove
Attachment #8416855 -
Flags: review?(jmcf) → review?(fabrice)
Comment 28•11 years ago
|
||
Using asyncStorage instead of 'contacts.Draft'. But I notice that asyncStorage does not have an error callback. This is bad in some critical cases.
Attachment #8416855 -
Attachment is obsolete: true
Attachment #8416855 -
Flags: review?(fabrice)
Attachment #8417766 -
Flags: review?(gal)
Attachment #8417766 -
Flags: review?(fabrice)
Comment 29•11 years ago
|
||
djf, see comment 28 about a problem with asyncStorage
Comment 30•11 years ago
|
||
Comment on attachment 8417766 [details] [diff] [review]
save_contact_draft.patch
r=me for 1.3T only. Probably not worth backporting the 1.4 tests for this but I would recommend very closely QAing this on the SPRD side. We are taking this as a vendor patch.
Attachment #8417766 -
Flags: review?(gal)
Attachment #8417766 -
Flags: review?(fabrice)
Attachment #8417766 -
Flags: review+
Updated•11 years ago
|
Flags: needinfo?(yang.zhao)
Comment 31•11 years ago
|
||
pull request:https://github.com/mozilla-b2g/gaia/pull/18967
Flags: needinfo?(yang.zhao)
Comment 32•11 years ago
|
||
I just help yang to merge this patch into v1.3t
https://github.com/mozilla-b2g/gaia/commit/aef737f4eae863949d4b42cd6c17339aec3a5fa0
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 33•11 years ago
|
||
I'm sorry, this broke Travis, can someone back out (I'm in the middle of something right now)
https://travis-ci.org/mozilla-b2g/gaia/jobs/24513588
Flags: needinfo?(ehung)
Comment 34•11 years ago
|
||
Reverted for Travis breackage:
v1.3t: 3e5af788dc532eadf4ce9fae2de9ac4fa76c930c
This probably needs at least a mock for asyncStorage.
Flags: needinfo?(ehung)
Updated•11 years ago
|
Comment 35•11 years ago
|
||
(In reply to Julien Wajsberg [:julienw] (away until 5th May) from comment #34)
> This probably needs at least a mock for asyncStorage.
Such a mock is available in the contacts app, so it should be easy to fix.
Michal, maybe you can help wrapping this up?
Flags: needinfo?(mbudzynski)
Comment 37•11 years ago
|
||
the provided patch is likely to be breaking the cold start-up time performance measure in contacts, as it is querying asyncStorage and that will take time. Ben, what do you think?
Updated•11 years ago
|
Flags: needinfo?(jmcf) → needinfo?(bkelly)
Comment 38•11 years ago
|
||
(In reply to Jose Manuel Cantera from comment #37)
> the provided patch is likely to be breaking the cold start-up time
> performance measure in contacts, as it is querying asyncStorage and that
> will take time. Ben, what do you think?
It almost certainly does as we are going from this:
showApp();
to essentially this:
asyncStorage.getItem('draft', function onDraftGot(draft) {
showApp();
}
However, the delay may only really be in showing the initial app chrome. Before we can populate the list we need to initialize and load IDB anyway which is the main cost of trying to get a non-existent draft. I would not be surprised if the total app launch time degraded, but only by a small amount.
Given that this is only targeting v1.3t, I think its ok. Particularly since the OEM is requesting the patch and has presumably examined its performance on the device.
Before landing something like this on master, though, we should check to see if the time to between these two points in the code increases significantly:
https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/js/contacts.js#L243
https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/js/views/list.js#L641
Bonus points if someone wants to validate that on v1.3t.
Flags: needinfo?(bkelly)
Comment 39•11 years ago
|
||
Of course, there is bug 1006331... so maybe we should be more concerned for v1.3t.
See Also: → 1006331
Comment 40•11 years ago
|
||
After adding asyncStorage mock to the tests one of them is still failing, investigating it now...
Flags: needinfo?(mbudzynski)
Comment 41•11 years ago
|
||
Very strage thing is happening now - in the form_test.js#L240 there is subject.render method call with mockContact as an argument, and before it's rendered mockContacts looks different than before the rendering (the object itself) - the type of a telephony number is missing. Maybe someone seen something like this before? I'm still digging into it and looking for an explanation.
Comment 42•11 years ago
|
||
Ok, so the problem is somewhere in saveContactOrDraft() in form.js, trying to figure out whats wrong there.
Comment 43•11 years ago
|
||
look at [1]: currentContact is being refilled from the input (myContact is filled at [2]).
I don't know at all if this is something that's supposed to happen in this case. It seems strange and wrong to me that calling "saveContact" would modify the contact...
[1] https://github.com/mozilla-b2g/gaia/commit/aef737f4eae863949d4b42cd6c17339aec3a5fa0#diff-7d9bc4ef89ed32b6f748a27867f44c2aL554
[2] https://github.com/mozilla-b2g/gaia/commit/aef737f4eae863949d4b42cd6c17339aec3a5fa0#diff-7d9bc4ef89ed32b6f748a27867f44c2aR550
Comment 44•11 years ago
|
||
Yes, I found this one, but it was like this before the draft patch and it worked. Any ideas?
Comment 45•11 years ago
|
||
Well, the draft patch added a "saveDraft" call in the path exercized by the test, that's why the test is failing.
But I really don't know if we should just fix the test or if there is a real bug lurking. TBH this code is very difficult to follow for me, with these shared variables modified in many places.
Michal, it's your call here!
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → sergi.mansilla
Assignee | ||
Comment 46•11 years ago
|
||
This patch completes the work on tests started by Michal and fixes some wrong 'data' attributes in `mock_form_dom.js.html` that were causing a test in `form_test.js` to fail.
Attachment #8418679 -
Flags: review?(etienne)
Assignee | ||
Updated•11 years ago
|
Attachment #8418679 -
Flags: review?(etienne) → review?(bkelly)
Comment 47•11 years ago
|
||
Comment on attachment 8418679 [details] [review]
Github Pull Request
r=me for the test fix. Please put the bug number in the commit message or squash with the previous commits.
I still have reservations about the original patch due to launch latency, but that was already reviewed.
Attachment #8418679 -
Flags: review?(bkelly) → review+
Assignee | ||
Comment 48•11 years ago
|
||
Squashed and with bug number now, but because of that the commit appears under my name although my changes were pretty small. Is that a problem?
Worth noting that I just tried switching away from an in-progress contact add/edit on iOS and immediately switching back, and it threw away the edit session.
Assignee | ||
Comment 50•11 years ago
|
||
Merged dddecf2ffe46bec137fd87f2ad99e5a22116b608.
https://github.com/mozilla-b2g/gaia/commit/dddecf2ffe46bec137fd87f2ad99e5a22116b608
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Comment 51•11 years ago
|
||
(In reply to Sergi Mansilla (Telenor) from comment #48)
> Squashed and with bug number now, but because of that the commit appears
> under my name although my changes were pretty small. Is that a problem?
For next time, you can use `git --author="<XXX>" --amend` if you want to change it back.
Comment 52•11 years ago
|
||
Verified for v1.3T. This issue does not reproduce on the latest v1.3T Tarako build:
v1.3T Environmental Variables:
Device: Tarako v1.3T MOZ RIL
BuildID: 20140530014002
Gaia: e68858693b71d917c9c5ee7e215f7ceea04635f7
Gecko: 1945abae19ff
Version: 28.1
Firmware Version: SP6821a-Gonk-4.0-5-12
-
The partially created non-saved contact is restored when the user returns to the app.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•