B2G RIL: refine DataCall's canHandleApn() decision logic

RESOLVED FIXED in Firefox OS v2.0M

Status

Firefox OS
RIL
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jessica, Assigned: jessica)

Tracking

unspecified
2.1 S6 (10oct)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.0M+, b2g-v2.0 affected, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Assignee)

Description

4 years ago
When deciding whether DataCall can handle another apn or not, we should consider all the properties used to set up a pdp connection, and not just apn, username and password. See [1]

[1] http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#4174
(Assignee)

Comment 1

4 years ago
Created attachment 8498678 [details] [diff] [review]
patch, v1.

When looking for a shareable DataCall, we now consider apn, username, password and authtype, and if IPv6 is supported, we consider protocol and roaming protocol as well.
As a consequence, we can have separate DataCalls even with the same apn. So, when receiving a data call state change, we should consider all of the above to find the appropriate DataCall to notify.

Edgar, may I have your review? Thanks.
Attachment #8498678 - Flags: review?(echen)

Updated

4 years ago
Blocks: 1054172

Updated

4 years ago
blocking-b2g: --- → 2.0M?

Updated

4 years ago
blocking-b2g: 2.0M? → 2.0+
status-b2g-v2.0: --- → affected
status-b2g-v2.0M: --- → affected
status-b2g-v2.1: --- → affected
status-b2g-v2.2: --- → affected
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM

Comment 2

4 years ago
Comment on attachment 8498678 [details] [diff] [review]
patch, v1.

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

Thank you.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +4143,5 @@
> +
> +    if (RILQUIRKS_HAVE_IPV6) {
> +      return (isIdentical &&
> +              (this.apnProfile.protocol || '') == (apnSetting.protocol || '') &&
> +              (this.apnProfile.roaming_protocol || '') == (apnSetting.roaming_protocol || ''));

nit: isIdentical = isIdentical && ...;

@@ +4213,5 @@
>      }
>  
>      let radioTechType = dataInfo.type;
>      let radioTechnology = RIL.GECKO_RADIO_TECH.indexOf(radioTechType);
> +    let authType = RIL.RIL_DATACALL_AUTH_TO_GECKO.indexOf(this.apnProfile.authType);

Nice catch

::: dom/system/gonk/ril_worker.js
@@ +4124,5 @@
>          if (newDataCallOptions) {
> +          this._sendDataCallError(newDataCallOptions, newDataCall.status);
> +        } else {
> +          this._sendDataCallError(newDataCall, newDataCall.status);
> +        }

nit: this._sendDataCallError(newDataCallOptions || newDataCall, newDataCall.status);
Attachment #8498678 - Flags: review?(echen) → review+
(Assignee)

Comment 3

4 years ago
Created attachment 8502367 [details] [diff] [review]
patch (final).

address nits in review comment 2.
Attachment #8498678 - Attachment is obsolete: true
Attachment #8502367 - Flags: review+
(Assignee)

Comment 5

4 years ago
(In reply to Jessica Jong [:jjong] [:jessica] from comment #3)
> Created attachment 8502367 [details] [diff] [review]
> patch (final).
> 
> address nits in review comment 2.

try: https://tbpl.mozilla.org/?tree=Try&rev=c0288ce337c1
(Assignee)

Comment 6

4 years ago
(In reply to Jessica Jong [:jjong] [:jessica] from comment #5)
> (In reply to Jessica Jong [:jjong] [:jessica] from comment #3)
> > Created attachment 8502367 [details] [diff] [review]
> > patch (final).
> > 
> > address nits in review comment 2.
> 
> try: https://tbpl.mozilla.org/?tree=Try&rev=c0288ce337c1

try green!

checkin-needed for master (attachment 8502367 [details] [diff] [review])
Keywords: checkin-needed

Comment 8

4 years ago
Dear Edgar,
Partner is asking whether we can land this bug before 12nd morning. If we cannot have approval for 2.0 soon, is it possible to land on 2.0M first? Currently there is no approval request needed for landing on 2.0M.
Thank you very much!
Flags: needinfo?(echen)
I'll handle for 2.0M. If 2.0 branch also need this patch, please nominate for approval‑mozilla‑b2g32.

Comment 11

4 years ago
Thank you, seinlin.
Flags: needinfo?(echen)
https://hg.mozilla.org/mozilla-central/rev/dc476f38e60f
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 13

4 years ago
Thanks seinlin and Edgar. The patch for v2.0 passes try:
https://tbpl.mozilla.org/?tree=Try&rev=a826373fea02

but please note that I haven't had chance to test it on real device, I'd like to ask approval on v2.0 once I've tested on real device. Thanks.

Updated

4 years ago
Blocks: 1080481
Don't forget to request b2g34 approval on this patch for v2.1 as well.
status-b2g-v2.2: affected → fixed
Flags: needinfo?(jjong)
Target Milestone: --- → 2.1 S6 (10oct)
(Assignee)

Comment 15

4 years ago
Comment on attachment 8502367 [details] [diff] [review]
patch (final).

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

There is a flaw in this patch, I will upload another patch to fix this. Weird that it I didn't catch it while testing.. :(

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1164,5 @@
>  
> +  _compareDataCallOptions: function(dataCall, newDataCall) {
> +    return dataCall.apnProfile.apn == newDataCall.apn &&
> +           dataCall.apnProfile.user == newDataCall.user &&
> +           dataCall.apnProfile.password == newDataCall.password &&

It should be newDataCall.passwd, sorry :(
Attachment #8502367 - Flags: review+
(Assignee)

Comment 16

4 years ago
(In reply to Jessica Jong [:jjong] [:jessica] from comment #15)
> Comment on attachment 8502367 [details] [diff] [review]
> patch (final).
> 
> Review of attachment 8502367 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There is a flaw in this patch, I will upload another patch to fix this.
> Weird that it I didn't catch it while testing.. :(
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +1164,5 @@
> >  
> > +  _compareDataCallOptions: function(dataCall, newDataCall) {
> > +    return dataCall.apnProfile.apn == newDataCall.apn &&
> > +           dataCall.apnProfile.user == newDataCall.user &&
> > +           dataCall.apnProfile.password == newDataCall.password &&
> 
> It should be newDataCall.passwd, sorry :(

master can work now using CHT sim, cause apn user and password are 'undefined' by default and 'newDataCall.password' results in undefined as well, so.. it can work by coincidence.
(Assignee)

Comment 17

4 years ago
Comment on attachment 8502367 [details] [diff] [review]
patch (final).

Keeping the r+ flag, will upload a new patch to fix it.
Attachment #8502367 - Flags: review+
(Assignee)

Comment 18

4 years ago
Created attachment 8503962 [details] [diff] [review]
(follow-up): fix undefined variable name.

Edgar, may I have you review? Thanks.
Attachment #8503962 - Flags: review?(echen)
(Assignee)

Comment 19

4 years ago
Created attachment 8503967 [details] [diff] [review]
patch (v2.0)

This is the patch for v2.0 with the follow-up fix.
Attachment #8502413 - Attachment is obsolete: true
Attachment #8503967 - Flags: review?(echen)

Updated

4 years ago
Attachment #8503962 - Flags: review?(echen) → review+

Updated

4 years ago
Attachment #8503967 - Flags: review?(echen) → review+
(Assignee)

Comment 20

4 years ago
Created attachment 8503976 [details] [diff] [review]
patch (v2.1)

Same as the patch for v2.0, can be applied automatically.
(Assignee)

Comment 21

4 years ago
(In reply to Jessica Jong [:jjong] [:jessica] from comment #18)
> Created attachment 8503962 [details] [diff] [review]
> (follow-up): fix undefined variable name.
> 
> Edgar, may I have you review? Thanks.


Thanks Edgar for the review.

try: https://tbpl.mozilla.org/?tree=Try&rev=2f00d7613f5b
(Assignee)

Comment 22

4 years ago
(In reply to Jessica Jong [:jjong] [:jessica] from comment #21)
> (In reply to Jessica Jong [:jjong] [:jessica] from comment #18)
> > Created attachment 8503962 [details] [diff] [review]
> > (follow-up): fix undefined variable name.
> > 
> > Edgar, may I have you review? Thanks.
> 
> 
> Thanks Edgar for the review.
> 
> try: https://tbpl.mozilla.org/?tree=Try&rev=2f00d7613f5b

marionette-webapi failures are known issues and this follow-up patch is pretty safe, so pushed:

https://hg.mozilla.org/integration/b2g-inbound/rev/7896f972454d
(Assignee)

Comment 25

4 years ago
(In reply to Kai-Zhen Li [:seinlin] from comment #24)
> http://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/7a3581beaa07

Thank you seinlin and sorry for this!
(Assignee)

Comment 26

4 years ago
Comment on attachment 8503967 [details] [diff] [review]
patch (v2.0)

try: https://tbpl.mozilla.org/?tree=Try&rev=0f5e68e7d495

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: some apn settings may not take effect, e.g. if user selects IPv6, it may still use IPv4 if there is another apn setting with the same apn, username and password and has IPv4 as protocol. (bug 1069809)
Testing completed: try and tested manually on real device
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none
Attachment #8503967 - Flags: approval-mozilla-b2g34?
Attachment #8503967 - Flags: approval-mozilla-b2g32?
(Assignee)

Updated

4 years ago
Attachment #8503967 - Flags: approval-mozilla-b2g34?
(Assignee)

Comment 27

4 years ago
Comment on attachment 8503976 [details] [diff] [review]
patch (v2.1)

try: https://tbpl.mozilla.org/?tree=Try&rev=da9e2cfbf9ea

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: some apn settings may not take effect, e.g. if user selects IPv6, it may still use IPv4 if there is another apn setting with the same apn, username and password and has IPv4 as protocol. (bug 1069809)
Testing completed: try and tested manually on real device
Risk to taking this patch (and alternatives if risky): low, can be backed-out if it breaks anything
String or UUID changes made by this patch: none
Attachment #8503976 - Flags: approval-mozilla-b2g34?
Flags: needinfo?(jjong)
Ken,

we are well aware of not landing RIL changes beyond FC for 2.0/2.1 branch, so can you help here? Can this be shared with the RIL team widely and help set expectation here?

If this is a change that was discussed with CAF and is good t land happy to a+, let me know.

Updated

4 years ago
Flags: needinfo?(kchang)

Comment 29

4 years ago
(In reply to bhavana bajaj [:bajaj] from comment #28)
> we are well aware of not landing RIL changes beyond FC for 2.0/2.1 branch,
Devs land patches according to the flag of bugs, we don't need to create any exceptional rule for any 
component. For this bug, the question is why device EPM nominates this bug as a blocker for 2.0, 2.1.
If you don't agree with EPM for these flags, you need to discuss with device EPM.
As I knew, this fix needs to be landed because other partners need it. 

By the way, although this bug is fixed in RIL, this fix doesn't affect partner's RIL.
Moreover, for 2.1 branch, I think we can land RIL codes after syncing with partners for 2.1 in
current stage. For 2.0, I wonder if this is a call of devices team. For CAF, We should create a stable
branch for them to avoid that CAF don't want the patches for some common bugs but other partners do.
Flags: needinfo?(kchang)

Comment 30

4 years ago
ni?josh, he might be able to provide more details for this bug.
Flags: needinfo?(jocheng)

Updated

4 years ago
Duplicate of this bug: 1069809
Attachment #8503976 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+

Comment 32

4 years ago
Hi Wesley,
This is generic bug.
I nominate the bug to 2.0M+ and please decide whether to land on 2.0.
Thanks!
blocking-b2g: 2.0+ → 2.0M+
Flags: needinfo?(jocheng) → needinfo?(whuang)
For now it's good enough to fix in 2.1 and master. 
2.0 already CC and CAF commercial RIL doesn't have such problem.
Let's keep this bug tracked on Woodduck_2.0_Generic in case we need it in the future.
Flags: needinfo?(whuang)
(Assignee)

Comment 35

4 years ago
Comment on attachment 8503967 [details] [diff] [review]
patch (v2.0)

Removing approval flag per comment 34.
Attachment #8503967 - Flags: approval-mozilla-b2g32?
You need to log in before you can comment on or make changes to this bug.