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

VERIFIED FIXED in Firefox 38

Status

defect
VERIFIED FIXED
4 years ago
6 months ago

People

(Reporter: liuke, Assigned: ferjm)

Tracking

({regression})

unspecified
mozilla38
ARM
Gonk (Firefox OS)
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:2.2+, firefox36 wontfix, firefox37 wontfix, firefox38 fixed, b2g-v2.1 unaffected, b2g-v2.2 verified, b2g-master verified)

Details

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Posted file 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
(Reporter)

Comment 1

4 years ago
Posted video 1720.MP4
[Blocking Requested - why for this release]: Regression on a basic use case.
blocking-b2g: --- → 2.2?
QA Contact: 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
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
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
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell) → needinfo?(reuben.bmo)
QA Contact: ddixon
triage: serious regression, data loss.
blocking-b2g: 2.2? → 2.2+
Assignee: nobody → 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.
Assignee: francisco → nobody
Component: Gaia::Contacts → DOM: Contacts
Product: Firefox OS → Core
Gregor suggested we contact Julien here.
Flags: needinfo?(felash)
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.
Flags: needinfo?(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 :)
Flags: needinfo?(francisco) → needinfo?(ferjmoreno)
(Assignee)

Updated

4 years ago
Assignee: nobody → ferjmoreno
Flags: needinfo?(ferjmoreno)
Posted patch v1 (obsolete) — Splinter Review
This fixes the issue for me. Working on the tests now.
Flags: needinfo?(reuben.bmo)
Flags: needinfo?(felash)
(Assignee)

Updated

4 years ago
Attachment #8548942 - Attachment is patch: true
Posted patch v1Splinter Review
Now with tests
Attachment #8548942 - Attachment is obsolete: true
Attachment #8549036 - Flags: review?(anygregor)
Comment on attachment 8549036 [details] [diff] [review]
v1

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

Ugh thanks!
Attachment #8549036 - Flags: review?(anygregor) → review+
Blocks: 1072773
https://hg.mozilla.org/mozilla-central/rev/2c7342538c03
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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 ?
Flags: needinfo?(ferjmoreno)
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!
Flags: needinfo?(ferjmoreno)
Please request b2g37 approval on this when you get a chance :)
Flags: needinfo?(ferjmoreno)
Posted patch Follow upSplinter Review
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfad13c0c594
Flags: needinfo?(ferjmoreno)
Attachment #8551200 - Flags: review?(anygregor)
(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.
(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 :)
Can we use an object store within a transaction that wasn't created for that object store?
Flags: needinfo?(reuben.bmo)
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.
Flags: needinfo?(reuben.bmo)
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
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
I think we should follow-up in a separate bug then...
Most users only have a single object store, so How Hard Can It Be™? :)

I filed bug 1123903.
Attachment #8551200 - Flags: review?(reuben.bmo)
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!
Attachment #8551200 - Flags: review?(reuben.bmo) → review+
(Assignee)

Updated

4 years ago
Attachment #8551200 - Flags: review?(anygregor)
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Keywords: verifyme
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Duplicate of this bug: 1116930
Please request b2g37 approval on this when you get a chance.
Flags: needinfo?(ferjmoreno)
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
Flags: needinfo?(ferjmoreno)
Attachment #8549036 - Flags: approval-mozilla-b2g37?
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
Attachment #8551200 - Flags: approval-mozilla-b2g37?
Attachment #8549036 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8551200 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Posted video 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
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][MGSEI-Triage+]
Keywords: verifyme

Updated

6 months ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.