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

Status: RESOLVED FIXED

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

Description

[Arthur Chen [:arthurcc][inactive after 6/30]]

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%

Comment 1

[Arthur Chen [:arthurcc][inactive after 6/30]]

Attachment: link to https://github.com/mozilla-b2g/gaia/pull/16477

WIP, waiting for bug 946589 landing.

Comment 2

[Arthur Chen [:arthurcc][inactive after 6/30]]

Nominate for 1.3T as it blocks a 1.3T blocker.

Comment 3

[Joe Cheng [:jcheng] (please needinfo)]

triage: 1.3T+

Comment 4

[Arthur Chen [:arthurcc][inactive after 6/30]]

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!

Comment 5

[José Antonio Olivera Ortega [:jaoo]]

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.

Comment 6

[Arthur Chen [:arthurcc][inactive after 6/30]]

Flagging Edgar per comment 5.

Comment 7

[Edgar Chen [:edgar]]

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. :)

Comment 8

[Arthur Chen [:arthurcc][inactive after 6/30]]

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.

Comment 9

[José Antonio Olivera Ortega [:jaoo]]

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!

Comment 10

[Arthur Chen [:arthurcc][inactive after 6/30]]

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?

Comment 11

[Arthur Chen [:arthurcc][inactive after 6/30]]

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.

Comment 12

[Edgar Chen [:edgar]]

(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

Comment 13

[Edgar Chen [:edgar]]

(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.

Comment 14

[José Antonio Olivera Ortega [:jaoo]]

(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.

Comment 15

[Arthur Chen [:arthurcc][inactive after 6/30]]

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!

Comment 16

[José Antonio Olivera Ortega [:jaoo]]

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.

Comment 17

[Arthur Chen [:arthurcc][inactive after 6/30]]

Edgar, does this behavior also apply to both setRoamingPreference and setVoicePrivacy?

Comment 18

[Arthur Chen [:arthurcc][inactive after 6/30]]

Jose, thanks for reviewing.

master: befb78cce80144ba6c0783aefcf6b98439d0a8e9

Comment 19

[Arthur Chen [:arthurcc][inactive after 6/30]]

backout. We should save the default value back to the settings db when there is no available value there.

80af23f8c74d9d2e9388d8ed3c204040b5c528ec

Comment 20

[Arthur Chen [:arthurcc][inactive after 6/30]]

Attachment: 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.

Comment 21

[José Antonio Olivera Ortega [:jaoo]]

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

Comment 22

[Arthur Chen [:arthurcc][inactive after 6/30]]

Thanks Jose!

master: 0c9701c77aefccf5bbf11db3843f3a34e43ceb94

Comment 23

[Arthur Chen [:arthurcc][inactive after 6/30]]

Attachment: 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

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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? :)

Comment 25

[Arthur Chen [:arthurcc][inactive after 6/30]]

Ryan, here you are: https://github.com/mozilla-b2g/gaia/pull/17678 Thanks!

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Thanks!

v1.3: https://github.com/mozilla-b2g/gaia/commit/6bbf1d8b5c1b7203d3b59e8a8613371fcf40fc0d

Comment 27

[Edgar Chen [:edgar]]

(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.