Closed Bug 838096 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: edgar, Assigned: edgar)

References

Details

Attachments

(5 files, 8 obsolete files)

1.44 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
2.77 KB, patch
Details | Diff | Splinter Review
3.01 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
1.12 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
4.12 KB, patch
Details | Diff | Splinter Review
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.
Blocks: 835729
Patch v2
Attachment #710101 - Attachment is obsolete: true
xpcshell tests
Attachment #710228 - Flags: review?(allstars.chh)
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)
Attached patch Part 2: Refactor readPNN, v1 (obsolete) — Splinter Review
Attachment #710228 - Attachment is obsolete: true
Attachment #710229 - Attachment is obsolete: true
Attachment #710229 - Flags: review?(allstars.chh)
Attachment #710525 - Flags: review?(allstars.chh)
Attachment #710527 - Flags: review?(allstars.chh)
Attachment #710529 - Flags: review?(allstars.chh)
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+
Address review comment #10
Attachment #710527 - Attachment is obsolete: true
Address review comment #11
Attachment #710529 - Attachment is obsolete: true
Attachment #710549 - Flags: review?
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)
(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.
(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?
Attachment #710531 - Attachment is obsolete: true
(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
Attachment #710583 - Flags: review?(allstars.chh)
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)
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+
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.