778360 - B2G Wifi: ensure NetworkManager state is correct after restarting the b2g process

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Hubert Figuiere [:hub]]

On restart of b2g process, NetworkManager believe wlan is disonnected.

See bug 777145 for more details.

On phone startup, it works. After disabling and reenabling wifi it works. But if b2g only is restarted it will never get the notification.

This is on a Samsung Galaxy S2 (ICS)

Comment 1

[Philipp von Weitershausen [:philikon]]

We need to fix this on the Wifi end. The simple nuke-the-site-from-orbit (only way to be sure) solution would be to shutdown Wifi and re-enable it whenever B2G starts up. We do this with the cellular radio (bug 775375) and I believe that's how Android does it, too.

Comment 2

[Hubert Figuiere [:hub]]

I'll have a look at this since it is blocking me.

Comment 3

[Hubert Figuiere [:hub]]

Attachment: Bug 778360 - Nuke the wifi connection on b2g startup.

This has been tested on SGS2 only.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 650742 [details] [diff] [review]
Bug 778360 - Nuke the wifi connection on b2g startup.

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

Some questions below.

::: dom/wifi/WifiWorker.js
@@ +64,4 @@
>    var controlWorker = new ChromeWorker(WIFIWORKER_WORKER);
>    var eventWorker = new ChromeWorker(WIFIWORKER_WORKER);
>  
> +  var _isInitialState = true;

Rather than do this, it seems like we could simply do this work when the wifi component is instantiated the first time. Right now it's a little tricky, since you don't want to race with an external request to turn on/off wifi.

@@ +885,5 @@
>    function prepareForStartup(callback) {
> +
> +    debug("prepareForStartup");
> +
> +    if(_isInitialState) {

What's with the crazy double-spacing here?

@@ +958,4 @@
>    // Public interface of the wifi service
>    manager.setWifiEnabled = function(enable, callback) {
> +
> +    debug("setWifiEnabled - state requested " + enable);

Here and below, why all the extra newlines?

Comment 5

[Hubert Figuiere [:hub]]

Comment on attachment 650742 [details] [diff] [review]
Bug 778360 - Nuke the wifi connection on b2g startup.

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

::: dom/wifi/WifiWorker.js
@@ +885,5 @@
>    function prepareForStartup(callback) {
> +
> +    debug("prepareForStartup");
> +
> +    if(_isInitialState) {

Habits from the a11y module owners. I'll fix them.

Comment 6

[Hubert Figuiere [:hub]]

Comment on attachment 650742 [details] [diff] [review]
Bug 778360 - Nuke the wifi connection on b2g startup.

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

::: dom/wifi/WifiWorker.js
@@ +64,4 @@
>    var controlWorker = new ChromeWorker(WIFIWORKER_WORKER);
>    var eventWorker = new ChromeWorker(WIFIWORKER_WORKER);
>  
> +  var _isInitialState = true;

I currently won't race with an external request. prepareForStartup is only called from the setter/getter of the enabled state, which is called when the worker is started.

Also I haven't found a more adequate place to put it with everything properly initialised.

Comment 7

[Hubert Figuiere [:hub]]

Attachment: Bug 778360 - Nuke the wifi connection on b2g startup.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

I will get to this review tomorrow.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 653836 [details] [diff] [review]
Bug 778360 - Nuke the wifi connection on b2g startup.

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

There's still a bunch of cleanup to do here.

::: dom/wifi/WifiWorker.js
@@ +903,5 @@
>    }
>  
>    function prepareForStartup(callback) {
> +    debug("prepareForStartup");
> +    if(_isInitialState) {

If we do this in the WifiWorker constructor (where we enable wifi on startup) we can avoid _isInitialState.

@@ +905,5 @@
>    function prepareForStartup(callback) {
> +    debug("prepareForStartup");
> +    if(_isInitialState) {
> +      debug("nuking the existing connection as we restart");
> +      disableWifi( function() { });

Doing this here makes the rest of this function obsolete, that means there's a bunch of code that can be ripped out :)

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: More complete patch

This patch simply gets rid of the code to reconnect to an already running supplicant. It also tries to make sure that the networking subsystem for wlan0 is properly down. Hub, this is slightly different than the patch that you tried out yesterday, so I'd appreciate a final confirmation that this still works. Sorry for the delay in getting to this.

Comment 11

[Hubert Figuiere [:hub]]

Comment on attachment 656855 [details] [diff] [review]
More complete patch

I tested this patch, and it works. Thanks !

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

https://hg.mozilla.org/mozilla-central/rev/246fdea0cf7e