Last Comment Bug 753612 - [b2g] properly close wifi connection so 3G-data can get its own route
: [b2g] properly close wifi connection so 3G-data can get its own route
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: mozilla15
Assigned To: Fabien Cazenave [:kaze]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 753520
Blocks: b2g-wifi b2g-3g
  Show dependency treegraph
 
Reported: 2012-05-09 17:33 PDT by Fabien Cazenave [:kaze]
Modified: 2012-05-12 09:02 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch proposal (1.67 KB, patch)
2012-05-09 17:33 PDT, Fabien Cazenave [:kaze]
philipp: review+
mrbkap: review+
Details | Diff | Splinter Review
patch proposal (4.52 KB, patch)
2012-05-10 16:45 PDT, Fabien Cazenave [:kaze]
philipp: review+
Details | Diff | Splinter Review
patch proposal (4.46 KB, patch)
2012-05-10 17:58 PDT, Fabien Cazenave [:kaze]
mounir: checkin+
Details | Diff | Splinter Review

Description Fabien Cazenave [:kaze] 2012-05-09 17:33:38 PDT
Created attachment 622591 [details] [diff] [review]
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] 2012-05-09 17:35:38 PDT
PS: bug 753520 should be landed before 3G connection really works.
Comment 2 Blake Kaplan (:mrbkap) (PTO until Jan. 2, 2017) 2012-05-10 08:40:46 PDT
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] 2012-05-10 09:13:53 PDT
Comment on attachment 622591 [details] [diff] [review]
patch proposal

I think mrbkap should review the Wifi changes.
Comment 4 Blake Kaplan (:mrbkap) (PTO until Jan. 2, 2017) 2012-05-10 10:10:35 PDT
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] 2012-05-10 15:08:25 PDT
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] 2012-05-10 16:45:20 PDT
Created attachment 622969 [details] [diff] [review]
patch proposal

Now properly removing old routes and updating the default route when necessary.
Comment 7 Fabien Cazenave [:kaze] 2012-05-10 16:47:21 PDT
Warning: bug 753520 *still* has to be landed before 3G connection really works.
Comment 8 Philipp von Weitershausen [:philikon] 2012-05-10 16:56:29 PDT
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] 2012-05-10 17:58:42 PDT
Created attachment 622997 [details] [diff] [review]
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] 2012-05-10 18:53:14 PDT
follow-up: bug 754152
Comment 11 Philipp von Weitershausen [:philikon] 2012-05-12 07:56:28 PDT
Thanks for landing this, mounir! Here's the cset:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e445fc14a38b
Comment 12 Matt Brubeck (:mbrubeck) 2012-05-12 09:02:48 PDT
https://hg.mozilla.org/mozilla-central/rev/e445fc14a38b

Note You need to log in before you can comment on or make changes to this bug.