1038531 - Unify NetworkService/Networker/WifiNetUtil/WifiUtil

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Henry Chang [:hchang]]

B2G has two threads to execute network commands asynchronously:

1) NetworkWorker.cpp::gWorkerThread for communicating with netd
   as well as calling libnetutils.so functions.

2) WifiProxyService::mControlThread for calling libnetutils.so functions (mixed with    
   libhardware_legacy::wifi.c)

The latter one is only used by wifi and can be extracted to NetworkWorker.
One downside of doing this is non-netd commands would block each other while
they can run concurrently. Introducing a thread pool for non-netd command
might be a good solution. 

This bug may be going to be a meta bug if the work becomes complicated
but now I intend to let it be a single bug. The action items are:

1) Put binding of libnetutils.so in WifiProxyService to NetworkService

2) Change the implementation of WifiNetUtil.jsm or replace the use
   of WifiNetUtil.jsm with NetworkService.

3) Bring in a thread pool to NetworkWorker for non-netd commands

The removal of libnetutils.so from WifiProxyService also increase
the concurrency of WifiProxyService. (wifi.c commands would be
blocked by network commands for now)

Comment 1

[Henry Chang [:hchang]]

Attachment: WIP (based on bug 1041394)

Comment 2

[Henry Chang [:hchang]]

Preliminary verified on devices. Pushed to try:

https://tbpl.mozilla.org/?tree=Try&rev=7ceb6648e5e0

Comment 3

[Henry Chang [:hchang]]

Attachment: WIP V2

Comment 4

[Henry Chang [:hchang]]

The scope of this bug will be limited to WIP v2. There will be other followups for this bug:

1) Remove the indirection use of NetworkOptions.
2) Make the callback arguments more consistent for all kinds of commands.
3) Remove the NetworkService js wrapping. (TBD)

Comment 5

[Henry Chang [:hchang]]

Attachment: Patch for review

The main purpose of this patch is to move around all the network commands to a single point to execute. Originally there are two worker threads to handle network commands. One is WifiProxyService and the other is NetworkWorker. This patch moves around the network commands executed in WifiProxyService to NetworkWorker. The benefit of doing so is the network commands would no longer block wifi commands in WifiProxyService as described in bug 994564. 

Followings are some notes regarding this patch:

1) nsINetworkService is extended to support more commands including:

   - configureInterface
   - enableInterface
   - disableInterface
   - dhcpRequest
   - resetConnections
   
   These commands are originally supported in WifiProxyService [1]

2) nsINetworkService::setDefaultRouteAndDNS is reduced to simply setDefaultRoute since      
   we want to keep nsINetworkService as simple as possible. This change requires 
   NetworkManager to define a helper function to combine a couple of network commands 
   instead of calling setDefaultRouteAndDNS directly.


3) Move around some properties from WifiOptions to NetworkOptions. Some duplicated name are suffixed with "long".

4) Remove the dependency of NetUtils from WifiUtils

5) Remove the copy ctor of NetworkParams since the compiler-generated member-wise copy ctor
   is good enough.


[1] http://hg.mozilla.org/mozilla-central/file/613e79262240/dom/wifi/WifiUtils.cpp#l451

Comment 6

[[:fabrice] Fabrice Desré]

Comment on attachment 8462323 [details] [diff] [review]
Patch for review

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

Very nice Henry! Can you check the comments? I didn't try the patch on device either, but I'd like to do that before landing.

::: dom/system/gonk/NetworkUtils.cpp
@@ +1100,5 @@
> +  // The native command refers to simply libnetutil C function calls.
> +  // They will be processed synchronously in the worker thread and
> +  // notify the callback handler registered by NetworkUtils client.
> +  if (ExecuteNativeCommand(aOptions)) {
> +    DEBUG("Native command \"%s\" executed.", NS_ConvertUTF16toUTF8(aOptions.mCmd).get());

nit: indentation

@@ +1165,5 @@
>    }
>  }
>  
> +bool NetworkUtils::ExecuteNativeCommand(NetworkParams& aOptions) {
> +  NS_ConvertUTF16toUTF8 ifname = NS_ConvertUTF16toUTF8(aOptions.mIfname);

NS_ConvertUTF16toUTF8 ifname(aOptions.mIfname) ?

@@ +1195,5 @@
> +  };
> +
> +  // Loop until we find the command name which matches aOptions.mCmd.
> +  CommandHandler handler = nullptr;
> +  for (size_t i = 0; i < CNT_OF_ARRAY(COMMAND_HANDLER_TABLE); i++) {

Please check if we can replace CNT_OF_ARRAY by MOZ_ARRAY_LENGTH

@@ +1200,5 @@
> +    if (aOptions.mCmd.EqualsASCII(COMMAND_HANDLER_TABLE[i].mCommandName)) {
> +      handler = COMMAND_HANDLER_TABLE[i].mCommandHandler;
> +      break;
> +    }
> +  }

Why not build a hashtable instead of the array to speed up the lookup?

@@ +1433,5 @@
> +
> +int32_t NetworkUtils::enableInterface(NetworkParams& aOptions,
> +                                      NetworkResultOptions& aResult) {
> +  NS_ConvertUTF16toUTF8 autoIfname(aOptions.mIfname);
> +  return mNetUtils->do_ifc_enable(autoIfname.get());

you can inline as: (also in the next two methods.
return mNetUtils->do_ifc_enable(NS_ConvertUTF16toUTF8(aOptions.mIfname).get());

::: dom/system/gonk/nsINetworkService.idl
@@ +150,5 @@
> +   *          - gateway:     long
> +   *          - dns1:        long
> +   *          - dns2:        long
> +   *          - server:      long
> +   *        

nit: trailing whitespace.

@@ +284,5 @@
>     * Set DNS.
>     *
>     * @param networkInterface
>     *        The network interface which contains the DNS we want to set.
> +   * 

here too.

@@ +432,5 @@
> +   * Issue a DHCP client request.
> +   *
> +   * @param networkInterface
> +   *        The network interface which we wnat to do the DHCP request on.
> +   * 

here too, and in all the following comments.

::: dom/webidl/NetworkOptions.webidl
@@ +59,5 @@
> +  long ipaddr;            // for "ifc_configure".
> +  long mask;              // for "ifc_configure".
> +  long gateway_long;      // for "ifc_configure".
> +  long dns1_long;         // for "ifc_configure".
> +  long dns2_long;         // for "ifc_configure".

nit: align the comments with the ones already there.

@@ +87,5 @@
>    boolean success = false;            // for "setDhcpServer".
>    DOMString curExternalIfname = "";   // for "updateUpStream".
>    DOMString curInternalIfname = "";   // for "updateUpStream".
> +
> +  //

remove that empty comment

Comment 7

[Henry Chang [:hchang]]

(In reply to Fabrice Desré [:fabrice] from comment #6)
> Comment on attachment 8462323 [details] [diff] [review]
> Patch for review
> 
> Review of attachment 8462323 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Very nice Henry! Can you check the comments? I didn't try the patch on
> device either, but I'd like to do that before landing.
> 

Thanks! I tried on flame and ICS emulator and everything is going well.
Actually some commands are covered by wifi test cases such as dhcp 
request.

> > +  // Loop until we find the command name which matches aOptions.mCmd.
> > +  CommandHandler handler = nullptr;
> > +  for (size_t i = 0; i < CNT_OF_ARRAY(COMMAND_HANDLER_TABLE); i++) {
> 
> Please check if we can replace CNT_OF_ARRAY by MOZ_ARRAY_LENGTH
> 

I knew there is a template version but didn't know gecko has it!
Thanks for your reminding!

> @@ +1200,5 @@
> > +    if (aOptions.mCmd.EqualsASCII(COMMAND_HANDLER_TABLE[i].mCommandName)) {
> > +      handler = COMMAND_HANDLER_TABLE[i].mCommandHandler;
> > +      break;
> > +    }
> > +  }
> 
> Why not build a hashtable instead of the array to speed up the lookup?
> 

I doubt hashing/binary search would be much faster than linear lookup
on such a small size table. But I am glad to change it if you think
we should do it for scalability at this time.

> @@ +1433,5 @@
> > +
> > +int32_t NetworkUtils::enableInterface(NetworkParams& aOptions,
> > +                                      NetworkResultOptions& aResult) {
> > +  NS_ConvertUTF16toUTF8 autoIfname(aOptions.mIfname);
> > +  return mNetUtils->do_ifc_enable(autoIfname.get());
> 
> you can inline as: (also in the next two methods.
> return
> mNetUtils->do_ifc_enable(NS_ConvertUTF16toUTF8(aOptions.mIfname).get());
> 

Thanks :)

Comment 8

[Henry Chang [:hchang]]

Attachment: Patch for review V2

Comment 9

[Henry Chang [:hchang]]

(In reply to Fabrice Desré [:fabrice] from comment #6)
> Comment on attachment 8462323 [details] [diff] [review]
> Patch for review
>
> @@ +1195,5 @@
> > +  };
> > +
> > +  // Loop until we find the command name which matches aOptions.mCmd.
> > +  CommandHandler handler = nullptr;
> > +  for (size_t i = 0; i < CNT_OF_ARRAY(COMMAND_HANDLER_TABLE); i++) {
> 
> Please check if we can replace CNT_OF_ARRAY by MOZ_ARRAY_LENGTH
> 

Unfortunately MOZ_ARRAY_LENGTH doesn't compile on ICS emulator which uses
the following compiler options [1] to compile NetworkUtils.cpp. The root
cause is the local type is used to instantiate the template, which is
not allowed in c++03. Even thought it's supported by c++0x, gcc 4.4
still doesn't support it even with 'std=gnu++0x' option [2].

I'd like to hoist the type definition to get around this on ICS emulator.

[1] /usr/bin/ccache /home/hchang/dev/b2g-emu-ics/prebuilt/linux-x86/toolchain/arm-linux-androideabi-4.4.x/bin/arm-linux-androideabi-g++ -o NetworkUtils.o -c  [omitted] -std=gnu++0x [omitted] /home/hchang/dev/b2g-emu-ics/mozilla-central/dom/system/gonk/NetworkUtils.cpp

[2] https://gcc.gnu.org/gcc-4.4/cxx0x_status.html

Comment 10

[Henry Chang [:hchang]]

Attachment: Patch for review V3

Try run: https://tbpl.mozilla.org/?tree=Try&rev=ca8cdc98eece

Comment 11

[Henry Chang [:hchang]]

Set dependent on bug 1043114

Comment 12

[Henry Chang [:hchang]]

Hi Fabrice,

Just let you know I am waiting for another bug's (1043114) landing
and will rebase this patch. Also, I've addressed your comments on 
comment 7 and 9. I will be asking for another review after rebasing.
Thanks!

Comment 13

[[:fabrice] Fabrice Desré]

(In reply to Henry Chang [:henry] from comment #12)
> Hi Fabrice,
> 
> Just let you know I am waiting for another bug's (1043114) landing
> and will rebase this patch. Also, I've addressed your comments on 
> comment 7 and 9. I will be asking for another review after rebasing.
> Thanks!

Sounds good, thanks!

Comment 14

[Henry Chang [:hchang]]

Attachment: Patch V4

Comment 15

[Henry Chang [:hchang]]

Attachment: Patch V4

Comment 16

[Henry Chang [:hchang]]

Comment on attachment 8474351 [details] [diff] [review]
Patch V4

Hi Fabrice,

Since the dependent bug has landed, I have rebased and am 
requesting for review again. In this rebased patch, I remove
the use of "isAsync" and the concept of "native command".
All handlers can either return a result code by linux 
convention or a complex "NetworkResultOption" or "Pending"
if the result is obtained asynchronously.

Jessica, Edgar, 
Please feel free to leave your comments since I know you
have been working on same stuff.

Thanks!

Comment 17

[Jessica Jong [:jessica] (inactive)]

(In reply to Henry Chang [:henry] from comment #16)
> Comment on attachment 8474351 [details] [diff] [review]
> Patch V4
> 
> Hi Fabrice,
> 
> Since the dependent bug has landed, I have rebased and am 
> requesting for review again. In this rebased patch, I remove
> the use of "isAsync" and the concept of "native command".
> All handlers can either return a result code by linux 
> convention or a complex "NetworkResultOption" or "Pending"
> if the result is obtained asynchronously.
> 
> Jessica, Edgar, 
> Please feel free to leave your comments since I know you
> have been working on same stuff.
> 
> Thanks!

I haven't gone into details, but the idea of getting rid of 'isAsync' sounds good to me. Thanks :)

Comment 18

[Edgar Chen [:edgar]]

Awesome!! It's really cool. :)

Comment 19

[[:fabrice] Fabrice Desré]

Comment on attachment 8474351 [details] [diff] [review]
Patch V4

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

Thanks Henry, I like that a lot!
You will need review from a DOM peer to land the dom/webidl changes though. I'm flagging mrbkap.

::: dom/system/gonk/NetworkUtils.cpp
@@ +409,5 @@
> +  mResult.mResultCode = aResultCode;
> +  if (aResultCode != SUCCESS) {
> +    // The returned value is sometimes negative, make sure we pass a positive
> +    // error number to strerror.
> +    mResult.mReason = NS_ConvertUTF8toUTF16(strerror(abs(aResultCode)));

strerror is not thread safe. Can this be a problem here? stererror_r is always better.

::: dom/wifi/WifiUtils.h
@@ +96,3 @@
>    nsAutoPtr<WifiHotspotUtils> mWifiHotspotUtils;
>  
> +  int32_t mSdkVersion;

nit: could probably be a uint32_t

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8474351 [details] [diff] [review]
Patch V4

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

r=me on the webidl changes.

Comment 21

[Henry Chang [:hchang]]

Thanks Fabrice and Blake!

Comment 22

[Henry Chang [:hchang]]

Attachment: Patch V5 (r+'d)

Waiting for full try run result: 

https://tbpl.mozilla.org/?tree=Try&rev=ef4db44a83a0

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/59611423c368

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Backed out for Marionette-webapi perma-fail.
https://hg.mozilla.org/integration/b2g-inbound/rev/e934b3bc1fea

https://tbpl.mozilla.org/php/getParsedLog.php?id=47308241&tree=B2g-Inbound

Comment 25

[Henry Chang [:hchang]]

Attachment: Patch v6 (fixed marionette test failure)

Comment 26

[Henry Chang [:hchang]]

Try run result:

https://tbpl.mozilla.org/?tree=Try&rev=7d7c9bdacff4

Comment 27

[Henry Chang [:hchang]]

(In reply to Henry Chang [:henry] from comment #26)
> Try run result:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=7d7c9bdacff4

Marionette test failure mentioned in comment #24 is fixed.

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/767e3d6ae319

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/767e3d6ae319