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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect, P1)

Description

[lecky]

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

Comment 1

[lecky]

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)

Comment 2

[lecky]

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;
            }
          }

Comment 3

[Wayne Chang [:wchang]]

Hi CTai,

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

Thanks.

Comment 4

[Wayne Chang [:wchang]]

Noming for koi

Comment 5

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

I am looking into it now.

Comment 6

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-914060.patch v1.0

direct compare national number

Comment 7

[lecky]

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

Comment 8

[Julien Wajsberg [:julienw]]

I think we're using the "match" operation (from the Contacts API) to match phone numbers.

What is the dialer doing ?

Comment 9

[Joe Cheng [:jcheng] (please needinfo)]

triage; koi+ to avoid future IOT issues

Comment 10

[Julien Wajsberg [:julienw]]

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

Comment 11

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-914060.patch v1.1

Comment 12

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

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.

Comment 13

[lecky]

Attachment: all.dmc

Comment 14

[lecky]

hi,ctai,what is the progress of the issue?

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

Comment 15

[Julien Wajsberg [:julienw]]

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.

Comment 16

[Joe Cheng [:jcheng] (please needinfo)]

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

Comment 17

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Joe, Sprint 3 is OK for me.

Comment 18

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-914060.patch v2.0

Comment 19

[Vicamo Yang [:vicamo][: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.

Comment 20

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-914060.patch v2.1

Comment 21

[Vicamo Yang [:vicamo][: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)|

Comment 22

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-914060.patch v2.2

Comment 23

[Vicamo Yang [:vicamo][: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.

Comment 24

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-914060.patch v2.3

Comment 25

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-914060.patch v2.4

Comment 26

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-914060.patch v2.5

Comment 27

[Gene Lian (I already quit Mozilla)]

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

Comment 29

[Vicamo Yang [:vicamo][:vyang]]

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 30

[Vicamo Yang [:vicamo][: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.

Comment 31

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-914060.patch v2.6

Comment 32

[Vicamo Yang [:vicamo][: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, "[phone]", "thread 18-2"));
> +checkFuncs.push(checkThread.bind(null, ["thread 18-1", "thread 18-2"],
> +                                 "thread 18-2", 0, ["555211018"]));

Aewsome!!!

Comment 33

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-914060.patch v2.7

Fix nitt.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/4195026ecb06

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4195026ecb06

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/8afd2da9a91c

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 38

[Julien Wajsberg [:julienw]]

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