1123204 - [FFOS2.0][Woodduck][GCF][STK]Text String:length is not ok when execute get input->27.22.4.3.1/8

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect, P1)

Description

[sync-1]

DEFECT DESCRIPTION:[GCF][STK]Text String:length is not ok when execute get input->27.22.4.3.1/8 
 
  REPRODUCING PROCEDURES:
 1. Load a simcard to the phone which can send "get input";
 2. Execute 27.22.4.3.1/8:GET INPUT, digits only, SMS default alphabet, ME to echo text, ME supporting 8 bit data Message;
 3. Terminal response is "81 03 01 23 00 02 02 82 81 83 01 00 
 8D A1 04 2A 2A..."-ko
 
  EXPECTED BEHAVIOUR:
 3. Terminal response should be"81 03 01 23 00 02 02 82 81 83 01 00 
 8D 81 A1 04 2A 2A...";
 
 Reporter's phone number: 0752-2639695
 
 This issue block GCF.
 
  ASSOCIATE SPECIFICATION:
 
  TEST PLAN REFERENCE:
 
  TOOLS AND PLATFORMS USED:
 
  USER IMPACT:
 
  REPRODUCING RATE:
 
  For FT PR, Please list reference mobile's behavior:

Comment 1

[sync-1]

Attachment: 27.22.4.3.1.8.rar

Comment 2

[Josh Cheng [:josh]]

Hi Sean,
Could you please help to check the problem? Thanks!

Comment 3

[Sean Lee [:seanlee][:weilonge]]

Hi Pengfei,

Here is the string entered by user:
"input":"***1111111111###***2222222222###***3333333333###***4444444444###***5555555555###***6666666666###***7777777777###***8888888888###***9999999999###***0000000000###"

and its length is 160 (HEX: A0).
So the length for STK command is A0 + 1 = A1

May I know why do we need the byte '81' here?

Thanks.

Comment 4

[Bevis Tseng[:bevis][:btseng](Exited)]

Hi Sean,

After more analysis offlined, we found the root cause is that when encoding the Text string C-TLV with length larger than 128, the length encoding of C-TLV is not applied.

The number of octets for encoding length value is varied from 1 to 4. [1]
However, we always encode it with 1 octets. [2] 

The test data is provided in |TERMINAL RESPONSE: GET INPUT 1.8.1| of 27.22.4.3 GET INPUT in TS 102 384.

We need to figure out a solution to encode the length with varied octets properly in ril_worker.js.

[1] https://hg.mozilla.org/releases/mozilla-release/file/c13c3b4992cf/dom/system/gonk/ril_worker.js#l11857
[2] https://hg.mozilla.org/releases/mozilla-release/file/c13c3b4992cf/dom/system/gonk/ril_worker.js#l2901

Comment 5

[Sean Lee [:seanlee][:weilonge]]

Hi Bevis, Thanks for your information!

Comment 7

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 1 v1: Encode Length of "Text String" C-TLV in Varied Length of Octets.

This is to 
1. encode the length of "Text String" C-TLV properly with varied number of octets.
2. encode unpacked GSM7bit alphabet correctly.
3. Fix the logical problem when fallback the encoding from extension table.

Hi Edgar,

May I have your review for this change?

Thanks!

Comment 8

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 2 v1: Add Test Case to Improve Test Coverage.

Add |TERMINAL RESPONSE: GET INPUT 1.8.1| of 27.22.4.3.1 GET INPUT (normal) in TS 102 384 into our test coverage.

Hi Edgar,

May I have your review for this test case?

Thanks!

Comment 9

[pengfei.huang]

Dear Bevis Tseng & Josh,
  This bug and bug1122330 both test pass with your patch comment7.
  Bevis you are awesome, many thanks.

Comment 10

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to pengfei.huang from comment #9)
> Dear Bevis Tseng & Josh,
>   This bug and bug1122330 both test pass with your patch comment7.
>   Bevis you are awesome, many thanks.

\o/

Comment 11

[Josh Cheng [:josh]]

Hi Bevis,
Thanks! 2.0M+

Comment 12

[Edgar Chen [:edgar]]

Comment on attachment 8551658 [details] [diff] [review]
Part 1 v1: Encode Length of "Text String" C-TLV in Varied Length of Octets.

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

::: dom/system/gonk/ril_worker.js
@@ +7532,5 @@
>        this.writeHexOctet(data & 0xFF);
>      }
>    },
>  
> +  writeStringAs8BitUnpacked: function(aText) {

Let's just follow the same coding style in ril_worker, e.g. no `a` prefix for argument. Thank you.

@@ +7536,5 @@
> +  writeStringAs8BitUnpacked: function(aText) {
> +    const langTable = PDU_NL_LOCKING_SHIFT_TABLES[PDU_NL_IDENTIFIER_DEFAULT];
> +    const langShiftTable = PDU_NL_SINGLE_SHIFT_TABLES[PDU_NL_IDENTIFIER_DEFAULT];
> +
> +    let i, c, octet;

One declaration per line and declare local variables as near to their use as possible.

@@ +7538,5 @@
> +    const langShiftTable = PDU_NL_SINGLE_SHIFT_TABLES[PDU_NL_IDENTIFIER_DEFAULT];
> +
> +    let i, c, octet;
> +    let len = aText ? aText.length : 0;
> +    for (i = 0; i < len; i++) {

for (let i ....

@@ +7539,5 @@
> +
> +    let i, c, octet;
> +    let len = aText ? aText.length : 0;
> +    for (i = 0; i < len; i++) {
> +      c = aText.charAt(i);

let c = ....;

@@ +7540,5 @@
> +    let i, c, octet;
> +    let len = aText ? aText.length : 0;
> +    for (i = 0; i < len; i++) {
> +      c = aText.charAt(i);
> +      octet = langTable.indexOf(c);

let octet = ...;

@@ +12165,5 @@
>      GsmPDUHelper.writeSwappedNibbleBCDNum(Math.floor(seconds / 60) % 60);
>      GsmPDUHelper.writeSwappedNibbleBCDNum(seconds % 60);
>    },
>  
> +  writeTextStringTlv: function(aText, aCoding) {

Ditto, no 'a' prefix for argument in ril_worker.

@@ +12169,5 @@
> +  writeTextStringTlv: function(aText, aCoding) {
> +    let GsmPDUHelper = this.context.GsmPDUHelper;
> +    let writeHexOctet = GsmPDUHelper.writeHexOctet;
> +    let buf = [];
> +    GsmPDUHelper.writeHexOctet = function(aOctet) {

I like this idea of putting the data into a cached area first until we know the whole length. ;)
But per off-line discussion, please help to try to move this idea to worker_buf? Thank you.

Comment 13

[Edgar Chen [:edgar]]

Comment on attachment 8551659 [details] [diff] [review]
Part 2 v1: Add Test Case to Improve Test Coverage.

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

Nice!!! Thank you.

Comment 14

[Josh Cheng [:josh]]

Hi Kai-Zhen,
2.0M+ Thanks!

Comment 15

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Josh Cheng [:josh] from comment #14)
> Hi Kai-Zhen,
> 2.0M+ Thanks!

Solution is not ready yet!
Patch Part 1 has to be reviewed again.

Comment 16

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 1 v2: Encode Length of "Text String" C-TLV in Varied Length of Octets.

address suggestions in comment 12.

Comment 17

[Edgar Chen [:edgar]]

Comment on attachment 8552410 [details] [diff] [review]
Part 1 v2: Encode Length of "Text String" C-TLV in Varied Length of Octets.

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

::: dom/system/gonk/ril_worker.js
@@ +7189,5 @@
> +   *        Function of how the data to be written into temporary buffer.
> +   *
> +   * @return array of written octets.
> +   **/
> +  writeWithBuffer: function(writeFunction) {

No better idea than this. Thank you, Bevis.

Comment 18

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8552410 [details] [diff] [review]
Part 1 v2: Encode Length of "Text String" C-TLV in Varied Length of Octets.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): NA
User impact if declined: Block partner's certificate for GCF/PTCRB test cases.
Testing completed: Yes. Test case is also included.
Risk to taking this patch (and alternatives if risky): No.
String or UUID changes made by this patch:NA

Comment 19

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8552410 [details] [diff] [review]
Part 1 v2: Encode Length of "Text String" C-TLV in Varied Length of Octets.

no need for v2.0

Comment 20

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8551659 [details] [diff] [review]
Part 2 v1: Add Test Case to Improve Test Coverage.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): NA
User impact if declined: Block partner's certificate for GCF/PTCRB test cases.
Testing completed: Yes. Test case is also included.
Risk to taking this patch (and alternatives if risky): No.
String or UUID changes made by this patch:NA

Comment 21

[Bevis Tseng[:bevis][:btseng](Exited)]

update tryserver result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7430c3bd2de

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/c2e6c6585d11
https://hg.mozilla.org/integration/b2g-inbound/rev/4c6e5c134982

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c2e6c6585d11
https://hg.mozilla.org/mozilla-central/rev/4c6e5c134982

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/6c4b49b5a56e
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/27e754b3f8f9

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/78a5a509b12b
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/72a504f92306

Comment 25

[Vincent Liu[:vliu]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/78a5a509b12b

Comment 26

[Kai-Zhen Li [:seinlin][:kli]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/72a504f92306

Comment 27

[Kai-Zhen Li [:seinlin][:kli]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/ad92e87542e9
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/9c62a8c9c36d