Closed Bug 1074037 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

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

RESOLVED FIXED
2.1 S6 (10oct)
blocking-b2g 2.0M+
Tracking Status
b2g-v2.0 --- affected
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: jessica, Assigned: jessica)

References

Details

Attachments

(4 files, 2 obsolete files)

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
Attached patch patch, v1. (obsolete) — Splinter Review
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)
Blocks: Woodduck
blocking-b2g: --- → 2.0M?
blocking-b2g: 2.0M? → 2.0+
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
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+
Attached patch patch (final).Splinter Review
address nits in review comment 2.
Attachment #8498678 - Attachment is obsolete: true
Attachment #8502367 - Flags: review+
Attached patch patch (2.0) (obsolete) — Splinter Review
(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
(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
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.
Thank you, seinlin.
Flags: needinfo?(echen)
https://hg.mozilla.org/mozilla-central/rev/dc476f38e60f
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
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.
Blocks: 1080481
Don't forget to request b2g34 approval on this patch for v2.1 as well.
Flags: needinfo?(jjong)
Target Milestone: --- → 2.1 S6 (10oct)
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+
(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 on attachment 8502367 [details] [diff] [review]
patch (final).

Keeping the r+ flag, will upload a new patch to fix it.
Attachment #8502367 - Flags: review+
Edgar, may I have you review? Thanks.
Attachment #8503962 - Flags: review?(echen)
Attached patch patch (v2.0)Splinter Review
This is the patch for v2.0 with the follow-up fix.
Attachment #8502413 - Attachment is obsolete: true
Attachment #8503967 - Flags: review?(echen)
Attachment #8503962 - Flags: review?(echen) → review+
Attachment #8503967 - Flags: review?(echen) → review+
Attached patch patch (v2.1)Splinter Review
Same as the patch for v2.0, can be applied automatically.
(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
(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
(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 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?
Attachment #8503967 - Flags: approval-mozilla-b2g34?
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.
Flags: needinfo?(kchang)
(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)
ni?josh, he might be able to provide more details for this bug.
Flags: needinfo?(jocheng)
Duplicate of this bug: 1069809
Attachment #8503976 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
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)
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.