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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Edgar Chen [:edgar]]

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.

Comment 1

[Edgar Chen [:edgar]]

Attachment: Part 1: Ignore the record of PNN if the contents are unused bytes, v1

Comment 2

[Edgar Chen [:edgar]]

Attachment: Part 1: Ignore the record of PNN if the contents are unused bytes, v2

Patch v2

Comment 3

[Edgar Chen [:edgar]]

Attachment: Part 2: xpcshell tests for getPNN, v1

xpcshell tests

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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.

Comment 5

[Edgar Chen [:edgar]]

Attachment: Part 1: Rename getPNN as readPNN, v1

Comment 6

[Edgar Chen [:edgar]]

Attachment: Part 2: Refactor readPNN, v1

Comment 7

[Edgar Chen [:edgar]]

Attachment: Part 3: Ignore the record of PNN if the contents are unused bytes, v3

Comment 8

[Edgar Chen [:edgar]]

Attachment: Part 4: xpcshell tests for readPNN, v2

Comment 9

[Edgar Chen [:edgar]]

try server: https://tbpl.mozilla.org/?tree=Try&rev=ecf420fd0bb7

Comment 10

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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.

Comment 11

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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.

Comment 12

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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;

Comment 13

[Edgar Chen [:edgar]]

Attachment: Part 2: Refactor readPNN, v2, r=allstars.chh

Address review comment #10

Comment 14

[Edgar Chen [:edgar]]

Attachment: Part 3: Ignore the record of PNN if the contents are unused bytes, v4

Address review comment #11

Comment 15

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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.

Comment 16

[Edgar Chen [:edgar]]

(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.

Comment 17

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(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.

Comment 18

[Edgar Chen [:edgar]]

(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?

Comment 19

[Edgar Chen [:edgar]]

Attachment: Part 3: Ignore the record of PNN if the contents are unused bytes, v5

Address comment #15, comment #18.

Comment 20

[Edgar Chen [:edgar]]

Attachment: Part 4: xpcshell tests for readPNN, v3

Comment 21

[Edgar Chen [:edgar]]

(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

Comment 22

[Edgar Chen [:edgar]]

Attachment: Part 5: Modify test_update_network_name, v1

Comment 23

[Edgar Chen [:edgar]]

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

Comment 24

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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 || {};

Comment 25

[Edgar Chen [:edgar]]

Attachment: Part 3: Ignore the record of PNN if the contents are unused bytes, v6, r=allstars.chh

Address review comment #24

Comment 26

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

I'll help to push

Comment 27

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Oh, inbound is closed now, check-in later.

Comment 28

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/516c6b936873
https://hg.mozilla.org/integration/mozilla-inbound/rev/991fbb07392c
https://hg.mozilla.org/integration/mozilla-inbound/rev/6deea379453a
https://hg.mozilla.org/integration/mozilla-inbound/rev/581ef9ed708d
https://hg.mozilla.org/integration/mozilla-inbound/rev/868939d72f57

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/516c6b936873
https://hg.mozilla.org/mozilla-central/rev/991fbb07392c
https://hg.mozilla.org/mozilla-central/rev/6deea379453a
https://hg.mozilla.org/mozilla-central/rev/581ef9ed708d
https://hg.mozilla.org/mozilla-central/rev/868939d72f57