926455 - Cannot enable data connection when updating from 1.1 to 1.2 via FOTA

Status: RESOLVED FIXED

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

Description

[Antonio Manuel Amaya Calvo (:amac)]

STR: 
1. Flash a phone with a 1.1 build
2. Create a FOTA for the phone with 1.2
3. Apply the FOTA
4. Try to enable the 3G data connection

Expected:

The data connection works

Actual: 

The data connection does not work. Logcat shows:

I/Gecko   (  441): -*- RadioInterface[0]: 'ril.data.enabled' is now false
I/Gecko   (  441): -*- RadioInterface[0]: We haven't gotten completely the APN data.
I/Gecko   (  441): -*- RadioInterface[0]: 'ril.data.roaming_enabled' is now false
I/Gecko   (  441): -*- RadioInterface[0]: We haven't gotten completely the APN data.
I/Gecko   (  441): -*- RadioInterface[0]: 'ril.data.enabled' is now true
I/Gecko   (  441): -*- RadioInterface[0]: We haven't gotten completely the APN data.
E/GeckoConsole(  394): [JavaScript Error: "apnsForIccCards is undefined" {file: "app://settings.gaiamobile.org/js/carrier.js" line: 232}]

The cause seems to be that ril.data.apnSettings does not exist on 1.1 and as such it's not on the settings database. When upgrading via FOTA it isn't created either.

Comment 1

[Antonio Manuel Amaya Calvo (:amac)]

Requesting koi+ since unless this is fixed we won't be able to update devices using FOTA (which practically means we won't be able to update them at all).

Comment 2

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v1

Aus, we need to touch/refactor the operator variant helper a bit. Right now it doesn't apply the setting when the operator numeric value (MCC and MNC codes) in the ICC card is the same we have cached. This might be wrong if some settings have been added for the new version and those settings must be set on boot. That is the case of the 'ril.data.apnSettings' setting. Even having the same SIM card we must check whether that setting doesn't exist and build a value for it then.

On the other hand while having a look at this issue I've noticed that if the ICC card is the same than in previous boots we never stop listening for MCC and MNC changes what could be a problem in case the ICC card is enabled with the roaming broker thing (see bug 860411 please).

If you want to take this bus, let me know. Current WIP fixes the problem but IMHO some refactor would be needed in the helper.

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #1)
> Requesting koi+ since unless this is fixed we won't be able to update
> devices using FOTA (which practically means we won't be able to update them
> at all).

Is it recoverable by the user? I mean, if the user could toggle the APN in the settings manually after upgrade and have the data connection back, we don't really need to block the release.

Comment 4

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #3)

> Is it recoverable by the user? I mean, if the user could toggle the APN in
> the settings manually after upgrade and have the data connection back, we
> don't really need to block the release.

No, it isn't. The 'ril.data.apnSettings' setting cannot be built and the device cannot set up the data call. We really need to block the release.

Comment 5

[Aus Lacroix [:aus]]

:jaoo, this is a safe change and it resolves the issue. Customizations that must only run once already have a mechanism to do so.

Comment 6

[Preeti Raghunath(:Preeti)]

Hi Ghislain,

Please provide risk analysis for the patch in question. It is clear that the user experience is bad and we should block on the same, but do provide what the risk is.

Comment 7

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Preeti Raghunath(:Preeti) from comment #6)
> Hi Ghislain,
> 
> Please provide risk analysis for the patch in question. It is clear that the
> user experience is bad and we should block on the same, but do provide what
> the risk is.

With all due respect, in this case, it doesn't really matter how risky this patch is: I don't think any operator is going to certify an update that leaves their customers without network. In other words, I don't think 1.2 is going to be offered as an update for any operator without fixing this.

Comment 8

[Aus Lacroix [:aus]]

The risk that exists is that we may end up updating the settings too often, during roaming. I'm not sure what the exact requirements are there, but there is a need for the original settings to remain when roaming. Based on the code, this should *not* happen, however, it's untested. 

But, I agree with :amac that this is a *must* fix.

Comment 9

[David Palomino [:dpalomino]]

Hi, 

Completely agree, this is an absolute certification blocker, any operator will not accept this during the certification. 

Thanks!
David

Comment 10

[Antonio Manuel Amaya Calvo (:amac)]

There has been no advance here in several days, is there any way to move this ahead? This should be a no-brainer blocker.

Comment 11

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #10)
> There has been no advance here in several days, is there any way to move
> this ahead? This should be a no-brainer blocker.

From the development point of view there is no problem with going ahead and landing the patch we already have. I'll go for it then. If this bug is koi+'ed in the next days we will uplift it. That's all.

Comment 12

[Bruce Huang [:bhuang] (inactive)]

Moving to koi+ as it's pretty critical to users.

Comment 13

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 817119 [details] [diff] [review]
v1

Thanks for identifying the issue. 

However I think it is safer to check if the customization has been applied or not instead of calling the listener passed every time, which essentially breaks the desired behavior of OperatorVariantHelper described in the file itself.

Comment 14

[Ivan Tsay (:ITsay)]

Hi Jose,

Would you please assign one of the target milestones (1.2 C3 ~ 1.2 C6) for this bug depending on time you need for resolving this one? We need the flag to check when this bug will be resolved. Thank you!

Ivan

Comment 15

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v2

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #13)
> Comment on attachment 817119 [details] [diff] [review]
> v1
> 
> Thanks for identifying the issue. 
> 
> However I think it is safer to check if the customization has been applied
> or not instead of calling the listener passed every time, which essentially
> breaks the desired behavior of OperatorVariantHelper described in the file
> itself.

I've ended with a different up after Tim's comment. He is right since the v1 patch breaks the behavior of OperatorVariantHelper object. I've been working on the refactor I commented about in comment #2 as it's the right thing IMHO. v2 patch doesn't bring big changes it just checks whether the customization has been applied.

:aus, would you mind to have a look please? Thanks!

Note: I'm dealing with the unit tests right now.

Comment 16

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v3

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #13)
> Comment on attachment 817119 [details] [diff] [review]
> v1
> 
> Thanks for identifying the issue. 

Thanks for your feedback.

> However I think it is safer to check if the customization has been applied
> or not instead of calling the listener passed every time, which essentially
> breaks the desired behavior of OperatorVariantHelper described in the file
> itself.

I've ended up with a different patch after Tim's comment. He is right since the v1 patch breaks the behavior of OperatorVariantHelper object. I've been working on the refactor I commented about in comment #2 as it's the right thing IMHO. v3 patch doesn't bring big changes it just checks whether the customization has been applied. Moreover it gets rid of some tricks we had for passing the test (the one related with not stop listening for ICC changes during tests in `applySettings()` function in operator_variant.js file). This version of the patch does things right and fix the issue we had with bug 860411 (see second paragraph of comment #2).

:aus, would you mind to have a look please? Thanks!

I'll request to our releng team to create a new FOTA and I'll do one more test.

Comment 17

[José Antonio Olivera Ortega [:jaoo]]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #16)
> Created attachment 821071 [details] [diff] [review]
> v3

> I'll request to our releng team to create a new FOTA and I'll do one more
> test.

Done, works correctly. Waiting for feedback from :aus.

Comment 18

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 821071 [details] [diff] [review]
v3

Tim, we should fix this issue by tomorrow as the milestone ends tomorrow. I used to request feedback at Fabien (kaze) but maybe it's better to request review at you since you are aware of the issue and you are owner. Feel free to pass this request to kaze if you think it's better. Thanks!

Comment 19

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 821071 [details] [diff] [review]
v3

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

I only read operator_variant.js w/o reading the tests and it looks alright.

Comment 20

[José Antonio Olivera Ortega [:jaoo]]

https://github.com/mozilla-b2g/gaia/commit/15c1089aea0490b9b73bc915b1495b3faa1b023c

Comment 21

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted 15c1089aea0490b9b73bc915b1495b3faa1b023c to:
v1.2: 672963cda418369a54d39e12ad34c6ac3bb71e33

Comment 22

[Aus Lacroix [:aus]]

Comment on attachment 821071 [details] [diff] [review]
v3

I can't say that I'm comfortable with the changes in this proposed patch.

The intent of the helper object is to provide the user of this object to detect a SIM card change and have it reported to him. He can then optionally run a customization step and either save the fact that it was run, or not. This is what all users of this object currently assume. 

We're also switching from assuming that the numeric and string values are interchangeable to them being strings only which alot of our code does _NOT_ assume.

Is there a way to isolate this fix in operator_variant.js specifically?

Apologies for the slowness on this feedback Jose. :(