957917 - [IPv6] To support IPv6 in RIL.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Comment 1

[Ken Chang[:kenkai]]

vyang, can you please take this bug? Thanks.

Comment 2

[Vicamo Yang [:vicamo][: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.

Comment 3

[Ken Chang[:kenkai]]

This bug is supported to change the interface between ril and network manager.

Comment 4

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

user story or feature work are not blocker.

Comment 5

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

I assume Fabrice intended to set 1.4+

Comment 6

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

Target milestone set to 2/28. Remove 1.4+ as this is user story.

Comment 7

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [Gecko] read PDP_type from APN settings: WIP

Comment 8

[Vicamo Yang [:vicamo][:vyang]]

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.

Comment 9

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Github pull request for hardware/ril

parse <PDP_type>

Comment 10

[Ken Chang[:kenkai]]

Make this bug clear. This bug is to get IPv6 relevant information from network. And Bug 975813 is to configure IPv6 through network manager.

Comment 11

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8379464 [details]
Github pull request for hardware/ril

No longer needed because <PDP_type> parsing is merged into bug 871475.

Comment 12

[Preeti Raghunath(:Preeti)]

Per comment 11 marking this resolved fixed.

Comment 13

[Ken Chang[:kenkai]]

I think we still need this bug, but it isn't in the blocker list provided by partner.

Comment 14

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [Gecko] part 3/N: test cases : WIP

Comment 15

[Vicamo Yang [:vicamo][:vyang]]

https://github.com/vicamo/b2g_platform_hardware_ril/tree/bugzilla/957917/firefoxos-ipv6
https://github.com/vicamo/b2g_platform_external_qemu/tree/bugzilla/957917/firefoxos-ipv6
qemu gives ipv6 addresses as expected, rild crashes :X

Comment 16

[Eric Chang [:ericcc] [:echang]]

http://mzl.la/1c73sCh IPv6+LTE

Comment 17

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Github PR for platform/external/qemu [DO NOT SUBMIT]

This PR depends on bug 871475, which was backed out and is not fixed yet.  The PR here is only for test purpose.

Comment 18

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Github pull request for platform/hardware/ril [DO NOT SUBMIT]

This PR depends on bug 871475, which was backed out and is not fixed yet.  The PR here is only for test purpose.

Comment 19

[Vicamo Yang [:vicamo][:vyang]]

Attachment: adb-logcat_with-qemu_without_976959_975813.log

Comment 20

[Vicamo Yang [:vicamo][:vyang]]

Attachment: adb-logcat_with-qemu_with_976959_975813.log

So far we have two things to do: 1) handle undefined dns2, 2) handle undefined httpProxyHost.

Comment 21

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [Gecko] read PDP_type from APN settings: WIP 2

Support roamingProtocol as well.

Comment 22

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [Gecko] read PDP_type from APN settings

Comment 23

[Vicamo Yang [:vicamo][:vyang]]

Attachment: adb-logcat_emulator-jb.txt.gz

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

Comment 24

[Vicamo Yang [:vicamo][:vyang]]

(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

Comment 25

[Vicamo Yang [:vicamo][:vyang]]

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

[José Antonio Olivera Ortega [:jaoo]]

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.

Comment 27

[Vicamo Yang [:vicamo][:vyang]]

Also works in emulator-kk.

Comment 28

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 29

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [Gecko] part 2/2: test cases : WIP 2

Comment 30

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8382886 [details] [review]
Github PR for platform/external/qemu [DO NOT SUBMIT]

Move to bug 978071.

Comment 31

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8382889 [details] [review]
Github pull request for platform/hardware/ril [DO NOT SUBMIT]

Move to bug 978071.

Comment 32

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 1/2: read PDP_type from APN settings

1) use 'roaming_protocol' as Gaia does.
2) fix missing a '!' in protocol/roaming_protocol selection

Comment 33

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2/2: test cases

This patch is currently disabled because of bug 978071.  It passes with pull requests in that bug pulled into external/qemu and hardware/ril.

Comment 34

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 35

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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?

Comment 36

[Vicamo Yang [:vicamo][:vyang]]

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

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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?

Comment 38

[Ken Chang[:kenkai]]

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

Comment 39

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 40

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 41

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 1/2: read PDP_type from APN settings : v2

Address comment 35.

Comment 42

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2/2: test cases : v2

Address review comments.

Comment 43

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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!

Comment 44

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8385197 [details] [diff] [review]
part 2/2: test cases : v2

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

Awesome!

Comment 45

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2/2: test cases : v3

Add a few more comments in |setDataEnabledAndWait()| since hsinyi still raised a question about it.

Comment 46

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 47

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/901717199390
https://hg.mozilla.org/mozilla-central/rev/a7ea2d51414d

Comment 48

[Ryan VanderMeulen [:RyanVM]]

Backed out for causing bug 980375.
https://hg.mozilla.org/mozilla-central/rev/8095b7dd8f58

Comment 49

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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.

Comment 50

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 1/2: read PDP_type from APN settings : v3

Only assign pdpType from protocol or roaming_protocol when a new RIL quirk "ro.moz.ril.ipv6" is set to "true".

Comment 51

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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

Comment 52

[Vicamo Yang [:vicamo][:vyang]]

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.

Comment 53

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/b2g-inbound/rev/cbf30c2f215b
https://hg.mozilla.org/integration/b2g-inbound/rev/765c7b0b461d

Comment 54

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cbf30c2f215b
https://hg.mozilla.org/mozilla-central/rev/765c7b0b461d