SMS cannot be sent to a 15-digit phone number (such as an iNum) which doesn't have a territory id

RESOLVED FIXED in Firefox 40

Status

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: julienw, Assigned: gwagner)

Tracking

({qawanted, verifyme})

unspecified
2.2 S9 (3apr)
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:2.2+, firefox38 wontfix, firefox39 wontfix, firefox40 fixed, b2g-v1.4 affected, b2g-v2.0 affected, b2g-v2.0M affected, b2g-v2.1 affected, b2g-v2.1S affected, b2g-v2.2 fixed, b2g-master fixed)

Details

(Whiteboard: [systemsfe])

Attachments

(5 attachments, 4 obsolete attachments)

(Reporter)

Description

4 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.
(Reporter)

Comment 1

4 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}]
(Reporter)

Updated

4 years ago
Blocks: 1136147
No longer depends on: 1136147
(Reporter)

Comment 2

4 years ago
I think this is bad enough to warrant a blocking status. Maybe even 2.1?
blocking-b2g: --- → 2.2?
(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?
Flags: needinfo?(btseng)
(Reporter)

Comment 4

4 years ago
Did you try the exact phone number from comment 0? (I used the version with the '+' prefix)
Flags: needinfo?(btseng)
Flags: needinfo?(btseng)
(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.
(Reporter)

Comment 8

4 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 ?
(Reporter)

Comment 9

4 years ago
Hey Gregor, are you the right person to ask for the PhoneNumber lib?
Flags: needinfo?(anygregor)
(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
(Assignee)

Comment 11

4 years ago
Fixing upstream first:
https://github.com/andreasgal/PhoneNumber.js/pull/37
Flags: needinfo?(anygregor)
(Assignee)

Comment 12

4 years ago
Posted file PR
Upstream fix. Will provide gecko patch once I get r+
Attachment #8571558 - Flags: review?(gal)
(Assignee)

Updated

4 years ago
Assignee: nobody → anygregor
(Assignee)

Updated

4 years ago
Summary: SMS cannot be sent to a 15-digit phone number (such as an iNum) → SMS cannot be sent to a 15-digit phone number (such as an iNum) which doesn't have a territory id
(Assignee)

Comment 13

4 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
(Assignee)

Updated

4 years ago
Attachment #8571558 - Flags: review?(gal)
triage: breaking basic function.
blocking-b2g: 2.2? → 2.2+
(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(). :(
(Assignee)

Comment 17

4 years ago
I can make the +883510000000094 case work. 883510000000094 doesn't work on android. can someone confirm that this case should work?
(Assignee)

Comment 18

4 years ago
Posted patch Patch (obsolete) — Splinter Review
(Assignee)

Comment 19

4 years ago
Posted patch patch (obsolete) — Splinter Review
Attachment #8572272 - Attachment is obsolete: true
Attachment #8572274 - Flags: review?(mhenretty)
(Assignee)

Updated

4 years ago
Whiteboard: [systemsfe]
(Reporter)

Comment 20

4 years ago
I don't think the version wtithout '+' should work.
(Assignee)

Comment 21

4 years ago
Posted patch TestsSplinter Review
(Assignee)

Updated

4 years ago
Attachment #8573568 - Flags: review?(mhenretty)
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.
Attachment #8572274 - Flags: review?(mhenretty) → review-
Comment on attachment 8573568 [details] [diff] [review]
Tests

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

LGTM.
Attachment #8573568 - Flags: review?(mhenretty) → review+
(Assignee)

Comment 25

4 years ago
Posted patch 1136211.diff (obsolete) — Splinter Review
Attachment #8572274 - Attachment is obsolete: true
(Assignee)

Comment 26

4 years ago
Posted patch patchSplinter Review
Attachment #8575678 - Attachment is obsolete: true
Attachment #8575679 - Flags: review?(mhenretty)
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 :)
Attachment #8575679 - Flags: review?(mhenretty)
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.
Attachment #8575679 - Flags: review+
(Assignee)

Comment 29

4 years ago
Posted patch patch (obsolete) — Splinter Review
Alternative approach.
(Assignee)

Comment 30

4 years ago
Comment on attachment 8577014 [details] [diff] [review]
patch

Hisinyi, I start with you since I dont know who owns MobileMessageDB.
Attachment #8577014 - Flags: feedback?(htsai)
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!
Attachment #8577014 - Flags: feedback?(htsai) → feedback?(btseng)
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
Attachment #8577014 - Flags: feedback?(btseng) → feedback+
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
Attachment #8577014 - Flags: feedback+ → feedback?(btseng)
(Assignee)

Comment 35

4 years ago
How can you send a SMS without a SIM card?
(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".
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"]));
Attachment #8577014 - Flags: feedback?(btseng) → feedback-
(Assignee)

Comment 38

4 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.
(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.
(Assignee)

Comment 40

4 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
Flags: needinfo?(btseng)
(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.
Flags: needinfo?(btseng)
(Assignee)

Updated

4 years ago
Blocks: 1149802
(Assignee)

Updated

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

Updated

4 years ago
No longer blocks: 1136147
(Reporter)

Updated

4 years ago
See Also: → 1136147
(Assignee)

Updated

4 years ago
Target Milestone: --- → 2.2 S9 (3apr)
Please nominate these patches for b2g37 approval when you get a chance.
Flags: needinfo?(anygregor)
Flags: in-testsuite+
(Assignee)

Comment 45

4 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
Flags: needinfo?(anygregor)
Attachment #8575679 - Flags: approval-mozilla-b2g37?
Attachment #8575679 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Could you help verify this bug?
Flags: needinfo?(pbylenga)
Flags: needinfo?(pbylenga)
Keywords: qawanted
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.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
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.
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker) → needinfo?(felash)
(Reporter)

Comment 50

4 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 !
Flags: needinfo?(felash) → needinfo?(denver)

Comment 51

4 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).
Flags: needinfo?(denver)
(Reporter)

Comment 52

4 years ago
I think I could reproduce the issue previously so I can try to verify it, NI me.
Flags: needinfo?(felash)
(Reporter)

Comment 53

4 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.
(Reporter)

Comment 54

4 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.
Flags: needinfo?(anygregor)
(Assignee)

Comment 55

4 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.
Flags: needinfo?(anygregor)
(Reporter)

Comment 56

4 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.
Flags: needinfo?(felash)
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.
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?], QAExclude
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?], QAExclude → [QAnalyst-Triage+], QAExclude
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.