Closed Bug 785988 Opened 12 years ago Closed 12 years ago

B2G RIL: possible empty carrier short name causes zero valued MNC/MCC

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: vicamo, Assigned: vicamo)

References

Details

Attachments

(3 files, 4 obsolete files)

We found "CLARO BR", a local carrier in Brazil has empty short name reported and this cause ril_worker skips assignments of MNC/MCC value and leaves them stay just initialized.

> I/Gecko   (  106): RIL Worker: Operator: CLARO BR,,72405
> I/Gecko   (  106): RIL Worker: Queuing operator network info message:
> {"rilMessageType":"operatorchange","longName":"CLARO BR","shortName":"","mcc":0,"mnc":0}
Attached image screenshot
Assignee: nobody → vyang
Status: NEW → ASSIGNED
Attached patch Part 1: fix ril_worker (obsolete) — Splinter Review
Comment on attachment 655756 [details] [diff] [review]
Part 1: fix ril_worker

Let me guess, we can't write a Marionette test for this (yet)?
(In reply to Philipp von Weitershausen [:philikon] from comment #3)
> Comment on attachment 655756 [details] [diff] [review]
> Part 1: fix ril_worker
> 
> Let me guess, we can't write a Marionette test for this (yet)?

I'm still working on a new emulator command 'operator xxx' to enable Marionette test:
https://github.com/vicamo/platform_external_qemu/tree/bugzilla/785988/master

I'll request for review when it is done, but I think Cell Broadcast is much important than this bug though.
Attached patch Part 2: test scripts (obsolete) — Splinter Review
Attachment #657278 - Flags: review?(marshall)
Attachment #655756 - Flags: review?(marshall)
Attachment #655756 - Flags: review?(marshall) → review+
Comment on attachment 657278 [details] [diff] [review]
Part 2: test scripts

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

I'm a little confused by this test -- shouldn't we be checking that network.mcc / mnc return valid values when shortName and longName are empty? Also the call to setEmulatorOperatorNames() doesn't pass any arguments, which seems problematic.

::: dom/network/tests/marionette/test_mobile_operator_names.js
@@ +1,4 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +// getNetworks() can take some time..

nit: cargo cult comment, I don't think you're even calling getNetworks here :)

@@ +31,5 @@
> +  ok(network.mcc != 0, "mcc != 0");
> +  ok(network.mnc != 0, "mnc != 0");
> +}
> +
> +function cleanUp() {

nit: cleanUp should probably be at the bottom of the file :)

@@ +54,5 @@
> +    testValidMccMnc();
> +    cleanUp();
> +  };
> +
> +  setEmulatorOperatorNames()

This doesn't pass the longName / shortName parameters.. is the test working?

Also, minor nit: missing a semicolon
Attachment #657278 - Flags: review?(marshall) → review-
(In reply to Marshall Culpepper [:marshall_law] from comment #7)
> This doesn't pass the longName / shortName parameters.. is the test working?

That's embarrassed. The emulator is kind of broken due to bug 787298, so this test case just doesn't get tested itself at all.
Depends on: 787298
Attached patch Part 2: test scripts : V2 (obsolete) — Splinter Review
It seems emulator marionette test still fails even with bug 787298 backed out. Not fully tested yet.
Attachment #657278 - Attachment is obsolete: true
Attached patch Part 2: test scripts : V3 (obsolete) — Splinter Review
Rebased & and verified with my updated qemu pull(https://github.com/mozilla-b2g/platform_external_qemu/pull/4) dba99ea.
Attachment #657390 - Attachment is obsolete: true
Attachment #658026 - Flags: review?(marshall)
Comment on attachment 658026 [details] [diff] [review]
Part 2: test scripts : V3

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

Ahh much better :) r+ with the things below addressed..

::: dom/network/tests/marionette/test_mobile_operator_names.js
@@ +29,5 @@
> +}
> +
> +function testValidMccMnc() {
> +  ok(network.mcc != 0, "mcc != 0");
> +  ok(network.mnc != 0, "mnc != 0");

Would it be better to check the known MNC / MCC values of the emulator here?

@@ +40,5 @@
> +  connection.addEventListener("voicechange", function onvoicechange() {
> +    if (network.longName != longNameTest) {
> +      return;
> +    }
> +

We probably should have an assertion for the short name here too:

is(network.shortName, null);
Attachment #658026 - Flags: review?(marshall) → review+
> Rebased & and verified with my updated qemu
> pull(https://github.com/mozilla-b2g/platform_external_qemu/pull/4) dba99ea.

I will try to get to reviewing your pull request later today, thanks!
add r=marshall_law in changelog summary only.
Attachment #655756 - Attachment is obsolete: true
1. address review comment #11
2. check more possible cases and restore original operator names at the end.
Attachment #658026 - Attachment is obsolete: true
(In reply to Marshall Culpepper [:marshall_law] from comment #12)
> I will try to get to reviewing your pull request later today, thanks!

I'm landing gecko patches first.
https://hg.mozilla.org/integration/mozilla-inbound/rev/6dc3e692ae9a
https://hg.mozilla.org/integration/mozilla-inbound/rev/4806831bdeeb
https://hg.mozilla.org/mozilla-central/rev/6dc3e692ae9a
https://hg.mozilla.org/mozilla-central/rev/4806831bdeeb
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Summary: B2G SMS: possible empty carrier short name causes zero valued MNC/MCC → B2G RIL: possible empty carrier short name causes zero valued MNC/MCC
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: