[B2G][Helix][message][zhaotao]wired combination algorithm when handling sending sms to different phone numbers:phonebook digits match not work

RESOLVED FIXED in Firefox 28, Firefox OS v1.2

Status

Firefox OS
RIL
P1
normal
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: lecky, Assigned: ctai)

Tracking

(Depends on: 1 bug)

unspecified
1.2 C4(Nov8)
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:koi+, firefox26 wontfix, firefox27 wontfix, firefox28 fixed, b2g-v1.2 fixed)

Details

(Whiteboard: [FT:RIL])

Attachments

(1 attachment, 10 obsolete attachments)

(Reporter)

Description

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

Updated

5 years ago
Severity: normal → blocker
blocking-b2g: --- → hd?
Priority: -- → P1
(Reporter)

Comment 1

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

Comment 2

5 years ago
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;
            }
          }
(Reporter)

Updated

5 years ago
Flags: needinfo?(wchang)
Hi CTai,

Can you check this problem with the suggestions in comment 2?

Thanks.
Flags: needinfo?(wchang) → needinfo?(ctai)
Noming for koi
Severity: blocker → normal
blocking-b2g: hd? → koi?
I am looking into it now.
Flags: needinfo?(ctai)
Created attachment 806561 [details] [diff] [review]
bug-914060.patch v1.0

direct compare national number
Assignee: nobody → ctai
Attachment #806561 - Flags: review?(vyang)
(Reporter)

Comment 7

5 years ago
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
blocking-b2g: koi? → koi+
ctai, with this patch, I think you'll end up with not matching national form and international form for the same number.
Created attachment 811480 [details] [diff] [review]
bug-914060.patch v1.1
Attachment #806561 - Attachment is obsolete: true
Attachment #806561 - Flags: review?(vyang)
Attachment #811480 - Flags: review?(vyang)
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.
(Reporter)

Comment 13

4 years ago
Created attachment 811648 [details]
all.dmc
(Reporter)

Updated

4 years ago
Attachment #811648 - Attachment is obsolete: true
(Reporter)

Comment 14

4 years ago
hi,ctai,what is the progress of the issue?

if it has been fixed,can you help uplift the modification to V1.1HD branch?
(Reporter)

Updated

4 years ago
Flags: needinfo?(ctai)
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.
Flags: needinfo?(ctai)
ctai: is this ok for sprint 3? ending oct 25? thanks
Joe, Sprint 3 is OK for me.
Created attachment 817607 [details] [diff] [review]
bug-914060.patch v2.0
Attachment #811480 - Attachment is obsolete: true
Attachment #811480 - Flags: review?(vyang)
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.
Target Milestone: --- → 1.3 Sprint 3 - 10/25
Created attachment 818228 [details] [diff] [review]
bug-914060.patch v2.1
Attachment #817607 - Attachment is obsolete: true
Attachment #818228 - Flags: feedback?(vyang)
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)|
Attachment #818228 - Flags: feedback?(vyang)
Created attachment 818315 [details] [diff] [review]
bug-914060.patch v2.2
Attachment #818228 - Attachment is obsolete: true
Attachment #818315 - Flags: feedback?(vyang)
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.
Attachment #818315 - Flags: feedback?(vyang)
Created attachment 818899 [details] [diff] [review]
bug-914060.patch v2.3
Attachment #818315 - Attachment is obsolete: true
Attachment #818899 - Flags: feedback?(vyang)
Created attachment 818938 [details] [diff] [review]
bug-914060.patch v2.4
Attachment #818899 - Attachment is obsolete: true
Attachment #818899 - Flags: feedback?(vyang)
Attachment #818938 - Flags: feedback?(vyang)
Created attachment 819535 [details] [diff] [review]
bug-914060.patch v2.5
Attachment #818938 - Attachment is obsolete: true
Attachment #818938 - Flags: feedback?(vyang)
Attachment #819535 - Flags: feedback?(vyang)
In addition to Bug 914440, Bug 929356 might be highly related to this bug as well.
Duplicate of this bug: 914440

Updated

4 years ago
Whiteboard: [FT:RIL]

Updated

4 years ago
Target Milestone: 1.3 Sprint 3 - 10/25 → 1.2 C3(Oct25)
Target Milestone: 1.2 C3(Oct25) → 1.2 C4(Nov8)
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.
Attachment #819535 - Flags: feedback?(vyang)
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.
Created attachment 825145 [details] [diff] [review]
bug-914060.patch v2.6
Attachment #819535 - Attachment is obsolete: true
Attachment #825145 - Flags: review?(vyang)
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!!!
Attachment #825145 - Flags: review?(vyang) → review+
Created attachment 825173 [details] [diff] [review]
bug-914060.patch v2.7

Fix nitt.
Attachment #825145 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4195026ecb06
Status: UNCONFIRMED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/8afd2da9a91c
status-b2g-v1.2: --- → fixed
status-firefox26: --- → wontfix
status-firefox27: --- → wontfix
status-firefox28: --- → fixed
Component: Gaia::SMS → RIL
Depends on: 945711
Depends on: 960741
(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.

Updated

4 years ago
No longer depends on: 960741
Jason, we usually mark regressing bugs like this, right ?
Depends on: 1136211
Depends on: 1149802
You need to log in before you can comment on or make changes to this bug.