Julien Wajsberg [:julienw] Reporter
	Description • 9 years ago

+++ This bug was initially created as a clone of Bug #1136147 +++

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Ubuntu Chromium/37.0.2062.120 Chrome/37.0.2062.120 Safari/537.36

Steps to reproduce:

Using Firefox OS 1.3 on a ZTE Open C with Build Identifier 20140523093151:
1. Open the SMS app, click the Compose icon.
2. In the "To:" field, enter any 15-digit phone number.  I tried the iNum test number (see http://www.inum.net/test-sms-and-voice/ ), both as "883510000000094" and "+883510000000094".  The results were the same in both cases.
3. Type a message in the Message box (I entered my email address).
4. Press Send.


Actual results:

The SMS application returned to an empty Compose screen and no message was sent.

Note that the results are the same if you first add a contact with the 15-digit phone number and attempt to send an SMS to that contact, rather than just typing the number into the "To:" field.


Expected results:

The SMS should have appeared in the message history of the SMS application, and the SMS should have been sent to the iNum test number.  I know my carrier supports these, as I have sent an SMS to the iNum test number before with a different phone and it sent me an email indicating receipt.

It's possible this is related to https://bugzilla.mozilla.org/show_bug.cgi?id=899961 though they seem slightly different.  Also, this issue happens with valid E.164 phone numbers, not just 20+-digit numbers.

	Julien Wajsberg [:julienw] Reporter
	Comment 1 • 9 years ago

Hey Bevis, here is the error I get in the logs on a master build:

I get this error:
02-24 18:19:18.129  4398  4398 W GeckoConsole: [JavaScript Error: "TypeError: md.internationalPrefix is undefined" {file: "resource://gre/modules/PhoneNumber.jsm" line: 323}]

	Julien Wajsberg [:julienw] Reporter
	Comment 2 • 9 years ago

I think this is bad enough to warrant a blocking status. Maybe even 2.1?

	Bevis Tseng[:bevis][:btseng](Exited)
	Comment 3 • 9 years ago

(In reply to Julien Wajsberg [:julienw] from comment #1)
> Hey Bevis, here is the error I get in the logs on a master build:
> 
> I get this error:
> 02-24 18:19:18.129  4398  4398 W GeckoConsole: [JavaScript Error:
> "TypeError: md.internationalPrefix is undefined" {file:
> "resource://gre/modules/PhoneNumber.jsm" line: 323}]

I was not able to reproduce this problem in master branch locally, even the mcc was hard-coded to '208' (FR) :(

Since this error was thrown from PhoneNumber.jsm, shall we have the file owner to comment instead?

	Julien Wajsberg [:julienw] Reporter
	Comment 4 • 9 years ago

Did you try the exact phone number from comment 0? (I used the version with the '+' prefix)

	Bevis Tseng[:bevis][:btseng](Exited)
	Comment 7 • 9 years ago

(In reply to Julien Wajsberg [:julienw] from comment #4)
> Did you try the exact phone number from comment 0? (I used the version with
> the '+' prefix)

Yes, I tried both and the sms can be sent properly.
Please see the screenshots.

	Julien Wajsberg [:julienw] Reporter
	Comment 8 • 9 years ago

My MCC/MNC is: MCC: 208, MNC: 01

Do you think this can change how the number is parsed?
Who's the owner for PhoneNumber ?

	Julien Wajsberg [:julienw] Reporter
	Comment 9 • 9 years ago

Hey Gregor, are you the right person to ask for the PhoneNumber lib?

	Bevis Tseng[:bevis][:btseng](Exited)
	Comment 10 • 9 years ago

(In reply to Julien Wajsberg [:julienw] from comment #8)
> My MCC/MNC is: MCC: 208, MNC: 01
> 
> Do you think this can change how the number is parsed?
MCC will be used to identify the region (208 -> FR) where the device is located.
For the number without '+', it is possible. [1]

[1] https://dxr.mozilla.org/mozilla-central/source/dom/phonenumberutils/PhoneNumber.jsm#318

	Gregor Wagner [:gwagner] Assignee
	Comment 11 • 9 years ago

Fixing upstream first:
https://github.com/andreasgal/PhoneNumber.js/pull/37

	Gregor Wagner [:gwagner] Assignee
	Comment 12 • 9 years ago

Upstream fix. Will provide gecko patch once I get r+

	Gregor Wagner [:gwagner] Assignee
	Comment 13 • 9 years ago

(In reply to Julien Wajsberg [:julienw] from comment #4)
> Did you try the exact phone number from comment 0? (I used the version with
> the '+' prefix)

I am confused. We shouldn't hit the exception from comment 1 if you use the '+' prefix. You should hit http://hg.mozilla.org/mozilla-central/file/0b3c520002ad/dom/phonenumberutils/PhoneNumber.jsm#l314

	Gregor Wagner [:gwagner] Assignee
	Comment 14 • 9 years ago

We are sending an invalid country name (001) here: http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/gonk/MobileMessageDB.jsm#1641

	Wesley Huang [:wesley_huang] (EPM) (NI me)
	Comment 15 • 9 years ago

triage: breaking basic function.

	Bevis Tseng[:bevis][:btseng](Exited)
	Comment 16 • 9 years ago

(In reply to Gregor Wagner [:gwagner] from comment #14)
> We are sending an invalid country name (001) here:
> http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/gonk/
> MobileMessageDB.jsm#1641

This is strange because the |countryName| sent from MobileMessageDB.jsm is also parsed from PhoneNumberUtils.parse(). :(

	Gregor Wagner [:gwagner] Assignee
	Comment 17 • 9 years ago

I can make the +883510000000094 case work. 883510000000094 doesn't work on android. can someone confirm that this case should work?

	Julien Wajsberg [:julienw] Reporter
	Comment 20 • 9 years ago

I don't think the version wtithout '+' should work.

	Michael Henretty [:mikehenrty][:mhenretty]
	Comment 22 • 9 years ago

Fixed in upstream: https://github.com/andreasgal/PhoneNumber.js/commit/18411320c4b25ff267a14ba5e0eedbd2e5c87a25

	Michael Henretty [:mikehenrty][:mhenretty]
	Comment 23 • 9 years ago

Comment on attachment 8572274 [details] [diff] [review]
patch

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

Took me a while to remember what these files do. I think overall the approach makes sense, and I especially like that we are adding protections against unexpected input. There are two things I would like to see changed before I can give the r+.

::: dom/phonenumberutils/PhoneNumber.jsm
@@ +315,5 @@
>      if (number[0] === '+')
>        return ParseInternationalNumber(number.replace(LEADING_PLUS_CHARS_PATTERN, ""));
>  
>      // Lookup the meta data for the given region.
>      var md = FindMetaDataForRegion(defaultRegion.toUpperCase());

Let's add some protection here such that when FindMetaDataForRegion doesn't return an object, we console.log an error and return null rather than throwing when trying to call md.internationalPrefix.test(number)).

::: dom/phonenumberutils/PhoneNumberUtils.jsm
@@ +155,5 @@
> +
> +    if (!this._countryNameCache[aCountryName]) {
> +      return null;
> +    }
> +

The flow here seems a little weird; if we get a country name multiple times that is not in the MCC_ISO3166_TABLE we will end up redundantly populating the cache each time. From what I understand, this table won't change during runtime?

I think better here would be something like:

```
if (!this.isValidCountryName(aCountryName)) {
  return null;
}
return PhoneNumber.Parse(aNumber, countryName);
```

Then, isValidCountryName could look something like:
```
if (!this._countryNameCache) {
  this._countryNameCache = Object.create(null);
  let keys = Object.keys(MCC_ISO3166_TABLE);
  for (let i = 0; i < keys.length; i++) {
    this._countryNameCache[keys[i]] = true;
  }
}
return !!this._countryNameCache(aCountryName);
```

That way you only populate the cache once, and it's more semantic that you are validating the country name itself.

	Michael Henretty [:mikehenrty][:mhenretty]
	Comment 24 • 9 years ago

Comment on attachment 8573568 [details] [diff] [review]
Tests

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

LGTM.

	Michael Henretty [:mikehenrty][:mhenretty]
	Comment 27 • 9 years ago

Comment on attachment 8575679 [details] [diff] [review]
patch

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

::: dom/phonenumberutils/PhoneNumberUtils.jsm
@@ +160,5 @@
> +      dump("Couldn't find country name: " + aCountryName + "\n");
> +      return null;
> +    }
> +
> +    return PhoneNumber.Parse(aNumber, aCountryName);

This part looks the same as before, and unless I am misunderstanding something and MCC_ISO3166_TABLE can change during runtime, we could end up re-populating the cache every time we call this function with an unknown country name.

Better is to make sure we populate the cache once, probably by ensure it's populated at the beginning of this function, and then returning null if it's not in the cache, otherwise we parse it. One way to do it was suggested at the bottom of comment 23.

It's been a while since I've been in this code, so if I'm confused please set me straight :)

	Michael Henretty [:mikehenrty][:mhenretty]
	Comment 28 • 9 years ago

Comment on attachment 8575679 [details] [diff] [review]
patch

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

I spoke with Gregor over IRC about this, my concerns of the cache populating twice are invalid since we specifically test that here.

Still, I think stylistically it would be better to structure the code such that we ensure the cache is populated first, then validated the country name, then parsed the number. Right now we have:

check cache for country,
ensure cache is populated,
check cache for country,
parse number.

It seems redundant to check the cache for the country name twice. But that said it doesn't stop this from working, so I'll leave it up to you if you want to restructure.

	Gregor Wagner [:gwagner] Assignee
	Comment 29 • 9 years ago

Alternative approach.

	Gregor Wagner [:gwagner] Assignee
	Comment 30 • 9 years ago

Comment on attachment 8577014 [details] [diff] [review]
patch

Hisinyi, I start with you since I dont know who owns MobileMessageDB.

	Gregor Wagner [:gwagner] Assignee
	Comment 31 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fded95ec1443

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 32 • 9 years ago

Comment on attachment 8577014 [details] [diff] [review]
patch

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

Thank you Gregor for the patch. Bevis is the one who can help!

	Bevis Tseng[:bevis][:btseng](Exited)
	Comment 33 • 9 years ago

Comment on attachment 8577014 [details] [diff] [review]
patch

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

It's nice that we can apply the same matching logic in PhoneNumberUtils.jsm.

While testing this patch, 
I found that the marionette test case is failure in [1].

However, after further investigation, I think this is because that, while 
fixing Bug 914060, we unnecessarily tried to compare a stored number from 
one SIM with the number received from another SIM and the country code of 
the received number will be used for parsing & matching if available. [2][3][4]

This doesn't make sense to me because the countries of this 2 SIMs could be different.

Hence, please also remove [Thread 17] & [Thread 18] from [1] to pass the test cases.

Thanks!

[1] https://dxr.mozilla.org/mozilla-central/source/dom/mobilemessage/tests/marionette/test_getthreads.js#355-369
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=914060#c19
[3] https://dxr.mozilla.org/mozilla-central/source/dom/mobilemessage/gonk/MobileMessageDB.jsm#1819
[4] https://dxr.mozilla.org/mozilla-central/source/dom/mobilemessage/gonk/MobileMessageDB.jsm#1640-1654

	Bevis Tseng[:bevis][:btseng](Exited)
	Comment 34 • 9 years ago

Comment on attachment 8577014 [details] [diff] [review]
patch

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

Sorry to recall my feedback+.

I'd like to do more testing next week for the following scenario |without SIM card inserted|:
1. send a SMS to 0987654321.
2. send a SMS to +886987654321. (TW)
and vise versa.

I am wonder if both international/national number of 0987654321 can be identified to be correctly
matched with the one of +886987654321 at [1].

[1] https://dxr.mozilla.org/mozilla-central/source/dom/phonenumberutils/PhoneNumberUtils.jsm#164

	Gregor Wagner [:gwagner] Assignee
	Comment 35 • 9 years ago

How can you send a SMS without a SIM card?

	Bevis Tseng[:bevis][:btseng](Exited)
	Comment 36 • 9 years ago

(In reply to Gregor Wagner [:gwagner] from comment #35)
> How can you send a SMS without a SIM card?

No, it can't be sent but it will be saved with |delivery-status| to "error".

	Bevis Tseng[:bevis][:btseng](Exited)
	Comment 37 • 9 years ago

Comment on attachment 8577014 [details] [diff] [review]
patch

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

After off-lined discussion with the owner of bug 914060, I realized the reason why we need PhoneNumberUtils.parseWithMCC(storedAddress, null) & PhoneNumberUtils.parseWithCountryName() in MobileMessageDB.matchParsedPhoneNumbers():
1. The major purpose is to ensure that all the local numbers stored in Contacts and used by the same device user could be matched with the corresponding international numbers even when a SIM card from different country is used. 
   That means, when travelling, it is possible for the same device user to buy a local SIM in the visited country to call/text-message with his/her friends in the HOME country.
2. To achieve this, 
   - PhoneNumberUtils.parseWithMCC(storedAddress, null) gives MobileMessageDB.jsm a change to parse the storedAddress if it's an international one to a parsed address.
   - PhoneNumberUtils.parseWithCountryName() gives MobileMessageDB.jsm a change to compare 2 numbers where one of this number was parsed with countryName available.

Hence, I've to set feedback- because we are not able to identify a stored local number with its international one in this scenario. :(

In addition, I still don't undersand how an invalid CountryName "001" will be provided as mentioned in comment 14.
This makes me wonder whether the root cause of this bug is identified or not.

BTW, the test data of [Thread 17] & [Thread 18] in test_getthreads.js is improper to identify this scenario, because the number was not in Brazil style +55aannnnnnnn.
It shall be modified as followed. I'll file another bug to address this instead.
//[Thread 17]
//Two sent messages, unreadCount = 0;
//One participant with two aliased addresses, participants = ["+551155211017"].
tasks.push(sendMessage.bind(null, "+551155211017", "thread 17-1"));
tasks.push(sendMessage.bind(null, "1155211017",      "thread 17-2"));
checkFuncs.push(checkThread.bind(null, ["thread 17-1", "thread 17-2"],
                                 "thread 17-2", 0, ["+551155211017"]));

//[Thread 18]
//Two sent messages, unreadCount = 0;
//One participant with two aliased addresses, participants = ["1155211018"].
tasks.push(sendMessage.bind(null, "1155211018",      "thread 18-1"));
tasks.push(sendMessage.bind(null, "+551155211018", "thread 18-2"));
checkFuncs.push(checkThread.bind(null, ["thread 18-1", "thread 18-2"],
                                 "thread 18-2", 0, ["1155211018"]));

	Gregor Wagner [:gwagner] Assignee
	Comment 38 • 9 years ago

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #37)
> It shall be modified as followed. I'll file another bug to address this
> instead.

Did you already file the bug? I want to fix the issue based on your use-case.

	Bevis Tseng[:bevis][:btseng](Exited)
	Comment 39 • 9 years ago

(In reply to Gregor Wagner [:gwagner] from comment #38)
> (In reply to Bevis Tseng[:bevistseng][:btseng] from comment #37)
> > It shall be modified as followed. I'll file another bug to address this
> > instead.
> 
> Did you already file the bug? I want to fix the issue based on your use-case.

Hi,

The test case is rewritten in patch part#1 of bug 1143596 in m-c.

	Gregor Wagner [:gwagner] Assignee
	Comment 40 • 9 years ago

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #39)
> (In reply to Gregor Wagner [:gwagner] from comment #38)
> > (In reply to Bevis Tseng[:bevistseng][:btseng] from comment #37)
> > > It shall be modified as followed. I'll file another bug to address this
> > > instead.
> > 
> > Did you already file the bug? I want to fix the issue based on your use-case.
> 
> Hi,
> 
> The test case is rewritten in patch part#1 of bug 1143596 in m-c.

Should we use another country since BR has substring matching?
http://mxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js#461

	Bevis Tseng[:bevis][:btseng](Exited)
	Comment 41 • 9 years ago

(In reply to Gregor Wagner [:gwagner] from comment #40)
> (In reply to Bevis Tseng[:bevistseng][:btseng] from comment #39)
> > (In reply to Gregor Wagner [:gwagner] from comment #38)
> > > (In reply to Bevis Tseng[:bevistseng][:btseng] from comment #37)
> > > > It shall be modified as followed. I'll file another bug to address this
> > > > instead.
> > > 
> > > Did you already file the bug? I want to fix the issue based on your use-case.
> > 
> > Hi,
> > 
> > The test case is rewritten in patch part#1 of bug 1143596 in m-c.
> 
> Should we use another country since BR has substring matching?
> http://mxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js#461

I think this is covered by other threads in the same test cases. (US numbers)
If not, please let me know.
https://dxr.mozilla.org/mozilla-central/source/dom/mobilemessage/tests/marionette/test_getthreads.js#88-160

IMHO, it will be perfect if we have the logic mentioned in comment 73 included in PhoneNumberUtils.fuzzyMatch(), and then we can have a unified way to match the phone numbers.

	Gregor Wagner [:gwagner] Assignee
	Comment 42 • 9 years ago

remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/4121ff863fd4
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/92ed4e955bef

	Carsten Book [:Tomcat]
	Comment 43 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/4121ff863fd4
https://hg.mozilla.org/mozilla-central/rev/92ed4e955bef

	Ryan VanderMeulen [:RyanVM]
	Comment 44 • 9 years ago

Please nominate these patches for b2g37 approval when you get a chance.

	Gregor Wagner [:gwagner] Assignee
	Comment 45 • 9 years ago

Comment on attachment 8575679 [details] [diff] [review]
patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): no regression
User impact if declined: international numbers without region dont work
Testing completed: on trunk
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch:NA

	Ryan VanderMeulen [:RyanVM]
	Comment 46 • 9 years ago

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/5a55fcac87dc
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/438e2c94b074

	Hermes Cheng (inactive after July 27, 2015)
	Comment 47 • 9 years ago

Could you help verify this bug?

	Yeojin Chung [:YeojinC]
	Comment 48 • 9 years ago

I was unable to reproduce this issue on a build from the reported date with my AT&T SIM. When I pressed the send button after typing my email address and put "+883510000000094" into the recipient, the message was sent and a new message thread was created properly.

Environmental Variables:
Device: Flame 3.0 (KK, 319mb, full flash)
Build ID: 20150226010233
Gaia: 7894b929f1b0394f3c997f72a6482bc7813e758d
Gecko: dd6353d61993
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 39.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0

Leaving qawanted for others to try and verify.

	KTucker [:KTucker][Inactive 3/4/2016]
	Comment 49 • 9 years ago

Julien, can you verify that this is fixed please? We can't seem to reproduce this on our end using the original build you reported this issue on.

	Julien Wajsberg [:julienw] Reporter
	Comment 50 • 9 years ago

Denver, because you originally reported the issue, can you help verifying whether it is fixed? The fix should be available in recent 3.0 and 2.2 builds.

Thanks a lot !

	Denver Gingerich
	Comment 51 • 9 years ago

Julien, is it possible to do this with the Simulator?  I looked at https://developer.mozilla.org/en-US/docs/Tools/Firefox_OS_Simulator but it says that WebSMS is not supported, which appears to be required for reproducing this.

The reason I ask is that the only "Firefox OS hardware" I have is my ZTE Open C and it seems like Firefox OS 2.2+ is not officially supported on it (and possibly not even unofficially supported).

	Julien Wajsberg [:julienw] Reporter
	Comment 52 • 9 years ago

I think I could reproduce the issue previously so I can try to verify it, NI me.

	Julien Wajsberg [:julienw] Reporter
	Comment 53 • 9 years ago

I checked I could send a SMS with current central. Now I need to check it was not working with the same phone + same SIMs before this patch.

	Julien Wajsberg [:julienw] Reporter
	Comment 54 • 9 years ago

I could send a SMS with the same device with a build from before the fix... I don't know how I could reproduce in comment 1.

If you can explain what was the issue here maybe I can try again. NI Gregor who can explain what the issue was.

	Gregor Wagner [:gwagner] Assignee
	Comment 55 • 9 years ago

(In reply to Julien Wajsberg [:julienw] from comment #54)
> I could send a SMS with the same device with a build from before the fix...
> I don't know how I could reproduce in comment 1.
> 
> If you can explain what was the issue here maybe I can try again. NI Gregor
> who can explain what the issue was.

The issue was that our library that deals with all the phone number conversion from local and international formats couldn't handle global numbers that are not mapped to a geographic area. This numbers are not very common and it took us some time to find this bug.
We generated an exception within the library and this caused the SMS app to panic and don't show a proper error to the user.
I dont think the simulator is a good framework for testing this. I added unit tests to the library and the SMS should handle such hickups in the library.

	Julien Wajsberg [:julienw] Reporter
	Comment 56 • 9 years ago

I can't reproduce with a build from ~24 Feb, but I _do_ reproduce with my v2.1 phone. So I don't really know how to move forward the check.

I'm only gonna say it's working now.

	Pi Wei Cheng [:piwei] (inactive)
	Comment 57 • 9 years ago

There is not enough information on this bug for us to proceed. Since we couldn't reproduce the bug, we couldn't verify it. Adding keyword to exclude this from our queries.