Closed Bug 978709 Opened 7 years ago Closed 7 years ago

To appropriately configure a interface having both IPv6 and IPv4 addresses in network manager.

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(feature-b2g:2.0, tracking-b2g:backlog, b2g-v1.4 affected)

RESOLVED FIXED
1.4 S3 (14mar)
feature-b2g 2.0
tracking-b2g backlog
Tracking Status
b2g-v1.4 --- affected

People

(Reporter: kchang, Assigned: jessica)

References

Details

(Whiteboard: [ucid: , 2.0, FT:RIL])

Attachments

(18 files, 47 obsolete files)

3.05 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
6.03 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
13.07 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
5.81 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
1.27 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
1.84 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
1.22 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
6.25 KB, patch
jessica
: review+
Details | Diff | Splinter Review
3.44 KB, patch
jessica
: review+
Details | Diff | Splinter Review
4.72 KB, patch
jessica
: review+
Details | Diff | Splinter Review
5.34 KB, patch
jessica
: review+
Details | Diff | Splinter Review
7.82 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
4.16 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
6.76 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
2.59 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
11.57 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
4.95 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
15.15 KB, patch
jessica
: review+
Details | Diff | Splinter Review
Network manager may receive a interface having both IPv6 and IPv4 addresses from WIFI or RIL. This kind of interfaces need to be appropriately configured in network manager.
Jessica, we need to have a draft patch for this bug before next week's workshop. Can you please help this?
Edger, it seems that you can provide some information for Jessica. Thanks.
Assignee: nobody → jjong
Flags: needinfo?(echen)
Target Milestone: --- → 1.4 S3 (14mar)
Depends on: 978711
(In reply to Jessica Jong [:jjong] [:jessica] from comment #2)
> Created attachment 8385215 [details] [diff] [review]
> Part 1: nsINetworkInterface changes.

This interface is what I have in mind. However, I found that NetworkManager is not the only one using nsINetworkInterface, see gonk_addrs [1], so I am not sure if jsval is the best type for 'addresses'.
I will work on NetworkManager, NetworkService and NetworkUtils changes first, and Edgar will help on jsval handling in gonk_addrs.

Any suggestion on the interface is welcome! Thanks.

[1] http://mxr.mozilla.org/mozilla-central/source/media/mtransport/gonk_addrs.cpp#67
Note that the I commented out the gonk_addrs part first for development.

TODOs:
1. Verify wifi part.
2. Handle secondary routes for IPv4v6.
Attachment #8385215 - Flags: feedback?(echen)
Attachment #8386001 - Flags: feedback?(echen)
Attachment #8386003 - Flags: feedback?(echen)
Attachment #8385215 - Flags: feedback?(echen) → feedback+
Attachment #8385215 - Flags: feedback+ → review+
Comment on attachment 8386001 [details] [diff] [review]
(WIP) Part 2: NetworkService changes.

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

::: dom/system/gonk/NetworkService.js
@@ +277,5 @@
> +        ifname: network.name,
> +        ip: address.ip,
> +        prefixLength: address.prefixLength
> +      };
> +      this.controlMessage(options, function() {});

nit: please remove that empty function.
Comment on attachment 8386001 [details] [diff] [review]
(WIP) Part 2: NetworkService changes.

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

Thank you, please see my below comment.

::: dom/system/gonk/NetworkService.js
@@ +264,5 @@
>      });
>    },
>  
>    resetRoutingTable: function(network) {
> +    for (let i = 0; i < network.addresses; i++) {

I guess it should be |network.addresses.length|.
Attachment #8386001 - Flags: feedback?(echen)
Comment on attachment 8386003 [details] [diff] [review]
(WIP) Part 3: NetworkUtils changes.

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

Please see my below comment. :)
Thank you.

::: dom/system/gonk/NetworkUtils.cpp
@@ +339,5 @@
> +    if ((getIpType(autoGateway.get()) == AF_INET &&
> +         getIpType(ip) == AF_INET) ||
> +        (getIpType(autoGateway.get()) == AF_INET6 &&
> +         getIpType(ip) == AF_INET6)) {
> +      strcpy(bestGateway, autoGateway.get());

Hmm ... do a strcpy() here, It looks strange to me. Could we return gateways[i] or index directly?

@@ +1166,5 @@
> +    for (uint32_t i = 0; i < length; i++) {
> +      NS_ConvertUTF16toUTF8 autoDns(aOptions.mDnses[i]);
> +
> +      char dns_prop_key[PROPERTY_VALUE_MAX];
> +      sprintf(dns_prop_key, "net.dns%d", i);

dns properties starts with "net.dns1". So I think it should be:

sprintf(dns_prop_key, "net.dns%d", i+1);

@@ +1217,5 @@
> +      }
> +
> +      if (type == AF_INET6) {
> +        if (!aOptions.mOldIfname.IsEmpty()) {
> +          mNetUtils->do_ifc_remove_route(GET_CHAR(mOldIfname), "::", 0, NULL);

I think we only need to execute removing route one time, so move it out of for loop.

@@ +1223,5 @@
> +
> +        mNetUtils->do_ifc_add_route(autoIfname.get(), "::", 0, gateway);
> +      } else { /* type == AF_INET */
> +        if (!aOptions.mOldIfname.IsEmpty()) {
> +          mNetUtils->do_ifc_remove_default_route(GET_CHAR(mOldIfname));

ditto

@@ +1242,5 @@
>      }
>  
> +    if (type == AF_INET6) {
> +      if (!aOptions.mOldIfname.IsEmpty()) {
> +        mNetUtils->do_ifc_remove_route(GET_CHAR(mOldIfname), "::", 0, NULL);

The "if (length > 0)" and "else" parts will both do removing route, so could we combine them into one?

@@ +1248,5 @@
>  
> +      mNetUtils->do_ifc_add_route(autoIfname.get(), "::", 0, gateway);
> +    } else { /* type == AF_INET */
> +      if (!aOptions.mOldIfname.IsEmpty()) {
> +        mNetUtils->do_ifc_remove_default_route(GET_CHAR(mOldIfname));

ditto

@@ +1294,5 @@
>    uint32_t length = aOptions.mHostnames.Length();
>    for (uint32_t i = 0; i < length; i++) {
>      NS_ConvertUTF16toUTF8 autoHostname(aOptions.mHostnames[i]);
> +    gateway = {0};
> +    selectGateway(aOptions.mGateways, autoHostname.get(), gateway);

Please see above comment in selectGateway().

@@ +1321,5 @@
>    uint32_t length = aOptions.mHostnames.Length();
>    for (uint32_t i = 0; i < length; i++) {
>      NS_ConvertUTF16toUTF8 autoHostname(aOptions.mHostnames[i]);
> +    gateway = {0};
> +    selectGateway(aOptions.mGateways, autoHostname.get(), gateway);

Please see above comment in selectGateway().

::: dom/webidl/NetworkOptions.webidl
@@ +17,4 @@
>    unsigned long prefixLength;         // for "removeNetworkRoute".
>    DOMString domain;                   // for "setDNS"
>    DOMString dns1_str;                 // for "setDNS", "setDefaultRouteAndDNS".
>    DOMString dns2_str;                 // for "setDNS", "setDefaultRouteAndDNS".

"setDefaultRouteAndDNS" won't use these two parameter any more, please help to correct the comment.

@@ +18,5 @@
>    DOMString domain;                   // for "setDNS"
>    DOMString dns1_str;                 // for "setDNS", "setDefaultRouteAndDNS".
>    DOMString dns2_str;                 // for "setDNS", "setDefaultRouteAndDNS".
>    DOMString oldIfname;                // for "setDefaultRouteAndDNS".
>    DOMString gateway_str;              // for "setDefaultRouteAndDNS".

Could we remove |gateway_str|? Is there any other command will use it?
If only "setDefaultRouteAndDns" will use this parameter, I think we can remove it given that you already replaced it by using |gateways| in Part2.
((And we also need to remove |mGateway_str| in NetworkUtils.h))

@@ +20,5 @@
>    DOMString dns2_str;                 // for "setDNS", "setDefaultRouteAndDNS".
>    DOMString oldIfname;                // for "setDefaultRouteAndDNS".
>    DOMString gateway_str;              // for "setDefaultRouteAndDNS".
>    DOMString gateway;                  // for "addHostRoute", "removeHostRoute",
>                                        //     "removeDefaultRoute".

Same as above. Could we remove this parameter?

@@ +44,5 @@
>    DOMString usbStartIp;               // for "setWifiTethering".
>    DOMString usbEndIp;                 // for "setWifiTethering".
>    DOMString dns1;                     // for "setWifiTethering".
>    DOMString dns2;                     // for "setWifiTethering".
> +  sequence<DOMString> dnses;          // for "addHostRoute", "removeHostRoute"

Please correct the comment.
// for "setDNS", "setDefaultRouteAndDNS".
Attachment #8386003 - Flags: feedback?(echen)
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #7)
> Comment on attachment 8386001 [details] [diff] [review]
> (WIP) Part 2: NetworkService changes.
> 
> Review of attachment 8386001 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/NetworkService.js
> @@ +277,5 @@
> > +        ifname: network.name,
> > +        ip: address.ip,
> > +        prefixLength: address.prefixLength
> > +      };
> > +      this.controlMessage(options, function() {});
> 
> nit: please remove that empty function.

Will do. Should we remove existing empty functions as well?
(In reply to Edgar Chen [:edgar][:echen] from comment #8)
> Comment on attachment 8386001 [details] [diff] [review]
> (WIP) Part 2: NetworkService changes.
> 
> Review of attachment 8386001 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you, please see my below comment.
> 
> ::: dom/system/gonk/NetworkService.js
> @@ +264,5 @@
> >      });
> >    },
> >  
> >    resetRoutingTable: function(network) {
> > +    for (let i = 0; i < network.addresses; i++) {
> 
> I guess it should be |network.addresses.length|.

Yes, thanks. :) Haven't verified this part with wifi yet...
Comment on attachment 8385215 [details] [diff] [review]
Part 1: nsINetworkInterface changes.

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

::: dom/system/gonk/nsINetworkManager.idl
@@ +79,5 @@
>    readonly attribute long httpProxyPort;
>  
>  };
>  
> +dictionary NetworkAddress

Bug 978611 plans to move all dictionary to Webidl. Please help to move it to Webidl. Thank you ;)
Flags: needinfo?(echen)
Attached patch Part 1: interface changes, v2. (obsolete) — Splinter Review
Address review comment 12:
- move dictionary to webidl.
Attachment #8385215 - Attachment is obsolete: true
Blocks: 978711
No longer depends on: 978711
Address review comment 7 and comment 8:
- remove empty function
- correct |network.addresses.length|.
Attachment #8386001 - Attachment is obsolete: true
Remove debug log.
Attachment #8386638 - Attachment is obsolete: true
Address review comment 9:
- change selectGateway(...) to return index.
- set dns property "net.dnsX" starting from index 1.
- per offline discussion, remove default route once for IPv4 and once for IPv6.
- parameters fix will be on the next patch.
Attachment #8386003 - Attachment is obsolete: true
Attachment #8386635 - Flags: review?(vyang)
Attachment #8386635 - Flags: feedback?(echen)
Attachment #8386640 - Flags: review?(vyang)
Attachment #8386640 - Flags: feedback?(echen)
Attachment #8386646 - Flags: review?(vyang)
Attachment #8386646 - Flags: feedback?(echen)
Attachment #8386654 - Flags: review?(vyang)
Attachment #8386654 - Flags: feedback?(echen)
Attachment #8386657 - Flags: review?(vyang)
Attachment #8386657 - Flags: feedback?(echen)
Attached patch Part 1: interface changes, v3. (obsolete) — Splinter Review
fix typo in dictionary.
Attachment #8386635 - Attachment is obsolete: true
Attachment #8386635 - Flags: review?(vyang)
Attachment #8386635 - Flags: feedback?(echen)
Attachment #8386669 - Flags: review?(vyang)
Attachment #8386669 - Flags: feedback?(echen)
The is for testing, we should discuss how to land this.
Attached patch (WIP) Changes for gonk_addrs.cpp (obsolete) — Splinter Review
Some code in webrtc will use NetworkInterface to get ip address. So we need to modify it as well.

This patch is WIP and works well with the patch that Jessica provided to me yesterday.

You can use following step to test webrtc in b2g:
1). Open browser.
2). Goto: http://mozilla.github.io/webrtc-landing/data_test.html
3). Press "Start!!" button.
4). If test success, you will see some Hello messages with blue color in text box. 

Thank you
Comment on attachment 8386669 [details] [diff] [review]
Part 1: interface changes, v3.

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

Looks good. Thank you.

::: dom/system/gonk/nsINetworkManager.idl
@@ +52,2 @@
>     */
> +  readonly attribute jsval addresses;

nit: .... // NetworkAddress[]
Attachment #8386669 - Flags: feedback?(echen) → feedback+
Comment on attachment 8386640 [details] [diff] [review]
Part 2: NetworkService changes, v2.

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

Looks good to me. Thank you.
Attachment #8386640 - Flags: feedback?(echen) → feedback+
Comment on attachment 8386654 [details] [diff] [review]
Part 3.2: remove unused parameters in NetworkOptions, v1.

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

Thank you.
Attachment #8386654 - Flags: feedback?(echen) → feedback+
Comment on attachment 8386669 [details] [diff] [review]
Part 1: interface changes, v3.

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

::: dom/system/gonk/nsINetworkManager.idl
@@ +52,2 @@
>     */
> +  readonly attribute jsval addresses;

From the commercial ril perspective, we would want this interface to not use "jsval" to pass arrays.
Instead of using an attribute, how about using a function which allows passing typed arrays such as:

  void getAddresses([array, size_is(aLength)] in DOMString aAddresses, in unsigned long aLength);
Comment on attachment 8386669 [details] [diff] [review]
Part 1: interface changes, v3.

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

Please help have a minor revise to be more friendly to native implementations. All addresses/gateways/dnses.  Thank you :)
Attachment #8386669 - Flags: review?(vyang)
Comment on attachment 8386640 [details] [diff] [review]
Part 2: NetworkService changes, v2.

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

::: dom/system/gonk/NetworkService.js
@@ +269,5 @@
> +      let address = network.addresses[i];
> +      if (!address.ip || !address.prefixLength) {
> +        if(DEBUG) debug("Either ip or prefixLength is null. Cannot reset routing table.");
> +        return;
> +      }

I think we no longer need this check.  Instead, we should make sure whenever an entry is pushed into |network.addresses|, its ip and prefixLength attributes must be available.
Attachment #8386640 - Flags: review?(vyang)
Comment on attachment 8386646 [details] [diff] [review]
Part 3.1: NetworkUtils changes, v2.

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

::: dom/system/gonk/NetworkUtils.cpp
@@ +328,5 @@
> +/**
> + * Helper function to find the best match gateway. Return the index in
> + * gateways if found, return invalid index otherwise.
> + */
> +static uint32_t selectGateway(nsTArray<nsString>& gateways, const char* ip)

When |selectGateway| gets called, we always have |getIpType(ip)| called as well in a few lines later.  So why not pass AF of ip instead?

@@ +337,5 @@
> +    NS_ConvertUTF16toUTF8 autoGateway(gateways[i]);
> +    if ((getIpType(autoGateway.get()) == AF_INET &&
> +         getIpType(ip) == AF_INET) ||
> +        (getIpType(autoGateway.get()) == AF_INET6 &&
> +         getIpType(ip) == AF_INET6)) {

We can surely cache |getIpType(ip)| here, can't we?

@@ +864,5 @@
> +  for (uint32_t i = 0; i < length; i++) {
> +    NS_ConvertUTF16toUTF8 autoDns(dnses[i]);
> +
> +    strcat(command, " ");
> +    strcat(command, autoDns.get());

|snprintf| returns the number of chars would be/have been written to the output buffer.  Please use it to calculate the start address of the next write.

@@ +1187,1 @@
>      property_set("net.dns1", interfaceProperties.dns1);

Why do we still have dns1/dns2 here?

@@ +1269,4 @@
>  
> +    int type = getIpType(autoGateway.get());
> +    if (type != AF_INET && type != AF_INET6) {
> +      return false;

continue;

::: dom/webidl/NetworkOptions.webidl
@@ +22,5 @@
>    DOMString oldIfname;                // for "setDefaultRouteAndDNS".
>    DOMString gateway_str;              // for "setDefaultRouteAndDNS".
> +  DOMString gateway;                  // for "addSecondaryRoute", "removeSecondaryRoute".
> +  sequence<DOMString> gateways;       // for "setDefaultRouteAndDNS", "removeDefaultRoute",
> +                                      //     "addHostRoute", "removeHostRoute".

I believe there are more to do.  For example, remove dns1_str and dns2_str.
Attachment #8386646 - Flags: review?(vyang)
Comment on attachment 8386654 [details] [diff] [review]
Part 3.2: remove unused parameters in NetworkOptions, v1.

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

Ok, it's here.
Attachment #8386654 - Flags: review?(vyang) → review+
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #26)
> Comment on attachment 8386669 [details] [diff] [review]
> Part 1: interface changes, v3.
> 
> Review of attachment 8386669 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please help have a minor revise to be more friendly to native
> implementations. All addresses/gateways/dnses.  Thank you :)

Hi Vicamo,

Do you mean we should change as pgravel suggested?
void getAddresses([array, size_is(aLength)] in DOMString aAddresses, in unsigned long aLength);

I think it should be 'out' instead of 'in', right? And should we use 'DOMString' for simplicity or use a newly created interface like nsINeworkAddress for addresses?

Thank you.
Comment on attachment 8386646 [details] [diff] [review]
Part 3.1: NetworkUtils changes, v2.

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

Thank you.

::: dom/system/gonk/NetworkUtils.cpp
@@ +1175,5 @@
> +    for (uint32_t i = 0; i < length; i++) {
> +      NS_ConvertUTF16toUTF8 autoDns(aOptions.mDnses[i]);
> +
> +      char dns_prop_key[PROPERTY_VALUE_MAX];
> +      sprintf(dns_prop_key, "net.dns%d", i+1);

snprintf(...)
Attachment #8386646 - Flags: feedback?(echen) → feedback+
(In reply to Jessica Jong [:jjong] [:jessica] from comment #30)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #26)
> > Comment on attachment 8386669 [details] [diff] [review]
> > Part 1: interface changes, v3.
> > 
> > Review of attachment 8386669 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Please help have a minor revise to be more friendly to native
> > implementations. All addresses/gateways/dnses.  Thank you :)
> 
> Hi Vicamo,
> 
> Do you mean we should change as pgravel suggested?
> void getAddresses([array, size_is(aLength)] in DOMString aAddresses, in
> unsigned long aLength);

Yes, please.

> I think it should be 'out' instead of 'in', right? And should we use
> 'DOMString' for simplicity or use a newly created interface like
> nsINeworkAddress for addresses?

Both aAddresses and aLength are output parameters.
Comment on attachment 8386657 [details] [diff] [review]
Part 4: secondary routing changes, v1.

Per offline discussion, we will use a simpler way to check if address is IPv6 or not. IP validity check will be done in NetworkUtils.
Attachment #8386657 - Flags: review?(vyang)
Attachment #8386657 - Flags: feedback?(echen)
Attached patch Part 1: interface changes, v4. (obsolete) — Splinter Review
Change interface from attributes to method calls.

For dnses and gateways, the array is returned for simplicity; for addresses, we can only return the length cause there are two arrays.
Attachment #8386669 - Attachment is obsolete: true
WIP v2 for new interface. I have not verify it yet.
change to use method calls instead of attributes.
Attachment #8386640 - Attachment is obsolete: true
address review comment 28 and squash part 3.2 into this commit.
Attachment #8386646 - Attachment is obsolete: true
Attachment #8386654 - Attachment is obsolete: true
use a simpler way to check if IP is IPv6 or not.
Attachment #8386657 - Attachment is obsolete: true
implement nsINetworkInterface new methods.
Attachment #8386674 - Attachment is obsolete: true
Modify WifiP2pManager implementation as well.
Attachment #8389643 - Attachment is obsolete: true
nsIRILDataCallback::receiveDataCallList has been obsoleted for a long long time and all the existing classes that implement nsIRILDataCallback leave the function body empty.  Since we're going to change the object layout of a |datacall| JS object, removing related but obsoleted functions saves us some time.
nsIRILDataCallback has been abused in RadioInterfaceLayer.js and is becoming an RIL internal utility.  Besides, nsIRILDataCallback has also racing problem as described in bug 976897.  We should really use NetworkManager observer events to replace the notification mechanism here.
Depending on bug 976897 to remove nsIRILDataCallback in GonkGPSGeolocationProvider.
Depends on: 976897
So far verified with all existing data connection test cases and the currently disabled one in bug 957917 -- IPv6 test cases.
fix "'network' undefined" in test_data_connection.js.
Attachment #8390246 - Attachment is obsolete: true
fix NetworkInterfaceListService.
Attachment #8390256 - Attachment is obsolete: true
1) fix an exception thrown when |getAddresses| is called.
2) we always have ips/prefixLengths/dnses/gateways assigned to an array in nsIRILNetworkInterface.
Attachment #8390250 - Attachment is obsolete: true
Attached patch [clean-up] part 6/6: test cases (obsolete) — Splinter Review
This entity is currently disabled on tryserver due to bug 871475, but it passes in my local setup.

Try: https://tbpl.mozilla.org/?tree=Try&rev=37c2bd2c6cc2
Attachment #8386688 - Attachment is obsolete: true
s/this.addresses/this.ips/
Attachment #8390324 - Attachment is obsolete: true
Duplicate of this bug: 978711
Duplicate of this bug: 969305
Address review comment 31:
- use snprintf(...) instead of sprintf(...)
Attachment #8390247 - Attachment is obsolete: true
Initialize ips/prefixLenghts/gateways/dnses to empty array (to be consistent with WifiNetworkInterface).
Attachment #8390252 - Attachment is obsolete: true
- Integrated Edgar's work in attachment 8390329 [details] [diff] [review], but preserved Vicamo's change in NetworkInterfaceListService, cause we need to expose the attributes state, type, name, etc.
- Modified dom/media/tests/mochitest/pc.js to avoid media mochitest failures.
Attachment #8390310 - Attachment is obsolete: true
Test full try: https://tbpl.mozilla.org/?tree=Try&rev=9715e0b1f02a
Not breaking the media mochitests! (the Gu and KK failures were known issues at the time I sent to try)
Attachment #8390245 - Flags: review?(vyang)
Attachment #8390308 - Flags: review?(vyang)
Attachment #8393931 - Flags: review?(vyang)
Attachment #8390249 - Flags: review?(vyang)
Attachment #8390338 - Flags: review?(vyang)
Attachment #8393933 - Flags: review?(vyang)
Attachment #8390255 - Flags: review?(vyang)
Attachment #8393934 - Flags: review?(vyang)
No longer blocks: 978711
Attachment #8390245 - Flags: review?(vyang) → review+
Attachment #8390249 - Flags: review?(vyang) → review+
update commit summary only.
Attachment #8390223 - Attachment is obsolete: true
Attachment #8396206 - Flags: review?(htsai)
update commit summary only
Attachment #8390225 - Attachment is obsolete: true
Attachment #8396230 - Flags: review?(htsai)
Attachment #8396206 - Attachment description: [clean-up] part 1/N: remove nsINetworkInterface::broadcast : v2 → [clean-up] part 1/6: remove nsINetworkInterface::broadcast : v2
update commit summary only.
Attachment #8390227 - Attachment is obsolete: true
Attachment #8396236 - Flags: review?(htsai)
Attachment #8396237 - Flags: review?(htsai)
update commit summary only.
Attachment #8390230 - Attachment is obsolete: true
Attachment #8396279 - Flags: review?(htsai)
update commit summary only.
Attachment #8390231 - Attachment is obsolete: true
Attachment #8396280 - Flags: review?(htsai)
Comment on attachment 8389021 [details] [diff] [review]
Part 1: interface changes, v4.

Replaced by attachment 8390245 [details] [diff] [review].
Attachment #8389021 - Attachment is obsolete: true
Comment on attachment 8389606 [details] [diff] [review]
Part 2: NetworkService changes, v3.

Replaced by attachment 8390308 [details] [diff] [review].
Attachment #8389606 - Attachment is obsolete: true
Comment on attachment 8389607 [details] [diff] [review]
Part 3: NetworkUtils changes, v3.

Replaced by attachment 8393931 [details] [diff] [review].
Attachment #8389607 - Attachment is obsolete: true
Comment on attachment 8390329 [details] [diff] [review]
(WIP) Changes for NetworkInterfaceListService, v1

Replaced by attachment 8393934 [details] [diff] [review].
Attachment #8390329 - Attachment is obsolete: true
update commit summary only.
Attachment #8390257 - Attachment is obsolete: true
Attachment #8396322 - Flags: review?(htsai)
update commit summary only.
Attachment #8390325 - Attachment is obsolete: true
Attachment #8396323 - Flags: review?(htsai)
Comment on attachment 8389639 [details] [diff] [review]
Part 4: secondary routing changes, v2.

Replaced by attachment 8390249 [details] [diff] [review].
Attachment #8389639 - Attachment is obsolete: true
Comment on attachment 8396234 [details] [diff] [review]
[clean-up] part 2.b/6: don't use nsIRILDataCallback in GonkGPSGeolocationProvider : v2

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

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ -783,5 @@
>  
> -  // We call Setting Service before we get the state of supl data connection
> -  // since it is possible that state of supl data connection haven't been
> -  // updated and will be updated after we finished this function (code that
> -  // updates the state is in another dataCallStateChanged callback).

Please keep this comment so it's clear why we need this indirection. Just remove the reference of dataCallStateChanged.
Attachment #8396234 - Flags: review?(kchen) → review+
Comment on attachment 8390308 [details] [diff] [review]
[clean-up] part 4.b/6: NetworkService changes to support IPV4V6 : v2

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

::: dom/system/gonk/NetworkService.js
@@ +284,5 @@
>    },
>  
>    setDNS: function(networkInterface) {
>      if(DEBUG) debug("Going DNS to " + networkInterface.name);
> +    let dnses = networkInterface.getDnses({});

nit: the first parameter of |nsINetworkInterface.getDnses()| is an optional output u_long integer.  You can simply omit them.  Same applies to |nsINetworkInterface.getGateways()|
Attachment #8390308 - Flags: review?(vyang) → review+
Comment on attachment 8393931 [details] [diff] [review]
[clean-up] part 4.c/6: NetworkUtils changes : v2

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

::: dom/system/gonk/NetworkUtils.cpp
@@ +854,5 @@
>                                     NetworkResultOptions& aResult)
>  {
>    char command[MAX_COMMAND_SIZE];
> +  int written = snprintf(command, MAX_COMMAND_SIZE - 1, "resolver setifdns %s %s",
> +                         GET_CHAR(mIfname), GET_CHAR(mDomain));

nit: `snprintf` writes at most (n - 1) characters, so the 2nd parameter here should be MAX_COMMAND_SIZE, or better, |sizeof command|.

@@ +868,5 @@
> +    }
> +    strcat(command, " ");
> +    strcat(command, autoDns.get());
> +
> +    written += strlen(autoDns.get()) + 1;

int ret = snprintf(command + written, sizeof(command) - written, " %s", autoDns.get());
if (ret <= 1) {
  command[written] = '\0';
  continue;
}

if ((ret + written) >= sizeof(command)) {
  command[written] = '\0';
  break;
}

@@ +1179,5 @@
> +    for (uint32_t i = 0; i < length; i++) {
> +      NS_ConvertUTF16toUTF8 autoDns(aOptions.mDnses[i]);
> +
> +      char dns_prop_key[PROPERTY_VALUE_MAX];
> +      snprintf(dns_prop_key, PROPERTY_VALUE_MAX - 1, "net.dns%d", i+1);

nit: `snprintf` writes at most (n - 1) characters, so the 2nd parameter here should be PROPERTY_VALUE_MAX, or better, |sizeof dns_prop_key|.

@@ +1305,5 @@
> +    uint32_t index = selectGateway(aOptions.mGateways, type);
> +    if (index < aOptions.mGateways.Length()) {
> +      strncpy(gateway, NS_ConvertUTF16toUTF8(aOptions.mGateways[index]).get(),
> +              sizeof(gateway) - 1);
> +    }

uint32_t index = selectGateway(aOptions.mGateways, type);
if (index >= aOptions.mGateways.Length()) {
  continue;
}

NS_ConvertUTF16toUTF8 autoGateway(aOptions.mGateways[index]);
prefix = type == AF_INET ? 32 : 128;
mNetUtils->do_ifc_add_route(autoIfname.get(), autoHostname.get(), prefix, autoGateway.get());

@@ +1333,5 @@
>      }
>  
> +    char gateway[128] = {0};
> +    uint32_t index = selectGateway(aOptions.mGateways, type);
> +    if (index < aOptions.mGateways.Length()) {

uint32_t index = selectGateway(aOptions.mGateways, type);
if (index >= aOptions.mGateways.Length()) {
  continue;
}

NS_ConvertUTF16toUTF8 autoGateway(aOptions.mGateways[index]);
prefix = type == AF_INET ? 32 : 128;
mNetUtils->do_ifc_remove_route(autoIfname.get(), autoHostname.get(), prefix, autoGateway.get());
Attachment #8393931 - Flags: review?(vyang)
Comment on attachment 8390249 [details] [diff] [review]
[clean-up] part 4.d/4: secondary routes changes

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

::: dom/system/gonk/NetworkManager.js
@@ +503,5 @@
>      }
>    },
>  
>    setSecondaryDefaultRoute: function(network) {
> +    let gateways = network.getGateways({});

nit: omit {}
Attachment #8390338 - Flags: review?(vyang) → review+
Comment on attachment 8393933 [details] [diff] [review]
[clean-up] part 4.f/6: P2pNetworkInterface changes : v2

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

::: dom/wifi/WifiP2pManager.jsm
@@ +505,5 @@
> +      if (!this.ips || !this.prefixLengths ||
> +          (this.ips.length != this.prefixLengths.length)) {
> +        ips.value = prefixLengths.value = [];
> +        return 0;
> +      }

nit: don't need such check because we have always a valid array-typed |this.ips|.
Attachment #8393933 - Flags: review?(vyang) → review+
Comment on attachment 8390255 [details] [diff] [review]
[clean-up] part 4.g/6: WifiNetworkInterface changes

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

::: dom/wifi/WifiWorker.js
@@ +1533,5 @@
> +    if (!this.ips || !this.prefixLengths ||
> +        (this.ips.length != this.prefixLengths.length)) {
> +      ips.value = prefixLengths.value = [];
> +      return 0;
> +    }

nit: don't need such check because we have always a valid array-typed |this.ips|.
Attachment #8390255 - Flags: review?(vyang) → review+
Comment on attachment 8393934 [details] [diff] [review]
[clean-up] part 4.h/6: NetworkInterfaceListService changes : v3

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

::: dom/media/tests/mochitest/pc.js
@@ +347,5 @@
> +
> +      for (var j = 0; j < length; j++) {
> +        var ip = ips.value[j];
> +        // skip IPv6 address
> +        if (ip && ip.indexOf(":") < 0) {

nit: ip should be always valid here, or it's a bug.

::: media/mtransport/gonk_addrs.cpp
@@ +68,5 @@
>      NetworkInterface interface;
>      memset(&(interface.addr), 0, sizeof(interface.addr));
>      interface.addr.sin_family = AF_INET;
> +
> +    if (NS_FAILED(iface->GetAddresses(&ips, &prefixs, &count))) {

We have to free memory allocated by GetAddresses().
Attachment #8393934 - Flags: review?(vyang)
Comment on attachment 8389219 [details] [diff] [review]
(WIP) Changes for gonk_addrs.cpp, v2

Replaced by attachment 8393934 [details] [diff] [review](4.h).
Attachment #8389219 - Attachment is obsolete: true
Attachment #8396206 - Flags: review?(htsai) → review+
Comment on attachment 8396230 [details] [diff] [review]
[clean-up] part 2.a/6: remove nsIRILDataCallback::receiveDataCallList : v2

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

Nice clean-up!
Attachment #8396230 - Flags: review?(htsai) → review+
Comment on attachment 8396230 [details] [diff] [review]
[clean-up] part 2.a/6: remove nsIRILDataCallback::receiveDataCallList : v2

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

Oh, just caught a miss.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ -1558,5 @@
> -   */
> -  handleDataCallList: function(message) {
> -    this._deliverDataCallCallback("receiveDataCallList",
> -                                  [message.datacalls, message.datacalls.length]);
> -  },

If we remove this, then how about [1]? We will get a Javascript referenced error. 

[1] http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js?from=RadioInterfaceLayer.js&case=true#2175
Attachment #8396230 - Flags: review+
Attachment #8396236 - Flags: review?(htsai) → review+
Comment on attachment 8396237 [details] [diff] [review]
[clean-up] part 2.d/6: don't throw in _deliverDataCallCallback loop : v2

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

Looks okay but I don't get a reason to enforce this. Could you share your thoughts?
Attachment #8396237 - Flags: review?(htsai) → review+
Attachment #8396279 - Flags: review?(htsai) → review+
(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #95)
> Comment on attachment 8396237 [details] [diff] [review]
> [clean-up] part 2.d/6: don't throw in _deliverDataCallCallback loop : v2
> -----------------------------------------------------------------
> Looks okay but I don't get a reason to enforce this. Could you share your
> thoughts?

When we register multiple callbacks to an event target, we expect these callback being called if a certain condition meets.  However, if one of the callbacks throws or the dispatching function throws in the dispatching loop, the callbacks standing in line after that executed/to be executed one starve.  That's why we have a try-block in the same function.
Comment on attachment 8396280 [details] [diff] [review]
[clean-up] part 3.b/6: support multiple addresses in ril_worker : v2

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +2153,5 @@
>          connHandler.handleDataCallError(message);
>          break;
>        case "datacallstatechange":
> +        let addresses = [];
> +        for (let entry of message.addresses) {

[1] shows performance issue of for-of is still valid. Let's consider trying our best to avoid that.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=969231#c9

@@ +4366,5 @@
>           datacall.state == RIL.GECKO_NETWORK_STATE_CONNECTED)) {
>        this.connecting = false;
>        this.cid = datacall.cid;
>        this.name = datacall.ifname;
> +      this.ip = datacall.addresses[0].address;

Assume you are going to support multiple addresses in the following patches.

::: dom/system/gonk/ril_worker.js
@@ +3883,5 @@
> +      return "deactivate";
> +    }
> +
> +    // If any existing address is missing, report as "deactivate".
> +    for (let address of currentDataCall.addresses) {

[1] shows performance issue of for-of is still valid. Let's consider trying our best to avoid that.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=969231#c9

@@ +3896,5 @@
> +      // reported.
> +      return "changed";
> +    }
> +
> +    for (let field of ["gateways", "dnses"]) {

ditto.
Attachment #8396280 - Flags: review?(htsai)
Comment on attachment 8396322 [details] [diff] [review]
[clean-up] part 5/6: really add an IPV4V6 option : v2

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

::: dom/system/gonk/ril_consts.js
@@ +2366,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_IPV4V6,

This part looks good but kindly me point me where the multiple addresses are handled. I didn't see that part in the pieces I reviewed.
Attachment #8396322 - Flags: review?(htsai) → review+
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #96)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #95)
> > Comment on attachment 8396237 [details] [diff] [review]
> > [clean-up] part 2.d/6: don't throw in _deliverDataCallCallback loop : v2
> > -----------------------------------------------------------------
> > Looks okay but I don't get a reason to enforce this. Could you share your
> > thoughts?
> 
> When we register multiple callbacks to an event target, we expect these
> callback being called if a certain condition meets.  However, if one of the
> callbacks throws or the dispatching function throws in the dispatching loop,
> the callbacks standing in line after that executed/to be executed one
> starve.  That's why we have a try-block in the same function.

Oh, got it! Thanks. :)
Address review comment 85:
- omit the parameter '{}' in |nsINetworkInterface.getDnses()| and |nsINetworkInterface.getGateways()|
Attachment #8390308 - Attachment is obsolete: true
Attachment #8397712 - Flags: review+
Address review comment 86:
- use a more appropriate parameter for snprintf
- refine |setInterfaceDns()| part
- refine the handling of the result of |selectGateway()|
Attachment #8393931 - Attachment is obsolete: true
Attachment #8397719 - Flags: review?(vyang)
Address review comment 87:
- Omit parameter {} in |network.getGateways()|
Attachment #8390249 - Attachment is obsolete: true
Attachment #8397722 - Flags: review+
(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #94)
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ -1558,5 @@
> > -   */
> > -  handleDataCallList: function(message) {
> > -    this._deliverDataCallCallback("receiveDataCallList",
> > -                                  [message.datacalls, message.datacalls.length]);
> > -  },
> 
> If we remove this, then how about [1]? We will get a Javascript referenced
> error. 

Oops! But we still won't get an error because nobody ever calls ril_worker:enumerateDataCalls, so "datacalllist" is never dispatched, and this function is never called.  Will add another patch to remove them completely.
Comment on attachment 8396323 [details] [diff] [review]
[clean-up] part 6/6: test cases : v2

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

Thank you!!!
Attachment #8396323 - Flags: review?(htsai) → review+
Address review comment 88:
- remove extra checks
Attachment #8393933 - Attachment is obsolete: true
Attachment #8397725 - Flags: review+
Address review comment 89:
- remove extra checks
Attachment #8390255 - Attachment is obsolete: true
Attachment #8397727 - Flags: review+
(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #97)
> Comment on attachment 8396280 [details] [diff] [review]
> [clean-up] part 3.b/6: support multiple addresses in ril_worker : v2
> -----------------------------------------------------------------
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +4366,5 @@
> >           datacall.state == RIL.GECKO_NETWORK_STATE_CONNECTED)) {
> >        this.connecting = false;
> >        this.cid = datacall.cid;
> >        this.name = datacall.ifname;
> > +      this.ip = datacall.addresses[0].address;
> 
> Assume you are going to support multiple addresses in the following patches.

They are attachment 8390338 [details] [diff] [review] (4.e, RIL), attachment 8393933 [details] [diff] [review] (4.f, P2P), and attachment 8390225 [details] [diff] [review] (4.g, WiFi).
Comment on attachment 8396323 [details] [diff] [review]
[clean-up] part 6/6: test cases : v2

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

I should also update "manifest.ini" with "disabled = bug 979137".
Address review comment 90:
- remove extra check for |ip|
- free memory allocated by |GetAddresses()|
Attachment #8393934 - Attachment is obsolete: true
Attachment #8397733 - Flags: review?(vyang)
Address review comment in comment 94.
Attachment #8396230 - Attachment is obsolete: true
Attachment #8397761 - Flags: review?(htsai)
Attachment #8396323 - Attachment is obsolete: true
Attachment #8397770 - Flags: review+
Comment on attachment 8397719 [details] [diff] [review]
[clean-up] part 4.c/6: NetworkUtils changes : v3

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

::: dom/system/gonk/NetworkUtils.cpp
@@ +1305,5 @@
>      if (type != AF_INET && type != AF_INET6) {
>        continue;
>      }
>  
> +    char gateway[128] = {0};

nit: unused variable.

@@ +1336,5 @@
>      if (type != AF_INET && type != AF_INET6) {
>        continue;
>      }
>  
> +    char gateway[128] = {0};

nit: unused variable.
Attachment #8397719 - Flags: review?(vyang) → review+
Attachment #8397733 - Flags: review?(vyang) → review+
No longer blocks: b2g-ipv6-1.4
Address nits in comment 115:
- removed unused variables
Attachment #8397719 - Attachment is obsolete: true
Attachment #8398288 - Flags: review+
Attachment #8397761 - Flags: review?(htsai) → review+
Attachment #8397765 - Flags: review?(htsai) → review+
Comment on attachment 8397767 [details] [diff] [review]
[clean-up] part 3.b/6: support multiple addresses in ril_worker : v3

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

Thank you~
Attachment #8397767 - Flags: review?(htsai) → review+
Hi Anshul,

The patches are already in trunk. Could someone from you help to test if this bug work as expected? as we don't have IPv4/v6 environment in Taiwan.

Thanks a lot.
Flags: needinfo?(anshulj)
Sure Jessica. Phil will give this a try once we are done adapting to this new interface. Please expect a few days of delay from our side.
Flags: needinfo?(anshulj) → needinfo?(pgravel)
Initial sanity tests look good. Verified both IPv4 and IPv6 routes are useable while in IPv4v6 mode.
Flags: needinfo?(pgravel)
(In reply to pgravel from comment #123)
> Initial sanity tests look good. Verified both IPv4 and IPv6 routes are
> useable while in IPv4v6 mode.

Thanks pgravel and Anshul, that's good to hear!
Please let us know if you have any other findings.
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #111)
> Created attachment 8397763 [details] [diff] [review]
> [clean-up] part 2.b/6: don't use nsIRILDataCallback in
> GonkGPSGeolocationProvider : v3
> 
> Rebase & r=kanru.

This part fails Flatfish build.  Will file a follow-up and fix it as soon as possible.
Vicamo, shouldn't this patch be uplifted to 1.4 as well?
Preeti, Madai is asking us about this feature and wondering if you could get this uplifted to 1.4.
blocking-b2g: --- → 1.4?
Flags: needinfo?(praghunath)
Target network had confirmed this is not needed during our last meet up, and discussed again in 3 way meeting yesterday with madai partners.

Adding partners, who were going to confirm again on this requirement.
Flags: needinfo?(ri-nagaike)
Flags: needinfo?(chryschoi)
Per comment 128, not blocking.
blocking-b2g: 1.4? → backlog
Confirmed with partner that IPv4v6 over one interface is not required.
Flags: needinfo?(ri-nagaike)
Flags: needinfo?(praghunath)
Whiteboard: [ucid: , FT:RIL]
feature-b2g: --- → 2.0
Whiteboard: [ucid: , FT:RIL] → [ucid: , 2.0, FT:RIL]
This feature has implemented on v2.0.
Flags: needinfo?(chryschoi)
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.