1114520 - [Flame][Contacts]All contacts will disappear after you delete one contact.

Status: VERIFIED FIXED

(Core Graveyard :: DOM: Contacts, defect)

Description

[Lance（leave from Mozilla）]

Attachment: logcat_1720.txt

[1.Description]:
[Flame][v2.2][Contacts]After you delete one contact and kill the Contact process, all contacts will disappear. 
Found time:17:20
See attachment:1720.mp4 and logcat_1720.txt

[2.Testing Steps]: 
1.Launch Contacts.
2.Create more than 2 contacts.
3.Tap one contact to view, then tap edit, delete the contact.
4.Back to Home page, then long press Home key to kill Contact progress.
5.Launch Contacts again.

Note: If you add a contact, then long press Home key to kill Contact progress, all contacts will display.
          If you doesn't add contact, long press Home key to kill Contact progress, all contacts still can't display.

S/N:25622

[3.Expected Result]: 
5.All contacts should be displayed normally.

[4.Actual Result]: 
5.All contacts can't be displayed.

[5.Reproduction build]: 
Gaia-Rev        ca6e91e09ef3ab417a0f6b6d6668d43597d85700
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/7b33ee7fd162
Build-ID        20141221040207
Version         37.0a1

[6.Reproduction Frequency]: 
Always Recurrence,5/5
TCID: Free Test

Comment 1

[Lance（leave from Mozilla）]

Attachment: 1720.MP4

Comment 2

[Johan Lorenzo [:jlorenzo]]

[Blocking Requested - why for this release]: Regression on a basic use case.

Comment 3

[Duane Dixon [:ddixon]]

B2G Inbound Regression Window

Last Working

Device: Flame 2.2
BuildID: 20141022125314
Gaia: 3536e6038212f9c5dc08a763ed6555494f24e38b
Gecko: eb4d4fbac027
Version: 36.0a1 (2.2) 
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

First Broken

Device: Flame 2.2
BuildID: 20141022151219
Gaia: 3536e6038212f9c5dc08a763ed6555494f24e38b
Gecko: badc56be9f64
Version: 36.0a1 (2.2) 
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Last Working Gaia and First Broken Gecko
Issue DOES occur here. 
Gaia: 3536e6038212f9c5dc08a763ed6555494f24e38b
Gecko: badc56be9f64

Last Working Gecko and First Broken Gaia
Issue DOES NOT occur here. 
Gaia: 3536e6038212f9c5dc08a763ed6555494f24e38b
Gecko: eb4d4fbac027

Gecko Pushlog:
http://hg.mozilla.org/integration/b2g-inbound/pushloghtml?fromchange=eb4d4fbac027&tochange=badc56be9f64

Comment 4

[Joshua Mitchell (Inactive)]

This is a much larger pushlog than normal but it seems the most likely candidate is the patch to bug 1072773 because it is dealing with saving and removing things in the Contacts database. - can you take a look Reuben

Comment 5

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

triage: serious regression, data loss.

Comment 6

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

I manage to reproduce this, and see the following error:

W/Communications( 2341): [JavaScript Error: "TypeError: contact is undefined" {file: "jar:file:///system/b2g/omni.ja!/components/ContactManager.js" line: 94}]

Seems the mozcontacts api is failing to return the contacts, and therefor the contacts app is showing the empty list.

Comment 7

[Andrew Overholt [:overholt]]

Gregor suggested we contact Julien here.

Comment 8

[Julien Wajsberg [:julienw]]

Actually I am fairly busy with the ideation process.

Francisco, do you think one of the Contacts peer could look at the Gecko API code? I'm thinking of ferjm :)

If no one else can look at this, I can dive in, but this will take longer because of my other engagements.

Comment 9

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

I think :ferjm is quite busy right now, better to ask him.

Fernando if you are overloaded just tell here and we can ask again Julien kindly to take a look :)

Comment 10

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

This fixes the issue for me. Working on the tests now.

Comment 11

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

Now with tests

Comment 12

[Fernando Jiménez Moreno [:ferjm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=be2bdd78439d

Comment 13

[Gregor Wagner [:gwagner]]

Comment on attachment 8549036 [details] [diff] [review]
v1

Review of attachment 8549036 [details] [diff] [review]:
-----------------------------------------------------------------

Ugh thanks!

Comment 14

[Fernando Jiménez Moreno [:ferjm]]

Thank you Gregor!

https://hg.mozilla.org/integration/b2g-inbound/rev/2c7342538c03

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2c7342538c03

Comment 16

[Julien Wajsberg [:julienw]]

I have an issue with this patch though: because the transaction is then reused in removeContact, is the contact really deleted now?

Shouldn't we open a transaction on the 2 objectstores ?

Comment 17

[Fernando Jiménez Moreno [:ferjm]]

Ugh, you are right. I wonder why the tests are not catching these issues :( ... I didn't notice that on the device while manually testing this patch cause I didn't evict the cache so I wasn't directly consuming the data from the contacts DB.

I'll fix it in a follow-up. Thanks for the catch!

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Please request b2g37 approval on this when you get a chance :)

Comment 19

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Follow up

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfad13c0c594

Comment 20

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #18)
> Please request b2g37 approval on this when you get a chance :)

Yes, I will do it as soon as the follow up lands, so we can uplift both patches at once.

Comment 21

[Reuben Morais [:reuben]]

(In reply to Julien Wajsberg [:julienw] from comment #16)
> I have an issue with this patch though: because the transaction is then
> reused in removeContact, is the contact really deleted now?
> 
> Shouldn't we open a transaction on the 2 objectstores ?

The transaction is reused in removeContact, but we explicitly grab the main object store there, so this is fine. The tests don't catch it because there's no bug :)

Comment 22

[Fernando Jiménez Moreno [:ferjm]]

Can we use an object store within a transaction that wasn't created for that object store?

Comment 23

[Reuben Morais [:reuben]]

When you pass a string to IndexedDBHelper.newTxn, the transaction is actually created with all object stores: https://mxr.mozilla.org/mozilla-central/source/dom/base/IndexedDBHelper.jsm#121

FWIW, I think the store_name parameter there and IndexedDBHelper passing stores in the callback is a footgun for ContactDB (or any users that handle more than 1 object store), even without this corner case.

Comment 24

[Fernando Jiménez Moreno [:ferjm]]

Ok, I see. So this worked cause we were passing "SAVED_GETALL_STORE_NAME" instead of "[SAVED_GETALL_STORE_NAME]" and so IndexedDBHelper was opening the transaction for all the object stores. That's a bit scary.

There might be reasons to do this this way, but to be honest the code at [1] doesn't look ok to me. If we pass "ObjectStore" we will be opening the transaction for all object stores in the DB, while if we pass "[ObjectStore]" we will be opening the transaction for that single object store. IMHO we shouldn't be opening the transaction for all the stores if we explicitly ask for a single store (IIUC that's not how the IDB API works [2]). Also at [3] if we pass "ObjectStore" we will be getting the object store instance for "ObjectStore" only, but we already opened the transaction for all the DB object stores, so we are lying.

[1] https://mxr.mozilla.org/mozilla-central/source/dom/base/IndexedDBHelper.jsm#121
[2] https://developer.mozilla.org/en-US/docs/Web/API/IDBDatabase.transaction#Parameters
[3] https://mxr.mozilla.org/mozilla-central/source/dom/base/IndexedDBHelper.jsm#124

Comment 25

[Julien Wajsberg [:julienw]]

I definitely agree, but this can be difficult to change because this is used a lot: [1]. Still I wonder if opening all those transactions on all dbStoresNames even when we don't need all of them (and probably without knowing it) can be costly.

[1] http://mxr.mozilla.org/mozilla-central/search?string=newTxn&filter=%5BNn%5DewTxn

Comment 26

[Julien Wajsberg [:julienw]]

I think we should follow-up in a separate bug then...

Comment 27

[Reuben Morais [:reuben]]

Most users only have a single object store, so How Hard Can It Be™? :)

I filed bug 1123903.

Comment 28

[Reuben Morais [:reuben]]

Comment on attachment 8551200 [details] [diff] [review]
Follow up

Review of attachment 8551200 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.

::: dom/contacts/tests/test_contacts_cache.xul
@@ +46,5 @@
> +    contactsDB.getRevision(function(revision) {
> +      ok(expectedRevision === revision, "Revision OK");
> +      then();
> +    }, function() {
> +      makeFailure("Could not get revision");

makeFailure only returns a function that can be passed as a callback or set as an event handler, so this should be:

contactsDB.getRevision(function(){…}, makeFailure("Could not get revision"));

@@ +69,5 @@
>      }
>    };
>  
>    let Tests = [function() {
> +    info("Washing hands before lunch");

Please use something more descriptive in these messages, like… "Deleting database" and "Checking initial revision" :)

@@ +79,5 @@
>      info("Save contact");
>      contactsDB.saveContact(CONTACT_PROPS, function() {
>        ok(true, "Saved contact successfully");
> +      checkRevision(1, next);
> +    }, makeFailure);

Same as the makeFailure comment from above.

@@ +85,5 @@
>      info("Save another contact");
>      contactsDB.saveContact(ANOTHER_CONTACT_PROPS, function() {
>        ok(true, "Saved contact successfully");
> +      checkRevision(2, next);
> +    }, makeFailure);

And here.

@@ +92,5 @@
>      contactsDB.getAll(function(contacts) {
>        ok(true, "Got all contacts " + contacts.length);
>        next();
>      }, function(e) {
>        makeFailure("Unexpected error getting contacts " + e);

And here… Looks like it was already being misused before.

@@ +117,2 @@
>      }, function() {
>        makeFailure("Unexpected error removing contact " + e);

And here.

@@ +122,5 @@
> +    contactsDB.newTxn("readonly", STORE_NAME, function(txn, store) {
> +      let req = store.openCursor(IDBKeyRange.only(CONTACT_PROPS.id));
> +      req.onsuccess = function(event) {
> +        if (event.target.result) {
> +          makeFailure("Should not have cursor");

And here.

@@ +128,5 @@
> +        }
> +        ok(true, "Yep, the contact was removed");
> +        next();
> +      };
> +      req.onerror = makeFailure;

And here.

@@ +136,5 @@
>      contactsDB.newTxn("readonly", SAVED_GETALL_STORE_NAME, function(txn, store) {
>        store.openCursor().onsuccess = function(e) {
>          let cursor = e.target.result;
>          if (!cursor) {
>            makeFailure;

What!

Comment 29

[Fernando Jiménez Moreno [:ferjm]]

Thanks Reuben.

https://hg.mozilla.org/integration/b2g-inbound/rev/b16ce884daf3

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b16ce884daf3

Comment 31

[Yeojin Chung [:YeojinC]]

This issue is verified fixed on Flame Master.

Result: Contacts do not disappear after deleting a contact, killing Contacts app from card view, and re-launching the app.
 
Device: Flame Master (319mb, full flash)
Build ID: 20150206010204
Gaia: 94af4b42d2ace6c9f38f31de77240604fac68af1
Gecko: 7c5f187b65bf
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 38.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.
-------------------------------------------

Leaving verifyme for 2.2 verification

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Please request b2g37 approval on this when you get a chance.

Comment 34

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8549036 [details] [diff] [review]
v1

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 #): Contacts API
User impact if declined: User looses contacts information after deleting one contact.
Testing completed: Unit tests added and QA verified.
Risk to taking this patch (and alternatives if risky): Low risk.
String or UUID changes made by this patch: None

Comment 35

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8551200 [details] [diff] [review]
Follow up

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 #): Contacts API
User impact if declined: User looses contacts information after deleting one contact.
Testing completed: Unit tests added and QA verified.
Risk to taking this patch (and alternatives if risky): Low risk.
String or UUID changes made by this patch: None

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/8c1935f12cdc

Comment 37

[Shine(Leave from Mozilla)]

Attachment: video

This problem is verified pass on latest build of Flame2.2.
See attachments: Verify_Flame2.2.MP4
Rate: 0/5

Flame 2.2 build:
Build ID               20150225002505
Gaia Revision          ca64f2fe145909f31af266b1730874051ba76c78
Gaia Date              2015-02-24 22:06:53
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/16804008c29f
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150225.041814
Firmware Date          Wed Feb 25 04:18:25 EST 2015
Bootloader             L1TC000118D0