788441 - B2G SMS: RangeError in semiOctetToBcdChar

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

I/Gecko   (  106): RIL Worker: New incoming parcel of size 688
I/Gecko   (  106): RIL Worker: Parcel (size 688): 1,0,0,0,235,3,0,0,80,1,0,0,48,0,55,0,57,0,49,0,52,0,51,0,48,0,54,0,48,0,57,0,49,0,57,0,57,0,53,0,70,0,57,0,48,0,52,0,48,0,69,0,68,0,49,0,67,0,68,0,66,0,55,0,51,0,68,0,51,0,68,0,65,0,55,0,56,0,55,0,69,0,53,0,48,0,48,0,48,0,48,0,50,0,49,0,57,0,48,0,53,0,48,0,49,0,49,0,56,0,51,0,52,0,50,0,56,0,48,0,57,0,70,0,68,0,50,0,55,0,55,0,66,0,56,0,57,0,68,0,55,0,54,0,57,0,70,0,52,0,49,0,54,0,57,0,66,0,55,0,70,0,57,0,65,0,68,0,48,0,51,0,52,0,49,0,69,0,53,0,69,0,53,0,55,0,49,0,70,0,65,0,51,0,68,0,48,0,55,0,56,0,68,0,68,0,70,0,54,0,69,0,53,0,48,0,68,0,50,0,49,0,65,0,48,0,52,0,65,0,49,0,67,0,65,0,54,0,69,0,53,0,48,0,57,0,57,0,48,0,68,0,50,0,65,0,69,0,51,0,69,0,57,0,70,0,50,0,66,0,48,0,53,0,66,0,53,0,68,0,57,0,54,0,66,0,70,0,52,0,49,0,70,0,51,0,66,0,55,0,49,0,66,0,52,0,52,0,52,0,69,0,67,0,70,0,69,0,57,0,54,0,57,0,51,0,55,0,70,0,68,0,51,0,68,0,48,0,55,0,57,0,49,0,67,0,66,0,50,0,48,0,70,0,54,0,55,0,66,0,48,0,69,0,55,0,50,0,56,0,55,0,67,0,55,0,69,0,57,0,66,0,55,0,51,0,66,0,67,0,67,0,50,0,69,0,67,0,
I/Gecko   (  106): RIL Worker: We have at least one complete parcel.
I/Gecko   (  106): RIL Worker: Unsolicited response for request type 1003
I/Gecko   (  106): RIL Worker: Handling parcel as UNSOLICITED_RESPONSE_NEW_SMS
I/Gecko   (  106): RIL Worker: Got new SMS, length 336
I/Gecko   (  106): RIL Worker: PDU: Going to read address: 14
I/Gecko   (  106): RIL Worker: Parcel handling threw RangeError
I/Gecko   (  106): semiOctetToBcdChar@resource://gre/modules/ril_worker.js:4211
I/Gecko   (  106): readSwappedNibbleBcdString@resource://gre/modules/ril_worker.js:4265
I/Gecko   (  106): readAddress@resource://gre/modules/ril_worker.js:4847
I/Gecko   (  106): readDeliverMessage@resource://gre/modules/ril_worker.js:5260
I/Gecko   (  106): readMessage@resource://gre/modules/ril_worker.js:5243
I/Gecko   (  106): _processReceivedSms@resource://gre/modules/ril_worker.js:2900
I/Gecko   (  106): _processSmsDeliver@resource://gre/modules/ril_worker.js:2918
I/Gecko   (  106): UNSOLICITED_RESPONSE_NEW_SMS@resource://gre/modules/ril_worker.js:3982
I/Gecko   (  106): handleParcel@resource://gre/modules/ril_worker.js:3192
I/Gecko   (  106): processParcel@resource://gre/modules/ril_worker.js:521
I/Gecko   (  106): processIncoming@resource://gre/modules/ril_worker.js:473
I/Gecko   (  106): onRILMessage@resource://gre/modules/ril_worker.js:6182
I/Gecko   (  106): 
I/Gecko   (  106): RIL Worker: Parcel handler didn't consume whole parcel, 604 bytes left over

Comment 1

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

response_type: RESPONSE_TYPE_UNSOLICITED
request_type: UNSOLICITED_RESPONSE_NEW_SMS
messageStringLength: 336
smscLength: 0x07
smscTypeOfAddress: 0x91
msg.SMSC: [phone]
firstOctet: 0x04
msg.mti: PDU_MTI_SMS_DELIVER
msg.udhi: 0x00
senderAddressLength: 0x0E (14)
senderAddressTypeOfAddress: 0xD1 (TON: alphanumeric, NPI: ISDN/telephone numbering plan)

We don't support TOA of the kind for now. See also bug 758658.

Comment 2

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

Attachment: Patch for SMS OA

Modfiy readAddress in ril_worker.js to support alpha numeric and mmi codes type OA.
Also add test case in test_ril_worker_sms.js.

Comment 3

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

Comment on attachment 677675 [details] [diff] [review]
Patch for SMS OA

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

Thanks for your work here :)
Some minor problems still have to be addressed and I'd really want to know whether or not is passing a MMI string to SMS database a must.

::: dom/system/gonk/ril_worker.js
@@ +5869,5 @@
>      // Type-of-Address
>      let toa = this.readHexOctet();
> +    let addr = "";
> +
> +    if ((toa >> 4) == (PDU_TOA_ALPHANUMERIC >> 4)) {

What about: if ((toa & 0xF0) == PDU_TOA_ALPHANUMERIC)

@@ +5870,5 @@
>      let toa = this.readHexOctet();
> +    let addr = "";
> +
> +    if ((toa >> 4) == (PDU_TOA_ALPHANUMERIC >> 4)) {
> +      addr = this.readSeptetsToString(Math.floor(len * 4 / 7), 0, 0, 0); //fixme: check length, paddingBits, langIndex, and langShiftIndex

If you have a "FIXME" in the comment, it has to be followed by a bug number to note future work. In this case, 3GPP TS 23.040 section 9.1.2.4 says "as the default alphabet defined in 3GPP TS 23.038", that means all the parameters are known, actually exactly what you have filed here.

BTW, we have also PDU_NL_IDENTIFIER_DEFAULT defined for 0 langIndex and langShiftIndex.

@@ +5871,5 @@
> +    let addr = "";
> +
> +    if ((toa >> 4) == (PDU_TOA_ALPHANUMERIC >> 4)) {
> +      addr = this.readSeptetsToString(Math.floor(len * 4 / 7), 0, 0, 0); //fixme: check length, paddingBits, langIndex, and langShiftIndex
> +    }else {

Instead of opening another "else" block, we tend to bail-out early if possible. That is,

  if ((toa & 0xF0) == PDU_TOA_ALPHANUMERIC) {
    // Do something.
    return addr;
  }

  // Other stuff.
  return addr_again;

@@ +5877,5 @@
> +      if (addr.length <= 0) {
> +        if (DEBUG) debug("PDU error: no number provided");
> +        return null;
> +      }
> +      if ((toa >> 4) == (PDU_TOA_INTERNATIONAL >> 4)) {

Ditto.

@@ +5889,5 @@
> +         *   *31#[phone]
> +         *   #31#[phone]
> +         *   [phone]#
> +         *   [phone]
> +         * Odd ball cases that some phones handled

80 chars per line

@@ +5900,5 @@
> +        let result = addr.match(pattern);
> +        if (result != null) {
> +
> +          if (result[2] !== "") {
> +            addr = result[1] + result[2] + result[3] + '+' + result[4] + result[5];

Do we really need an address that contains MMI? If we just discard it here, will this cause any nonconformance?

@@ +5905,5 @@
> +          } else {
> +            addr = result[1] + result[3] + result[4] + result[5] + '+';
> +          }
> +        } else {
> +          pattern = /(^[#*])(.*)([#*])(.*)/;

This can be merged into previous pattern as ^([#*])([^#*]*)([#*])([^#*]*)(#?)$

::: dom/system/gonk/tests/test_ril_worker_sms.js
@@ +729,5 @@
> +    }
> +
> +  });
> +  worker.debug = function (message) {
> +    do_print(message);

Don't commit personal debug code.

@@ +774,5 @@
> +    let parseAddr = helper.readAddress(length);
> +    do_check_eq(parseAddr, addrString);
> +  }
> +
> +//  For AlphaNumeric

Indentation here.

@@ +779,5 @@
> +  test_address("04D01100", "_@");
> +  test_address("04D01000", "\u0394@");
> +
> +//  Below tests are for all 4 possibility for append + in PDU_TOA_INTERNATIONAL
> +//  For MMI codes

Ditto

Comment 4

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

Attachment: Patch for SMS OA

Thanks for the review, I modify the code for your suggestion.
Except the question "Do we really need an address that contains MMI? If we just discard it here, will this cause any nonconformance?"
I think we need to keep MMI related information in this stage. Because MMI is for control the service, discard it might mislead the behaviour.

Comment 5

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

Comment on attachment 678610 [details] [diff] [review]
Patch for SMS OA

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

Please check again indentations and the regular expression part. And, storing MMI-embedded recipient address results in inability to search these messages. For me, "might mislead the behaviour" is not strong enough to merge such database semantics change.

Comment 6

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

Attachment: FIx indentations

Thanks for your feedback.
I already fix the wrong indentations.
For regular expression part, I think it is right.
Let me explain it by example.
I use /(^[#*])(.*)([#*])(.*)$/ to check these cases.
1. *21#
The result[1] = *, result[2] = 21, result[3] = #, result[4] = ""
2. #21#
The result[1] = #, result[2] = 21, result[3] = #, result[4] = ""

Case 1 and 2 don't match result[2].match(/(.+)([#*])(.+)$/). So just append the + after result[3]. 
3. *#21# 
The result[1] = *, result[2] = #21, result[3] = #, result[4] = ""
Case 3 don't match result[2].match(/(.+)([#*])(.+)$/). So just append the + after result[3]. 
4. *31#10123456789
The result[1] = *, result[2] = 31, result[3] = #, result[4] = "10123456789"
5. #31#[phone]
The result[1] = #, result[2] = 31, result[3] = #, result[4] = "18901234567"
Case 4 and 5 don't match result[2].match(/(.+)([#*])(.+)$/). So just append the + after result[3]. 
6. 18881234567#
The result is null. Insert '+' before the address.
7. **21*886288888888#
The result[1] = *, result[2] = *21*886288888888, result[3] = #, result[4] = ""
Case 7 match result[2].match(/(.+)([#*])(.+)$/). So append '+' before result2[3].
I guess that is the tricky part.

Please let me know if you have any question about the regular expression.

For MMI code removal part, I use an android phone to set call forward by MMI code. After that step, no phone number shows in call log.
I don't think end user can send a message which OA with MMI code.
So the use case should an operator or some service provider insert the MMI code to OA for special reason. For example, maybe the operator can send a message which OA is ##02# to cancel all call forwarding. If we press the OA button in that message, it should dial the number directly to cancel all call forwarding.
If we remove the MMI code, that sounds weird for me.
And the OA from operator or some service should not match in your contacts. So I think in match contacts, we can skip all MMI code starting number. Not just discard MMI part.
Thanks

Comment 7

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

Comment on attachment 679034 [details] [diff] [review]
FIx indentations

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

r=me with removal of MMI code in result OA address.

::: dom/system/gonk/ril_worker.js
@@ +5896,5 @@
> +    * where there is no dialing number so they
> +    * append the "+"
> +    *   *21#+
> +    *   **21#+
> +    */

Please indent these comments. They are in if block. And, in comment #3, I had "80 chars per line".

@@ +5897,5 @@
> +    * append the "+"
> +    *   *21#+
> +    *   **21#+
> +    */
> +      let pattern = /(^[#*])(.*)([#*])(.*)$/;

In your previous patch, you had:

  let pattern = /(^[#*])(.*)([#*])(.*)(#)$/;
  pattern = /(^[#*])(.*)([#*])(.*)/;

You can simply merge them into one:

  let pattern = /^([#*])(.*)([#*])(.*)(#?)$/;

@@ +5911,5 @@
> +            }
> +          addr = result[1] + result[2] + result[3] + '+' + result[4];
> +          return addr;
> +        }
> +      } 

Tailing white space

Comment 8

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

Attachment: OA with mmi code

A backup patch which return OA with mmi code

Comment 9

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

Attachment: OA with mmi code

Comment 10

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

Attachment: OA with mmi code

Comment 11

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

Attachment: OA with mmi code

Comment 12

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

Attachment: OA without mmi code

Please review this patch.
MMI code will show not only TOA = international but also other case.
Please check the ril_worker and test case.
Thanks

Comment 13

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

Link of try server for patch Attachment #679531 [details] [diff]
https://tbpl.mozilla.org/?tree=Try&rev=4dc2a576bdaa

Comment 14

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

Comment on attachment 679531 [details] [diff] [review]
OA without mmi code

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

I've talked to Fernando, author of MMI works. Actually, both of us don't really think MMI code in DA is ever possible. These days I've been trying to find some use cases that utilize MMI codes in SMS DA but in vain (of course we're all new to this field and our reach could be pretty limited). Besides, I think these MMI parse code is mainly come from Android's shared address parsing implementation. MMI codes exist in call addresses, but it doesn't follow that SMS DA has the same thing. 3GPP TS 23.040 doesn't mention it, neither do 27.004 and 27.005. Third, there is already a more accurate/complete _parseMMI() helper function written by Fernando right in ril_worker. Should we find any use case that depends on MMI in SMS DA, we can still easily support it with that helper function. So, I'm sorry this review process takes so much time, but I'd like you to remove MMI parsing parts and simply read the normal alphanumeric address.

Comment 15

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

Attachment: No MMI parsing patch

Thanks for reviewing. I upload the patch which only dealing with alpha-numeric and append the '+' when TOA is international. You are right. Maybe there is no use case for a OA with MMI code.
By the way, for bug 758658 - B2G SMS: test scripts for GSM address parsing, I think I also solved this bug, and I right?
If yes, maybe I can take this bug. And mark as duplicated to 788441.
Thanks

Comment 16

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

Try sever result:
https://tbpl.mozilla.org/?tree=Try&rev=bd7dcfb4ebc9

Comment 17

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

Comment on attachment 680499 [details] [diff] [review]
No MMI parsing patch

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

Thank you, Chia-hung.

::: dom/system/gonk/ril_worker.js
@@ +5910,3 @@
>        addr = '+' + addr;
> +      return addr;
> +    }

Please always try to keep minimum change set if possible. Don't include unnecessary changes when you upload a patch.

Comment 18

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

Attachment: No MMI parsing patch

Comment 20

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/34903538f458

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/34903538f458

Comment 23

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

Duplicates bug 836383, which is a tef+.

Comment 24

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

Attachment: [b2g18_v1_0_1 and b2g18] patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 836383
User impact if declined: SMS with alphanumeric sender address is not received.
Testing completed: b2g18, b2g18_v1_0_1
Risk to taking this patch (and alternatives if risky): already in m-c since mozilla-19 and no any known follow-ups
String or UUID changes made by this patch: (none)

Comment 25

[Daniel Coloma:dcoloma]

blocking based on comment 23

Comment 26

[Alex Keybl [:akeybl]]

Comment on attachment 730573 [details] [diff] [review]
[b2g18_v1_0_1 and b2g18] patch

Given tef+, no need for approval.

Comment 27

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

b2g18:
https://hg.mozilla.org/releases/mozilla-b2g18/rev/d8736e5b5624

b2g18 v1.0.1:
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/b28463f2e718

Comment 28

[Zibi Braniecki [:zbraniecki][:gandalf]]

verified to fix bug 836383 for me.