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

RESOLVED FIXED in Firefox OS v2.2

Status

Firefox OS
Gaia::E-Mail
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: Chaochao Huang, Assigned: jrburke)

Tracking

unspecified
2.1 S9 (21Nov)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(b2g-v1.4 wontfix, b2g-v2.0 wontfix, b2g-v2.1 wontfix, b2g-v2.2 fixed)

Details

Attachments

(6 attachments)

(Reporter)

Description

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

Comment 1

4 years ago
Created attachment 8509278 [details]
There  be two of the same contact in the contact list

There  be two of the same contact in the contact list

Comment 2

4 years ago
Those aren't the same contact (the
(Reporter)

Comment 3

4 years ago
(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?
Flags: needinfo?(squibblyflabbetydoo)

Comment 4

4 years ago
(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.
Flags: needinfo?(squibblyflabbetydoo)
(Reporter)

Comment 5

4 years ago
 I don't quite understand what you mean. Can you explain more? 
Thanks~ :)
Flags: needinfo?(squibblyflabbetydoo)

Comment 6

4 years ago
"Postmaster" and "postmaster" are different strings, and the difference has to be coming from somewhere.
Flags: needinfo?(squibblyflabbetydoo)
(Reporter)

Comment 7

4 years ago
Created attachment 8509316 [details]
same contact in the contact list

Sorry, last picture isnot appropriate. You can see this picture.
(Reporter)

Comment 8

4 years ago
Hi Jim, 

You can download a video about this issue from http://pan.baidu.com/s/1sjpvV4p
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?
Flags: needinfo?(chaochao.huang)

Comment 10

4 years ago
chaochao:

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

And the log required in comment 9
Summary: [dolphin][email]There will be two same contact in the contact list after adding recipient email address for the contact on the 163 mailbox. → [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.
(Reporter)

Comment 11

4 years ago
Created attachment 8510048 [details]
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 'Postermaster@163.com' in  the video. And add the log 'email_log' in attachment.
Flags: needinfo?(chaochao.huang)
(Reporter)

Comment 12

4 years ago
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  'Postermaster@163.com' 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.
(Reporter)

Comment 13

4 years ago
Hi  Andrew,

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

Thanks.
Flags: needinfo?(bugmail)
(Reporter)

Comment 14

4 years ago
Hi Andrew,

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

Comment 15

4 years ago
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 'Postermaster@163.com'.

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.
Flags: needinfo?(bugmail)
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.
(Assignee)

Comment 17

4 years ago
Created attachment 8514708 [details] [review]
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
Attachment #8514708 - Flags: review?(bugmail)
(Assignee)

Updated

4 years ago
Assignee: nobody → jrburke
Status: NEW → ASSIGNED
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.
Attachment #8514708 - Flags: review?(bugmail) → review+
(Reporter)

Comment 20

4 years ago
James, Could you please help? We need land this PR into v1.4. 
Thanks
Flags: needinfo?(jrburke)
(Assignee)

Comment 21

4 years ago
Created attachment 8515795 [details] [review]
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.
Flags: needinfo?(jrburke)
(Reporter)

Comment 22

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

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

Updated

4 years ago
status-b2g-v1.4: --- → affected
status-b2g-v2.0: --- → affected
status-b2g-v2.1: --- → affected
(Reporter)

Comment 23

4 years ago
Andrew, 

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

Thanks.
Flags: needinfo?(bugmail)
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.
Attachment #8515795 - Flags: review+
Attachment #8515795 - Flags: approval-gaia-v1.4?
Flags: needinfo?(bugmail)
See Also: → bug 1093105
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.
Attachment #8515795 - Flags: approval-gaia-v1.4?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1097504
(Assignee)

Comment 28

4 years ago
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.
(Assignee)

Comment 29

4 years ago
Created attachment 8521574 [details] [review]
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.
Attachment #8521574 - Flags: review?(bugmail)
Comment on attachment 8521574 [details] [review]
GELAM pull request, part 2

Thanks for the high quality investigation and the resulting comment!
Attachment #8521574 - Flags: review?(bugmail) → review+
(Assignee)

Comment 31

4 years ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 32

4 years ago
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.
Attachment #8515795 - Flags: approval-gaia-v1.4?
: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.
Flags: needinfo?(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.
Flags: needinfo?(wchang)
status-b2g-v1.4: affected → wontfix
status-b2g-v2.0: affected → wontfix
status-b2g-v2.1: affected → wontfix
status-b2g-v2.2: --- → fixed
Target Milestone: --- → 2.1 S9 (21Nov)
Attachment #8515795 - Flags: approval-gaia-v1.4?
You need to log in before you can comment on or make changes to this bug.