1087184 - [dolphin][email][v1.4]There will be two same contact in the contact list after adding recipient email address for the contact on the 163 mailbox.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::E-Mail, defect)

Description

[Chaochao Huang]

STR：
1. Open the Email app.
2. Load on the 163 mailbox.
3. Open an email.
4. Click msg-envelope-bar, and populate context menu.
5. Click 'Save new contact' on context menu.
6. Add contact and Click 'Done'.
7. Back to email APP automatically, click 'msg-back-btn'.
8. Open the smae email again.
9. Do the same things from 4 to 6

OBSERVED:
10. There will be two  same contact in the contact list  

EXPECTED:
11. There will not be two  same contact in the contact list

Comment 1

[Chaochao Huang]

Attachment: There be two of the same contact in the contact list

There  be two of the same contact in the contact list

Comment 2

[Jim Porter (:squib)]

Those aren't the same contact (the

Comment 3

[Chaochao Huang]

(In reply to Jim Porter (:squib) from comment #2)
> Those aren't the same contact (the

The details of two contacts are same. Why not the same contact?

Comment 4

[Jim Porter (:squib)]

(In reply to Chaochao Huang from comment #3)
> (In reply to Jim Porter (:squib) from comment #2)
> > Those aren't the same contact (the
> 
> The details of two contacts are same. Why not the same contact?

Well, I didn't mean to post that (I meant to hit back), but the case of the email addresses is different, and per the RFC, that means they're different addresses.

Comment 5

[Chaochao Huang]

 I don't quite understand what you mean. Can you explain more? 
Thanks~ :)

Comment 6

[Jim Porter (:squib)]

"Postmaster" and "postmaster" are different strings, and the difference has to be coming from somewhere.

Comment 7

[Chaochao Huang]

Attachment: same contact in the contact list

Sorry, last picture isnot appropriate. You can see this picture.

Comment 8

[Chaochao Huang]

Hi Jim, 

You can download a video about this issue from http://pan.baidu.com/s/1sjpvV4p

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

If you could provide a logcat as described at https://wiki.mozilla.org/Gaia/Email/RequiredBugInfo retrieved while reproducing, that could be very helpful.  It looks like we either aren't receiving or processing mozContacts updates appropriately.  Once the contact is updated we should be recognizing the contact and updating the DOM state.  In general, we always want a logcat with any email bugs.  Thanks!

Also, am I right in assuming this is Firefox OS v1.4?

Comment 10

[ying.xu]

chaochao:

Please add the test result of flame v1.4 and the possibilities of this bug

And the log required in comment 9

Comment 11

[Chaochao Huang]

Attachment: email_log

(In reply to Andrew Sutherland [:asuth] from comment #9)

Sorry for my late reply. Flame v1.4 also exists this issue. The reproduced rate is 100% for a specific email address as '[e-mail]' in  the video. And add the log 'email_log' in attachment.

Comment 12

[Chaochao Huang]

email/js/ext/mailapi/main-frame-setup.js

this.author = ContactCache.resolvePeep(wireRep.author);

   resolvePeep: function(addressPair) {
    var emailAddress = addressPair.address;
    var entry = this._contactCache[emailAddress], contact, peep;

After adding the email address  '[e-mail]' for the contact, click 'msg-back-btn' to Inbox, open ths same email again, you will find that the  'entry' is still null. In normal condition, the  'entry' should be mozContact or undefined instead of null.

Comment 13

[Chaochao Huang]

Hi  Andrew,

Plz see comment 11 and comment 12, and check this bug.

Thanks.

Comment 14

[Chaochao Huang]

Hi Andrew,

Could you help check this bug? Plz see comment 11 and comment 12. Thanks~

Comment 15

[James Burke [:jrburke]]

Andrew is in the middle of some other tasks, so I took a look. I was able to reproduce on master with a flame device.

The main issue is a casing one:

The email app asks navigator.mozContacts.find() to do a filterOp of 'equals' on the email address. However, the contacts DB seems to do some normalization to lower case because it does not find a match when we ask for '[e-mail]'.

Looking at the contacts db code:
https://mxr.mozilla.org/mozilla-central/source/dom/contacts/fallback/ContactDB.jsm#1270

'equals' is case sensitive. If I switch to 'startsWith' which is case insensitive, then we get a match. However, we do not want to use that, we want exact matches. I also tried 'match' but that is not correct for this kind of search, it is for telephone numbers.

The 'equals' filterOp also works if I do this change in our code:

diff --git a/apps/email/js/ext/mailapi.js b/apps/email/js/ext/mailapi.js
index ffb5f18..7749920 100644
--- a/apps/email/js/ext/mailapi.js
+++ b/apps/email/js/ext/mailapi.js
@@ -697,7 +697,7 @@ var ContactCache = exports.ContactCache = {
       var req = contactsAPI.find({
                   filterBy: ['email'],
                   filterOp: 'equals',
-                  filterValue: emailAddress
+                  filterValue: emailAddress.toLowerCase()
                 });
       var self = this, handleResult = function() {
         if (req.result && req.result.length) {


However, stepping back further, we likely want to do this normalization internally when we first create our address data structures, as we may want to do matching ourselves where we would not want case issues to mess up matches.

So I think the right longer term fix is perhaps in addressparser.js, to lower case the name as part of normalization work. There could be an issue of users wanting to see mixed case names for display though, so may need a bit more thought.

Comment 16

[Andrew Sutherland [:asuth] (he/him)]

Thanks for figuring out what's going on here, James!

The local part of the mail address is case-sensitive and so it would be inappropriate for us or addressparser to normalize the entire email address to be lowercase.  It appears ContactsDB creates a normalized variant of the email address for searching purposes only.  (makeImport creates a contact.search sub-obj which is what all the indices are built off of and uses toLowerCase on that.)

As such I think we have two action items here:
- Lowercase the email address when calling find, as your example fix already does.  But add a comment indicating that the mozContacts API lowercases email addresses for search purposes but has not actually normalized the real email address to lower-case.

- Put a "CONSIDER TODO SOMEDAY" in the result processor to handle the case where there are multiple matches and find the "best" match case-wise.  Presumably this would be us running along the email address and picking the one with the most matching characters.  In the normal, sane case, there'd be just one contact that matches and the spec is happy and real humans are happy.  In the weird case where the user actually wants different contacts for different email capitalization, they can be happy too.

The latter point is a little bit silly to seriously consider implementing, but it's a useful call-out to the potential issues we have for identity equivalent/invariants if we touch that code in the future.

Comment 17

[James Burke [:jrburke]]

Attachment: GELAM pull request

Pull request that does the toLowerCase and adds the comment about possibly doing best case match in the future.

Related gaia pull request is here:
https://github.com/mozilla-b2g/gaia/pull/25678

Comment 18

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8514708 [details] [review]
GELAM pull request

Looks good!  Of course, it turns out that we had some new persistent failures unrelated to this.  Specifically, I think a change in b2g event ordering in very recent builds surfaced a race related to account saving for test_mutation-style tests.  I've pushed a commit to your repo/pull request with a fix for that (details in commit message) and some test coverage for your change (where we're regrettably using a mock; deets in that commit too).

I leaving landing in your hands assuming the travis tests are green enough.

Comment 19

[James Burke [:jrburke]]

Merged in gaia master:
https://github.com/mozilla-b2g/gaia/commit/3c1668ad793d4f1a40831c46638acef38e0db230

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

GELAM master merge:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/commit/2715361b8682999524c15c3952226e64cafcf441

from pull request:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/346

Comment 20

[Chaochao Huang]

James, Could you please help? We need land this PR into v1.4. 
Thanks

Comment 21

[James Burke [:jrburke]]

Attachment: v1.4 gaia pull request

Here is a pull request for the basic fix for v1.4.

I have not tried it on device though as I do not have a v1.4 device readily configured. So it would be good to get confirmation from someone that it does fix the issue, also a bit out of practice for v1.4 changes, but it looks like the approval-gaia-v1.4+ flag might need to be set for it to qualify for landing.

Comment 22

[Chaochao Huang]

(In reply to James Burke [:jrburke] from comment #21)

James, I've checked the patch on the branch v1.4. Verify OK.

Comment 23

[Chaochao Huang]

Andrew， 

Could you help review the patch Comment 21 and land it into v1.4?

Thanks.

Comment 24

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8515795 [details] [review]
v1.4 gaia pull request

Patch is effectively identical to already reviewed patch in terms of implementation changes so my review carries-over fine, but explicit r=asuth on this new incarnation of the patch just in case.

[Approval Request Comment]
[Bug caused by]: mozContacts API edge case.  The under-documented API treats email addresses inconsistently for case-sensitivity purposes.
[User impact] if declined: The email app will never think that email addresses that have any upper-case contents in it are associated with contacts, even if the email app literally just helped the user create the contact.
[Testing completed]: Testing of the v1.4 patch on v1.4 by the bug reporter.  Trunk testing on trunk devices, automated test coverage with an updated mozContacts API that correctly mimics the true mozContacts API behaviour.  
[Risk to taking this patch] (and alternatives if risky): No risk.  It's lower-casing a string which will cause a db lookup to find a result instead of not finding a result.  Finding a result where we did not previously find a result is not going to cause new problems because we would already have found a result for the lower-case case.  The potential multiplicity issues are addressed by us always choosing the 0th match even if there are multiple matches.
[String changes made]: No l10n strings affected.

Comment 25

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8515795 [details] [review]
v1.4 gaia pull request

We found an edge case that causes breakage relating to the "group" syntax used in the "undisclosed-recipients:;" idiom when there are no listed recipients of a message.  We want to defer uplift until we've addressed the problem and have test coverage.

Comment 26

[Andrew Sutherland [:asuth] (he/him)]

addressparser (at least the one we use for 2.1/2.2) returns { name, group: [ {name, address}... ]}  instead of just { name, address } when it sees a group.

On trunk we probably want to:
- Create a flatAddressParser that has normalized output so that all entries in the array are of the form { name, address } with name optionally being null.
- Have the invariant that all addresses always look like that prior to contact normalization.

For v1.4 and likewise older stuff, we might want to just guard against "address" being null for this fix.  Or not uplifting the fix.

Comment 28

[James Burke [:jrburke]]

I filed bug 1097820 to track the full addressparser normalization, but will just close the specific issue via a simple emailAddress guard since this is preventing real world use of the email client right now, see bug 1097504.

Comment 29

[James Burke [:jrburke]]

Attachment: GELAM pull request, part 2

This pull request adds an emailAddress guard to the original pull request change. If this pull request is approved, then I will generate and land the gaia pull request with the same change, and create a v1.4 equivalent pull request. 

No plans for 2.0/2.1 yet as original fix not applied there, and this is enough of an edge case issue to avoid uplift if it is not seen as a blocker for those parties shipping with those versions.

Tested on device, and querying still works as expected. Tested with an actual undisclosed recipients email, which led to using '' as the filterValue instead of undefined to avoid getting back data in the query result.

Comment 30

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8521574 [details] [review]
GELAM pull request, part 2

Thanks for the high quality investigation and the resulting comment!

Comment 31

[James Burke [:jrburke]]

Part two of the fix:

Merged in GELAM master:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/commit/a6b5039a011a7ade9fa3f489f2831783b01e34aa

from pull request:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/354

---

Merged in Gaia master:
https://github.com/mozilla-b2g/gaia/commit/53bcb9fada8ec72c066464fea79c22d7c37de4d2

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

I will update the v1.4 pull request in a moment.

Comment 32

[James Burke [:jrburke]]

Comment on attachment 8515795 [details] [review]
v1.4 gaia pull request

This pull request is updated with both parts of the fix, so reactivating the previously removed approval request, per bug reporter's request.

[Approval Request Comment]
[Bug caused by]: mozContacts API edge case.  The under-documented API treats email addresses inconsistently for case-sensitivity purposes.

[User impact] if declined: The email app will never think that email addresses that have any upper-case contents in it are associated with contacts, even if the email app literally just helped the user create the contact.

[Testing completed]: Trunk testing on trunk devices, automated test coverage with an updated mozContacts API that correctly mimics the true mozContacts API behaviour.  

[Risk to taking this patch] (and alternatives if risky): No risk.  It's lower-casing a string which will cause a db lookup to find a result instead of not finding a result.  Finding a result where we did not previously find a result is not going to cause new problems because we would already have found a result for the lower-case case.  The potential multiplicity issues are addressed by us always choosing the 0th match even if there are multiple matches.

[String changes made]: No l10n strings affected.

Comment 33

[bhavana bajaj [:bajaj]]

:wayne do we need to land this on our trees for 1.4 ? To me this is an edge case and can ride the trains to get fixed.

Comment 34

[Wayne Chang [:wchang]]

Agree - let's not uplift this and let it ride an appropriate train.

(In reply to bhavana bajaj [:bajaj] from comment #33)
> :wayne do we need to land this on our trees for 1.4 ? To me this is an edge
> case and can ride the trains to get fixed.