Last Comment Bug 788441 - B2G SMS: RangeError in semiOctetToBcdChar
: B2G SMS: RangeError in semiOctetToBcdChar
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla19
Assigned To: Chia-hung Tai [:ctai :ctai_mozilla :cht]
:
Mentors:
: 758658 836383 (view as bug list)
Depends on:
Blocks: 758658 859876
  Show dependency treegraph
 
Reported: 2012-09-05 02:43 PDT by Vicamo Yang [:vicamo][:vyang]
Modified: 2013-04-09 13:07 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
tef+
fixed
fixed
wontfix
fixed


Attachments
Patch for SMS OA (5.18 KB, patch)
2012-11-02 00:02 PDT, Chia-hung Tai [:ctai :ctai_mozilla :cht]
no flags Details | Diff | Splinter Review
Patch for SMS OA (4.40 KB, patch)
2012-11-05 20:00 PST, Chia-hung Tai [:ctai :ctai_mozilla :cht]
vicamo: review-
Details | Diff | Splinter Review
FIx indentations (4.45 KB, patch)
2012-11-06 20:05 PST, Chia-hung Tai [:ctai :ctai_mozilla :cht]
no flags Details | Diff | Splinter Review
OA with mmi code (4.55 KB, patch)
2012-11-07 18:31 PST, Chia-hung Tai [:ctai :ctai_mozilla :cht]
no flags Details | Diff | Splinter Review
OA with mmi code (4.48 KB, patch)
2012-11-07 19:21 PST, Chia-hung Tai [:ctai :ctai_mozilla :cht]
no flags Details | Diff | Splinter Review
OA with mmi code (4.48 KB, patch)
2012-11-07 19:25 PST, Chia-hung Tai [:ctai :ctai_mozilla :cht]
no flags Details | Diff | Splinter Review
OA with mmi code (4.49 KB, patch)
2012-11-07 19:36 PST, Chia-hung Tai [:ctai :ctai_mozilla :cht]
no flags Details | Diff | Splinter Review
OA without mmi code (4.60 KB, patch)
2012-11-07 19:41 PST, Chia-hung Tai [:ctai :ctai_mozilla :cht]
no flags Details | Diff | Splinter Review
No MMI parsing patch (2.96 KB, patch)
2012-11-11 18:39 PST, Chia-hung Tai [:ctai :ctai_mozilla :cht]
vicamo: review+
Details | Diff | Splinter Review
No MMI parsing patch (2.93 KB, patch)
2012-11-12 20:00 PST, Chia-hung Tai [:ctai :ctai_mozilla :cht]
no flags Details | Diff | Splinter Review
[b2g18_v1_0_1 and b2g18] patch (1.30 KB, patch)
2013-03-28 00:52 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review

Description Vicamo Yang [:vicamo][:vyang] 2012-09-05 02:43:41 PDT
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] 2012-09-12 00:34:31 PDT
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.
Comment 2 Chia-hung Tai [:ctai :ctai_mozilla :cht] 2012-11-02 00:02:47 PDT
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.
Comment 3 Vicamo Yang [:vicamo][:vyang] 2012-11-02 01:30:32 PDT
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
Comment 4 Chia-hung Tai [:ctai :ctai_mozilla :cht] 2012-11-05 20:00:52 PST
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.
Comment 5 Vicamo Yang [:vicamo][:vyang] 2012-11-06 03:30:19 PST
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] 2012-11-06 20:05:30 PST
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
Comment 7 Vicamo Yang [:vicamo][:vyang] 2012-11-07 07:13:34 PST
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] 2012-11-07 18:31:48 PST
Created attachment 679506 [details] [diff] [review]
OA with mmi code

A backup patch which return OA with mmi code
Comment 9 Chia-hung Tai [:ctai :ctai_mozilla :cht] 2012-11-07 19:21:55 PST
Created attachment 679524 [details] [diff] [review]
OA with mmi code
Comment 10 Chia-hung Tai [:ctai :ctai_mozilla :cht] 2012-11-07 19:25:24 PST
Created attachment 679525 [details] [diff] [review]
OA with mmi code
Comment 11 Chia-hung Tai [:ctai :ctai_mozilla :cht] 2012-11-07 19:36:09 PST
Created attachment 679529 [details] [diff] [review]
OA with mmi code
Comment 12 Chia-hung Tai [:ctai :ctai_mozilla :cht] 2012-11-07 19:41:57 PST
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
Comment 13 Chia-hung Tai [:ctai :ctai_mozilla :cht] 2012-11-07 19:55:17 PST
Link of try server for patch Attachment #679531 [details] [diff]
https://tbpl.mozilla.org/?tree=Try&rev=4dc2a576bdaa
Comment 14 Vicamo Yang [:vicamo][:vyang] 2012-11-09 11:14:43 PST
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] 2012-11-11 18:39:33 PST
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
Comment 16 Chia-hung Tai [:ctai :ctai_mozilla :cht] 2012-11-11 19:56:30 PST
Try sever result:
https://tbpl.mozilla.org/?tree=Try&rev=bd7dcfb4ebc9
Comment 17 Vicamo Yang [:vicamo][:vyang] 2012-11-12 18:13:29 PST
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] 2012-11-12 20:00:26 PST
Created attachment 680932 [details] [diff] [review]
No MMI parsing patch
Comment 19 Chia-hung Tai [:ctai :ctai_mozilla :cht] 2012-11-12 20:22:01 PST
*** Bug 758658 has been marked as a duplicate of this bug. ***
Comment 20 Vicamo Yang [:vicamo][:vyang] 2012-11-12 21:49:15 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/34903538f458
Comment 21 Ed Morley [:emorley] 2012-11-13 09:28:49 PST
https://hg.mozilla.org/mozilla-central/rev/34903538f458
Comment 22 Vicamo Yang [:vicamo][:vyang] 2013-03-28 00:35:15 PDT
*** Bug 836383 has been marked as a duplicate of this bug. ***
Comment 23 Vicamo Yang [:vicamo][:vyang] 2013-03-28 00:43:06 PDT
Duplicates bug 836383, which is a tef+.
Comment 24 Vicamo Yang [:vicamo][:vyang] 2013-03-28 00:52:58 PDT
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)
Comment 25 Daniel Coloma:dcoloma 2013-03-28 01:50:54 PDT
blocking based on comment 23
Comment 26 Alex Keybl [:akeybl] 2013-03-29 14:50:57 PDT
Comment on attachment 730573 [details] [diff] [review]
[b2g18_v1_0_1 and b2g18] patch

Given tef+, no need for approval.
Comment 28 Zibi Braniecki [:gandalf][:zibi] 2013-04-08 18:48:42 PDT
verified to fix bug 836383 for me.

Note You need to log in before you can comment on or make changes to this bug.