1175817 - [NetworkManager] remove old default routes explicitly

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Jessica Jong [:jessica] (inactive)]

Quoted from Bug 1173671 comment 40:

"The setDefaultRoute() now supports removing default route on old interface and then setting default route on new interface.
But we already have removeDefaultRoute()[1] in nsINetworkService for removing default routing, it seems to me that the setDefaultRoute() could focus on setting default route only.
For removing default route, we could just reuse the removeDefaultRoute()."

---

We expect that the old interface that becomes 'not active' be 'disconnected' afterwards, so that it's default and host routes will be removed properly. However, if we are going to remove default routes first in setAndConfigureActive(), we should do it explicitly.

Comment 1

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

[Tracking Requested - why for this release]:

Comment 2

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v1.

Comment 3

[Jessica Jong [:jessica] (inactive)]

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

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

Comment 4

[Edgar Chen [:edgar]]

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

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

Looks good to me, thank you.

::: dom/system/gonk/NetworkManager.js
@@ +981,5 @@
>      });
>    },
>  
> +  _setDefaultRouteAndProxy: function(aNetwork, aOldNetwork) {
> +    if (aOldNetwork && aOldNetwork != aNetwork) {

I know this is from the current code, just curious why need to skip removing if |aOldNetwork == aNetwork|. Couldn't we just still do remove? It seems only results in removing and setting default route on the same interface. Besides, if aNetwork == aOldNetwork, ideally we don't have to call _setDefaultRouteAndProxy() since the network isn't changed.

Comment 5

[Jessica Jong [:jessica] (inactive)]

Thanks Edgar!

(In reply to Edgar Chen [:edgar][:echen] from comment #4)
> Comment on attachment 8656976 [details] [diff] [review]
> patch, v1.
> 
> Review of attachment 8656976 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me, thank you.
> 
> ::: dom/system/gonk/NetworkManager.js
> @@ +981,5 @@
> >      });
> >    },
> >  
> > +  _setDefaultRouteAndProxy: function(aNetwork, aOldNetwork) {
> > +    if (aOldNetwork && aOldNetwork != aNetwork) {
> 
> I know this is from the current code, just curious why need to skip removing
> if |aOldNetwork == aNetwork|. Couldn't we just still do remove? It seems
> only results in removing and setting default route on the same interface.
> Besides, if aNetwork == aOldNetwork, ideally we don't have to call
> _setDefaultRouteAndProxy() since the network isn't changed.

At first, I returned directly in _setDefaultRouteAndProxy() if aNetwork == aOldNetwork, but it causes some problem, since the first thing we do when a network is connected, is to remove its default route [1]. So if we don't add it back here, there is a chance we'll be left with no default route.

We could skip the check and do removing and adding though, it may result in socket reconnection, but since we already remove default route on [1], it doesn't matter if we remove it or not here. Unless we remove [1] part, and do the all the checking/comparing here.

What do you think?


[1] http://hg.mozilla.org/mozilla-central/file/dd2a1d737a64/dom/system/gonk/NetworkManager.js#l339

Comment 6

[Edgar Chen [:edgar]]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #5)
> Thanks Edgar!
> 
> (In reply to Edgar Chen [:edgar][:echen] from comment #4)
> > Comment on attachment 8656976 [details] [diff] [review]
> > patch, v1.
> > 
> > Review of attachment 8656976 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looks good to me, thank you.
> > 
> > ::: dom/system/gonk/NetworkManager.js
> > @@ +981,5 @@
> > >      });
> > >    },
> > >  
> > > +  _setDefaultRouteAndProxy: function(aNetwork, aOldNetwork) {
> > > +    if (aOldNetwork && aOldNetwork != aNetwork) {
> > 
> > I know this is from the current code, just curious why need to skip removing
> > if |aOldNetwork == aNetwork|. Couldn't we just still do remove? It seems
> > only results in removing and setting default route on the same interface.
> > Besides, if aNetwork == aOldNetwork, ideally we don't have to call
> > _setDefaultRouteAndProxy() since the network isn't changed.
> 
> At first, I returned directly in _setDefaultRouteAndProxy() if aNetwork ==
> aOldNetwork, but it causes some problem, since the first thing we do when a
> network is connected, is to remove its default route [1]. So if we don't add
> it back here, there is a chance we'll be left with no default route.
> 
> We could skip the check and do removing and adding though, it may result in
> socket reconnection, but since we already remove default route on [1], it
> doesn't matter if we remove it or not here. Unless we remove [1] part, and
> do the all the checking/comparing here.
> 
> What do you think?

If skipping the check doesn't hurt anything, I prefer removing it. :)
Thanks for this information.

> 
> 
> [1]
> http://hg.mozilla.org/mozilla-central/file/dd2a1d737a64/dom/system/gonk/
> NetworkManager.js#l339

Comment 7

[Jessica Jong [:jessica] (inactive)]

(In reply to Edgar Chen [:edgar][:echen] from comment #6)
> (In reply to Jessica Jong [:jjong] [:jessica] from comment #5)
> > Thanks Edgar!
> > 
> > (In reply to Edgar Chen [:edgar][:echen] from comment #4)
> > > Comment on attachment 8656976 [details] [diff] [review]
> > > patch, v1.
> > > 
> > > Review of attachment 8656976 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Looks good to me, thank you.
> > > 
> > > ::: dom/system/gonk/NetworkManager.js
> > > @@ +981,5 @@
> > > >      });
> > > >    },
> > > >  
> > > > +  _setDefaultRouteAndProxy: function(aNetwork, aOldNetwork) {
> > > > +    if (aOldNetwork && aOldNetwork != aNetwork) {
> > > 
> > > I know this is from the current code, just curious why need to skip removing
> > > if |aOldNetwork == aNetwork|. Couldn't we just still do remove? It seems
> > > only results in removing and setting default route on the same interface.
> > > Besides, if aNetwork == aOldNetwork, ideally we don't have to call
> > > _setDefaultRouteAndProxy() since the network isn't changed.
> > 
> > At first, I returned directly in _setDefaultRouteAndProxy() if aNetwork ==
> > aOldNetwork, but it causes some problem, since the first thing we do when a
> > network is connected, is to remove its default route [1]. So if we don't add
> > it back here, there is a chance we'll be left with no default route.
> > 
> > We could skip the check and do removing and adding though, it may result in
> > socket reconnection, but since we already remove default route on [1], it
> > doesn't matter if we remove it or not here. Unless we remove [1] part, and
> > do the all the checking/comparing here.
> > 
> > What do you think?
> 
> If skipping the check doesn't hurt anything, I prefer removing it. :)
> Thanks for this information.
> 

Sure! Will update this on the next patch.

> > 
> > 
> > [1]
> > http://hg.mozilla.org/mozilla-central/file/dd2a1d737a64/dom/system/gonk/
> > NetworkManager.js#l339

Comment 8

[Jessica Jong [:jessica] (inactive)]

Attachment: patch. r=echen

- Skip check for comparing old and new network, so always do remove default route for old network.

Comment 9

[Jessica Jong [:jessica] (inactive)]

try looks good!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=699feb6d9d35&group_state=expanded&exclusion_profile=false

Comment 10

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8659743 [details] [diff] [review]
patch. r=echen

Hi Olli, changes to WebIDL files require review from a DOM peer, would you mind reviewing the NetworkOptions.webidl part? I'm just removing an option that is not used anymore.

Thanks.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8659743 [details] [diff] [review]
patch. r=echen

sure.

Comment 12

[Jessica Jong [:jessica] (inactive)]

Attachment: patch. r=echen,smaug

Thanks Olli!

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/249a0976312d

Comment 14

[Jessica Jong [:jessica] (inactive)]

This was merged in m-c:
https://hg.mozilla.org/mozilla-central/rev/249a0976312d