1207066 - [NetworkManager] implement nsINetworkInterface.activate()/deactivate() in wifi network interface

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Wifi, defect)

Description

[Jessica Jong [:jessica] (inactive)]

In bug 911713, we are introducing new two methods in nsINetworkInterface: activate() and deactivate(). The purpose for this is to let NetworkManager handle all networks connection as well as policy control in one same place, so NetworkManager is the one that decides which network gets activated and which gets deactivated.

Therefore, we need wifi network interface to support this two methods, and it should wait for "activate()" to be called to actually connect to wifi.

Please refer to this diagram for the current design in mind: http://goo.gl/j8TDng

Comment 1

[Ethan Tseng [:ethan]]

Henry and Tim,
We need your support on this bug.

Comment 2

[Tim Huang[:timhuang]]

Attachment: bug_1207066_v1.patch

Comment 3

[Henry Chang [:hchang]]

Comment on attachment 8699938 [details] [diff] [review]
bug_1207066_v1.patch

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

The implementation looks definitely are on the right track! What I am uncertain is the use of observer topic "profile-change-teardown".

::: dom/wifi/WifiWorker.js
@@ +1114,4 @@
>          // command blocking in control thread until socket timeout.
>          notify("stopconnectioninfotimer");
>  
> +        manager.deactivate(function() {

Should we do this on our own? I assume NetworkManager is supposed to do this properly?

@@ +1587,5 @@
>      return sdkVersion;
>    }
>  
> +  manager.activate = function(aCallback) {
> +    // If the interface has activated, return.

has been activated?

@@ +1594,5 @@
> +      return;
> +    }
> +
> +    // If the supplicant does not start when calling activate, we only set the
> +    // flag to indicate the wifi interface has activated. The rest activating

has been activated?

@@ +1603,5 @@
> +      return;
> +    }
> +
> +    wifiCommand.enableNetwork("all", false, function() {});
> +    wifiCommand.saveConfig(function() {

Should we do "saveConfing" in the callback of enableNetwork?

@@ +1933,4 @@
>  
>    Services.obs.addObserver(this, kMozSettingsChangedObserverTopic, false);
>    Services.obs.addObserver(this, "xpcom-shutdown", false);
> +  Services.obs.addObserver(this, "profile-change-teardown", false);

According to 

https://dxr.mozilla.org/mozilla-central/source/toolkit/profile/notifications.txt#9

"profile-change-teardown" is defined as the following:

  All async activity must be stopped in this phase. Typically,
  the application level observer will close all open windows.
  This is the last phase in which the subject's vetoChange()
  method may still be called.
  The next notification will be either
  profile-change-teardown-veto or profile-before-change.

I am not sure what it means to this module actually. May I know how do you find this topic and choose it? I typically use xpcom-shutdown to capture the XPCOM "will shutdown" event.

Comment 4

[Tim Huang[:timhuang]]

(In reply to Henry Chang [:henry] from comment #3)
> Comment on attachment 8699938 [details] [diff] [review]
> bug_1207066_v1.patch
> 
> Review of attachment 8699938 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The implementation looks definitely are on the right track! What I am
> uncertain is the use of observer topic "profile-change-teardown".
> 
> ::: dom/wifi/WifiWorker.js
> @@ +1114,4 @@
> >          // command blocking in control thread until socket timeout.
> >          notify("stopconnectioninfotimer");
> >  
> > +        manager.deactivate(function() {
> 
> Should we do this on our own? I assume NetworkManager is supposed to do this
> properly?

We perform a deactivate here since the wpa_supplicant is going to be terminated before we disable the wifi network interface. In other words, we cannot write the wpa_supplicant.conf at the time that network manager deactivates the wifi network interface because the wpa_supplicant had been terminated at this moment. Hence, we need to deactivate the interface before the termination of the wpa_supplicant when disabling Wifi.

> 
> @@ +1587,5 @@
> >      return sdkVersion;
> >    }
> >  
> > +  manager.activate = function(aCallback) {
> > +    // If the interface has activated, return.
> 
> has been activated?
> 
> @@ +1594,5 @@
> > +      return;
> > +    }
> > +
> > +    // If the supplicant does not start when calling activate, we only set the
> > +    // flag to indicate the wifi interface has activated. The rest activating
> 
> has been activated?
> 
> @@ +1603,5 @@
> > +      return;
> > +    }
> > +
> > +    wifiCommand.enableNetwork("all", false, function() {});
> > +    wifiCommand.saveConfig(function() {
> 
> Should we do "saveConfing" in the callback of enableNetwork?

The reason we save config here is that we want to make sure the state of networks, enable or disable, is synchronized between wpa_supplicant and wpa_supplicant.conf. Or don't we have to make sure they are sync?

> 
> @@ +1933,4 @@
> >  
> >    Services.obs.addObserver(this, kMozSettingsChangedObserverTopic, false);
> >    Services.obs.addObserver(this, "xpcom-shutdown", false);
> > +  Services.obs.addObserver(this, "profile-change-teardown", false);
> 
> According to 
> 
> https://dxr.mozilla.org/mozilla-central/source/toolkit/profile/notifications.
> txt#9
> 
> "profile-change-teardown" is defined as the following:
> 
>   All async activity must be stopped in this phase. Typically,
>   the application level observer will close all open windows.
>   This is the last phase in which the subject's vetoChange()
>   method may still be called.
>   The next notification will be either
>   profile-change-teardown-veto or profile-before-change.
> 
> I am not sure what it means to this module actually. May I know how do you
> find this topic and choose it? I typically use xpcom-shutdown to capture the
> XPCOM "will shutdown" event.

The first time that I learned this notification is from [1]. And then, I found this page[2] describes "profile-change-teardown" as a part of shutdown. And it seems that the "xpcom-shutdown" would not be called when I shutdown my aries, but "profile-change-teardown" does. So I choose this "profile-change-teardown" to deactivate wifi network.

Perhaps the network manager will take care the deactivating of network interfaces when shutdown the system, but we don't know this yet. So we put a deactivate here for making sure the interface is going to be deactivated when shutdown the system.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/alarm/AlarmService.jsm#57 
[2] https://developer.mozilla.org/en/docs/Observer_Notifications

Comment 5

[Henry Chang [:hchang]]

(In reply to Tim Huang[:timhuang] from comment #4)
> (In reply to Henry Chang [:henry] from comment #3)
> > Comment on attachment 8699938 [details] [diff] [review]
> > bug_1207066_v1.patch
> > 
> > Review of attachment 8699938 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > 
> > @@ +1587,5 @@
> > >      return sdkVersion;
> > >    }
> > >  
> > > +  manager.activate = function(aCallback) {
> > > +    // If the interface has activated, return.
> > 
> > has been activated?
> > 
> > @@ +1594,5 @@
> > > +      return;
> > > +    }
> > > +
> > > +    // If the supplicant does not start when calling activate, we only set the
> > > +    // flag to indicate the wifi interface has activated. The rest activating
> > 
> > has been activated?
> > 
> > @@ +1603,5 @@
> > > +      return;
> > > +    }
> > > +
> > > +    wifiCommand.enableNetwork("all", false, function() {});
> > > +    wifiCommand.saveConfig(function() {
> > 
> > Should we do "saveConfing" in the callback of enableNetwork?
> 
> The reason we save config here is that we want to make sure the state of
> networks, enable or disable, is synchronized between wpa_supplicant and
> wpa_supplicant.conf. Or don't we have to make sure they are sync?
> 

I meant, shouldn't it be:

wifiCommand.enableNetwork("all", false, function() {
  wifiCommand.saveConfig(function() {
});

Comment 6

[Tim Huang[:timhuang]]

(In reply to Henry Chang [:henry] from comment #5)
> (In reply to Tim Huang[:timhuang] from comment #4)
> 
> I meant, shouldn't it be:
> 
> wifiCommand.enableNetwork("all", false, function() {
>   wifiCommand.saveConfig(function() {
> });

Oh, I misunderstood your words. Yes, I agree with your suggestion that we should put saveConfig in the callback of enableNetwork.

Comment 7

[Tim Huang[:timhuang]]

Attachment: WIP: Enable the activate/deactivate feature of Wifi network interface.

Comment 8

[Tim Huang[:timhuang]]

I've been shifted to other project, leaving this bug to nobody.

Comment 9

[BMO Automation]

Firefox OS is not being worked on