B2G RIL: If the EF_PNN are all 0xff, the network name becomes empty string.

RESOLVED FIXED

Status

Firefox OS
General
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: edgar, Assigned: edgar)

Tracking

unspecified
ARM
Gonk (Firefox OS)
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 8 obsolete attachments)

1.44 KB, patch
allstars
: review+
Details | Diff | Splinter Review
2.77 KB, patch
Details | Diff | Splinter Review
3.01 KB, patch
allstars
: review+
Details | Diff | Splinter Review
1.12 KB, patch
allstars
: review+
Details | Diff | Splinter Review
4.12 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
I found this bug when I was testing the patch of bug 835729 with Chungwa SIM card. The EF_PNN of Chungwa SIM card are all 0xff and the PNN will be read as empty string. When updating network name, according to TS 31.102 session 4.2.58,  the first record in EF_PNN is used for default network name. So the network name will be updated as empty string. In this case, I think the 0xff is used to indicate the unused bytes, we shall not read EF_PNN as empty string but ignore it.
(Assignee)

Updated

6 years ago
Blocks: 835729
(Assignee)

Comment 1

6 years ago
Created attachment 710101 [details] [diff] [review]
Part 1: Ignore the record of PNN if the contents are unused bytes, v1
(Assignee)

Comment 2

6 years ago
Created attachment 710228 [details] [diff] [review]
Part 1: Ignore the record of PNN if the contents are unused bytes, v2

Patch v2
Attachment #710101 - Attachment is obsolete: true
(Assignee)

Comment 3

6 years ago
Created attachment 710229 [details] [diff] [review]
Part 2: xpcshell tests for getPNN, v1

xpcshell tests
(Assignee)

Updated

6 years ago
Attachment #710228 - Flags: review?(allstars.chh)
(Assignee)

Updated

6 years ago
Attachment #710229 - Flags: review?(allstars.chh)
Comment on attachment 710228 [details] [diff] [review]
Part 1: Ignore the record of PNN if the contents are unused bytes, v2

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

The parcel parsing style isn't consistent with others,
can you create a patch (in Part 1) to correct them first?

And move this patch to part 2

::: dom/system/gonk/ril_worker.js
@@ +9381,5 @@
>     */
>    getPNN: function getPNN() {
>      let pnn = [];
>      function callback(options) {
> +      let pnnElement = null;

Maybe define pnnElement until we really need it.

@@ +9386,1 @@
>        let len = Buf.readUint32();

s/len/strLen/
and add a 'octLen' variable

@@ +9386,3 @@
>        let len = Buf.readUint32();
>        let readLen = 0;
>        while (len > readLen) {

revise this
should be readLen < octetLen

@@ +9386,5 @@
>        let len = Buf.readUint32();
>        let readLen = 0;
>        while (len > readLen) {
>          let tlvTag = GsmPDUHelper.readHexOctet();
>          readLen = readLen + 2; // 1 Hex octet

Use octet as unit, i.e. should be readLen++;

@@ +9390,5 @@
>          readLen = readLen + 2; // 1 Hex octet
>          if (tlvTag == 0xFF) {
>            // Unused byte
>            continue;
>          }

I think we could also do as others by using seekIncoming

@@ +9394,5 @@
>          }
> +
> +        if (!pnnElement) {
> +          pnnElement = {fullName: "", shortName: ""};
> +        }

Here should be moved to Part 2,
but the if check seems useless, since pnnElement is always null

@@ +9408,5 @@
>            break;
>          default:
>            Buf.seekIncoming(PDU_HEX_OCTET_SIZE * tlvLen);
>          }
>          readLen += (tlvLen * 2 + 2);

revise here.
Attachment #710228 - Flags: review?(allstars.chh)
(Assignee)

Comment 5

5 years ago
Created attachment 710525 [details] [diff] [review]
Part 1: Rename getPNN as readPNN, v1
(Assignee)

Comment 6

5 years ago
Created attachment 710527 [details] [diff] [review]
Part 2: Refactor readPNN, v1
(Assignee)

Comment 7

5 years ago
Created attachment 710529 [details] [diff] [review]
Part 3: Ignore the record of PNN if the contents are unused bytes, v3
Attachment #710228 - Attachment is obsolete: true
(Assignee)

Comment 8

5 years ago
Created attachment 710531 [details] [diff] [review]
Part 4: xpcshell tests for readPNN, v2
Attachment #710229 - Attachment is obsolete: true
Attachment #710229 - Flags: review?(allstars.chh)
(Assignee)

Updated

5 years ago
Attachment #710525 - Flags: review?(allstars.chh)
(Assignee)

Updated

5 years ago
Attachment #710527 - Flags: review?(allstars.chh)
(Assignee)

Updated

5 years ago
Attachment #710529 - Flags: review?(allstars.chh)
(Assignee)

Updated

5 years ago
Attachment #710531 - Flags: review?(allstars.chh)
Attachment #710525 - Flags: review?(allstars.chh) → review+
Comment on attachment 710527 [details] [diff] [review]
Part 2: Refactor readPNN, v1

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

::: dom/system/gonk/ril_worker.js
@@ +9408,5 @@
> +          case PNN_IEI_SHORT_NETWORK_NAME:
> +            pnnElement.shortName = GsmPDUHelper.readNetworkName(tlvLen);
> +            break;
> +          default:
> +            Buf.seekIncoming(tlvLen * PDU_HEX_OCTET_SIZE);

nit, add 'break;' here.
Attachment #710527 - Flags: review?(allstars.chh) → review+
Comment on attachment 710529 [details] [diff] [review]
Part 3: Ignore the record of PNN if the contents are unused bytes, v3

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

The default value of pnn seems from Bug 822384,
in that bug the problem is shortName is an optional field from spec and that SIM happens to not have this value.
However I think that's not the right patch in that bug.

In this bug, all bytes from EF_PNN are all 0xff so not just shortName but longName could also be empty.

Therefore I think in your patch 
1. You don't have to initialize pnn with two properties 'longName' and 'shortName'
   Since shortName could be optional and also entire PNN record could be optional.
2. In the function updateNetworkName, it should also check if pnn has longName and shortName defined, otherwise it should use "" as default.
Attachment #710529 - Flags: review?(allstars.chh)
Comment on attachment 710531 [details] [diff] [review]
Part 4: xpcshell tests for readPNN, v2

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

::: dom/system/gonk/tests/test_ril_worker_icc.js
@@ +1058,5 @@
> +    // Fake get response
> +    if (!options.totalRecords) {
> +      options.totalRecords = records.length;
> +      options.recordSize = records[0].length;
> +      options.p1 = 1;

p1 isn't from GET_RESPONSE,
if you want to have to default value here,
simply options.p1 = options.p1 || 1;

also the "if (!options.totalRecords)" seems wired to me
maybe you could use a fake getResponse function to initialize totalRecords and recordSize,
or likewise
options.totalRecords = options.totalRecords || records.length;
options.recordSize = options.recordSize || records[0].length;
Attachment #710531 - Flags: review?(allstars.chh) → review+
(Assignee)

Comment 13

5 years ago
Created attachment 710542 [details] [diff] [review]
Part 2: Refactor readPNN, v2, r=allstars.chh

Address review comment #10
Attachment #710527 - Attachment is obsolete: true
(Assignee)

Comment 14

5 years ago
Created attachment 710549 [details] [diff] [review]
Part 3: Ignore the record of PNN if the contents are unused bytes, v4

Address review comment #11
Attachment #710529 - Attachment is obsolete: true
Attachment #710549 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #710549 - Flags: review? → review?(allstars.chh)
Comment on attachment 710549 [details] [diff] [review]
Part 3: Ignore the record of PNN if the contents are unused bytes, v4

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

Seems okay, but need to have a better way handling PNN.

Also in most cases PNN doesn't have 254 records,
it would be better if we didn't try to load every one of them.

::: dom/system/gonk/ril_worker.js
@@ +1356,2 @@
>      }
>      return null;

if (pnnEntry) {
  pnnEntry.fullName = pnnEntry.fullName || "";
  pnnEntry.shortName = pnnEntry.shortName || "";
}
return pnnEntry;

@@ +9422,5 @@
>  
> +      if (pnnElement) {
> +        if (DEBUG) {
> +          debug("PNN: [" + (pnn.length + 1) + "]: " + JSON.stringify(pnnElement));
> +        }

I got concerned with this line,
once pnnElement is initialized,
the log will be flood by PNN.

For example, PNN has 5 available records, but remaining 2XX records are 0xff.
Attachment #710549 - Flags: review?(allstars.chh)
(Assignee)

Comment 16

5 years ago
(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #15)
> Comment on attachment 710549 [details] [diff] [review]
> Part 3: Ignore the record of PNN if the contents are unused bytes, v4
> 
> Review of attachment 710549 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Seems okay, but need to have a better way handling PNN.
> 
> Also in most cases PNN doesn't have 254 records,
> it would be better if we didn't try to load every one of them.
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +1356,2 @@
> >      }
> >      return null;
> 
> if (pnnEntry) {
>   pnnEntry.fullName = pnnEntry.fullName || "";
>   pnnEntry.shortName = pnnEntry.shortName || "";
> }
> return pnnEntry;

I plan to refactor this on bug 835729, so keep the original coding style here.
(In reply to Edgar Chen [:edgar][:echen] from comment #16)
> > 
> > if (pnnEntry) {
> >   pnnEntry.fullName = pnnEntry.fullName || "";
> >   pnnEntry.shortName = pnnEntry.shortName || "";
> > }
> > return pnnEntry;
> 
> I plan to refactor this on bug 835729, so keep the original coding style
> here.

The problem is updateNetworkName should be totally rewritten with _processOperator and we still need to fix this bug.

We cannot replace bad patch with another bad patch here.
(Assignee)

Comment 18

5 years ago
(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #15)
> Comment on attachment 710549 [details] [diff] [review]
> Part 3: Ignore the record of PNN if the contents are unused bytes, v4
> 
> Review of attachment 710549 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Seems okay, but need to have a better way handling PNN.
> 
> Also in most cases PNN doesn't have 254 records,
> it would be better if we didn't try to load every one of them.

I think the records of PNN should be continuous in EF.
How about we ignore remaining records when we got one record are 0xff?
(Assignee)

Comment 19

5 years ago
Created attachment 710583 [details] [diff] [review]
Part 3: Ignore the record of PNN if the contents are unused bytes, v5

Address comment #15, comment #18.
Attachment #710549 - Attachment is obsolete: true
(Assignee)

Comment 20

5 years ago
Created attachment 710585 [details] [diff] [review]
Part 4: xpcshell tests for readPNN, v3
Attachment #710531 - Attachment is obsolete: true
(Assignee)

Comment 21

5 years ago
(In reply to Edgar Chen [:edgar][:echen] from comment #20)
> Created attachment 710585 [details] [diff] [review]
> Part 4: xpcshell tests for readPNN, v3

Address comment #12
(Assignee)

Comment 22

5 years ago
Created attachment 710586 [details] [diff] [review]
Part 5: Modify test_update_network_name, v1
(Assignee)

Updated

5 years ago
Attachment #710583 - Flags: review?(allstars.chh)
(Assignee)

Comment 23

5 years ago
Comment on attachment 710585 [details] [diff] [review]
Part 4: xpcshell tests for readPNN, v3

I have added one more test record, so request review again. Thanks
Attachment #710585 - Flags: review?(allstars.chh)
(Assignee)

Updated

5 years ago
Attachment #710586 - Flags: review?(allstars.chh)
Comment on attachment 710583 [details] [diff] [review]
Part 3: Ignore the record of PNN if the contents are unused bytes, v5

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

::: dom/system/gonk/ril_worker.js
@@ +9405,5 @@
>  
> +        // Needs this check to avoid initializing twice.
> +        if (!pnnElement) {
> +          pnnElement = {};
> +        }

pnnElement = pnnElement || {};
Attachment #710583 - Flags: review?(allstars.chh) → review+
Attachment #710585 - Flags: review?(allstars.chh) → review+
Attachment #710586 - Flags: review?(allstars.chh) → review+
(Assignee)

Comment 25

5 years ago
Created attachment 710609 [details] [diff] [review]
Part 3: Ignore the record of PNN if the contents are unused bytes, v6, r=allstars.chh

Address review comment #24
Attachment #710583 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
I'll help to push
Keywords: checkin-needed
Oh, inbound is closed now, check-in later.
You need to log in before you can comment on or make changes to this bug.