Closed Bug 999458 Opened 10 years ago Closed 10 years ago

B2G RIL: illegal gsmLocationAreaCode and gsmCellId reported

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.0 S1 (9may)

People

(Reporter: vicamo, Assigned: vicamo)

Details

(Whiteboard: [p=1])

Attachments

(7 files, 3 obsolete files)

62 bytes, text/x-github-pull-request
edgar
: review+
hsinyi
: review+
Details | Review
61 bytes, text/x-github-pull-request
edgar
: review+
Details | Review
1.39 KB, patch
edgar
: review+
Details | Diff | Splinter Review
6.92 KB, patch
edgar
: review+
Details | Diff | Splinter Review
1.37 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
9.64 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
6.48 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
From ril_worker.js line 3580, gsmLocationAreaCode off the range is still accepted and reported to content.  So we may have:

  "cell":{"gsmLocationAreaCode":4294967295,"gsmCellId":4294967295}

Actually, a bug in hardware/ril makes this always happen inside emulator.  From ril.h line 1239 [2], "((const char **)response)[1] is LAC if registered on a GSM/WCDMA system or NULL if not. Valid LAC are 0x0000 - 0xffff".  However, in reference-ril.c line L1602 [3], rild always prints out invalid hex octets even it's previously set to -1 in line 1470 [4].

[1]: http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#3580
[2]: https://github.com/mozilla-b2g/platform_hardware_ril/blob/master/include/telephony/ril.h#L1239
[3]: https://github.com/mozilla-b2g/platform_hardware_ril/blob/master/reference-ril/reference-ril.c#L1602
[4]: https://github.com/mozilla-b2g/platform_hardware_ril/blob/master/reference-ril/reference-ril.c#L1470
Several things in concern:

1) Per comment 0, rild may return illegal lac/cid values.  This is supposed to be fixed in https://github.com/mozilla-b2g/platform_hardware_ril/pull/35 .

2) Per comment 1, we should initialize lac/cid to UNKNOWN, which is numeric 0, instead of -1 in qemu.  This is supposed to be fixed in https://github.com/mozilla-b2g/platform_external_qemu/pull/76 part 1.

3) I also have part 2 in https://github.com/mozilla-b2g/platform_external_qemu/pull/76 to rewrite OPL/PNN records for complex tests.  However, such rewrite breaks current test cases and there seems to be no easy way to merge them concurrently.  I have to land a patch to disable related tests in Gecko first, and then merge PRs in qemu and rild, and then land patches to adopt new OPL/PNN contents and rewrite test cases.

4) In Gecko, I have a patch part 1/2 to temporarily disable GSM location related test cases, and part 2.a/2 to revert it.  Part 2.b/2 replaces some duplicated code and creates two shared utility function setEmulatorGsmLocationAndWait and setEmulatorOperatorNamesAndWait.  Part 2.c/2 is the corresponding patch for rild pull request.  Part 2.d/2 rewrites test_mobile_operator_names_plmnlist to include more OPL/PNN scenarios.

5) During the process, one thing is worth a special note here.  In RIL worker function _processOperator [1], we only re-evaluate network name when either given longName/shortName or mcc/mnc changes.  However, determining network name also involves location area code.  So we might have a corner case that:

  1) OPL contains a record that specifies mcc/mnc and a limited, effective LAC range,
  2) the user is currently camped on to a network with that mcc/mnc but he's out of that range,
  3) he moves and is registered on a network with the same provider but he's now inside that LAC range,
  4) network name is not updated as PNN/OPL suggest.

When this problem is mixed with screen state, it becomes even more complex because SET_SCREEN_STATE request may turn off modem lac/cid position reporting and we'll get negative gsmLocationAreaCode from RIL worker.  Such transition should not be taken into consideration and should not result in network names change.  This problem is not addressed in this bug.

[1]: http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#3658
Assignee: nobody → vyang
Whiteboard: [p=3]
Target Milestone: --- → 1.4 S6 (25apr)
This, if Bluetooth on JB or above (bug 944299) is not taken into account, is the last known unconditional failure on emulator-kk.
Please see comment 3 2) and 3).
Attachment #8411579 - Flags: review?(htsai)
Attachment #8411579 - Flags: review?(echen)
Please see comment 3 1).
Attachment #8411580 - Flags: review?(echen)
Attachment #8411584 - Flags: review?(echen)
Attachment #8411584 - Attachment description: [Gecko] 2.a/2: revert part 1/2 → [Gecko] part 2.a/2: revert part 1/2
Attached patch [Gecko] part 2.d/2: test cases (obsolete) — Splinter Review
Attachment #8411587 - Flags: review?(echen)
Landing sequence: 1) Gecko part 1, 2) qemu, rild pull requests, 3) Gecko part 2.x parts.
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #3)
> However, such rewrite breaks current
> test cases and there seems to be no easy way to merge them concurrently.  I
> have to land a patch to disable related tests in Gecko first, and then merge
> PRs in qemu and rild, and then land patches to adopt new OPL/PNN contents
> and rewrite test cases.

Ummm .... it would be really nice to have the ability to adjust SIM EF files at runtime :X
Attachment #8411579 - Attachment mime type: text/plain → text/x-github-pull-request
Attachment #8411581 - Flags: review?(echen) → review+
Comment on attachment 8411580 [details] [review]
Github PR for hardware/ril, master branch

r=me with comments on github addressed. Thank you.
Attachment #8411580 - Flags: review?(echen) → review+
Comment on attachment 8411585 [details] [diff] [review]
[Gecko] part 2.b/2: add two utility functions to head.js

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

::: dom/mobileconnection/tests/marionette/head.js
@@ +543,5 @@
> + *        A boolean value.  Default false.
> + *
> + * @return A deferred promise.
> + */
> +function setEmulatorGsmLocationAndWait(aLac, aCid, aWaitVoice, aWaitData) {

nit: setEmulatorGsmLocationAndWait( ... , aWaitVoice = true, ...)

@@ +657,5 @@
> + *
> + * @return A deferred promise.
> + */
> +function setEmulatorOperatorNamesAndWait(aOperator, aLongName, aShortName,
> +                                         aMcc, aMnc, aWaitVoice, aWaitData) {

nit: aWaitVoice = true
Attachment #8411585 - Flags: review?(echen) → review+
Attachment #8411586 - Flags: review?(echen) → review+
Attachment #8411579 - Flags: review?(echen) → review+
Comment on attachment 8411587 [details] [diff] [review]
[Gecko] part 2.d/2: test cases

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

Thank you. :)

::: dom/mobileconnection/tests/marionette/test_mobile_operator_names_plmnlist.js
@@ +92,5 @@
> +
> +        // Test if we match MNC "01" and "001" correctly.
> +        //
> +        // Use |diffCid| to ensure voicechange event will be triggered in the
> +        // next run.

I guess this comment is for the last test (the reset one). If it is the case, could you please help to adjust it? Thank you.
Attachment #8411587 - Flags: review?(echen) → review+
Attachment #8411584 - Flags: review?(echen) → review+
Comment on attachment 8411579 [details] [review]
Github PR for external/qemu, master branch

Looks good.
Attachment #8411579 - Flags: review?(htsai) → review+
update commit summary only.
Attachment #8411584 - Attachment is obsolete: true
Attachment #8415019 - Flags: review+
1) Remove redundant comments pointed out in comment 15.  I had two similar comments before.  The first one had been removed before that patch being ever uploaded, but I missed the second one.

2) Add a few more test cases to ensure we don't miss anything and don't go wrong on some corner cases.
Attachment #8411587 - Attachment is obsolete: true
Attachment #8415023 - Flags: review+
https://hg.mozilla.org/integration/b2g-inbound/rev/876baa06feb9
Whiteboard: [p=3] → [p=1][leave open]
Target Milestone: 1.4 S6 (25apr) → 2.0 S1 (9may)
You need to log in before you can comment on or make changes to this bug.