Edgar Chen [:edgar] Assignee
	Description • 11 years ago

+++ This bug was initially created as a clone of Bug #961927 +++

If data call is established and enter into no coverage area, the registration status is still 'registered'.
I think it is caused by below error, so that gecko doesn't update status successfully.

$$01-22 16:30:43.000   107   107 I Gecko   : -*- RadioInterface[0]: Received message from worker: {"rilMessageType":"networkinfochanged","voiceRegistrationState":{"regState":0,"state":"notSearching","connected":false,"roaming":false,"emergencyCallsOnly":true,"cell":{"gsmLocationAreaCode":-1,"gsmCellId":-1},"radioTech":0,"type":null,"rilMessageType":"voiceregistrationstatechange"},"dataRegistrationState":{"regState":0,"state":"notSearching","connected":false,"roaming":false,"emergencyCallsOnly":true,"cell":{"gsmLocationAreaCode":-1,"gsmCellId":-1},"radioTech":0,"type":null,"rilMessageType":"dataregistrationstatechange"},"operator":{"rilMessageType":"operatorchange","longName":null,"shortName":null,"mcc":null,"mnc":null},"signal":{"voice":{"signalStrength":null,"relSignalStrength":null},"data":{"signalStrength":null,"relSignalStrength":null},"rilMessageType":"signalstrengthchange"}}
$$01-22 16:30:43.000   107   107 I Gecko   : -*- RadioInterface[0]: RIL is not ready for data connection: Phone's not registered or doesn't have data connection.
$$01-22 16:30:43.000   107   107 E GeckoConsole: [JavaScript Error: "operator is null" {file: "jar:file:///system/b2g/omni.ja!/components/RadioInterfaceLayer.js" line: 1690}]

	Edgar Chen [:edgar] Assignee
	Comment 1 • 11 years ago

After checking the RadioInterfaceLayer.js, I guess the JavaScript Error is happened in below function.

checkRoamingBetweenOperators: function(registration) {
  let iccInfo = this.rilContext.iccInfo;
  if (!iccInfo || !registration.connected) {
    return;
  }

  let spn = iccInfo.spn && iccInfo.spn.toLowerCase();
  let operator = registration.network;
  let longName = operator.longName && operator.longName.toLowerCase();
  let shortName = operator.shortName && operator.shortName.toLowerCase();

  let equalsLongName = longName && (spn == longName);
  let equalsShortName = shortName && (spn == shortName);
  let equalsMcc = iccInfo.mcc == operator.mcc;

  registration.roaming = registration.roaming &&
                         !(equalsMcc && (equalsLongName || equalsShortName));
}

I think I know the possible case to trigger this error.
But unfortunately I did not reproduce yet. :(

	Edgar Chen [:edgar] Assignee
	Comment 2 • 11 years ago

Another log reproduced on my fugu device.

	Edgar Chen [:edgar] Assignee
	Comment 3 • 11 years ago

(In reply to Edgar Chen [:edgar][:echen] from comment #1)
> After checking the RadioInterfaceLayer.js, I guess the JavaScript Error is
> happened in below function.
> 
> checkRoamingBetweenOperators: function(registration) {
>   let iccInfo = this.rilContext.iccInfo;
>   if (!iccInfo || !registration.connected) {

1). We should check |network| here as well for the case that operator information is not ready yet.
2). Use |connected| as a condition is not so accurate. The |connected| of data is true only if the "default" connection is established. So for the case that device doesn't have "default" connection, the |roaming| of data will not be updated correctly. So I propose check |state| instead.

------
  if (!iccInfo ||
      !registration.network ||
      registartion.state != "registered"){
    return;
  }
-----

>     return;
>   }
> 
>   let spn = iccInfo.spn && iccInfo.spn.toLowerCase();
>   let operator = registration.network;
>   let longName = operator.longName && operator.longName.toLowerCase();
>   let shortName = operator.shortName && operator.shortName.toLowerCase();
> 
>   let equalsLongName = longName && (spn == longName);
>   let equalsShortName = shortName && (spn == shortName);
>   let equalsMcc = iccInfo.mcc == operator.mcc;
> 
>   registration.roaming = registration.roaming &&
>                          !(equalsMcc && (equalsLongName || equalsShortName));
> }

	Edgar Chen [:edgar] Assignee
	Comment 5 • 11 years ago

Now I am working on marionette test cases.

	Edgar Chen [:edgar] Assignee
	Comment 6 • 11 years ago

Block bug 961927 which is 1.3?, so mark as 1.3? as well.

	Edgar Chen [:edgar] Assignee
	Comment 7 • 11 years ago

Detailed information about this patch:
1). Put common code in mobile_header.js.
2). Refactor test_mobile_voice_state.js.
- Include mobile_header.js
- Move the cell information tests for voice to a separated file, test_mobile_voice_location.js. (Just like we have test_mobile_data_location.js for data)
- In original cell information tests, we did not restore cell information to default, it will affect subsequent tests. Correct this missing.
3). Refactor test_mobile_data_state.js.
- Include mobile_header.js
- Add more tests for roaming status of data.

(((Note: I think we should also refactor other tests. Let's do it in another bug. ;))))

	Edgar Chen [:edgar] Assignee
	Comment 8 • 11 years ago

Comment on attachment 8364953 [details] [diff] [review]
Part 1: Fix the JavaScript error thrown in checkRoamingBetweenOperators() when the operator information is not available, v1

Please see comment #3. Thank you.

	Edgar Chen [:edgar] Assignee
	Comment 9 • 11 years ago

Comment on attachment 8365846 [details] [diff] [review]
Part 2: Refactor test_mobile_{voice|data}_state.js and add more tests for data roaming status, v1

Please see comment #7. Thank you.

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 10 • 11 years ago

Comment on attachment 8364953 [details] [diff] [review]
Part 1: Fix the JavaScript error thrown in checkRoamingBetweenOperators() when the operator information is not available, v1

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

Makes sense, thanks.

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 11 • 11 years ago

Comment on attachment 8365846 [details] [diff] [review]
Part 2: Refactor test_mobile_{voice|data}_state.js and add more tests for data roaming status, v1

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

Thanks for the patch! Please remember to file another bug for refactoring the remaining test cases.

::: dom/mobileconnection/tests/marionette/mobile_header.js
@@ +44,5 @@
> +    this.pendingCommandCount++;
> +    runEmulatorCmd(cmd, function(results) {
> +      this.pendingCommandCount--;
> +
> +      let result = results[results.length - 1]

A semicolon is missing.

::: dom/mobileconnection/tests/marionette/test_mobile_data_state.js
@@ +70,4 @@
>  
> +      let cell = data.cell;
> +      if (!expect.cell) {
> +        is(cell, expect.cell, "check data.cell");

nit: ok(!cell, "check data.cell");

::: dom/mobileconnection/tests/marionette/test_mobile_voice_state.js
@@ +68,4 @@
>  
> +      let cell = voice.cell;
> +      if (!expect.cell) {
> +        is(cell, expect.cell, "check voice.cell");

nit: ok(!cell, "check voice.cell");

	Edgar Chen [:edgar] Assignee
	Comment 12 • 11 years ago

(In reply to Hsin-Yi Tsai (OOO Jan. 29 ~ Feb. 5th) [:hsinyi] from comment #11)
> Comment on attachment 8365846 [details] [diff] [review]
> Part 2: Refactor test_mobile_{voice|data}_state.js and add more tests for
> data roaming status, v1
> 
> Review of attachment 8365846 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the patch! Please remember to file another bug for refactoring
> the remaining test cases.
Bug 964680.

> 
> ::: dom/mobileconnection/tests/marionette/mobile_header.js
> @@ +44,5 @@
> > +    this.pendingCommandCount++;
> > +    runEmulatorCmd(cmd, function(results) {
> > +      this.pendingCommandCount--;
> > +
> > +      let result = results[results.length - 1]
> 
> A semicolon is missing.
Aha, thanks for catching this.

	Edgar Chen [:edgar] Assignee
	Comment 13 • 11 years ago

Address review comment #11.

	Edgar Chen [:edgar] Assignee
	Comment 14 • 11 years ago

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

	Edgar Chen [:edgar] Assignee
	Comment 15 • 11 years ago

(In reply to Edgar Chen [:edgar][:echen] from comment #6)
> Block bug 961927 which is 1.3?, so mark as 1.3? as well.

This bug blocks bug 961927 which was marked as 1.3+ already.
I think this bug should be 1.3+ as well. Could someone help to triage this?
Thank you.

	Jason Smith [:jsmith]
	Comment 16 • 11 years ago

(In reply to Edgar Chen [:edgar][:echen] from comment #15)
> (In reply to Edgar Chen [:edgar][:echen] from comment #6)
> > Block bug 961927 which is 1.3?, so mark as 1.3? as well.
> 
> This bug blocks bug 961927 which was marked as 1.3+ already.
> I think this bug should be 1.3+ as well. Could someone help to triage this?
> Thank you.

I already +ed the bug.

	Jason Smith [:jsmith]
	Comment 17 • 11 years ago

(In reply to Jason Smith [:jsmith] from comment #16)
> (In reply to Edgar Chen [:edgar][:echen] from comment #15)
> > (In reply to Edgar Chen [:edgar][:echen] from comment #6)
> > > Block bug 961927 which is 1.3?, so mark as 1.3? as well.
> > 
> > This bug blocks bug 961927 which was marked as 1.3+ already.
> > I think this bug should be 1.3+ as well. Could someone help to triage this?
> > Thank you.
> 
> I already +ed the bug.

Oh wait - wrong bug - this one I haven't triaged yet. Fixing this now.

	Edgar Chen [:edgar] Assignee
	Comment 18 • 11 years ago

(In reply to Edgar Chen [:edgar][:echen] from comment #14)
> Try server: https://tbpl.mozilla.org/?tree=Try&rev=b715309ce7cd

All green \o/

https://hg.mozilla.org/integration/b2g-inbound/rev/b8a673c86b0b
https://hg.mozilla.org/integration/b2g-inbound/rev/53ed3e402dca

	Edgar Chen [:edgar] Assignee
	Comment 19 • 11 years ago

Patch for v1.3 branch.

	Edgar Chen [:edgar] Assignee
	Comment 20 • 11 years ago

Patch for v1.3 branch.

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/b8a673c86b0b
https://hg.mozilla.org/mozilla-central/rev/53ed3e402dca

	Ryan VanderMeulen [:RyanVM]
	Comment 22 • 11 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/16469e7f2d4f
https://hg.mozilla.org/releases/mozilla-aurora/rev/9e33a15f75cb