User Agent: Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 6.1; Trident/4.0; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; Media Center PC 6.0; .NET4.0C; .NET4.0E; aff-kingsoft-ciba; Zune 4.7) Steps to reproduce: 【Detail Description*】：wired combination algorithm when handling sending sms to different phone numbers:phonebook digits match not work 【Repro Steps*】： 1、use a chinese sim card,and boot the phone(by the way,i have customized the phonebook digits to 9) 2、open contact,and create a new contact,named AAAA,with phone number:123456789 3、open message,send sms to 123456789, 4、open message,send sms to 23456789 Actual results: 【Real Result*】： 3.send sms to 123456789 match to AAAA,and a new message thread is created 4.send sms to 23456789 match to AAAA too,the message merged with previous sms. This handling is totally wrong,because i have customized the phonebook digits match to 9,23456789 should not match 123456789,a new message thread should be created. By the way,in dialer app,phonebook digits match works fine,when call 123456789,AAAA will be matched,when call 23456789,AAAA will not be matched. 【Test Count*】：5 【Found Count*】：5 【Gaia commit ID*】:c0ea0a4943dc8d3751b07f5b5c5d3abe06364a14 【Gecko commit ID*】:170f9e477571127cd40997fa2abe262ed43f0e4d Expected results: 【Expect Result*】： 3.send sms to 123456789 should match to AAAA,and a new message thread should be created 4.send sms to 23456789 should not match to AAAA,and a new message thread should be created

by the way,there is another wired issue about message number match. sending sms to 23456789,sending sms to 3456789,sending sms to 456789,sendint sms to 56789 all come to one message thread.(i have customized the phonebook digits to 9)

hi,wayne,could you check the issue? i have checked the code in gecko: in gecko\dom\mobilemessage\src\ril\MobileMessageDatabaseService.js: in function findParticipantRecordByAddress, after the phone number parse success,storedAddress.endsWith(parsedAddress.nationalNumber) is called to try to match sms receiver's phone number. So this will cause phonebook digit match will not work correctly in message app. please check the following code snippet. for (let storedAddress of participantRecord.addresses) { let match = false; if (parsedAddress) { // 2-1) If input number is an international one, then a potential // participant must be stored as local type. Then just check // if stored number ends with the national number(987654321) of // the input number. if (storedAddress.endsWith(parsedAddress.nationalNumber)) { match = true; } }

Hi CTai, Can you check this problem with the suggestions in comment 2? Thanks.

Noming for koi

I am looking into it now.

direct compare national number

Hi BEATRIZ Could you accept this issue from Telefonica on V1.1HD?

I think we're using the "match" operation (from the Contacts API) to match phone numbers. What is the dialer doing ?

triage; koi+ to avoid future IOT issues

ctai, with this patch, I think you'll end up with not matching national form and international form for the same number.

julenw, Thanks for pointing out. (In reply to Julien Wajsberg [:julienw] from comment #10) > ctai, with this patch, I think you'll end up with not matching national form > and international form for the same number.

hi,ctai,what is the progress of the issue? if it has been fixed,can you help uplift the modification to V1.1HD branch?

hey lecky, it's not fixed yet, and it needs to be hd+ to land on v1.1hd. As it is currently (koi+) we'll land it only on the 1.2 branch.

ctai: is this ok for sprint 3? ending oct 25? thanks

Joe, Sprint 3 is OK for me.

Comment on attachment 817607 [details] [diff] [review] bug-914060.patch v2.0 Review of attachment 817607 [details] [diff] [review]: ----------------------------------------------------------------- Have to update participant store to reflect changes here. ::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js @@ +942,5 @@ > > let participantRecord = cursor.value; > for (let storedAddress of participantRecord.addresses) { > let match = false; > + let normalizedStoredAddress = PhoneNumberUtils.normalize(storedAddress, false); |storedAddress| is already normalized because we only store normalized addresses. @@ +946,5 @@ > + let normalizedStoredAddress = PhoneNumberUtils.normalize(storedAddress, false); > + if (normalizedAddress === normalizedStoredAddress) { > + match = true; > + } > + let parsedStroedAddress = PhoneNumberUtils.parse(normalizedStoredAddress); Use |parseWithMcc(storedAddress, null)| because by the time we're creating threads for new messages, we can't assure that we have the same SIM card with the one used when |storedAddress| was inserted. Besides, if there is an international version of that |storedAddress|, it had been inserted as well in a few lines later. @@ +954,3 @@ > match = true; > } > } We still need the "dom.phonenumber.substringmatching.<countryName>" part.

Comment on attachment 818228 [details] [diff] [review] bug-914060.patch v2.1 Review of attachment 818228 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js @@ +829,5 @@ > + } > + } > + if (foundWrong) { > + participantStore.delete(participantRecord.id).onsuccess = function(event) { > + let request = threadStore.index("participantIds").get(participantRecord.id); This won't work. |threadStore.participantIds| is a sorted array of participant id. The only way we can do is walk through threadStore and see if there is thread records whose |participantIds| attribute contains a given participant id. And, since we're going to walk through threadStore, we'd better walk through it only once. So: let invalidParticipantIds = []; participantStore.openCursor().onsuccess = function(event) { let cursor = event.target.result; if (cursor) { let participantRecord = cursor.value; // Check if this participant record is valid if (isInvalid(participantRecord)) { invalidParticipantIds.push(participantRecord.id); cursor.delete(); } cursor.continue(); return; } // Participant store cursor iteration done. if (!invalidParticipantIds.length) { return; } threadStore.openCursor().onsuccess = function(event) { let cursor = event.target.result; ... @@ +1103,5 @@ > let participantRecord = cursor.value; > for (let storedAddress of participantRecord.addresses) { > let match = false; > + if (normalizedAddress === storedAddress) { > + match = true; It can't be. If we have a direct match to any stored addresses, we'll get a valid record in let request = aParticipantStore.index("addresses").get(needles.pop()); @@ +1106,5 @@ > + if (normalizedAddress === storedAddress) { > + match = true; > + } > + let parsedStroedAddress = PhoneNumberUtils.parseWithMCC(storedAddress, null); > + if (parsedAddress && parsedStroedAddress) { typo: parsedStroedAddres @@ +1113,3 @@ > match = true; > } > + } else if (!parsedStroedAddress && parsedAddress) { just |else if (parsedAddress)| @@ +1113,4 @@ > match = true; > } > + } else if (!parsedStroedAddress && parsedAddress) { > + let parsedStroedAddress = PhoneNumberUtils.parseWithCountryName(storedAddress, parsedAddress.countryName); don't reuse variable names. @@ +1119,5 @@ > + || (parsedAddress.nationalNumber && parsedAddress.nationalNumber === parsedStroedAddress.nationalNumber)) { > + match = true; > + } > + } > + } else if (!parsedAddress && parsedStroedAddress) { just |else if (parsedStoredAddress)|

Comment on attachment 818315 [details] [diff] [review] bug-914060.patch v2.2 Review of attachment 818315 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js @@ +835,5 @@ > + } > + > + // Participant store cursor iteration done. > + if (!invalidParticipantIds.length) { > + return; Sorry, my bad in previous review. This has to be |next()|. @@ +840,5 @@ > + } > + > + threadStore.openCursor().onsuccess = function(event) { > + let cursor = event.target.result; > + let participantIds = cursor.participantIds; You have to check availability of |cursor.value|. We can have an empty threadStore while having a non-empty participant store. Just do as you have done to participant store traversal. threadStore.openCursor().onsuccess = function(event) { let cursor = event.target.result; if (cursor) { let threadRecord = cursor.value; // Delete invalid thread record and continue. return; } // Thread store cursor iteration done. @@ +842,5 @@ > + threadStore.openCursor().onsuccess = function(event) { > + let cursor = event.target.result; > + let participantIds = cursor.participantIds; > + let foundInvalid = false; > + for (let invalidParticipantId in invalidParticipantIds) { nit: for (let ... of ...) @@ +901,5 @@ > + notifyResult(Cr.NS_ERROR_FAILURE); > + return; > + } > + > + let updateMessageRecord = function (threadId) { You don't need this. We have already |messageRecord.id| and that doesn't change throughout the process. The thing you'll need is to create and thread record and update messageRecord for just once.

In addition to Bug 914440, Bug 929356 might be highly related to this bug as well.

Comment on attachment 819535 [details] [diff] [review] bug-914060.patch v2.5 Review of attachment 819535 [details] [diff] [review]: ----------------------------------------------------------------- Try to avoid long long functions and reuse whenever possible. I still have concern about the duplicated saveRecord lines. Let's see what can we do next. ::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js @@ +863,5 @@ > + for (let ix = 0 ; ix < participantRecord.addresses.length - 1; ix++) { > + address1 = participantRecord.addresses[ix]; > + for (let iy = ix + 1 ; iy < participantRecord.addresses.length; iy ++) { > + address2 = participantRecord.addresses[iy]; > + if (!bestGuessMatch(address1, address2)) { As pointed out below, have: let entries = []; for (let addr of participantRecord.addresses) { entries.push({ normalized: addr, parsed: PhoneNumberUtils.parseWithMCC(addr, null); }) } for (let ix = 0 ; ix < entries.length - 1; ix++) { let entry1 = entries[ix]; for (let iy = ix + 1 ; iy < entries.length; iy ++) { let entry2 = entries[iy]; if (!matchPhoneNumbers(entry1.normalized, entry1.parsed, entry2.normalized, entry2.parsed)) { return true; } } } return false; @@ +1180,5 @@ > let match = false; > + let parsedStoredAddres = PhoneNumberUtils.parseWithMCC(storedAddress, null); > + if (parsedAddress && parsedStoredAddres) { > + if ((parsedAddress.internationalNumber && parsedAddress.internationalNumber === parsedStoredAddres.internationalNumber) > + || (parsedAddress.nationalNumber && parsedAddress.nationalNumber === parsedStoredAddres.nationalNumber)) { We can have some utility functions: function matchParsedPhoneNumbers(addr1, parsedAddr1, addr2, parsedAddr2) { if ((parsedAddr1.internationalNumber && parsedAddr1.internationalNumber === parsedAddr2.internationalNumber) || (parsedAddr1.nationalNumber && parsedAddr1.nationalNumber === parsedAddr2.nationalNumber)) { return true; } if (parsedAddr1.countryName != parsedAddr2.countryName) { return false; } let ssPref = "dom.phonenumber.substringmatching." + parsedAddr1.countryName; if (Services.prefs.getPrefType(ssPref) != Ci.nsIPrefBranch.PREF_INT) { return false; } let val = Services.prefs.getIntPref(ssPref); return addr1.length > val && addr2.length > val && normalizedAddress.slice(-val) === storedAddress.slice(-val); }, function matchPhoneNumbers(addr1, parsedAddr1, addr2, parsedAddr2) { if (parsedAddr1 && parsedAddr2) { return matchParsedPhoneNumbers(addr1, parsedAddr1, addr2, parsedAddr2); } if (parsedAddr1) { parsedAddr2 = PhoneNumberUtils.parseWithCountryName(addr2, parsedAddr1.countryName); if (parsedAddr2) { return matchParsedPhoneNumbers(addr1, parsedAddr1, addr2, parsedAddr2); } return false; } if (parsedAddr2) { parsedAddr1 = PhoneNumberUtils.parseWithCountryName(addr1, parsedAddr2.countryName); if (parsedAddr1) { return matchParsedPhoneNumbers(addr1, parsedAddr1, addr2, parsedAddr2); } } return false; } Then we have here: let parsedStoredAddres = PhoneNumberUtils.parseWithMCC(storedAddress, null); let match = this.matchPhoneNumbers(normalizedAddress, parsedAddress, storedAddress, parsedStoredAddress); @@ +1597,5 @@ > + // add it into participants because we know that number is our own. > + // 3. receivers.length >= 2 > + // If the receivers contain multiple phone numbers, we need to add all > + // of them but not the user's own number into participants. > + if (receivers.length >= 2) { nit: bail-out early. @@ +1638,5 @@ > } > return; > } > let threadParticipants = [aMessage.sender]; > if (aMessage.type == "mms" && !DISABLE_MMS_GROUPING_FOR_RECEIVING) { Please move condition |!DISABLE_MMS_GROUPING_FOR_RECEIVING| into |findReceivedMMSThreadParticipants|: if (DISABLE_MMS_GROUPING_FOR_RECEIVING) { return; } And please rename it to 'fillReceivedMmsThreadParticipants' for it doesn't return anything.

Comment on attachment 819535 [details] [diff] [review] bug-914060.patch v2.5 Review of attachment 819535 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js @@ +911,5 @@ > + return; > + } > + wrongThreads.push(threadRecord.id); > + threadCursor.delete(); > + threadCursor.continue(); if (foundInvalid) { wrongThreads.push(threadRecord.id); threadCursor.delete(); } threadCursor.continue(); return; @@ +915,5 @@ > + threadCursor.continue(); > + return; > + } > + let ix = 0; > + let createUpdateThreadAndParticipant = function (ix) { Please have following lines before this line: if (!wrongThreads.length) { next(); return; } Then you can have: (function createUpdateThreadAndParticipant(ix) { .... })(0); @@ +929,5 @@ > + } > + createUpdateThreadAndParticipant(ix); > + return; > + } > + let messageRecord = messageCursor.value; nit: please insert an empty line before this line. @@ +934,5 @@ > + let timestamp = messageRecord.timestamp; > + let threadParticipants = []; > + // Recaculate the thread participants of received message. > + if (messageRecord.delivery === DELIVERY_RECEIVED) { > + threadParticipants = [messageRecord.sender]; if (messageRecord.delivery === DELIVERY_RECEIVED || messageRecord.delivery === DELIVERY_NOT_DOWNLOADED) { threadParticipants.push(messageRecord.sender); @@ +943,5 @@ > + // Recaculate the thread participants of sent messages and error > + // messages. In error sms messages, we don't have error received sms. > + // In received MMS, we don't update the error to deliver field but > + // deliverStatus. So we only consider sent message in DELIVERY_ERROR. > + if (messageRecord.delivery === DELIVERY_SENT || |else if| @@ +951,5 @@ > + } else if (messageRecord.type == "mms") { > + threadParticipants = messageRecord.receivers; > + } > + } > + if (messageRecord.delivery === DELIVERY_NOT_DOWNLOADED) { Then we don't need this. @@ +962,5 @@ > + function (threadRecord, > + participantIds) { > + if (!participantIds) { > + debug("participantIds is empty!"); > + return; nit: indentation @@ +991,5 @@ > + // Setup participantIdsIndex. > + messageRecord.participantIdsIndex = []; > + for each (let id in participantIds) { > + messageRecord.participantIdsIndex.push([id, timestamp]); > + } Move re-assigning |messageRecord.participantIdsIndex| before this if-block because we still need them when creating new thread record is necessary. @@ +997,5 @@ > + messageCursor.continue(); > + return; > + } > + > + threadStore.add({participantIds: participantIds, Please have: let threadRecord = { ... }; threadStore.add(threadRecord).onsuccess = ... @@ +1013,5 @@ > + // Setup participantIdsIndex. > + messageRecord.participantIdsIndex = []; > + for each (let id in participantIds) { > + messageRecord.participantIdsIndex.push([id, timestamp]); > + } Remove these lines.

Comment on attachment 825145 [details] [diff] [review] bug-914060.patch v2.6 Review of attachment 825145 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js @@ +818,5 @@ > + let entry1 = entries[ix]; > + for (let iy = ix + 1 ; iy < entries.length; iy ++) { > + let entry2 = entries[iy]; > + if (!self.matchPhoneNumbers(entry1.normalized, entry1.parsed, > + entry2.normalized, entry2.parsed)) { nit: alignment ::: dom/mobilemessage/tests/marionette/test_getthreads.js @@ +371,5 @@ > +//One participant with two aliased addresses, participants = ["555211018"]. > +tasks.push(sendMessage.bind(null, "555211018", "thread 18-1")); > +tasks.push(sendMessage.bind(null, "+5511555211018", "thread 18-2")); > +checkFuncs.push(checkThread.bind(null, ["thread 18-1", "thread 18-2"], > + "thread 18-2", 0, ["555211018"])); Aewsome!!!

Fix nitt.

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #36) > https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/8afd2da9a91c This backport was done incorrectly and broke database upgrading. See bug 960741.

Jason, we usually mark regressing bugs like this, right ?