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)
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: vicamo, Assigned: vicamo)
References
Details
Attachments
(3 files, 4 obsolete files)
30.59 KB,
image/jpeg
|
Details | |
1010 bytes,
patch
|
Details | Diff | Splinter Review | |
3.02 KB,
patch
|
Details | Diff | Splinter Review |
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}
Assignee | ||
Comment 1•12 years ago
|
||
Assignee: nobody → vyang
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•12 years ago
|
||
Comment 3•12 years ago
|
||
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)?
Assignee | ||
Comment 4•12 years ago
|
||
(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.
Assignee | ||
Comment 5•12 years ago
|
||
https://github.com/mozilla-b2g/platform_external_qemu/pull/4 pull request for emulator.
Assignee | ||
Comment 6•12 years ago
|
||
Attachment #657278 -
Flags: review?(marshall)
Assignee | ||
Updated•12 years ago
|
Attachment #655756 -
Flags: review?(marshall)
Updated•12 years ago
|
Attachment #655756 -
Flags: review?(marshall) → review+
Comment 7•12 years ago
|
||
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-
Assignee | ||
Comment 8•12 years ago
|
||
(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
Assignee | ||
Comment 9•12 years ago
|
||
It seems emulator marionette test still fails even with bug 787298 backed out. Not fully tested yet.
Attachment #657278 -
Attachment is obsolete: true
Assignee | ||
Comment 10•12 years ago
|
||
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 11•12 years ago
|
||
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+
Comment 12•12 years ago
|
||
> 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!
Assignee | ||
Comment 13•12 years ago
|
||
add r=marshall_law in changelog summary only.
Attachment #655756 -
Attachment is obsolete: true
Assignee | ||
Comment 14•12 years ago
|
||
1. address review comment #11 2. check more possible cases and restore original operator names at the end.
Attachment #658026 -
Attachment is obsolete: true
Assignee | ||
Comment 15•12 years ago
|
||
(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
Comment 16•12 years ago
|
||
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
Assignee | ||
Updated•11 years ago
|
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.
Description
•