Closed Bug 957917 Opened 6 years ago Closed 6 years ago

[IPv6] To support IPv6 in RIL.

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
1.4 S3 (14mar)

People

(Reporter: wesley_huang, Assigned: vicamo)

References

Details

(Whiteboard: [ucid: , 1.4, FT:RIL][ipv6])

User Story

As a user, I should have my FireFoxOS device supporting IPv6.

Attachments

(2 files, 15 obsolete files)

31.70 KB, patch
Details | Diff | Splinter Review
4.73 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
No description provided.
User Story: (updated)
vyang, can you please take this bug? Thanks.
Flags: needinfo?(vyang)
Assignee: nobody → vyang
Flags: needinfo?(vyang)
There is still dozens of networking errors in KitKat emulator.  I should have them fixed first for a basic networking environment.  For KitKat emulator builds, see bug 957526.
blocking-b2g: --- → 1.4+
Whiteboard: [ucid: , 1.4, FT:RIL]
This bug is supported to change the interface between ril and network manager.
Target Milestone: --- → 1.4 S3 (14mar)
Flags: in-testsuite?
Flags: in-moztrap?(echang)
user story or feature work are not blocker.
blocking-b2g: 1.4+ → backlog
Blocks: 960372
blocking-b2g: backlog → 1.3T+
I assume Fabrice intended to set 1.4+
blocking-b2g: 1.3T+ → 1.4+
Summary: [IPv6] Add IPv6 support for FireFox OS → [IPv6] Add IPv6 support for Firefox OS
Target milestone set to 2/28. Remove 1.4+ as this is user story.
blocking-b2g: 1.4+ → ---
Target Milestone: 1.4 S3 (14mar) → 1.4 S2 (28feb)
Depending on bug 871475 for tests.  However it was backed-out because of causing perm-orange of one reftest and one crashtest.  If that can't meet the schedule, I'll remove it from the dependency list and file a follow-up for test cases.
Depends on: 871475
Attached file Github pull request for hardware/ril (obsolete) —
parse <PDP_type>
Make this bug clear. This bug is to get IPv6 relevant information from network. And Bug 975813 is to configure IPv6 through network manager.
Depends on: 975813
Summary: [IPv6] Add IPv6 support for Firefox OS → [IPv6] To support IPv6 in RIL.
No longer blocks: 960372
Comment on attachment 8379464 [details]
Github pull request for hardware/ril

No longer needed because <PDP_type> parsing is merged into bug 871475.
Attachment #8379464 - Attachment is obsolete: true
Per comment 11 marking this resolved fixed.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
I think we still need this bug, but it isn't in the blocker list provided by partner.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
No longer blocks: b2g--telephony-1.4
Blocks: 976427
No longer blocks: 976427
http://mzl.la/1c73sCh IPv6+LTE
Flags: in-moztrap?(echang) → in-moztrap+
QA Contact: echang
Depends on: 976959
This PR depends on bug 871475, which was backed out and is not fixed yet.  The PR here is only for test purpose.
This PR depends on bug 871475, which was backed out and is not fixed yet.  The PR here is only for test purpose.
So far we have two things to do: 1) handle undefined dns2, 2) handle undefined httpProxyHost.
Support roamingProtocol as well.
Attachment #8379454 - Attachment is obsolete: true
Attachment #8382924 - Attachment is obsolete: true
Attachment #8383095 - Flags: review?(htsai)
Attachment #8382895 - Attachment is obsolete: true
Attachment #8382899 - Attachment is obsolete: true
Attached file adb-logcat_emulator-jb.txt.gz (obsolete) —
The same patch set doesn't work on emulator-jb. :X

D/NetUtils(   46): ifc_add_route(eth1, 2001:200::a00:202, 128, 2001:200::a00:202) = 0
E/NetUtils(   46): getaddrinfo failed: invalid gateway eth1
D/NetUtils(   46): ifc_remove_route(eth1, ::, 0, eth1) = -22
D/NetUtils(   46): ifc_init_returning 0
D/NetUtils(   46): ifc_close
D/NetUtils(   46): ifc_add_route(eth1, 2001:200::a00:202, 128, 2001:200::a00:202) = -113
E/NetUtils(   46): getaddrinfo failed: invalid gateway eth1
D/NetUtils(   46): ifc_remove_route(eth1, ::, 0, eth1) = -22
E/NetUtils(   46): getaddrinfo failed: invalid gateway eth1
D/NetUtils(   46): ifc_add_route(eth1, ::, 0, eth1) = -22
D/NetUtils(   36): ifc_init_returning 0
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #23)
> The same patch set doesn't work on emulator-jb. :X

Looks like emulator rild has done his job well.  Somehow that |aOptions.gateway| becomes "eth1".

$ adb shell ip addr show
3: eth1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP qlen 1000
    link/ether 52:54:00:12:34:57 brd ff:ff:ff:ff:ff:ff
    inet6 2001:200::a00:264/64 scope global
       valid_lft forever preferred_lft forever
    inet6 fe80::5054:ff:fe12:3457/64 scope link
       valid_lft forever preferred_lft forever

$ adb shell ip link show
3: eth1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP qlen 1000
    link/ether 52:54:00:12:34:57 brd ff:ff:ff:ff:ff:ff

$ adb shell ip route show
10.0.2.0/24 dev eth0  proto kernel  scope link  src 10.0.2.15
default via 10.0.2.2 dev eth0  metric 2
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #24)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #23)
> > The same patch set doesn't work on emulator-jb. :X
> 
> Looks like emulator rild has done his job well.  Somehow that
> |aOptions.gateway| becomes "eth1".

Root cause found and fixed in bug 975813 attachment 8383200 [details] [diff] [review].  Basically GET_CHAR() marco returns a C string pointer owned by a temporarily, in-stack object.
Comment on attachment 8383095 [details] [diff] [review]
[Gecko] read PDP_type from APN settings

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

::: dom/system/gonk/ril_consts.js
@@ +2368,5 @@
> +this.RIL_DATACALL_PDP_TYPES = [
> +  GECKO_DATACALL_PDP_TYPE_IP,
> +  GECKO_DATACALL_PDP_TYPE_IPV6,
> +];
> +

Vicamo, the valid values for the roaming and roaming_protocol properties in the APNs are 'none' (this one would means there is no value), 'IP', 'IPV6', and 'IPV4V6' as they are the possible valid values that these properties have in the AOSP database and in our apn.json database.
Also works in emulator-kk.
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #26)
> Vicamo, the valid values for the roaming and roaming_protocol properties in
> the APNs are 'none' (this one would means there is no value), 'IP', 'IPV6',
> and 'IPV4V6' as they are the possible valid values that these properties
> have in the AOSP database and in our apn.json database.

Hi, we're not going to have IPV4V6 in v1.4, and we fallback to "IP" given "none" is passed.
Attachment #8381357 - Attachment is obsolete: true
Blocks: 978071
Comment on attachment 8382886 [details] [review]
Github PR for platform/external/qemu [DO NOT SUBMIT]

Move to bug 978071.
Attachment #8382886 - Attachment is obsolete: true
Comment on attachment 8382889 [details] [review]
Github pull request for platform/hardware/ril [DO NOT SUBMIT]

Move to bug 978071.
Attachment #8382889 - Attachment is obsolete: true
Attachment #8383136 - Attachment is obsolete: true
1) use 'roaming_protocol' as Gaia does.
2) fix missing a '!' in protocol/roaming_protocol selection
Attachment #8383095 - Attachment is obsolete: true
Attachment #8383095 - Flags: review?(htsai)
Attachment #8383701 - Flags: review?(htsai)
Attached patch part 2/2: test cases (obsolete) — Splinter Review
This patch is currently disabled because of bug 978071.  It passes with pull requests in that bug pulled into external/qemu and hardware/ril.
Attachment #8383550 - Attachment is obsolete: true
Attachment #8383702 - Flags: review?(htsai)
Comment on attachment 8383701 [details] [diff] [review]
part 1/2: read PDP_type from APN settings

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +4513,5 @@
>        }
>        authType = RIL.RIL_DATACALL_AUTH_TO_GECKO.indexOf(RIL.GECKO_DATACALL_AUTH_DEFAULT);
>      }
> +    let pdptype = !radioInterface.rilContext.data.roaming
> +                ? this.apnSetting.protocol : this.apnSetting.roaming_protocol;

nit: the style usually used is making ? and : align

::: dom/system/gonk/ril_consts.js
@@ +2365,5 @@
> +this.GECKO_DATACALL_PDP_TYPE_IPV6 = "IPV6";
> +this.GECKO_DATACALL_PDP_TYPE_DEFAULT = GECKO_DATACALL_PDP_TYPE_IP;
> +this.RIL_DATACALL_PDP_TYPES = [
> +  GECKO_DATACALL_PDP_TYPE_IP,
> +  GECKO_DATACALL_PDP_TYPE_IPV6,

Looks like type 'IPV4V6' is missing?

::: dom/system/gonk/ril_worker.js
@@ +3962,5 @@
>        }
>        currentDataCall.gw = updatedDataCall.gw;
>        if (updatedDataCall.dns) {
> +        for (let i = 0; i < updatedDataCall.dns.length; i++) {
> +          currentDataCall.dns.push(updatedDataCall.dns[i]);

This line makes updated dns attended to existing currentDataCall.dns. Is this expected behaviour?
Attachment #8383701 - Flags: review?(htsai)
(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #35)
> Looks like type 'IPV4V6' is missing?

We don't have IPV4V6 now.  Can't find the correct format through AOSP source code.  Need of real hardware references.  Will file a follow-up and place a comment here instead.

> ::: dom/system/gonk/ril_worker.js
> @@ +3962,5 @@
> >        }
> >        currentDataCall.gw = updatedDataCall.gw;
> >        if (updatedDataCall.dns) {
> > +        for (let i = 0; i < updatedDataCall.dns.length; i++) {
> > +          currentDataCall.dns.push(updatedDataCall.dns[i]);
> 
> This line makes updated dns attended to existing currentDataCall.dns. Is
> this expected behaviour?

Oops! A reset to |currentDataCall.dns| is missed. :X
Comment on attachment 8383702 [details] [diff] [review]
part 2/2: test cases

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

Please file a followup to replace mobile_head.js with head.js.

::: dom/mobileconnection/tests/marionette/head.js
@@ +10,5 @@
> +let Promise = Cu.import("resource://gre/modules/Promise.jsm").Promise;
> +
> +let _pendingEmulatorCmdCount = 0;
> +
> +/* Send emulator command with safe guard.

nit: the format we are using is

/*
 * Comment starts...
 * ...
 * comment ends.
 */

Please make the style consistent all over the file.

@@ +50,5 @@
> + *
> + * Resolve if that mozSettings value is retrieved successfully, reject
> + * otherwise.
> + *
> + * Forfill params:

s/Forfill/Fulfill

@@ +82,5 @@
> +/* Set mozSettings values.
> + *
> + * Resolve if that mozSettings value is set successfully, reject otherwise.
> + *
> + * Forfill params: (none)

ditto.

@@ +113,5 @@
> +/* Set mozSettings value with only one key.
> + *
> + * Resolve if that mozSettings value is set successfully, reject otherwise.
> + *
> + * Forfill params: (none)

ditto.

@@ +205,5 @@
> +  SpecialPowers.pushPermissions(permissions, function() {
> +    ok(true, "permissions pushed: " + JSON.stringify(permissions));
> +
> +    // Permission changes can't change existing Navigator.prototype
> +    // objects, so grab our objects from a new Navigator

nit: place a period at the end of the line

@@ +237,5 @@
> +/* Wait for one named MobileConnection event.
> + *
> + * Resolve if that named event occurs.  Never reject.
> + *
> + * Forfill params: the DOMEvent passed.

ditto.

@@ +261,5 @@
> +/* Set data connection enabling state and wait for "datachange" event.
> + *
> + * Resolve if data connection state changed to the expected one.  Never reject.
> + *
> + * Forfill params: (none)

ditto.

@@ +281,5 @@
> +      deferred.resolve();
> +      return;
> +    }
> +
> +    return waitForManagerEvent("datachange").then(keepWaiting);

Any case that we don't treat it as a failure even we receive 'connected' not equal to expected 'aEnabled?'

@@ +282,5 @@
> +      return;
> +    }
> +
> +    return waitForManagerEvent("datachange").then(keepWaiting);
> +  })

A semicolon is missing.

@@ +289,5 @@
> +}
> +
> +/* Set voice/data roaming emulation and wait for state change.
> + *
> + * Forfill params: (none)

ditto.

@@ +366,5 @@
> + * This function ensures global |mobileConnection| variable is available during
> + * the process and performs clean-ups as well.
> + *
> + * @param aTestCaseMain
> + *        A function that take one parameter -- mobileConnection.

nit: s/take/takes/

::: dom/mobileconnection/tests/marionette/test_mobile_data_ipv6.js
@@ +12,5 @@
> +      let nm = getNetworkManager();
> +      let active = nm.active;
> +      ok(active, "Active network interface");
> +
> +      log("  Interface: " + active.name);

nit: remove extra ws after "

@@ +13,5 @@
> +      let active = nm.active;
> +      ok(active, "Active network interface");
> +
> +      log("  Interface: " + active.name);
> +      log("  Address: " + active.ip);

ditto.

@@ +26,5 @@
> +
> +function doTestHome(aApnSettings, aProtocol) {
> +  log("Testing \"" + aProtocol + "\"@HOME... ");
> +
> +  aApnSettings[0][0].protocol = aProtocol;

Please comment [0][0].

@@ +35,5 @@
> +
> +function doTestRoaming(aApnSettings, aRoaminProtocol) {
> +  log("Testing \"" + aRoaminProtocol + "\"@ROMAING... ");
> +
> +  delete aApnSettings[0][0].protocol;

ditto.

@@ +46,5 @@
> +  let origApnSettings;
> +
> +  return setDataRoamingEnabled(true)
> +    .then(getDataApnSettings)
> +    .then(function(aResult) {

Not very big deal, but sometime verbose style is used in a promise chain while sometimes the compact one is. Any guideline of the distinction?  Why not use the same one over the code?
Attachment #8383702 - Flags: review?(htsai)
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #36)
> We don't have IPV4V6 now.  Can't find the correct format through AOSP source
> code.  Need of real hardware references.  Will file a follow-up and place a
> comment here instead.
File a follow-up bug, bug 978711.
(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #37)
> ::: dom/mobileconnection/tests/marionette/head.js
> @@ +10,5 @@
> > +let Promise = Cu.import("resource://gre/modules/Promise.jsm").Promise;
> > +
> > +let _pendingEmulatorCmdCount = 0;
> > +
> > +/* Send emulator command with safe guard.
> 
> nit: the format we are using is
> 
> /*
>  * Comment starts...
>  * ...
>  * comment ends.
>  */
> 
> Please make the style consistent all over the file.

I think you mean:

/**
 * Blah ...
 */

> @@ +50,5 @@
> > + *
> > + * Resolve if that mozSettings value is retrieved successfully, reject
> > + * otherwise.
> > + *
> > + * Forfill params:
> 
> s/Forfill/Fulfill

Will fix "dom/bluetooth/tests/marionette/head.js" as well.

> @@ +281,5 @@
> > +      deferred.resolve();
> > +      return;
> > +    }
> > +
> > +    return waitForManagerEvent("datachange").then(keepWaiting);
> 
> Any case that we don't treat it as a failure even we receive 'connected' not
> equal to expected 'aEnabled?'

Don't get you quite well.  I do check |connected == aEnabled| here.

> ::: dom/mobileconnection/tests/marionette/test_mobile_data_ipv6.js
> @@ +12,5 @@
> > +      let nm = getNetworkManager();
> > +      let active = nm.active;
> > +      ok(active, "Active network interface");
> > +
> > +      log("  Interface: " + active.name);
> 
> nit: remove extra ws after "

Why?

> @@ +46,5 @@
> > +  let origApnSettings;
> > +
> > +  return setDataRoamingEnabled(true)
> > +    .then(getDataApnSettings)
> > +    .then(function(aResult) {
> 
> Not very big deal, but sometime verbose style is used in a promise chain
> while sometimes the compact one is. Any guideline of the distinction?  Why
> not use the same one over the code?

I personally use compact style when the action to take is a simple, state-less call that doesn't depend on the resolved value of previous run.
Blocks: 979134
(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #37)
> Please file a followup to replace mobile_head.js with head.js.

Filed as bug 979134.
Address comment 35.
Attachment #8383701 - Attachment is obsolete: true
Attachment #8385195 - Flags: review?(htsai)
Attached patch part 2/2: test cases : v2 (obsolete) — Splinter Review
Address review comments.
Attachment #8383702 - Attachment is obsolete: true
Attachment #8385197 - Flags: review?(htsai)
Comment on attachment 8385195 [details] [diff] [review]
part 1/2: read PDP_type from APN settings : v2

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

Thank you!
Attachment #8385195 - Flags: review?(htsai) → review+
Comment on attachment 8385197 [details] [diff] [review]
part 2/2: test cases : v2

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

Awesome!
Attachment #8385197 - Flags: review?(htsai) → review+
Add a few more comments in |setDataEnabledAndWait()| since hsinyi still raised a question about it.
Attachment #8385197 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/901717199390
https://hg.mozilla.org/mozilla-central/rev/a7ea2d51414d
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Depends on: 980375
Backed out for causing bug 980375.
https://hg.mozilla.org/mozilla-central/rev/8095b7dd8f58
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: 1.4 S2 (28feb) → ---
We should think about a case that a device may not support IPv6. See https://bugzilla.mozilla.org/show_bug.cgi?id=980375#c11.

So, even apnSetting.protocol is IPv6, we should still use "IP" as default while the device itself is lack of ipv6 capability.
Only assign pdpType from protocol or roaming_protocol when a new RIL quirk "ro.moz.ril.ipv6" is set to "true".
Attachment #8385195 - Attachment is obsolete: true
Whiteboard: [ucid: , 1.4, FT:RIL] → [ucid: , 1.4, FT:RIL][ipv6]
Attachment #8388338 - Flags: review?(htsai)
Comment on attachment 8388338 [details] [diff] [review]
part 1/2: read PDP_type from APN settings : v3

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

Thank you :)
Attachment #8388338 - Flags: review?(htsai) → review+
Verified by applying PRs in bug 978071 with and without the third PR, attachment 8389016 [details] [review].  When the third PR was applied, test cases here passed as before; when without, two test cases failed because it got an IPv4 address instead of the expected IPv6 one.
https://hg.mozilla.org/mozilla-central/rev/cbf30c2f215b
https://hg.mozilla.org/mozilla-central/rev/765c7b0b461d
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → 1.4 S3 (14mar)
Blocks: 1183002
You need to log in before you can comment on or make changes to this bug.