The default bug view has changed. See this FAQ.

B2G SMS: RangeError in semiOctetToBcdChar

RESOLVED FIXED in Firefox 19

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: vicamo, Assigned: ctai)

Tracking

unspecified
mozilla19
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:tef+, firefox19 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

Details

Attachments

(2 attachments, 9 obsolete attachments)

(Reporter)

Description

5 years ago
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
(Reporter)

Comment 1

5 years ago
response_type: RESPONSE_TYPE_UNSOLICITED
request_type: UNSOLICITED_RESPONSE_NEW_SMS
messageStringLength: 336
smscLength: 0x07
smscTypeOfAddress: 0x91
msg.SMSC: +12609091599
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.
(Reporter)

Updated

5 years ago
Blocks: 758658
Assignee: nobody → ctai
Created attachment 677675 [details] [diff] [review]
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.
Attachment #677675 - Flags: review?(vyang)
(Reporter)

Comment 3

5 years ago
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#+10123456789
> +         *   #31#+18901234567
> +         *   +18881234567#
> +         *   +18881234567
> +         * 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
Attachment #677675 - Flags: review?(vyang)
Created attachment 678610 [details] [diff] [review]
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.
Attachment #677675 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #678610 - Flags: review?(vyang)
(Reporter)

Comment 5

4 years ago
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.
Attachment #678610 - Flags: review?(vyang) → review-
Created attachment 679034 [details] [diff] [review]
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#+18901234567
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
Attachment #678610 - Attachment is obsolete: true
Attachment #679034 - Flags: feedback?(vyang)
(Reporter)

Comment 7

4 years ago
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
Attachment #679034 - Flags: feedback?(vyang)
Created attachment 679506 [details] [diff] [review]
OA with mmi code

A backup patch which return OA with mmi code
Attachment #679034 - Attachment is obsolete: true
Created attachment 679524 [details] [diff] [review]
OA with mmi code
Attachment #679506 - Attachment is obsolete: true
Created attachment 679525 [details] [diff] [review]
OA with mmi code
Attachment #679524 - Attachment is obsolete: true
Created attachment 679529 [details] [diff] [review]
OA with mmi code
Attachment #679525 - Attachment is obsolete: true
Created attachment 679531 [details] [diff] [review]
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
Attachment #679529 - Attachment is obsolete: true
Attachment #679531 - Flags: review?(vyang)
Link of try server for patch Attachment #679531 [details] [diff]
https://tbpl.mozilla.org/?tree=Try&rev=4dc2a576bdaa
(Reporter)

Comment 14

4 years ago
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.
Attachment #679531 - Flags: review?(vyang)
Created attachment 680499 [details] [diff] [review]
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
Attachment #679531 - Attachment is obsolete: true
Attachment #680499 - Flags: review?(vyang)
Try sever result:
https://tbpl.mozilla.org/?tree=Try&rev=bd7dcfb4ebc9
(Reporter)

Comment 17

4 years ago
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.
Attachment #680499 - Flags: review?(vyang) → review+
Created attachment 680932 [details] [diff] [review]
No MMI parsing patch
Attachment #680499 - Attachment is obsolete: true
Attachment #680932 - Flags: review?(vyang)
Keywords: checkin-needed
Duplicate of this bug: 758658
Attachment #680932 - Flags: review?(vyang)
(Reporter)

Comment 20

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/34903538f458
https://hg.mozilla.org/mozilla-central/rev/34903538f458
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
(Reporter)

Updated

4 years ago
Duplicate of this bug: 836383
(Reporter)

Comment 23

4 years ago
Duplicates bug 836383, which is a tef+.
blocking-b2g: --- → tef?
status-b2g18: --- → affected
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → affected
status-firefox19: --- → fixed
tracking-b2g18: --- → ?
(Reporter)

Comment 24

4 years ago
Created attachment 730573 [details] [diff] [review]
[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)
Attachment #730573 - Flags: approval-mozilla-b2g18?
blocking based on comment 23
blocking-b2g: tef? → tef+
tracking-b2g18: ? → ---
Comment on attachment 730573 [details] [diff] [review]
[b2g18_v1_0_1 and b2g18] patch

Given tef+, no need for approval.
Attachment #730573 - Flags: approval-mozilla-b2g18?
(Reporter)

Comment 27

4 years ago
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
status-b2g18: affected → fixed
status-b2g18-v1.0.1: affected → fixed
verified to fix bug 836383 for me.
Blocks: 859876
You need to log in before you can comment on or make changes to this bug.