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

RESOLVED FIXED in mozilla15

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: kaze, Assigned: kaze)

Tracking

(Blocks: 1 bug)

Trunk
mozilla15
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
PS: bug 753520 should be landed before 3G connection really works.
(Assignee)

Updated

5 years ago
Attachment #622591 - Flags: review?(philipp)
Blocks: 710493
Depends on: 753520
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 on attachment 622591 [details] [diff] [review]
patch proposal

I think mrbkap should review the Wifi changes.
Attachment #622591 - Flags: review?(philipp)
Attachment #622591 - Flags: review?(mrbkap)
Attachment #622591 - Flags: review+
Blocks: 717123
Comment on attachment 622591 [details] [diff] [review]
patch proposal

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

r=me with the change mentioned above.
Attachment #622591 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 5

5 years ago
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.
(Assignee)

Comment 6

5 years ago
Created attachment 622969 [details] [diff] [review]
patch proposal

Now properly removing old routes and updating the default route when necessary.
Assignee: nobody → kaze
Attachment #622591 - Attachment is obsolete: true
Attachment #622969 - Flags: review?(philipp)
Attachment #622969 - Flags: review?(mrbkap)
(Assignee)

Comment 7

5 years ago
Warning: bug 753520 *still* has to be landed before 3G connection really works.
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
  }
Attachment #622969 - Flags: review?(philipp)
Attachment #622969 - Flags: review?(mrbkap)
Attachment #622969 - Flags: review+
(Assignee)

Comment 9

5 years ago
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.
Attachment #622969 - Attachment is obsolete: true
(Assignee)

Comment 10

5 years ago
follow-up: bug 754152
Attachment #622997 - Flags: checkin+
Target Milestone: --- → mozilla15
Thanks for landing this, mounir! Here's the cset:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e445fc14a38b
https://hg.mozilla.org/mozilla-central/rev/e445fc14a38b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.