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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Jessica Jong [:jessica] (inactive)]

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

Comment 1

[Jessica Jong [:jessica] (inactive)]

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

Comment 2

[Edgar Chen [:edgar]]

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);

Comment 3

[Jessica Jong [:jessica] (inactive)]

Attachment: patch (final).

address nits in review comment 2.

Comment 4

[Jessica Jong [:jessica] (inactive)]

Attachment: patch (2.0)

Comment 5

[Jessica Jong [:jessica] (inactive)]

(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

Comment 6

[Jessica Jong [:jessica] (inactive)]

(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])

Comment 7

[Edgar Chen [:edgar]]

https://hg.mozilla.org/integration/b2g-inbound/rev/dc476f38e60f

Comment 8

[Josh Cheng [:josh]]

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!

Comment 9

[Kai-Zhen Li [:seinlin][:kli]]

I'll handle for 2.0M. If 2.0 branch also need this patch, please nominate for approval‑mozilla‑b2g32.

Comment 10

[Kai-Zhen Li [:seinlin][:kli]]

http://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/8427608704b0

Comment 11

[Edgar Chen [:edgar]]

Thank you, seinlin.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dc476f38e60f

Comment 13

[Jessica Jong [:jessica] (inactive)]

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.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Don't forget to request b2g34 approval on this patch for v2.1 as well.

Comment 15

[Jessica Jong [:jessica] (inactive)]

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

Comment 16

[Jessica Jong [:jessica] (inactive)]

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

Comment 17

[Jessica Jong [:jessica] (inactive)]

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

Keeping the r+ flag, will upload a new patch to fix it.

Comment 18

[Jessica Jong [:jessica] (inactive)]

Attachment: (follow-up): fix undefined variable name.

Edgar, may I have you review? Thanks.

Comment 19

[Jessica Jong [:jessica] (inactive)]

Attachment: patch (v2.0)

This is the patch for v2.0 with the follow-up fix.

Comment 20

[Jessica Jong [:jessica] (inactive)]

Attachment: patch (v2.1)

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

Comment 21

[Jessica Jong [:jessica] (inactive)]

(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

Comment 22

[Jessica Jong [:jessica] (inactive)]

(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

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7896f972454d

Comment 24

[Kai-Zhen Li [:seinlin][:kli]]

http://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/7a3581beaa07

Comment 25

[Jessica Jong [:jessica] (inactive)]

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

Comment 26

[Jessica Jong [:jessica] (inactive)]

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

Comment 27

[Jessica Jong [:jessica] (inactive)]

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

Comment 28

[bhavana bajaj [:bajaj]]

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.

Comment 29

[Ken Chang[:kenkai]]

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

Comment 30

[Ken Chang[:kenkai]]

ni?josh, he might be able to provide more details for this bug.

Comment 32

[Josh Cheng [:josh]]

Hi Wesley,
This is generic bug.
I nominate the bug to 2.0M+ and please decide whether to land on 2.0.
Thanks!

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/37025b11a7fb

Comment 34

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

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.

Comment 35

[Jessica Jong [:jessica] (inactive)]

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

Removing approval flag per comment 34.