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

RESOLVED FIXED in Firefox OS v1.2

Status

Firefox OS
Gaia::Settings
--
major
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: amac, Assigned: jaoo)

Tracking

unspecified
1.2 C3(Oct25)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:koi+, b2g-v1.2 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
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.
(Reporter)

Comment 1

4 years ago
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).
blocking-b2g: --- → koi?
(Assignee)

Comment 2

4 years ago
Created attachment 817119 [details] [diff] [review]
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.
Attachment #817119 - Flags: feedback?
Flags: needinfo?(aus)
(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.
(Assignee)

Comment 4

4 years ago
(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

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

Updated

4 years ago
Flags: needinfo?(aus)

Updated

4 years ago
Attachment #817119 - Flags: feedback? → feedback+
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.
Flags: needinfo?(aus)
(Reporter)

Comment 7

4 years ago
(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

4 years ago
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.
Flags: needinfo?(aus)
Hi, 

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

Thanks!
David
(Reporter)

Comment 10

4 years ago
There has been no advance here in several days, is there any way to move this ahead? This should be a no-brainer blocker.
(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.
Moving to koi+ as it's pretty critical to users.
blocking-b2g: koi? → koi+
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

4 years ago
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
Flags: needinfo?(josea.olivera)
(Assignee)

Updated

4 years ago
Flags: needinfo?(josea.olivera)
Target Milestone: --- → 1.2 C3(Oct25)
Created attachment 820958 [details] [diff] [review]
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.
Attachment #817119 - Attachment is obsolete: true
Attachment #820958 - Flags: feedback?(aus)
Flags: needinfo?(aus)
Created attachment 821071 [details] [diff] [review]
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.
Attachment #820958 - Attachment is obsolete: true
Attachment #820958 - Flags: feedback?(aus)
Attachment #821071 - Flags: feedback?(aus)
(Assignee)

Updated

4 years ago
Flags: needinfo?(aus)
(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 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!
Attachment #821071 - Flags: review?(timdream)
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.
Attachment #821071 - Flags: review?(timdream) → review+
https://github.com/mozilla-b2g/gaia/commit/15c1089aea0490b9b73bc915b1495b3faa1b023c
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-b2g-v1.2: --- → affected
Resolution: --- → FIXED
Uplifted 15c1089aea0490b9b73bc915b1495b3faa1b023c to:
v1.2: 672963cda418369a54d39e12ad34c6ac3bb71e33
status-b2g-v1.2: affected → fixed

Comment 22

4 years ago
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. :(
Attachment #821071 - Flags: feedback?(aus) → feedback-
You need to log in before you can comment on or make changes to this bug.