973543 - B2G RIL: host routes removed unexpectedly when data call is shared

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Jessica Jong [:jessica] (inactive)]

If a data call is shared among two active apn types, they have the same dns(es). When one of the apn types is requested to be deactivated, NetworkManager will remove its host routes (dns1, dns2, httpProxyHost) from the routing table when DISCONNECTED event is received; this is not right cause the other apn type still needs them.

In our current design, the DISCONNECTED event for the disconnected apn type is never sent (bug?), and the network interface is unregistered/registered again to update the status [1], so there is no problem. But this problem will become apparent after bug 939046.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#1434

Comment 1

[Jessica Jong [:jessica] (inactive)]

After some tests, I think it would be better if we solve this bug after bug 939046.
This is because in our current design, we do not handle each network type separately, so consider the following case: if two types of data connections are shared, e.g default and mms, both are connected and then one gets disconnected, when this happens, DataConnectionHandler will register/unregister the shared network interface and send a "network-interface-state-changed" topic for this interface [1], which state will still be "connected". This is very confusing and unbalanced, so I'd prefer to solve bug 939046 and 990458 first.


[1] http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#1440

Comment 2

[Jessica Jong [:jessica] (inactive)]

Changing dependency per comment 1.

Comment 3

[Ken Chang[:kenkai]]

Put this bug into backlog.

Comment 4

[Jessica Jong [:jessica] (inactive)]

Per offline discussion, we would like to move host routes handling into each network (bug 997654) first.

Comment 5

[Jessica Jong [:jessica] (inactive)]

Attachment: proposed patch, v1.

The reference count of host routes in NetworkService is simple: we just add each host route to an array every time it is added, and remove it from the array when it is removed, so when there is no instance of that host route in the array, means that it is no longer needed, in that case we can remove the host route safely.

NetworkManager, in the other hand, tracks the host routes set per network type. It handles host route 'difference' by removing the old routes and adding the new routes when a network type condition changes. It is also in charge of cleaning the host routes when a network type is disconnected.

Note that we don't handle add/remove host route failure here, cause I think there is no harm re-adding a route or removing a non-existing route.

Comment 6

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8533618 [details] [diff] [review]
proposed patch, v1.

Edgar, may I have your feedback on this? Thanks.

Comment 7

[Edgar Chen [:edgar]]

Comment on attachment 8533618 [details] [diff] [review]
proposed patch, v1.

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

Overall direction is right, but there are few things need to be improved. Please see my comments below. Thank you.

::: dom/system/gonk/NetworkManager.js
@@ +133,5 @@
>      return obj[name] = new RegExp(pattern);
>    });
>  }
>  
> +function NetworkInterfaceLinks()

I think we should also cache the `gateway` information.
The gateway in network could be cleaned when the network is disconnected.

@@ +369,5 @@
>  #ifdef MOZ_B2G_RIL
>          // Remove host route for data calls
>          if (this.isNetworkTypeMobile(network.type)) {
> +          this._handleRoutes(networkId, network);
> +          this._cleanupExtraRoutes(networkId, network);

Shall we handle the case that the network is unregistering without reporting disconnecting first?
Although it may not a valid operation that a network shall do, but since we add some reference counting mechanism for routing, we should be more careful for the routing maintenance.

@@ +463,5 @@
>    },
>  
> +  _handleRoutes: function(networkId, network) {
> +
> +    function getDifference(left, right) {

`left` and `right` are confused to me, I can't know which one is the `base` of the comparison.
Please use different name for the argument and help to add some commit on it. Thank you.

@@ +475,5 @@
> +
> +      return [];
> +    }
> +
> +    for (let field of ["dnses", "httpProxyHost"]) {

One question, why we need to separate "dnses" and "httpProxyHost"? They shall have no difference in routing perspective.

@@ +477,5 @@
> +    }
> +
> +    for (let field of ["dnses", "httpProxyHost"]) {
> +      let currentLinks = this.networkInterfaceLinks[networkId][field];
> +      let newLinks = network.state === Ci.nsINetworkInterface.NETWORK_STATE_CONNECTED

Move this check out of this utility function.
Let's make this utility function just accept new routing information and do corresponding routing changes.

@@ +531,5 @@
> +      .then((ipAddresses) => {
> +        let networkId = this.getNetworkId(network);
> +
> +        this.networkInterfaceLinks[networkId].extraRoutes =
> +          this.networkInterfaceLinks[networkId].extraRoutes.concat(ipAddresses);

Shall we need to filter out the duplicated one for extra route?

@@ +556,5 @@
> +            this.networkInterfaceLinks[networkId].extraRoutes.splice(found, 1);
> +          }
> +        });
> +
> +        this._updateRoutes(false,

Shall we remove the route only if it was found in |extraRoutes|?

::: dom/system/gonk/NetworkService.js
@@ +355,5 @@
>  
>    removeHostRoute: function(interfaceName, gateway, host) {
> +    let indexes = [];
> +    this.hostRoutes.forEach(function(route, index) {
> +      if (route.isIdentical(interfaceName, gateway, host)) {

With this data structure, it contains duplicated elements and we have to iterate each element to get the reference count. Do you think we could use other data structure to make it simpler?

Comment 8

[Jessica Jong [:jessica] (inactive)]

Thank you Edgar for the feedback!

(In reply to Edgar Chen [:edgar][:echen] from comment #7)
> Comment on attachment 8533618 [details] [diff] [review]
> proposed patch, v1.
> 
> Review of attachment 8533618 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall direction is right, but there are few things need to be improved.
> Please see my comments below. Thank you.
> 
> ::: dom/system/gonk/NetworkManager.js
> @@ +133,5 @@
> >      return obj[name] = new RegExp(pattern);
> >    });
> >  }
> >  
> > +function NetworkInterfaceLinks()
> 
> I think we should also cache the `gateway` information.
> The gateway in network could be cleaned when the network is disconnected.

You are right, 'gateway' may change as well. Will update this, thanks.

> 
> @@ +369,5 @@
> >  #ifdef MOZ_B2G_RIL
> >          // Remove host route for data calls
> >          if (this.isNetworkTypeMobile(network.type)) {
> > +          this._handleRoutes(networkId, network);
> > +          this._cleanupExtraRoutes(networkId, network);
> 
> Shall we handle the case that the network is unregistering without reporting
> disconnecting first?
> Although it may not a valid operation that a network shall do, but since we
> add some reference counting mechanism for routing, we should be more careful
> for the routing maintenance.

No problem, will do it in unregisterNetworkInterface() as well.

> 
> @@ +463,5 @@
> >    },
> >  
> > +  _handleRoutes: function(networkId, network) {
> > +
> > +    function getDifference(left, right) {
> 
> `left` and `right` are confused to me, I can't know which one is the `base`
> of the comparison.
> Please use different name for the argument and help to add some commit on
> it. Thank you.

Sure, will do.

> 
> @@ +475,5 @@
> > +
> > +      return [];
> > +    }
> > +
> > +    for (let field of ["dnses", "httpProxyHost"]) {
> 
> One question, why we need to separate "dnses" and "httpProxyHost"? They
> shall have no difference in routing perspective.

Mm.. yes, we could concat dnses and httpProxyHost and handle them altogether.

> 
> @@ +477,5 @@
> > +    }
> > +
> > +    for (let field of ["dnses", "httpProxyHost"]) {
> > +      let currentLinks = this.networkInterfaceLinks[networkId][field];
> > +      let newLinks = network.state === Ci.nsINetworkInterface.NETWORK_STATE_CONNECTED
> 
> Move this check out of this utility function.
> Let's make this utility function just accept new routing information and do
> corresponding routing changes.

Okay, do you mean we should reset the values (dnses, httpProxyHost) in 'network' before calling this function?

> 
> @@ +531,5 @@
> > +      .then((ipAddresses) => {
> > +        let networkId = this.getNetworkId(network);
> > +
> > +        this.networkInterfaceLinks[networkId].extraRoutes =
> > +          this.networkInterfaceLinks[networkId].extraRoutes.concat(ipAddresses);
> 
> Shall we need to filter out the duplicated one for extra route?

I though about filtering too, but consider the following case: two different mms services call addHostRoute() on a same route and one of them call removeHostRoute() first. If we filter duplicate routes, we will record only one route and it will be removed in this case, which is not right. So I think we should still call NetworkService.addHostRoute() and let NetworkService worry about reference counting. What do you think?

> 
> @@ +556,5 @@
> > +            this.networkInterfaceLinks[networkId].extraRoutes.splice(found, 1);
> > +          }
> > +        });
> > +
> > +        this._updateRoutes(false,
> 
> Shall we remove the route only if it was found in |extraRoutes|?

Yes, we should. Thanks.

> 
> ::: dom/system/gonk/NetworkService.js
> @@ +355,5 @@
> >  
> >    removeHostRoute: function(interfaceName, gateway, host) {
> > +    let indexes = [];
> > +    this.hostRoutes.forEach(function(route, index) {
> > +      if (route.isIdentical(interfaceName, gateway, host)) {
> 
> With this data structure, it contains duplicated elements and we have to
> iterate each element to get the reference count. Do you think we could use
> other data structure to make it simpler?

We could use 'Map' instead, with Route as key and reference count as value. However, using object as key is not really convenient since it does not compare object values when finding the key, so we might need to convert it to string before setting the key.

Comment 9

[Jessica Jong [:jessica] (inactive)]

Attachment: proposed patch, v2.

Edgar, I have made some changes according to the feedback in comment 7. May I have your review on this? Thanks.

Comment 10

[Edgar Chen [:edgar]]

Comment on attachment 8537703 [details] [diff] [review]
proposed patch, v2.

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

Please see my below comments. Thank you.

::: dom/system/gonk/NetworkManager.js
@@ +136,5 @@
>  
> +function NetworkInterfaceLinks()
> +{
> +  this.dnses = [];
> +  this.httpProxyHost = "";

I think we could merge |dnses| and |httpProxyHost| into one and have a generic name for it, e.g. `li
nkRoutes`.

@@ +138,5 @@
> +{
> +  this.dnses = [];
> +  this.httpProxyHost = "";
> +  this.gateways = [];
> +  this.name = "";

s/name/interfaceName/

@@ +139,5 @@
> +  this.dnses = [];
> +  this.httpProxyHost = "";
> +  this.gateways = [];
> +  this.name = "";
> +  this.extraRoutes = [];

Maybe we could just call |resetLinks()| for initialization.

@@ +148,5 @@
> +  gateways: null,
> +  name: null,
> +  extraRoutes: null,
> +
> +  getLinks: function() {

`getLinks` seems not a proper naming. It is easy to get confused because it doesn't return all link information, but only dnses + httpProxyHost. Could you help to rename it?

@@ +380,5 @@
>          // Add host route for data calls
>          if (this.isNetworkTypeMobile(network.type)) {
> +          // If gateways have changed, remove all old routes first.
> +          this._handleGateways(networkId, network.getGateways())
> +          .then(() => {

nit: indention.

@@ +472,5 @@
>      if (!(networkId in this.networkInterfaces)) {
>        throw Components.Exception("No network with that type registered.",
>                                   Cr.NS_ERROR_INVALID_ARG);
>      }
> +#ifdef MOZ_B2G_RIL

Let's fix bug 1054148 first. ;)

@@ +473,5 @@
>        throw Components.Exception("No network with that type registered.",
>                                   Cr.NS_ERROR_INVALID_ARG);
>      }
> +#ifdef MOZ_B2G_RIL
> +        // This is for in case a network gets unregistered without being

nit: indention

@@ +537,5 @@
> +      return;
> +    }
> +
> +    this._setHostRoutes(false, removedLinks, interfaceName, gateways)
> +    .then(this._setHostRoutes(true, addedLinks, interfaceName, gateways));

nit: indention.

@@ +576,5 @@
> +      .then((ipAddresses) => {
> +        let networkId = this.getNetworkId(network);
> +
> +        this.networkInterfaceLinks[networkId].extraRoutes =
> +          this.networkInterfaceLinks[networkId].extraRoutes.concat(ipAddresses);

If the host route can not be added successfully, we should not add it into |extraRoutes|.
We should do the same thing for linkRoute (dnses + httpProxyHost) as well, I think.

@@ +601,5 @@
> +            this.networkInterfaceLinks[networkId].extraRoutes.splice(found, 1);
> +          }
> +        });
> +
> +        this._setHostRoutes(false,

Remove the route only if it was found in |extraRoutes|.

@@ +635,5 @@
>    },
>  
>  #ifdef MOZ_B2G_RIL
> +  _handleGateways: function(networkId, gateways) {
> +    if (this.networkInterfaceLinks[networkId].gateways.length == 0 ||

Should we clear route if the new gateways is empty?

::: dom/system/gonk/NetworkService.js
@@ +334,5 @@
>      });
>      return deferred.promise;
>    },
>  
> +  routeToString: function(interfaceName, gateway, host) {

Add '_' prefix, s/routeToString/_routeToString/

@@ +354,1 @@
>      return this._setHostRoute(true, interfaceName, gateway, host);

One question just came to my mind, how should we handle the the reference counting if the route operation returns error for some reason?
And per offline discussion, it seems we need to queue all the request and process them one by one to make sure the reference counting and routing could be maintained correctly, including error case.

@@ +359,5 @@
> +    let count = this.hostRoutes.get(route);
> +
> +    if (DEBUG) debug("removeHostRoute: " + route + " -> " + count);
> +
> +    if (count == undefined) {

I think `if (!count)` should be okay.

Comment 11

[Jessica Jong [:jessica] (inactive)]

Thank you Edgar for the review.

(In reply to Edgar Chen [:edgar][:echen] from comment #10)
> Comment on attachment 8537703 [details] [diff] [review]
> proposed patch, v2.
> 
> Review of attachment 8537703 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please see my below comments. Thank you.
> 
> ::: dom/system/gonk/NetworkManager.js
> @@ +136,5 @@
> >  
> > +function NetworkInterfaceLinks()
> > +{
> > +  this.dnses = [];
> > +  this.httpProxyHost = "";
> 
> I think we could merge |dnses| and |httpProxyHost| into one and have a
> generic name for it, e.g. `li
> nkRoutes`.

Yes, we could. Will update it in the next version.

> 
> @@ +138,5 @@
> > +{
> > +  this.dnses = [];
> > +  this.httpProxyHost = "";
> > +  this.gateways = [];
> > +  this.name = "";
> 
> s/name/interfaceName/

Will do.

> 
> @@ +139,5 @@
> > +  this.dnses = [];
> > +  this.httpProxyHost = "";
> > +  this.gateways = [];
> > +  this.name = "";
> > +  this.extraRoutes = [];
> 
> Maybe we could just call |resetLinks()| for initialization.

Yes, we could. Will update it in the next version.

> 
> @@ +148,5 @@
> > +  gateways: null,
> > +  name: null,
> > +  extraRoutes: null,
> > +
> > +  getLinks: function() {
> 
> `getLinks` seems not a proper naming. It is easy to get confused because it
> doesn't return all link information, but only dnses + httpProxyHost. Could
> you help to rename it?

Sure.

> 
> @@ +380,5 @@
> >          // Add host route for data calls
> >          if (this.isNetworkTypeMobile(network.type)) {
> > +          // If gateways have changed, remove all old routes first.
> > +          this._handleGateways(networkId, network.getGateways())
> > +          .then(() => {
> 
> nit: indention.
> 
> @@ +472,5 @@
> >      if (!(networkId in this.networkInterfaces)) {
> >        throw Components.Exception("No network with that type registered.",
> >                                   Cr.NS_ERROR_INVALID_ARG);
> >      }
> > +#ifdef MOZ_B2G_RIL
> 
> Let's fix bug 1054148 first. ;)
> 
> @@ +473,5 @@
> >        throw Components.Exception("No network with that type registered.",
> >                                   Cr.NS_ERROR_INVALID_ARG);
> >      }
> > +#ifdef MOZ_B2G_RIL
> > +        // This is for in case a network gets unregistered without being
> 
> nit: indention
> 
> @@ +537,5 @@
> > +      return;
> > +    }
> > +
> > +    this._setHostRoutes(false, removedLinks, interfaceName, gateways)
> > +    .then(this._setHostRoutes(true, addedLinks, interfaceName, gateways));
> 
> nit: indention.
> 
> @@ +576,5 @@
> > +      .then((ipAddresses) => {
> > +        let networkId = this.getNetworkId(network);
> > +
> > +        this.networkInterfaceLinks[networkId].extraRoutes =
> > +          this.networkInterfaceLinks[networkId].extraRoutes.concat(ipAddresses);
> 
> If the host route can not be added successfully, we should not add it into
> |extraRoutes|.
> We should do the same thing for linkRoute (dnses + httpProxyHost) as well, I
> think.

Actually, it would still work fine if we don't set it according to the result, cause we are not filtering routes that are already in 'extraRoutes' (or dsnses/httProxyHost), so the only problem would be later when removing a non-existing route, which does not harm. A bigger problem is that it would be weird to return error, in unsuccesful cases, but still add it to 'extraRoutes'. So, I will correct this part.

> 
> @@ +601,5 @@
> > +            this.networkInterfaceLinks[networkId].extraRoutes.splice(found, 1);
> > +          }
> > +        });
> > +
> > +        this._setHostRoutes(false,
> 
> Remove the route only if it was found in |extraRoutes|.

Okay.

> 
> @@ +635,5 @@
> >    },
> >  
> >  #ifdef MOZ_B2G_RIL
> > +  _handleGateways: function(networkId, gateways) {
> > +    if (this.networkInterfaceLinks[networkId].gateways.length == 0 ||
> 
> Should we clear route if the new gateways is empty?

We could. But I was thinking about gateway changes.. If gateways have changed, we clear all the link routes (dnses and proxy), should we clear extra routes as well? if so, we'll need to set the new extra routes, right? And should we accept if the new gateway is empty? cause nothing will work without a gateway..

> 
> ::: dom/system/gonk/NetworkService.js
> @@ +334,5 @@
> >      });
> >      return deferred.promise;
> >    },
> >  
> > +  routeToString: function(interfaceName, gateway, host) {
> 
> Add '_' prefix, s/routeToString/_routeToString/

Sure.

> 
> @@ +354,1 @@
> >      return this._setHostRoute(true, interfaceName, gateway, host);
> 
> One question just came to my mind, how should we handle the the reference
> counting if the route operation returns error for some reason?
> And per offline discussion, it seems we need to queue all the request and
> process them one by one to make sure the reference counting and routing
> could be maintained correctly, including error case.

Yes, a queue is needed to handle the reference counting properly. Working on it...

> 
> @@ +359,5 @@
> > +    let count = this.hostRoutes.get(route);
> > +
> > +    if (DEBUG) debug("removeHostRoute: " + route + " -> " + count);
> > +
> > +    if (count == undefined) {
> 
> I think `if (!count)` should be okay.

Yes, will update it in the next version.

Comment 12

[Jessica Jong [:jessica] (inactive)]

Attachment: proposed patch, v3.

Comment 13

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8541549 [details] [diff] [review]
proposed patch, v3.

Changes since v2:
- Added a queue in NetworkService to handle route ref count properly
- Handle 'dnses' and 'httpProxyHost' together as 'linkRoutes'
- Re-add extra routes with new gateway when gateway has been changed
- Add to linkRoutes/extraRoutes only if the routes were sucesfully set
- rebased after bug 1054148 

Edgar, may I have your review again? As to your question "Should we clear route if the new gateways is empty?", we will cause compareGateways() will return false.

Comment 14

[Edgar Chen [:edgar]]

Comment on attachment 8541549 [details] [diff] [review]
proposed patch, v3.

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

Thanks for the patch, Jessica. Please see my comments below.

::: dom/system/gonk/NetworkManager.js
@@ +156,5 @@
> +    this.interfaceName = "";
> +    this.extraRoutes = [];
> +  },
> +
> +  appendExtraRoute: function(extraRoute) {

s/appendExtraRoute/appendExtraRoutes/
s/extraRoute/extraRoutes/

@@ +551,5 @@
> +
> +        ipAddresses.forEach((aIpAddress) => {
> +          let promise =
> +            this._setHostRoutes(true, [aIpAddress], network.name, network.getGateways())
> +              .then(() => this.networkInterfaceLinks[networkId].appendExtraRoute(aIpAddress));

Now we handle |ipAddresses| one by one, so I think we could just use push() to add extra route, instead of using concat().

@@ +640,5 @@
> +                               hostRoutes,
> +                               currentNetworkLinks.interfaceName,
> +                               currentNetworkLinks.gateways)
> +      .catch((aError) => {
> +        debug("Error on _cleanupAllHostRoutes, keep proceeding");

Do we need to show |aErrror| in debug message?

::: dom/system/gonk/NetworkService.js
@@ +60,5 @@
>  }
>  
> +function Task(command, callback) {
> +  this.command = command;
> +  this.callback = callback;

`callback` is more like used for passing the task result back.
How about using `task` or something else for the naming?

@@ +89,5 @@
> +
> +  dequeue: function(command) {
> +    if (DEBUG) debug("dequeue: " + command);
> +    let task = this.tasks.shift();
> +    if (!task || task.command != command) {

The |task.command| here looks like an identifier to me.
How about using a unique id for each task? Maybe an increasing number or UUID.

@@ +408,5 @@
> +
> +      this._setHostRoute(true, interfaceName, gateway, host)
> +        .then(() => {
> +          this.hostRoutes.set(route, 1);
> +          this.netWorkerRequestQueue.dequeue(CMD_ADD_HOST_ROUTE);

Is it possible to handle the dequeue things in |netWorkerRequestQueue|?
I guess the biggest problem is the task here is asynced and I don't have any good idea now.

@@ +411,5 @@
> +          this.hostRoutes.set(route, 1);
> +          this.netWorkerRequestQueue.dequeue(CMD_ADD_HOST_ROUTE);
> +          return deferred.resolve();
> +        }, () => {
> +          this.netWorkerRequestQueue.dequeue(CMD_ADD_HOST_ROUTE);

Ditto.

@@ +412,5 @@
> +          this.netWorkerRequestQueue.dequeue(CMD_ADD_HOST_ROUTE);
> +          return deferred.resolve();
> +        }, () => {
> +          this.netWorkerRequestQueue.dequeue(CMD_ADD_HOST_ROUTE);
> +          return deferred.reject();

The promise of _setHostRoute() rejects with `reason`, should we propagate it?

@@ +446,5 @@
> +        }, () => {
> +          // We should remove it even if the operation failed.
> +          this.hostRoutes.delete(route);
> +          this.netWorkerRequestQueue.dequeue(CMD_REMOVE_HOST_ROUTE);
> +          return deferred.reject();

Ditto

Comment 15

[Jessica Jong [:jessica] (inactive)]

Attachment: proposed patch, v4.

Per offline discussion, handle all queue stuff in a same place, (|controlMessage| and |handleWorkerMessage|).
Also addressed other review comments in comment 14.

Edgar, may I have your review again? Thanks.

Comment 16

[Edgar Chen [:edgar]]

Comment on attachment 8545788 [details] [diff] [review]
proposed patch, v4.

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

Please see my comments below. And I'd like to see the rebased version for bug 1107303. ;)
Thank you.

::: dom/system/gonk/NetworkManager.js
@@ -323,5 @@
>      switch (network.state) {
>        case Ci.nsINetworkInterface.NETWORK_STATE_CONNECTED:
>          // Add host route for data calls
>          if (this.isNetworkTypeMobile(network.type)) {
> -          gNetworkService.removeHostRoutes(network.name);

It seems no one calls |removeHostRoutes| any more, if so, could you also help to remove |NetworkService.removeHostRoutes|? Thank you.

@@ +501,5 @@
> +    let addedLinks = getDifference(newLinks, oldLinks);
> +    let removedLinks = getDifference(oldLinks, newLinks);
> +
> +    if (addedLinks.length === 0 && removedLinks.length === 0) {
> +      return;

return Promise.resolve();

@@ +504,5 @@
> +    if (addedLinks.length === 0 && removedLinks.length === 0) {
> +      return;
> +    }
> +
> +    this._setHostRoutes(false, removedLinks, interfaceName, gateways)

return this._setHostRoutes(....

::: dom/system/gonk/NetworkService.js
@@ +80,5 @@
> +      ret = task.setupFunction();
> +    }
> +
> +    if (!ret) {
> +      this.networkService.handleWorkerMessage({id: task.id});

I am wondering does this have any chance to cause any unexpected behaviour because |handleWorkerMessage| will trigger the callback and we only carry `id` in the response data.

@@ +398,5 @@
> +      if (DEBUG) debug(command + ": " + route + " -> " + count);
> +
> +      // Return false if there is no need to send the command to network worker.
> +      if ((doAdd && count) ||
> +          (!doAdd && !count || count > 1)) {

Nit: the preference of && is higher than ||, but I guess what you wanna check is `!doAdd && (!count || count > 1)`.
     If so, I suggest add () for || operation though the result of whole logic seems correct.

@@ +414,5 @@
>        ifname: interfaceName,
>        gateway: gateway,
>        ip: host
>      };
>      this.controlMessage(options, function(data) {

Use arrow function for the callback, otherwise you will get an undefined |this|.

Comment 17

[Jessica Jong [:jessica] (inactive)]

(In reply to Edgar Chen [:edgar][:echen] from comment #16)
> Comment on attachment 8545788 [details] [diff] [review]
> proposed patch, v4.
> 
> Review of attachment 8545788 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please see my comments below. And I'd like to see the rebased version for
> bug 1107303. ;)
> Thank you.
> 
> ::: dom/system/gonk/NetworkManager.js
> @@ -323,5 @@
> >      switch (network.state) {
> >        case Ci.nsINetworkInterface.NETWORK_STATE_CONNECTED:
> >          // Add host route for data calls
> >          if (this.isNetworkTypeMobile(network.type)) {
> > -          gNetworkService.removeHostRoutes(network.name);
> 
> It seems no one calls |removeHostRoutes| any more, if so, could you also
> help to remove |NetworkService.removeHostRoutes|? Thank you.

Will do.

> 
> @@ +501,5 @@
> > +    let addedLinks = getDifference(newLinks, oldLinks);
> > +    let removedLinks = getDifference(oldLinks, newLinks);
> > +
> > +    if (addedLinks.length === 0 && removedLinks.length === 0) {
> > +      return;
> 
> return Promise.resolve();
> 
> @@ +504,5 @@
> > +    if (addedLinks.length === 0 && removedLinks.length === 0) {
> > +      return;
> > +    }
> > +
> > +    this._setHostRoutes(false, removedLinks, interfaceName, gateways)
> 
> return this._setHostRoutes(....

Thanks for catching this and above.

> 
> ::: dom/system/gonk/NetworkService.js
> @@ +80,5 @@
> > +      ret = task.setupFunction();
> > +    }
> > +
> > +    if (!ret) {
> > +      this.networkService.handleWorkerMessage({id: task.id});
> 
> I am wondering does this have any chance to cause any unexpected behaviour
> because |handleWorkerMessage| will trigger the callback and we only carry
> `id` in the response data.

Mm, looks okay for now, since |handleWorkerMessage| only uses |id| to look for the callback, and the callback of addHostRoute/removeHostRoute only check if |data.error| exists to determine success/failure. Or is there a better way to handle this? Are we able to use NetworkResultOptions?

> 
> @@ +398,5 @@
> > +      if (DEBUG) debug(command + ": " + route + " -> " + count);
> > +
> > +      // Return false if there is no need to send the command to network worker.
> > +      if ((doAdd && count) ||
> > +          (!doAdd && !count || count > 1)) {
> 
> Nit: the preference of && is higher than ||, but I guess what you wanna
> check is `!doAdd && (!count || count > 1)`.
>      If so, I suggest add () for || operation though the result of whole
> logic seems correct.

You are right, thanks.

> 
> @@ +414,5 @@
> >        ifname: interfaceName,
> >        gateway: gateway,
> >        ip: host
> >      };
> >      this.controlMessage(options, function(data) {
> 
> Use arrow function for the callback, otherwise you will get an undefined
> |this|.

Will do, strange that it works now.


Thanks Edgar, I'll provide a rebased patch after bug 1104664.

Comment 18

[Edgar Chen [:edgar]]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #17)
> (In reply to Edgar Chen [:edgar][:echen] from comment #16)
> > Comment on attachment 8545788 [details] [diff] [review]
> > proposed patch, v4.
> > 
> > Review of attachment 8545788 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/system/gonk/NetworkService.js
> > @@ +80,5 @@
> > > +      ret = task.setupFunction();
> > > +    }
> > > +
> > > +    if (!ret) {
> > > +      this.networkService.handleWorkerMessage({id: task.id});
> > 
> > I am wondering does this have any chance to cause any unexpected behaviour
> > because |handleWorkerMessage| will trigger the callback and we only carry
> > `id` in the response data.
> 
> Mm, looks okay for now, since |handleWorkerMessage| only uses |id| to look
> for the callback, and the callback of addHostRoute/removeHostRoute only
> check if |data.error| exists to determine success/failure. Or is there a
> better way to handle this? Are we able to use NetworkResultOptions?

I don't have better idea yet. :p
Since this is used for addHostRoute/removeHostRoute only, I am fine with current setup, but please help to add some comments about this. Thank you.

Comment 19

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: add reference counting to added routes, v5.

Addressed review comment 16 and rebased after bug 1107303.

Edgar, may I have your review again? Thanks.

Comment 20

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: remove 'removeHostRoutes()' as it is no longer needed, v1.

Removed 'removeHostRoutes()'.

Comment 21

[Edgar Chen [:edgar]]

Comment on attachment 8556958 [details] [diff] [review]
Part 1: add reference counting to added routes, v5.

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

Looks good to me, but I would like to do some test in real devices before giving r+. Thank you.

::: dom/system/gonk/NetworkService.js
@@ +82,5 @@
> +
> +    // If setupFunction returns false, skip sending to Network Worker but call
> +    // handleWorkerMessage() directly with task id as if the response was
> +    // returned from Network Worker.
> +    if (!ret) {

It seems we don't need |ret|, maybe we could just put it in `if (typeof task.setupFunction === 'function')` block.

e.g.

if (typeof task.setupFunction === 'function') {
  //  If setupFunction returns false, ....
  if (task.setupFunction()) {
    this.networkService.handleWorkerMessage({id: task.id});
    return;
  }
}

Comment 22

[Edgar Chen [:edgar]]

Comment on attachment 8556959 [details] [diff] [review]
Part 2: remove 'removeHostRoutes()' as it is no longer needed, v1.

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

Thank you.

Comment 23

[Edgar Chen [:edgar]]

Comment on attachment 8556958 [details] [diff] [review]
Part 1: add reference counting to added routes, v5.

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

It works good on nexus-l and flame-kk. Thank you, Jessica.

Comment 24

[Jessica Jong [:jessica] (inactive)]

Attachment: [final] Part 1: add reference counting to added routes. r= echen

Address review comment 21:
- no need for |ret|, do the operation inside the |if| statement

Thanks a bunch, Edgar, for the review!

Comment 25

[Jessica Jong [:jessica] (inactive)]

Attachment: [final] Part 2: remove 'removeHostRoutes()' as it is no longer needed. r=echen

rebased.

Comment 26

[Jessica Jong [:jessica] (inactive)]

try looks good!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2756a44266a4

Comment 27

[Jessica Jong [:jessica] (inactive)]

https://hg.mozilla.org/integration/b2g-inbound/rev/a75fca8e0b9d
https://hg.mozilla.org/integration/b2g-inbound/rev/fe2b1047b5c8

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a75fca8e0b9d
https://hg.mozilla.org/mozilla-central/rev/fe2b1047b5c8

Comment 29

[Jessica Jong [:jessica] (inactive)]

I'd like to uplift this to v2.2r, to be able to handle host routes properly, e.g. not removed accidentally by other modules/apps. See also: bug 1179097.

Comment 30

[Jessica Jong [:jessica] (inactive)]

Attachment: [v2.2r] Part 1: add reference counting to added routes.

Comment 31

[Jessica Jong [:jessica] (inactive)]

Attachment: [v2.2r] Part 2: remove 'removeHostRoutes()' as it is no longer needed.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/608de6dfa1dd
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/9d6a45e199a1