992120 - [Tarako][Contact] Pressing "Home" button will discard the contact info when creating new contact/editing an existed contact

Status: VERIFIED FIXED

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

Description

[Bingqing Li]

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.

Comment 1

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

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.

Comment 2

[John Hammink]

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 :)

Comment 3

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

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

[Jason Smith [:jsmith]]

(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

[bhavana bajaj [:bajaj]]

: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.

Comment 6

[Jason Smith [:jsmith]]

(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

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

triage: per discussion with Product, let's not block with this

Comment 8

[Marvin Khoo [:Marvin_Khoo]]

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.

Comment 9

[Marvin Khoo [:Marvin_Khoo]]

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.

Comment 10

[James Zhang (Spreadtrum)]

Jesse, please attach your patch here.

Comment 11

[Jesse]

Attachment: save_contact_draft.patch

Thomas, could you find someone to review my patch?

Comment 12

[James Zhang (Spreadtrum)]

Need owner review this patch, it's spreadtrum block issue.

Comment 13

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

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

Comment 14

[Andreas Gal :gal]

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.

Comment 15

[James Zhang (Spreadtrum)]

(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.

Comment 16

[Andreas Gal :gal]

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

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

Jesse, i wonder if you have updated patch ?Thanks

Comment 18

[James Zhang (Spreadtrum)]

Jesse will.

Comment 19

[Jesse]

(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.

Comment 20

[Andreas Gal :gal]

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.

Comment 21

[David Teller [:Yoric] - still alive but not very active]

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

[John Hammink]

NI to retest when a patch lands.

Comment 23

[Jesse]

Attachment: save_contact_draft.patch

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.

Comment 24

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

ni? Jose for the review since Francisco's off

Comment 25

[Faramarz Rashed]

this has been waiting for a review for over a couple of days now!

Comment 26

[Andreas Gal :gal]

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

[Andreas Gal :gal]

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

Comment 28

[Jesse]

Attachment: save_contact_draft.patch

Using asyncStorage instead of 'contacts.Draft'. But I notice that asyncStorage does not have an error callback. This is bad in some critical cases.

Comment 29

[Andreas Gal :gal]

djf, see comment 28 about a problem with asyncStorage

Comment 30

[Andreas Gal :gal]

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.

Comment 31

[yang.zhao]

pull request:https://github.com/mozilla-b2g/gaia/pull/18967

Comment 32

[Evelyn Hung]

I just help yang to merge this patch into v1.3t
https://github.com/mozilla-b2g/gaia/commit/aef737f4eae863949d4b42cd6c17339aec3a5fa0

Comment 33

[Julien Wajsberg [:julienw]]

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

Comment 34

[Julien Wajsberg [:julienw]]

Reverted for Travis breackage:
v1.3t: 3e5af788dc532eadf4ce9fae2de9ac4fa76c930c

This probably needs at least a mock for asyncStorage.

Comment 35

[Julien Wajsberg [:julienw]]

(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?

Comment 36

[Steven Yang [:styang]]

Triage: 1.3T+

Comment 37

[Jose Manuel Cantera]

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?

Comment 38

[Ben Kelly [:bkelly, not reviewing]]

(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.

Comment 39

[Ben Kelly [:bkelly, not reviewing]]

Of course, there is bug 1006331... so maybe we should be more concerned for v1.3t.

Comment 40

[Michał Budzyński (:michalbe)]

After adding asyncStorage mock to the tests one of them is still failing, investigating it now...

Comment 41

[Michał Budzyński (:michalbe)]

Attachment: Tests console screenshot

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

[Michał Budzyński (:michalbe)]

Ok, so the problem is somewhere in saveContactOrDraft() in form.js, trying to figure out whats wrong there.

Comment 43

[Julien Wajsberg [:julienw]]

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

[Michał Budzyński (:michalbe)]

Yes, I found this one, but it was like this before the draft patch and it worked. Any ideas?

Comment 45

[Julien Wajsberg [:julienw]]

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!

Comment 46

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Attachment: Github Pull Request

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.

Comment 47

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 48

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

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?

Comment 49

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

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.

Comment 50

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Merged dddecf2ffe46bec137fd87f2ad99e5a22116b608.

https://github.com/mozilla-b2g/gaia/commit/dddecf2ffe46bec137fd87f2ad99e5a22116b608

Comment 51

[Julien Wajsberg [:julienw]]

(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

[Roland Kunkel [:RolandK]]

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.