Closed Bug 974253 Opened 7 years ago Closed 7 years ago

Preferred network type setting will not be saved if reboot DUT right after change the setting.

Categories

(Firefox OS Graveyard :: Gaia::Settings, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:1.3+, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)

RESOLVED FIXED
1.4 S4 (28mar)
blocking-b2g 1.3+
Tracking Status
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: arthurcc, Assigned: arthurcc)

References

Details

Attachments

(2 files, 1 obsolete file)

Preferred network type setting cannot be saved after reboot. It will be restored back to the default value. This can only be reproduced on fugu as its gonk does not preserve the settings. However, gaia should always save the user preference in this case.

* Reproduce Steps
1. Set preferred network type to any item other than the default value.
2. Reboot the device.

* Expected Result
New preferred network type setting should be saved even reboot the device. 

* Actual Result
New preferred network type setting is not saved.

* Occurrence rate
100%
Assignee: nobody → arthur.chen
Depends on: 946589
Target Milestone: --- → 1.4 S2 (28feb)
Target Milestone: 1.4 S2 (28feb) → ---
Nominate for 1.3T as it blocks a 1.3T blocker.
blocking-b2g: --- → 1.3T?
triage: 1.3T+
blocking-b2g: 1.3T? → 1.3T+
Comment on attachment 8378903 [details]
link to https://github.com/mozilla-b2g/gaia/pull/16477

Jose, Alive, could you help review the patch? Which is similar to other telephony settings. Thanks!
Attachment #8378903 - Flags: review?(josea.olivera)
Attachment #8378903 - Flags: review?(alive)
Attachment #8378903 - Flags: review?(alive) → review+
Comment on attachment 8378903 [details]
link to https://github.com/mozilla-b2g/gaia/pull/16477

The patch LGTM but I want Edgar to provide the feedback requested in the PR. Once he provides that feedback I'll r+ the patch. Thanks for the work Arthur.
Attachment #8378903 - Flags: review?(josea.olivera)
Flagging Edgar per comment 5.
Flags: needinfo?(echen)
comment on github.(In reply to José Antonio Olivera Ortega [:jaoo] from comment #5)
> Comment on attachment 8378903 [details]
> link to https://github.com/mozilla-b2g/gaia/pull/16477
> 
> The patch LGTM but I want Edgar to provide the feedback requested in the PR.
> Once he provides that feedback I'll r+ the patch. Thanks for the work Arthur.

It looks good to me, please see comments in github, thank you. :)
Flags: needinfo?(echen)
Comment on attachment 8378903 [details]
link to https://github.com/mozilla-b2g/gaia/pull/16477

Jose, could you check if the function is okay based on Edgar's comment? Thanks.
Attachment #8378903 - Flags: review?(josea.olivera)
Comment on attachment 8378903 [details]
link to https://github.com/mozilla-b2g/gaia/pull/16477

Thanks for the reply Edgar. I left a comment on the PR guys, would you mind to take a look a it please? Thanks!
Attachment #8378903 - Flags: review?(josea.olivera)
It seems we have a timing issue. Edgar, it seems we are not allowed to call "setPreferredNetworkType" immediately after starting up. Is there any way get the proper timing to call to the method?
Flags: needinfo?(echen)
I checked the issue with Jessica and she said that the error was due to the radio state off. If we call to setPreferredNetworkType when the radio is off, gecko returns an error at first, but will do the setting when radio is turned on. In general the patch works as expected.
Flags: needinfo?(josea.olivera)
(In reply to Arthur Chen [:arthurcc] from comment #11)
> I checked the issue with Jessica and she said that the error was due to the
> radio state off. If we call to setPreferredNetworkType when the radio is
> off, gecko returns an error at first, but will do the setting when radio is
> turned on. In general the patch works as expected.
Yes, gecko side will cache the value and set it again when radio is on.
I have another idea. How about calling setPreferredNetworkType() after radioState becomes to "enabled"?
Thank you
Flags: needinfo?(echen)
(In reply to Edgar Chen [:edgar][:echen] from comment #12)
> (In reply to Arthur Chen [:arthurcc] from comment #11)
> > I checked the issue with Jessica and she said that the error was due to the
> > radio state off. If we call to setPreferredNetworkType when the radio is
> > off, gecko returns an error at first, but will do the setting when radio is
> > turned on. In general the patch works as expected.
> Yes, gecko side will cache the value and set it again when radio is on.
> I have another idea. How about calling setPreferredNetworkType() after
> radioState becomes to "enabled"?
> Thank you

To be more clear, for the radio off case, currently gecko will cache the last value comes from setPreferredNetworkType() and set it again when radio is on. But it will still dispatch error event to gaia.
I think we should have more discussion about this api (already file a bug, bug 986395).
To me, I will suggest gaia calls setPreferredNetworkType() after radioState becomes to "enabled" given that it more consist with other handling in gaia.

Thank you.
(In reply to Arthur Chen [:arthurcc] from comment #11)
> I checked the issue with Jessica and she said that the error was due to the
> radio state off. If we call to setPreferredNetworkType when the radio is
> off, gecko returns an error at first, but will do the setting when radio is
> turned on. In general the patch works as expected.

Left my opinion on the PR. IMHO to wait until radio is on is the right thing to do. Thanks guys.
Flags: needinfo?(josea.olivera)
Comment on attachment 8378903 [details]
link to https://github.com/mozilla-b2g/gaia/pull/16477

Jose, I've updated the patch on the basis of Edgar's suggestion and added related tests. Could you help review it? Thanks!
Attachment #8378903 - Flags: review?(josea.olivera)
Comment on attachment 8378903 [details]
link to https://github.com/mozilla-b2g/gaia/pull/16477

LGTM. r=me

Test pass, Travis went green and It seems everything works correctly. I tested the patch on peak and the timing issue is gone. IMHO we should take care of the timing issue for the rest of the mozMobileConnection API usages in this file. Could you please file a follow-up bug for it please? Thanks!

Rebase the work, squash the commits and land once travis goes green.

Thanks for the patch and sorry for the changes about the timing issue.
Attachment #8378903 - Flags: review?(josea.olivera) → review+
Edgar, does this behavior also apply to both setRoamingPreference and setVoicePrivacy?
Flags: needinfo?(echen)
Jose, thanks for reviewing.

master: befb78cce80144ba6c0783aefcf6b98439d0a8e9
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
backout. We should save the default value back to the settings db when there is no available value there.

80af23f8c74d9d2e9388d8ed3c204040b5c528ec
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Jose, I think I missed two cases in which we should save the values back to the settings db when starting up. The last commit fixed the issue and added unit tests. Could you help review it? Thanks.
Attachment #8378903 - Attachment is obsolete: true
Attachment #8396182 - Flags: review?(josea.olivera)
Comment on attachment 8396182 [details]
link to https://github.com/mozilla-b2g/gaia/pull/17562

(In reply to Arthur Chen [:arthurcc] from comment #20)
> Created attachment 8396182 [details]
> link to https://github.com/mozilla-b2g/gaia/pull/17562
> 
> Jose, I think I missed two cases in which we should save the values back to
> the settings db when starting up. The last commit fixed the issue and added
> unit tests. Could you help review it? Thanks.

LGTM, you're right. As we have commented on IRC, this is the right thing to do IMHO. Thanks for this good catch Arthur. r=me
Attachment #8396182 - Flags: review?(josea.olivera) → review+
Thanks Jose!

master: 0c9701c77aefccf5bbf11db3843f3a34e43ceb94
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Attached file Pull request for v1.3t
NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): bug 946589, gecko no longer maintains 'ril.radio.preferredNetworkType' and gaia should handle it. 
[User impact] if declined: The user preferred network type is not kept after reboot.
[Testing completed]: Yes, manual testing. Unit tests added in master. Not able to add unit tests in v1.3t given the code structure.
[Risk to taking this patch] (and alternatives if risky): low
[String changes made]: N/A
Attachment #8396930 - Flags: approval-gaia-v1.3?(fabrice)
Attachment #8396930 - Flags: approval-gaia-v1.3?(fabrice) → approval-gaia-v1.3+
blocking-b2g: 1.3T+ → 1.3+
v1.4: https://github.com/mozilla-b2g/gaia/commit/c719cb2b2a0b1cf8e2382313558eafca1a1769c5

Arthur, can you please post an updated PR against v1.3 instead of v1.3t? :)
Flags: needinfo?(arthur.chen)
Target Milestone: --- → 1.4 S4 (28mar)
Ryan, here you are: https://github.com/mozilla-b2g/gaia/pull/17678 Thanks!
Flags: needinfo?(arthur.chen)
(In reply to Arthur Chen [:arthurcc] from comment #17)
> Edgar, does this behavior also apply to both setRoamingPreference and
> setVoicePrivacy?

Yes, I think so, thank you.
Flags: needinfo?(echen)
Depends on: 988979
You need to log in before you can comment on or make changes to this bug.