885280 - [SMS] The sms app does not group together correctly the numbers with prefix

Status: VERIFIED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[rafael.marquez]

V1-train
Gecko: 856e202
Gaia: aea6834
Ril: Moz Ril

*Procedure
1. Open sms app
2. Send a sms to the number 666 666 666
3. Send a sms to the number 0034 666 666 666

*Expected Result
Only a conversation is created

*Actual Result
Two conversation are created

Comment 1

[rafael.marquez]

The same bug is reproduced if you have a contact with the number 0034 666 666 666 and you send an SMS to the number 0034 666 666 666 and another to the number 666 666 666(two conversations are created)

Comment 2

[Rick Waldron [:rwaldron]]

Updating the component. This should be handled by mozMobileMessage when determining the threadId for the newly created message.

Comment 3

[Lukas Blakk [:lsblakk] use ?needinfo]

Triage suspects this is a regression - if it is that helps lean more towards blocking - can you confirm?  Only if this is a regression from 1.0.1 can this be a blocker at this time.

Comment 4

[Natalya Kot [:nkot]]

I am trying to reproduce this issue within US, unable to reproduce
 1. send an SMS to a number 650-933-6961
 2. send an SMS to a number 1-650-933-6961
--> one SMS thread is created

or it's not the case... ?

Leo
Gecko  http://hg.mozilla.org/releases/mozilla-b2g18/rev/62049a1c5d36
Gaia   885b874029d8143035765ae903c147a9415fcc52
Build  20130624070224
Version 18.1

Comment 5

[Rick Waldron [:rwaldron]]

@nkot this is correct behaviour(In reply to nkot from comment #4)
> I am trying to reproduce this issue within US, unable to reproduce
>  1. send an SMS to a number 650-933-6961
>  2. send an SMS to a number 1-650-933-6961
> --> one SMS thread is created
> 
> or it's not the case... ?

This is the same behaviour I see.

> 
> Leo
> Gecko  http://hg.mozilla.org/releases/mozilla-b2g18/rev/62049a1c5d36
> Gaia   885b874029d8143035765ae903c147a9415fcc52
> Build  20130624070224
> Version 18.1

Comment 6

[isabelrios]

This is reproducible:
v1-train
Gecko-26944ea
Gaia-05ff7b4

Please see screenshot attached.
For the same number with and without country code, two threads are created

Comment 7

[isabelrios]

Attachment: number with and without country code

Comment 8

[isabelrios]

I tried with v1.0.1 and cannot reproduce that. Messages to the same number with and without country code are grouped in the same thread

Comment 9

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Gregor, is this the same thing as bug 883770?

Comment 10

[Gene Lian (I already quit Mozilla)]

(In reply to Isabel Rios [:isabel_rios] from comment #7)
> Created attachment 767130 [details]
> number with and without country code

Hi Isabel,

Thanks for the screen shot! Have your ever tried to killed the Message App and launched that again to see if this issue is still present?

I'd believe the backend should be able to put these two messages into the same thread, because the Gecko didn't modify the logic for creating threads recently. I'm wondering the Gaia end doesn't call the getThread(...) again to refresh the thread list.

Comment 11

[isabelrios]

(In reply to Gene Lian [:gene] from comment #10)
> (In reply to Isabel Rios [:isabel_rios] from comment #7)
> > Created attachment 767130 [details]
> > number with and without country code
> 
> Hi Isabel,
> 
> Thanks for the screen shot! Have your ever tried to killed the Message App
> and launched that again to see if this issue is still present?
> 
> I'd believe the backend should be able to put these two messages into the
> same thread, because the Gecko didn't modify the logic for creating threads
> recently. I'm wondering the Gaia end doesn't call the getThread(...) again
> to refresh the thread list.

Hi Gene, 
I tried killing the app and even powering the device off and on, the messages are not grouped.

Comment 12

[Gene Lian (I already quit Mozilla)]

Thanks Check for taking this! Really appreciate!

Comment 13

[Gene Lian (I already quit Mozilla)]

s/Check/Chuck/

Comment 14

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

I have tried to compare the difference of codes between central and b2g18 1.0.1, it shows that there is no difference in code involving message grouping, the call flow is [1] -> [2] -> [3].
And according the flow, "666-666-666" and "0034-666-666-666" will be treated as different number. Because the |defaultRegion| is null and these numbers are not start with '+', the |endsWidth()| test will never be applied.
Maybe the grouping issue is handled in gaia or elsewhere.

But I found in ITU-T E.164, Clause 12[6], shows that except prefix '+', prefix '00' also can be used as international prefix, which is not handled in PhoneNumber.jsm now.

So I think adding support for prefix '00' in PhoneNumber.jsm can solve the issue.

[1] MobileMessageDatabaseService.js : findParticipantRecordByAddress()
    central : http://hg.mozilla.org/mozilla-central/file/3b955f306226/dom/mobilemessage/src/ril/MobileMessageDatabaseService.js
    b2g18 1.0.1 : http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/file/9c62297d11b0/dom/sms/src/ril/SmsDatabaseService.js
[2] PhoneNumberUtils.jsm : parseWithMCC()
    central : http://hg.mozilla.org/mozilla-central/file/3b955f306226/dom/phonenumberutils/PhoneNumberUtils.jsm
    b2g18 1.0.1 : http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/file/9c62297d11b0/dom/phonenumberutils/PhoneNumberUtils.jsm
[3] PhoneNumber.jsm : parsenumber()
    central : http://hg.mozilla.org/mozilla-central/file/3b955f306226/dom/phonenumberutils/PhoneNumber.jsm
    b2g18 1.0.1 : http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/file/9c62297d11b0/dom/phonenumberutils/PhoneNumber.jsm
[4] http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/file/9c62297d11b0/dom/phonenumberutils/PhoneNumber.jsm#l331
[5] http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/file/9c62297d11b0/dom/sms/src/ril/SmsDatabaseService.js#l864
[6] http://www.itu.int/rec/dologin_pub.asp?lang=e&id=T-REC-E.164-201011-I!!PDF-E&type=items

Comment 15

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: WIP - Treat prefix "00" in address as prefix of international number.

Treat prefix "00" in address as prefix of international number as mentioned in comment 14.

Comment 16

[Gene Lian (I already quit Mozilla)]

Comment on attachment 768230 [details] [diff] [review]
WIP - Treat prefix "00" in address as prefix of international number.

Review of attachment 768230 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!

Comment 17

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

Comment on attachment 768230 [details] [diff] [review]
WIP - Treat prefix "00" in address as prefix of international number.

Review of attachment 768230 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you :)

Comment 18

[Gregor Wagner [:gwagner]]

Please add tests and also create an upstream PR for https://github.com/andreasgal/PhoneNumber.js

Thanks!

Comment 19

[Jason Smith [:jsmith]]

Looks like we've got a r+ here, so we have the information we need to move forward to fix this bug. So we probably don't need a regression window now. Feel free to set the flag again if you still need it.

Comment 20

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0001. Treat prefix "00" in address as prefix of international number.

Comment 21

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0002. Add test case for prefix "00".

Comment 22

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Try : https://tbpl.mozilla.org/?tree=Try&rev=d52f185609dd
Test failed while parsing number "0011-650-333-6000"

21:14:18     INFO -  try: 0011-650-333-6000, SG
21:14:18     INFO -  4142 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/phonenumberutils/tests/test_phonenumber.xul | got: 0011-650-333-6000 SG

It seems that some carriers provide special prefixes, '001' in the test case, for dialing international number for special rate or service.
In such case, we have to replace the whole prefix, '001' in the test case, into '+' instead of replace '00' only.
So the solution of replacing prefix '00' into '+' directly can't work for all and might cause other problem.

We are looking for other solutions. :(

Comment 23

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Based on the result of this try run : https://tbpl.mozilla.org/?tree=Try&rev=017a08e470cb
The test cases passes with patch 0002 along, which means we can handle the prefix correctly without patch 0001.

The difference between test case and SMS DB is that, SMS DB[1] doesn't assign a default MCC, so the Phone Number Utility can't handle prefix properly.

The new solution is get MCC from RIL and provide it to |PhoneNumberUtils.parseWithMCC()| in SMS DB.

But there might be a problem if user changes SIM to different country with different prefix pattern(maybe on business trip or something). This will affect the phone number parsing and the grouping will be wrong again.

[1] https://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js#846

Comment 24

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0001. Use country of device in parseWithMCC() if no MCC is provided.

Phone Number Utility already has the ability to handle IDD, but requires a country code.
So if MCC is not assigned, use the MCC of device.

This doesn't solve the issue of user changing SIM card with different MCC(apply different IDD rule based on SIM card) or no SIM card in device(apply IDD of default MCC, now it's hard-coded to Brazil).
The only way I can come up with to resolve this is, save the MCC of every SMS, and parse number with it.
This solution needs lots of work, and considering this is a rare use case, I prefer we can ignore it for now.

Comment 25

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0002. Add test case for prefix "00".

Rebase.

Comment 26

[Gregor Wagner [:gwagner]]

(In reply to Chuck Lee [:chucklee] from comment #24)
> Created attachment 772009 [details] [diff] [review]
> 0001. Use country of device in parseWithMCC() if no MCC is provided.

This is exactly what PhoneNumberUtils.Parse does. No?

Comment 27

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

Attachment: patch

Use 'PhoneNumberUtils.parse(number)' instead of 'PhoneNumberUtils.parseWithMCC(number, null)' for phone number parsing.  Parsed international number of 01115555211014 is [phone].  Address this possible difference as well.

Also add test cases for various phone number match cases in MobileMessageDatabaseService.  Originated from bug 849739 attachment 734592 [details] [diff] [review].

Comment 28

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

Attachment: patch : v2

Remove one redundant comment line.  Sorry for the noises.

Comment 29

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

https://tbpl.mozilla.org/?tree=Try&rev=3c7f394fddf1

Comment 30

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

Attachment: patch : v3

Rebase.

Comment 31

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

https://hg.mozilla.org/projects/birch/rev/f471c8057f90

Comment 32

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

For b2g18 patch, I think we'd at least need changes from bug 871433, which normalize phone numbers before searching.

Comment 33

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

Attachment: b2g18 patch

depends on bug 871433 attachment 775532 [details] [diff] [review].

Comment 34

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f471c8057f90

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/6062fdf2deb8

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/6062fdf2deb8

Comment 37

[isabelrios]

Re-opening since looks like this patch is causing a problem in b2g18:

Trying to send a message to '123123123' does not work. After tapping on send, the 'To' and the 'Text' field are removed. Nothing is sent, there is not any error message shown.

This is working fine with 07/15 build: Gecko-2b310a1-Gaia-d52ce22

But not with 07/16 one: Gecko-ea7cfd4-Gaia-c8f432c

Log:
E/GeckoConsole(  461): Content JS LOG at app://sms.gaiamobile.org/js/message_manager.js:389 in onError: Error Sending: {}
E/GeckoConsole(  109): [JavaScript Error: "[Exception... "Data provided to an operation does not meet requirements."  code: "0" nsresult: "0x80660005 (DataError)"  location: "jar:file:///system/b2g/omni.ja!/components/MobileMessageDatabaseService.js Line: 850"]" {file: "jar:file:///system/b2g/omni.ja!/components/MobileMessageDatabaseService.js" line: 850}]
E/GeckoConsole(  109): [JavaScript Error: "aMessageRecord is undefined" {file: "jar:file:///system/b2g/omni.ja!/components/MobileMessageDatabaseService.js" line: 751}]

Comment 38

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

Confirmed. Object returned from |PhoneNumberUtils.parse()| may have a null |internationalNumber|, so we may have |needles = ['123123123', null]|, which skips storing that message into object store in following steps.

See: http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js#837

Comment 39

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

Attachment: m-c follow-up

Check nullity of |parsedAddress.internationalNumber|.

Comment 40

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

Attachment: b2g18 follow-up

Comment 41

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

https://hg.mozilla.org/projects/birch/rev/2df208471aed
https://hg.mozilla.org/releases/mozilla-b2g18/rev/21ceb62ae689

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2df208471aed

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/21ceb62ae689

Comment 44

[isabelrios]

Cannot reproduce the original bug or the problem described in comment 37.

Verified with Unagi device 07/18 v1-train build:
Gecko-4bc3f2f
Gaia-f1d2e3f
ref ril