Closed Bug 992120 Opened 6 years ago Closed 5 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)

x86_64
Linux
defect
Not set

Tracking

(blocking-b2g:1.3T+, b2g-v1.3T verified)

VERIFIED FIXED
blocking-b2g 1.3T+
Tracking Status
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.
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
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
blocking-b2g: --- → 1.3T?
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? → -
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
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
(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.
Keywords: dataloss
: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)
(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.
triage: per discussion with Product, let's not block with this
blocking-b2g: 1.3T? → backlog
Whiteboard: OOM → OOM[sprd294848]
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)
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.
blocking-b2g: backlog → 1.3T?
Jesse, please attach your patch here.
Flags: needinfo?(jesse.ji)
Attached patch save_contact_draft.patch (obsolete) — Splinter Review
Thomas, could you find someone to review my patch?
Flags: needinfo?(jesse.ji) → needinfo?(ttsai)
Need owner review this patch, it's spreadtrum block issue.
Whiteboard: OOM[sprd294848] → OOM[sprd294848][partner-blocker]
Flags: needinfo?(gal)
Flags: needinfo?(fabrice)
Attachment #8413573 - Flags: review?(francisco.jordano)
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-
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)
(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.
Flags: needinfo?(fabrice)
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.
Jesse, i wonder if you have updated patch ?Thanks
Flags: needinfo?(jesse.ji)
Jesse will.
(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)
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.
NI to retest when a patch lands.
Attached patch save_contact_draft.patch (obsolete) — Splinter Review
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)
Flags: needinfo?(james.zhang) → needinfo?(fabrice)
Whiteboard: OOM[sprd294848][partner-blocker] → OOM[sprd294848][partner-blocker][tarako_only]
ni? Jose for the review since Francisco's off
Flags: needinfo?(jmcf)
Attachment #8416855 - Flags: review?(francisco.jordano) → review?(jmcf)
Flags: needinfo?(fabrice)
this has been waiting for a review for over a couple of days now!
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 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)
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)
djf, see comment 28 about a problem with asyncStorage
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+
Flags: needinfo?(yang.zhao)
pull request:https://github.com/mozilla-b2g/gaia/pull/18967
Flags: needinfo?(yang.zhao)
I just help yang to merge this patch into v1.3t
https://github.com/mozilla-b2g/gaia/commit/aef737f4eae863949d4b42cd6c17339aec3a5fa0
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
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)
Reverted for Travis breackage:
v1.3t: 3e5af788dc532eadf4ce9fae2de9ac4fa76c930c

This probably needs at least a mock for asyncStorage.
Flags: needinfo?(ehung)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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)
Triage: 1.3T+
blocking-b2g: 1.3T? → 1.3T+
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?
Flags: needinfo?(jmcf) → needinfo?(bkelly)
(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)
Of course, there is bug 1006331... so maybe we should be more concerned for v1.3t.
See Also: → 1006331
After adding asyncStorage mock to the tests one of them is still failing, investigating it now...
Flags: needinfo?(mbudzynski)
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.
Ok, so the problem is somewhere in saveContactOrDraft() in form.js, trying to figure out whats wrong there.
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
Yes, I found this one, but it was like this before the draft patch and it worked. Any ideas?
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: nobody → sergi.mansilla
Attached file 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.
Attachment #8418679 - Flags: review?(etienne)
Attachment #8418679 - Flags: review?(etienne) → review?(bkelly)
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+
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.
Merged dddecf2ffe46bec137fd87f2ad99e5a22116b608.

https://github.com/mozilla-b2g/gaia/commit/dddecf2ffe46bec137fd87f2ad99e5a22116b608
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
(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.
Depends on: 1009083
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.