912015 - Lack of wpa_supplicant multi-connection support

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Henry Chang [:hchang]]

We now use "wlan0" as the only one interface that wpa_supplicant connects to ( See dom/wifi/WifiUtils.cpp) However, for some cases we need multiple connection. Precisely speaking, we have to connect to "p2p0" aside from "wlan0" for the new feature "wifi direct". My tentative solution is:

1. Add the support of multiple listener in nsIWifiProxyService::start() or add new functions like nsIWifiProxyService::addListener(in nsIWifiEventListener listener, in AString interface)
2. Modify nsIWifiProxyService::sendCommand to sendCommand(in jsval parameters, in AString interface) 
3. Modfiy nsIWifiProxyService::waitForEvent to waitForEvent(in AString interface)

I will attach a patch to include these changes and ask for comments later.

Comment 1

[Henry Chang [:hchang]]

Attachment: tentative solution (interface part)

Comment 2

[Henry Chang [:hchang]]

If these changes are good enough, I will be working on the corresponding modification on WifiWorker.js, WifiUtils.cpp. Thanks.

Comment 3

[[:fabrice] Fabrice Desré]

Comment on attachment 798842 [details] [diff] [review]
tentative solution (interface part)

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

Maybe you will also need to change onWaitEvent and onCommand in nsIWifiEventListener to pass the interface back, so you'll be able to use the same listener for different interfaces.

::: dom/wifi/nsIWifiService.idl
@@ +7,1 @@
>  [scriptable, uuid(e3d54c8d-b1c7-4ed5-b760-e26d3075e3e5)]

nit: update the uuid

Comment 4

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 798842 [details] [diff] [review]
tentative solution (interface part)

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

Add interface to onWaitEvent and onCommand is a good idea to me. So that we may have two state machine, one is for wifi(wlan0) and the other is for wifi direct(p2p_wlan0).

Comment 5

[Henry Chang [:hchang]]

Attachment: Bug-912015.patch

Comment 6

[[:fabrice] Fabrice Desré]

Comment on attachment 799389 [details] [diff] [review]
Bug-912015.patch

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

Looks sane to me, but we need Blake to review.

::: dom/wifi/WifiProxyService.cpp
@@ +29,5 @@
>  class WifiEventDispatcher : public nsRunnable
>  {
>  public:
> +  WifiEventDispatcher(const nsAString& aEvent, const nsACString& aInterface)
> +    : mEvent(aEvent), mInterface(aInterface)

We usually indent like that to:
 : mEvent(aEvent)
 , mInterface(aInterface)

@@ +127,5 @@
>  class ControlRunnable : public nsRunnable
>  {
>  public:
> +  ControlRunnable(CommandOptions aOptions, const nsACString& aInterface) 
> +    : mOptions(aOptions), mInterface(aInterface)

here also, trailing whitespace and indentation

@@ +214,2 @@
>    }
> +  

trailing ws.

@@ +217,5 @@
>    if (NS_FAILED(rv)) {
>      NS_WARNING("Can't create wifi control thread");
> +
> +    // Shutdown the event threads.
> +    for(size_t i = 0; i < mEventThreadList.size(); i++)

you could use a <vector> iterator (though the code here is simple enough to not gain much).

@@ +246,5 @@
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
> +WifiProxyService::SendCommand(const JS::Value& aOptions, const nsACString& aInterface, 

trailing ws.

::: dom/wifi/WifiProxyService.h
@@ +30,5 @@
>    static already_AddRefed<WifiProxyService>
>    FactoryCreate();
>  
> +  void DispatchWifiEvent(const nsAString& aEvent, const nsACString& aInterface);
> +  void DispatchWifiResult(const mozilla::dom::WifiResultOptions& aOptions, 

not: trailing whitespace.

::: dom/wifi/WifiWorker.js
@@ +107,5 @@
>  
>    let wifiListener = {
> +    onWaitEvent: function(event, iface) {
> +      if (manager.ifname === iface) {
> +        if (handleEvent(event)) {

Could be if (manager.ifname && handleEvent(event))

::: dom/wifi/nsIWifiService.idl
@@ +14,3 @@
>  interface nsIWifiProxyService : nsISupports {
> +  void start(in nsIWifiEventListener listener,
> +             [array, size_is(aNumOfInterface)] in string aInterfaces, 

nit: trailing whitespace
also, I think you also want ACstring here like for other methods.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

FYI, I'm not going to get to this review until after I get back from vacation.

Comment 8

[Henry Chang [:hchang]]

(In reply to Fabrice Desré [:fabrice] from comment #6)
> Comment on attachment 799389 [details] [diff] [review]
> Bug-912015.patch
> 
> Review of attachment 799389 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks sane to me, but we need Blake to review.
> 
> ::: dom/wifi/WifiWorker.js
> @@ +107,5 @@
> >  
> >    let wifiListener = {
> > +    onWaitEvent: function(event, iface) {
> > +      if (manager.ifname === iface) {
> > +        if (handleEvent(event)) {
> 
> Could be if (manager.ifname && handleEvent(event))

Did you mean "if (manager.ifname === iface && handleEvent(event))" ?

> ::: dom/wifi/nsIWifiService.idl
> @@ +14,3 @@
> >  interface nsIWifiProxyService : nsISupports {
> > +  void start(in nsIWifiEventListener listener,
> > +             [array, size_is(aNumOfInterface)] in string aInterfaces, 
> 
> nit: trailing whitespace
> also, I think you also want ACstring here like for other methods.

Using "array of ACString" here generates invalid C++ argument type:
"const ACString& *aInterface". I think it's a bug of xpidl...

Comment 9

[[:fabrice] Fabrice Desré]

(In reply to Henry Chang [:henry] from comment #8)

> > 
> > Could be if (manager.ifname && handleEvent(event))
> 
> Did you mean "if (manager.ifname === iface && handleEvent(event))" ?

Yes, sorry.


> Using "array of ACString" here generates invalid C++ argument type:
> "const ACString& *aInterface". I think it's a bug of xpidl...

ha, so let's keep string.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 799389 [details] [diff] [review]
Bug-912015.patch

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

I have a few comments to address.

::: dom/wifi/WifiProxyService.cpp
@@ +24,1 @@
>  

It isn't in the patch, but would you mind marking gWifiProxyService as |static| as was intended?

@@ +183,5 @@
>  
>  NS_IMETHODIMP
> +WifiProxyService::Start(nsIWifiEventListener* aListener,
> +                        const char ** aInterfaces,
> +                        uint32_t aNumOfInterface)

aNumOfInterfaces, please.

@@ +190,5 @@
>    MOZ_ASSERT(aListener);
>  
> +  nsresult rv;
> +
> +  NS_WARNING("Running WifiProxyService::Start");

Please get rid of this before checking in.

@@ +197,5 @@
> +  // spin a thread for each interface.
> +  // (See the WpaSupplicant::WaitForEvent)
> +  mEventThreadList.resize(aNumOfInterface);
> +  for(uint32_t i = 0; i < aNumOfInterface; i++)
> +  {

Nits (here and below): Space after for before the ( and { goes on the same line as the for.

@@ +232,5 @@
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
>  WifiProxyService::Shutdown()

Looking through the code, I don't see anything that calls this function. I wonder if you should null check mControlThread to make sure that we don't crash if it gets called twice. That way, we could use it to clean up if WIfiProxyService::Start fails instead of duplicating the teardown code in 3 places.

::: dom/wifi/WifiProxyService.h
@@ +20,5 @@
> +  {
> +    nsCOMPtr<nsIThread> mThread;
> +    nsCString mInterface;
> +  };
> +  typedef std::vector<EventThreadListEntry> EventThreadList;

This should use nsTArray<EventThreadListEntry>. I also probably wouldn't bother with the typedef, as the type is only ever used once.

::: dom/wifi/WifiUtils.h
@@ +120,5 @@
>  public:
>    WpaSupplicant();
>  
> +  // Use nsCString as the type of aInterface to gaurantee it's
> +  // null-terminated so that we can pass it to c API without

"guarantee"

::: dom/wifi/WifiWorker.js
@@ +149,4 @@
>        controlCallbacks[id] = callback;
> +    }
> +    if (!obj.iface) {
> +      obj.iface = manager.ifname;

Is this just for now? If not, please add a comment here that controlMessages default to the default wifi interface.

Comment 11

[Henry Chang [:hchang]]

(In reply to Blake Kaplan (:mrbkap) from comment #10)
> Comment on attachment 799389 [details] [diff] [review]
> Bug-912015.patch
> 
> Review of attachment 799389 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have a few comments to address.
> 
> ::: dom/wifi/WifiProxyService.cpp
> @@ +24,1 @@
> >  
> 
> It isn't in the patch, but would you mind marking gWifiProxyService as
> |static| as was intended?
> 
Sure!

> @@ +232,5 @@
> >    return NS_OK;
> >  }
> >  
> >  NS_IMETHODIMP
> >  WifiProxyService::Shutdown()
> 
> Looking through the code, I don't see anything that calls this function. I
> wonder if you should null check mControlThread to make sure that we don't
> crash if it gets called twice. That way, we could use it to clean up if
> WIfiProxyService::Start fails instead of duplicating the teardown code in 3
> places.
> 

How do you think if we create another bug to address this issue?

> ::: dom/wifi/WifiProxyService.h
> @@ +20,5 @@
> > +  {
> > +    nsCOMPtr<nsIThread> mThread;
> > +    nsCString mInterface;
> > +  };
> > +  typedef std::vector<EventThreadListEntry> EventThreadList;
> 
> This should use nsTArray<EventThreadListEntry>. I also probably wouldn't
> bother with the typedef, as the type is only ever used once.
> 
Is there any concern or policy to refrain us from using std::vector?

> ::: dom/wifi/WifiUtils.h
> @@ +120,5 @@
> >  public:
> >    WpaSupplicant();
> >  
> > +  // Use nsCString as the type of aInterface to gaurantee it's
> > +  // null-terminated so that we can pass it to c API without
> 
> "guarantee"
> 
> ::: dom/wifi/WifiWorker.js
> @@ +149,4 @@
> >        controlCallbacks[id] = callback;
> > +    }
> > +    if (!obj.iface) {
> > +      obj.iface = manager.ifname;
> 
> Is this just for now? If not, please add a comment here that controlMessages
> default to the default wifi interface.

Regarding this change, bug 919426 is intended to move all uses of controlMessage
to WifiCommand.jsm and WifiNetUtil.jsm. You can see the WIP here:

https://bitbucket.org/changhenry/mozilla-central/src/f3a0dd3ec575d5c637cf2a496aebab6a43e275ee/dom/wifi/WifiCommand.jsm

After introducing WifiCommand.jsm, every time we call controlMessage, the interface
name will explicitly specified. (such as #409 #417 in WifiCommand.jsm)

Probably I should mark this issue depending on bug 919426 ...

Thanks for you review!

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Henry Chang [:henry] from comment #11)
> How do you think if we create another bug to address this issue?

Well, I'd like to change the failure cases in WifiProxyService::Start to use the shutdown function to avoid having that code in the tree three times, so I'd like to see the change done here.

> Is there any concern or policy to refrain us from using std::vector?

There's been a bunch of talk on the newsgroups of starting to use the STL (and therefore std::vector), but we haven't been able to make the plunge, yet. In particular, not all STL implementations play well with us turning off exceptions and there are also concerns about them not being optimized for our use-cases and code-size questions.

> After introducing WifiCommand.jsm, every time we call controlMessage, the
> interface
> name will explicitly specified. (such as #409 #417 in WifiCommand.jsm)

Great.

Comment 13

[Henry Chang [:hchang]]

(In reply to Blake Kaplan (:mrbkap) from comment #12)
> (In reply to Henry Chang [:henry] from comment #11)
> > How do you think if we create another bug to address this issue?
> 
> Well, I'd like to change the failure cases in WifiProxyService::Start to use
> the shutdown function to avoid having that code in the tree three times, so
> I'd like to see the change done here.
>

Okay i got it. By the way, Shutdown() will be automatically called when the 
global |gWifiProxyService| goes out its scope because of |ClearOnShutdown|


> > Is there any concern or policy to refrain us from using std::vector?
> 
> There's been a bunch of talk on the newsgroups of starting to use the STL
> (and therefore std::vector), but we haven't been able to make the plunge,
> yet. In particular, not all STL implementations play well with us turning
> off exceptions and there are also concerns about them not being optimized
> for our use-cases and code-size questions.
> 

No problem. I'll change it to nsTArray

Thanks!

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Henry Chang [:henry] from comment #13)
> Okay i got it. By the way, Shutdown() will be automatically called when the 
> global |gWifiProxyService| goes out its scope because of |ClearOnShutdown|

ClearOnShutdown only sets the given pointer to null, it doesn't call any functions on it and we don't currently call it from the destructor.

Comment 15

[Henry Chang [:hchang]]

(In reply to Blake Kaplan (:mrbkap) from comment #14)
> (In reply to Henry Chang [:henry] from comment #13)
> > Okay i got it. By the way, Shutdown() will be automatically called when the 
> > global |gWifiProxyService| goes out its scope because of |ClearOnShutdown|
> 
> ClearOnShutdown only sets the given pointer to null, it doesn't call any
> functions on it and we don't currently call it from the destructor.

Oops you're right! I thought http://dxr.mozilla.org/mozilla-central/source/xpcom/base/ClearOnShutdown.h#l97 calls |Shutdown| of the object the pointer points to. (but it's actually the |PointerClearer::Shutdown()|) 

p.s. now I also don't what WifiProxyService::Shutdown is intended to do originally...

Comment 16

[Henry Chang [:hchang]]

Attachment: Bug-912015.patch (revised)

Comment 17

[Henry Chang [:hchang]]

Attachment: Bug-912015.patch (revised)

Revised version based on bug 919426. This patch also fixed the review comments of the last patch

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 816466 [details] [diff] [review]
Bug-912015.patch (revised)

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

This looks really good. I have one question, but r=me with it addressed. Thanks!

::: dom/wifi/WifiProxyService.cpp
@@ +262,5 @@
>    MOZ_ASSERT(NS_IsMainThread());
> +
> +  // Dispatch to the event thread which has the given interface name
> +  for (size_t i = 0; i < mEventThreadList.Length(); i++) {
> +    if (mEventThreadList[i].mInterface.Equals(aInterface)) {

Should we assert that mEventThreadList[i].mThread is non-null?

Comment 19

[Henry Chang [:hchang]]

(In reply to Blake Kaplan (:mrbkap) from comment #18)
> Comment on attachment 816466 [details] [diff] [review]
> Bug-912015.patch (revised)
> 
> Review of attachment 816466 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks really good. I have one question, but r=me with it addressed.
> Thanks!
> 
> ::: dom/wifi/WifiProxyService.cpp
> @@ +262,5 @@
> >    MOZ_ASSERT(NS_IsMainThread());
> > +
> > +  // Dispatch to the event thread which has the given interface name
> > +  for (size_t i = 0; i < mEventThreadList.Length(); i++) {
> > +    if (mEventThreadList[i].mInterface.Equals(aInterface)) {
> 
> Should we assert that mEventThreadList[i].mThread is non-null?

Thanks Blake!

Only in one case mEventThreadList[i].mThread can be null here:

1. User calls WifiProxyService::Start() but one of NS_NewThread() returns error, which
   may result in non-empty mEventThreadList containing null mThread.
2. Then User calls WifiProxyService::WaitForEvent() in spite of the failure of WifiProxyService::Start().

Instead of asserting here, I would rather clean mEventThreadList in WifiProxyService::Shutdown()
so that mEventThreadList[i].mThread would never ever be null. What do you think?

Thanks :)

Comment 20

[Henry Chang [:hchang]]

Attachment: Bug-912015.patch (problem addressed and r+'d. Please check in this.)

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Henry Chang [:henry] from comment #19)
> so that mEventThreadList[i].mThread would never ever be null. What do you
> think?

Sorry for missing this comment. This plan sounds fine to me.

Comment 22

[Henry Chang [:hchang]]

Thanks Blake :)

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e749f99448d

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7e749f99448d