Wesley Huang [:wesley_huang] (EPM) (NI me) Reporter
	Description • 11 years ago

      No description provided.

	Ken Chang[:kenkai]
	Comment 1 • 11 years ago

vyang, can you please take this bug? Thanks.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 2 • 11 years ago

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.

	Ken Chang[:kenkai]
	Comment 3 • 11 years ago

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

	Wesley Huang [:wesley_huang] (EPM) (NI me) Reporter
	Comment 4 • 11 years ago

user story or feature work are not blocker.

	Wesley Huang [:wesley_huang] (EPM) (NI me) Reporter
	Comment 5 • 11 years ago

I assume Fabrice intended to set 1.4+

	Wesley Huang [:wesley_huang] (EPM) (NI me) Reporter
	Comment 6 • 11 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 8 • 11 years ago

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.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 9 • 11 years ago

parse <PDP_type>

	Ken Chang[:kenkai]
	Comment 10 • 11 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 11 • 11 years ago

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

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

	Preeti Raghunath(:Preeti)
	Comment 12 • 11 years ago

Per comment 11 marking this resolved fixed.

	Ken Chang[:kenkai]
	Comment 13 • 11 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 15 • 11 years ago

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

	Eric Chang [:ericcc] [:echang]
	Comment 16 • 11 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 17 • 11 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 18 • 11 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 20 • 11 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 21 • 11 years ago

Support roamingProtocol as well.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 23 • 11 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 24 • 11 years ago

(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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 25 • 11 years ago

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

	José Antonio Olivera Ortega [:jaoo]
	Comment 26 • 11 years ago

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.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 27 • 11 years ago

Also works in emulator-kk.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 28 • 11 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 30 • 11 years ago

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

Move to bug 978071.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 31 • 11 years ago

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

Move to bug 978071.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 32 • 11 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 33 • 11 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 34 • 11 years ago

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

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 35 • 11 years ago

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?

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 36 • 11 years ago

(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

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 37 • 11 years ago

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?

	Ken Chang[:kenkai]
	Comment 38 • 11 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 39 • 11 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 40 • 11 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 41 • 11 years ago

Address comment 35.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 42 • 11 years ago

Address review comments.

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 43 • 11 years ago

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!

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 44 • 11 years ago

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

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

Awesome!

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 45 • 11 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 46 • 11 years ago

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

	Carsten Book [:Tomcat]
	Comment 47 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 48 • 11 years ago

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

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 49 • 11 years ago

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.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 50 • 11 years ago

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

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 51 • 11 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 52 • 11 years ago

Verified by applying PRs in bug 978071 with and without the third PR, attachment 8389016 [details].  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.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 53 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 54 • 11 years ago

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