753612 - [b2g] properly close wifi connection so 3G-data can get its own route

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Fabien Cazenave [:kaze]]

Attachment: patch proposal

Steps to reproduce (with a b2g device):
1. disable Wi-Fi
2. enable 3G-data 
3. open a page in the browser

Expected result: device should be connected to the internet
Actual result: no internet connection.

When switching to 3G, no new route is set and the DNS are still the obsolete ones used for the wifi connection. This patch should fix it.

Comment 1

[Fabien Cazenave [:kaze]]

PS: bug 753520 should be landed before 3G connection really works.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 622591 [details] [diff] [review]
patch proposal

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

::: dom/wifi/WifiWorker.js
@@ +738,4 @@
>        manager.connectionInfo.bssid = null;
>        manager.connectionInfo.ssid = null;
>        manager.connectionInfo.id = -1;
> +      notifyStateChange({ state: "DISCONNECTED", BSSID: null, id: -1 });

In passing: this change shouldn't be needed. The rest of the WifiWorker changes look good, though.

Comment 3

[Philipp von Weitershausen [:philikon]]

Comment on attachment 622591 [details] [diff] [review]
patch proposal

I think mrbkap should review the Wifi changes.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 622591 [details] [diff] [review]
patch proposal

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

r=me with the change mentioned above.

Comment 5

[Fabien Cazenave [:kaze]]

Hmm, I realize that this patch allows to set *two* routes when both 3G and wifi are enabled. Something’s terribly wrong here.

(In reply to Blake Kaplan (:mrbkap) from comment #2)
> > +      notifyStateChange({ state: "DISCONNECTED", BSSID: null, id: -1 });
> 
> In passing: this change shouldn't be needed.

I confirm it works without this change.

Comment 6

[Fabien Cazenave [:kaze]]

Attachment: patch proposal

Now properly removing old routes and updating the default route when necessary.

Comment 7

[Fabien Cazenave [:kaze]]

Warning: bug 753520 *still* has to be landed before 3G connection really works.

Comment 8

[Philipp von Weitershausen [:philikon]]

Comment on attachment 622969 [details] [diff] [review]
patch proposal

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

Nice work! Just a nit below about code flow/readability. Wifi parts haven't changed since the previous version that mrbkap already r+'ed, so I'm clearing that flag.

::: dom/system/gonk/NetworkManager.js
@@ +186,2 @@
>      if (this.active.dhcp) {
> +      options = { cmd: "runDHCPAndSetDefaultRouteAndDNS", ifname: this.active.name };

Nit: s/var/let, move `ifname` definition into `options`, assume a default for `cmd`, and don't overwrite it with a new obj:

  let options = {cmd: "setDefaultRouteAndDNS",
                 ifname: this.active.name};
  if (this.active.dhcp) {
    options.cmd = "runDHCPAndSetDefaultRouteAndDNS";
  }
  if (oldInterface) {
    options.oldIfname = oldInterface.name;
  }

or even more concise:

  let options = {
    cmd: this.active.dhcp ? "runDHCPAndSetDefaultRouteAndDNS" : "setDefaultRouteAndDNS",
    ifname: this.active.name,
    oldIfname: oldInterface ? oldInterface.name : null
  }

Comment 9

[Fabien Cazenave [:kaze]]

Attachment: patch proposal

same patch with a less ugly code.

I’ve just found out that the initial conditions might still cause issues (e.g. when the 3G is enabled at startup, it has to be disabled/enabled to work properly). I’ll fill a follow-up.

Comment 10

[Fabien Cazenave [:kaze]]

follow-up: bug 754152

Comment 11

[Philipp von Weitershausen [:philikon]]

Thanks for landing this, mounir! Here's the cset:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e445fc14a38b

Comment 12

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/e445fc14a38b