1007487 - B2G RIL: incorrect isDisplayNetworkNameRequired for not matching both HPLMN and EF_SPDI.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[shawn ku [:sku]]

isDisplayNetworkNameRequired is set to false if not matching both HPLMN and EF_SPDI(see [1]). This is not correct behaviour.

By checking A.4 Service Provider Name indication in 3GPP TS 22.101, isDisplayNetworkNameRequired should be true for not matching both HPLMN and EF_SPDI case.


// Snippet of spec.
A.4
Service Provider Name indication
The service provider name is stored in the USIM in text and/or optionally graphic format. It shall be possible to associate at least 10 PLMN Identifications (MCC+MNC combination) with the same SP Name.

When registered on the HPLMN, or one of the PLMN Identifications used for Service Provider Name display:
  (i) The SP Name shall be displayed;
  (ii) Display of the PLMN Name is an operator"s option by setting the appropriate fields in the USIM (i.e. the Service Provider name shall be displayed either in parallel to the PLMN Name or instead of the PLMN Name).

When registered on neither the HPLMN, nor one of the PLMN Identifications used for Service Provider Name display:
  (i) The PLMN name shall be displayed;
  (ii) Display of the SP Name is an operator"s option by setting the appropriate fields in the USIM.



[1] - http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js?from=ril_worker.js&case=true#13833

Comment 1

[shawn ku [:sku]]

ni? Hsinyi for spec./code checking first.

Comment 2

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

(In reply to shawn ku [:sku] from comment #1)
> ni? Hsinyi for spec./code checking first.

Good catch! I think you are right~

Comment 3

[shawn ku [:sku]]

Attachment: Bug 1007487 Part 1: RIL patch - B2G RIL: incorrect isDisplayNetworkNameRequired for not matching both HPLMN and EF_SPDI.

Comment 4

[shawn ku [:sku]]

Attachment: Bug 1007487 Part 2: test patch - B2G RIL: incorrect isDisplayNetworkNameRequired for not matching both HPLMN and EF_SPDI.

Comment 5

[shawn ku [:sku]]

Attachment: Bug 1007487 Part 2: test patch - B2G RIL: incorrect isDisplayNetworkNameRequired for not matching both HPLMN and EF_SPDI.

Comment 6

[shawn ku [:sku]]

Attachment: Bug 1007487 Part 2: test patch - B2G RIL: incorrect isDisplayNetworkNameRequired for not matching both HPLMN and EF_SPDI.

Comment 7

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

Comment on attachment 8444922 [details] [diff] [review]
Bug 1007487 Part 1: RIL patch - B2G RIL: incorrect isDisplayNetworkNameRequired for not matching both HPLMN and EF_SPDI.

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

::: dom/system/gonk/ril_worker.js
@@ +13959,5 @@
>            }
>          }
>        }
>  
> +      if (RIL.voiceRegistrationState.connected) {

Can you explain this new if-clause?

@@ +13988,2 @@
>        } else {
> +        // Reset flags to false in case we hit no network, but show SPN case.

It seems fine with the original code because if there's no network, RIL.operator is null and isOnMatchingPlmn is false. The original code has already covered this case. I don't see the reason to force the values to be false. Can you explain the reason and point out the reference spec for this change?

Comment 8

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

Comment on attachment 8444973 [details] [diff] [review]
Bug 1007487 Part 2: test patch - B2G RIL: incorrect isDisplayNetworkNameRequired for not matching both HPLMN and EF_SPDI.

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

Bug 973024 split test_ril_worker_icc.js into several pieces. Please rebase properly and ask for review again. Sorry about that!

Comment 9

[shawn ku [:sku]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #7)
> Comment on attachment 8444922 [details] [diff] [review]
> Bug 1007487 Part 1: RIL patch - B2G RIL: incorrect
> isDisplayNetworkNameRequired for not matching both HPLMN and EF_SPDI.
> 
> Review of attachment 8444922 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +13959,5 @@
> >            }
> >          }
> >        }
> >  
> > +      if (RIL.voiceRegistrationState.connected) {
> 
> Can you explain this new if-clause?
> 
> @@ +13988,2 @@
> >        } else {
> > +        // Reset flags to false in case we hit no network, but show SPN case.
> 
> It seems fine with the original code because if there's no network,
> RIL.operator is null and isOnMatchingPlmn is false. The original code has
> already covered this case. I don't see the reason to force the values to be
> false. Can you explain the reason and point out the reference spec for this
> change?

If there is no network, yes, RIL.operator is null and isOnMatchingPlmn is false. however, isDisplaySpnRequired might be true if the 2nd bit of display condition is 0.
And,
TS 22.101 A.4 mainly talk about "registered" condition (Bug description).

In other works, we might get one case below
RIL.voiceRegistrationState.connected = false;
isDisplayNetworkNameRequired = false;
isDisplaySpnRequired = true;

when device do not camp on network which might confuse gaia.

That's why I choice to reset both flags to false.

Please let me know your opinion/concern if any.
Thanks!!
Shawn

Comment 10

[shawn ku [:sku]]

Hi HsinYi:
 Could you please check my reply on comment 9 to see if any opinion or concern you may have?
Thanks!!
Shawn

Comment 11

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

(In reply to shawn ku [:sku] from comment #9)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #7)
> > Comment on attachment 8444922 [details] [diff] [review]
> > Bug 1007487 Part 1: RIL patch - B2G RIL: incorrect
> > isDisplayNetworkNameRequired for not matching both HPLMN and EF_SPDI.
> > 
> > Review of attachment 8444922 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/system/gonk/ril_worker.js
> > @@ +13959,5 @@
> > >            }
> > >          }
> > >        }
> > >  
> > > +      if (RIL.voiceRegistrationState.connected) {
> > 
> > Can you explain this new if-clause?
> > 
> > @@ +13988,2 @@
> > >        } else {
> > > +        // Reset flags to false in case we hit no network, but show SPN case.
> > 
> > It seems fine with the original code because if there's no network,
> > RIL.operator is null and isOnMatchingPlmn is false. The original code has
> > already covered this case. I don't see the reason to force the values to be
> > false. Can you explain the reason and point out the reference spec for this
> > change?
> 
> If there is no network, yes, RIL.operator is null and isOnMatchingPlmn is
> false. however, isDisplaySpnRequired might be true if the 2nd bit of display
> condition is 0.
> And,
> TS 22.101 A.4 mainly talk about "registered" condition (Bug description).
> 

Hi Shawan,

Agree that to precisely meet the spec, we shall consider "registered" condition.

For this "registered" condition, I think it is better to do:

In [1] we have one more check |if (regState == NETWORK_CREG_STATE_REGISTERED_HOME)|?

Sounds good?

[1] dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js?from=ril_worker.js#13951

> In other works, we might get one case below
> RIL.voiceRegistrationState.connected = false;
> isDisplayNetworkNameRequired = false;
> isDisplaySpnRequired = true;
> 
> when device do not camp on network which might confuse gaia.

So far, I think it's fine, and no conflict to the spec. I'd like to keep the original logic.

> 
> That's why I choice to reset both flags to false.
> 
> Please let me know your opinion/concern if any.
> Thanks!!
> Shawn

Comment 12

[shawn ku [:sku]]

Attachment: Bug 1007487 Part 1: RIL patch - B2G RIL: incorrect isDisplayNetworkNameRequired for not matching both HPLMN and EF_SPDI. v2.

Comment 13

[shawn ku [:sku]]

Attachment: Bug 1007487 Part 2: test patch - B2G RIL: incorrect isDisplayNetworkNameRequired for not matching both HPLMN and EF_SPDI.

Rebase the patch.

Comment 14

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

Comment on attachment 8446430 [details] [diff] [review]
Bug 1007487 Part 1: RIL patch - B2G RIL: incorrect isDisplayNetworkNameRequired for not matching both HPLMN and EF_SPDI. v2.

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

Thank you!

Comment 15

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

Comment on attachment 8446431 [details] [diff] [review]
Bug 1007487 Part 2: test patch - B2G RIL: incorrect isDisplayNetworkNameRequired for not matching both HPLMN and EF_SPDI.

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

The fundamental structure looks good, but I'd like to add more checks :P

::: dom/system/gonk/tests/test_ril_worker_icc_SimRecordHelper.js
@@ +529,5 @@
> +    do_check_eq(ril.iccInfo.isDisplayNetworkNameRequired, expectedPlmnNameDisplay);
> +    do_check_eq(ril.iccInfo.isDisplaySpnRequired, expectedSpnDisplay);
> +  }
> +
> +  ril.voiceRegistrationState = {

I think we don't need this.

@@ +537,5 @@
> +  // Create empty operator object.
> +  ril.operator = {};
> +  // Setup SIM MCC/MNC to 310-260.
> +  ril.iccInfo.mcc = 310;
> +  ril.iccInfo.mnc = 260;

Can you also help add checks for 1) undefined displayCondition case, 2) the empty operator case and 3)  not HPLMN but in PLMN list? Thank you!

Comment 16

[shawn ku [:sku]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #15)
> Comment on attachment 8446431 [details] [diff] [review]
> Bug 1007487 Part 2: test patch - B2G RIL: incorrect
> isDisplayNetworkNameRequired for not matching both HPLMN and EF_SPDI.
> 
> Review of attachment 8446431 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The fundamental structure looks good, but I'd like to add more checks :P
> 
> ::: dom/system/gonk/tests/test_ril_worker_icc_SimRecordHelper.js
> @@ +529,5 @@
> > +    do_check_eq(ril.iccInfo.isDisplayNetworkNameRequired, expectedPlmnNameDisplay);
> > +    do_check_eq(ril.iccInfo.isDisplaySpnRequired, expectedSpnDisplay);
> > +  }
> > +
> > +  ril.voiceRegistrationState = {
> 
> I think we don't need this.

Thanks for catching this.
> 
> @@ +537,5 @@
> > +  // Create empty operator object.
> > +  ril.operator = {};
> > +  // Setup SIM MCC/MNC to 310-260.
> > +  ril.iccInfo.mcc = 310;
> > +  ril.iccInfo.mnc = 260;
> 
> Can you also help add checks for 1) undefined displayCondition case, 2) the
> empty operator case and 3)  not HPLMN but in PLMN list? Thank you!

Sure, will try to address it in next patch. Thanks!!

Comment 17

[shawn ku [:sku]]

Attachment: Bug 1007487 Part 2: test patch - B2G RIL: incorrect isDisplayNetworkNameRequired for not matching both HPLMN and EF_SPDI. v2.

Comment 18

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

Comment on attachment 8447822 [details] [diff] [review]
Bug 1007487 Part 2: test patch - B2G RIL: incorrect isDisplayNetworkNameRequired for not matching both HPLMN and EF_SPDI. v2.

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

r=me with comment addressed. Thanks :)

::: dom/system/gonk/tests/test_ril_worker_icc_SimRecordHelper.js
@@ +500,2 @@
>  
> +  function do_test_spdi(expectedPlmnNameDisplay, expectedSpnDisplay) {

If I understand this correctly, then |expectedPlmnNameDisplay| is always true and |expectedSpnDisplay| is always false. So having these two arguments for do_test_spdi() seem useless. Please consider removing them. Also, move the comments line 567 and line 568 to here. Thank you.

@@ +561,5 @@
> +  // Create empty operator object.
> +  ril.operator = {};
> +  // Setup SIM MCC-MNC to 310-260 as home network.
> +  ril.iccInfo.mcc = 310;
> +  ril.iccInfo.mnc = 260;

Please also have these:

do_test_spn(0x00, true, true);
do_test_spn(0x01, true, true);
do_test_spn(0x02, true, false);
do_test_spn(0x03, true, false);

Comment 19

[shawn ku [:sku]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #18)
> Comment on attachment 8447822 [details] [diff] [review]
> Bug 1007487 Part 2: test patch - B2G RIL: incorrect
> isDisplayNetworkNameRequired for not matching both HPLMN and EF_SPDI. v2.
> 
> Review of attachment 8447822 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with comment addressed. Thanks :)
> 
> ::: dom/system/gonk/tests/test_ril_worker_icc_SimRecordHelper.js
> @@ +500,2 @@
> >  
> > +  function do_test_spdi(expectedPlmnNameDisplay, expectedSpnDisplay) {
> 
> If I understand this correctly, then |expectedPlmnNameDisplay| is always
> true and |expectedSpnDisplay| is always false. So having these two arguments
> for do_test_spdi() seem useless. Please consider removing them. Also, move
> the comments line 567 and line 568 to here. Thank you.

Agree with you, thanks!!

> 
> @@ +561,5 @@
> > +  // Create empty operator object.
> > +  ril.operator = {};
> > +  // Setup SIM MCC-MNC to 310-260 as home network.
> > +  ril.iccInfo.mcc = 310;
> > +  ril.iccInfo.mnc = 260;
> 
> Please also have these:
> 
> do_test_spn(0x00, true, true);
> do_test_spn(0x01, true, true);
> do_test_spn(0x02, true, false);
> do_test_spn(0x03, true, false);

I believe above 4 tests should be the same condition as the line 595 ~ 601, that's non-home network case although one is with registered case, the other is not.

Should I add them in final version patch?

Thanks!!
Shawn

Comment 20

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

(In reply to shawn ku [:sku] from comment #19)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #18)
> > Comment on attachment 8447822 [details] [diff] [review]
> > Bug 1007487 Part 2: test patch - B2G RIL: incorrect
> > isDisplayNetworkNameRequired for not matching both HPLMN and EF_SPDI. v2.
> > 
> > Review of attachment 8447822 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > r=me with comment addressed. Thanks :)
> > 
> > ::: dom/system/gonk/tests/test_ril_worker_icc_SimRecordHelper.js
> > @@ +500,2 @@
> > >  
> > > +  function do_test_spdi(expectedPlmnNameDisplay, expectedSpnDisplay) {
> > 
> > If I understand this correctly, then |expectedPlmnNameDisplay| is always
> > true and |expectedSpnDisplay| is always false. So having these two arguments
> > for do_test_spdi() seem useless. Please consider removing them. Also, move
> > the comments line 567 and line 568 to here. Thank you.
> 
> Agree with you, thanks!!
> 
> > 
> > @@ +561,5 @@
> > > +  // Create empty operator object.
> > > +  ril.operator = {};
> > > +  // Setup SIM MCC-MNC to 310-260 as home network.
> > > +  ril.iccInfo.mcc = 310;
> > > +  ril.iccInfo.mnc = 260;
> > 
> > Please also have these:
> > 
> > do_test_spn(0x00, true, true);
> > do_test_spn(0x01, true, true);
> > do_test_spn(0x02, true, false);
> > do_test_spn(0x03, true, false);
> 
> I believe above 4 tests should be the same condition as the line 595 ~ 601,
> that's non-home network case although one is with registered case, the other
> is not.

As one is registered case, and the other is not, they are two different conditions, no?

Though in the end, null operator case hits the same result as non-home network, the conditions are different. It's better to cover more *conditions*.

> 
> Should I add them in final version patch?
> 

Yes, please! Thank you~ :)

> Thanks!!
> Shawn

Comment 21

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

Comment on attachment 8447822 [details] [diff] [review]
Bug 1007487 Part 2: test patch - B2G RIL: incorrect isDisplayNetworkNameRequired for not matching both HPLMN and EF_SPDI. v2.

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

::: dom/system/gonk/tests/test_ril_worker_icc_SimRecordHelper.js
@@ +561,5 @@
> +  // Create empty operator object.
> +  ril.operator = {};
> +  // Setup SIM MCC-MNC to 310-260 as home network.
> +  ril.iccInfo.mcc = 310;
> +  ril.iccInfo.mnc = 260;

I ignored line 571 in the previous review.

The test coverage looks good to me.

Comment 22

[shawn ku [:sku]]

Attachment: Bug 1007487 Part 1: RIL patch - B2G RIL: incorrect isDisplayNetworkNameRequired for not matching both HPLMN and EF_SPDI. v2. r=HsinYi.

Comment 23

[shawn ku [:sku]]

Attachment: Bug 1007487 Part 2: test patch - B2G RIL: incorrect isDisplayNetworkNameRequired for not matching both HPLMN and EF_SPDI. v2. r=HsinYi.

Comment 24

[shawn ku [:sku]]

try is green - https://tbpl.mozilla.org/?tree=Try&rev=033ab51b7227

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/1056a35af1c4
https://hg.mozilla.org/integration/b2g-inbound/rev/ae0542323b12

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1056a35af1c4
https://hg.mozilla.org/mozilla-central/rev/ae0542323b12

Comment 27

[shawn ku [:sku]]

Attachment: [2.0m] Bug 1007487 - B2G RIL: incorrect isDisplayNetworkNameRequired for not matching both HPLMN and EF_SPDI. r=HsinYi.

Comment 28

[shawn ku [:sku]]

try green on 2.0m - https://tbpl.mozilla.org/?tree=Try&rev=87c8cc11c119

Hi Seinlin:
 Please help push to 2.0m branch.

For 2.1, I will check if solution is there or not.

Comment 30

[shawn ku [:sku]]

Solution is there on 2.1. there is no effort for 2.1 uplifting.

Comment 31

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

http://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/670db989e603

Comment 32

[Josh Cheng [:josh]]

Hi Shawn,
partner uploaded new log. 
https://bugzilla.mozilla.org/show_bug.cgi?id=1087880#c4
Could you check again? Thanks!

Comment 33

[shawn ku [:sku]]

Already replied in https://bugzilla.mozilla.org/show_bug.cgi?id=1087880#c6.

Gecko part works as expectation, one gaia bug needs to be fixed now.