Closed Bug 1038531 Opened 6 years ago Closed 5 years ago

Unify NetworkService/Networker/WifiNetUtil/WifiUtil

Categories

(Firefox OS Graveyard :: General, defect)

x86_64
Gonk (Firefox OS)
defect
Not set

Tracking

(b2g-v2.0 wontfix, b2g-v2.0M wontfix, b2g-v2.1 affected, b2g-v2.1S affected, b2g-v2.2 fixed, b2g-master fixed)

RESOLVED FIXED
2.1 S2 (15aug)
Tracking Status
b2g-v2.0 --- wontfix
b2g-v2.0M --- wontfix
b2g-v2.1 --- affected
b2g-v2.1S --- affected
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: hchang, Assigned: hchang)

References

Details

(Whiteboard: [p=10][ft:ril])

Attachments

(1 file, 8 obsolete files)

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)
Assignee: nobody → hchang
Whiteboard: [p=10][ft:ril]
Depends on: 1041394
Attached patch WIP (based on bug 1041394) (obsolete) — Splinter Review
Preliminary verified on devices. Pushed to try:

https://tbpl.mozilla.org/?tree=Try&rev=7ceb6648e5e0
Attached patch WIP V2 (obsolete) — Splinter Review
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)
Attached patch Patch for review (obsolete) — Splinter 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
Attachment #8460121 - Attachment is obsolete: true
Attachment #8461429 - Attachment is obsolete: true
Attachment #8462323 - Flags: review?(fabrice)
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
Attachment #8462323 - Flags: review?(fabrice) → feedback+
(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 :)
Target Milestone: --- → 2.1 S2 (15aug)
Attached patch Patch for review V2 (obsolete) — Splinter Review
(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
Attached patch Patch for review V3 (obsolete) — Splinter Review
Try run: https://tbpl.mozilla.org/?tree=Try&rev=ca8cdc98eece
Attachment #8462323 - Attachment is obsolete: true
Attachment #8464510 - Attachment is obsolete: true
Depends on: 1043114
Set dependent on bug 1043114
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!
Flags: needinfo?(fabrice)
(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!
Flags: needinfo?(fabrice)
Attached patch Patch V4 (obsolete) — Splinter Review
Attachment #8465223 - Attachment is obsolete: true
Attached patch Patch V4 (obsolete) — Splinter Review
Attachment #8474350 - Attachment is obsolete: true
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!
Attachment #8474351 - Flags: review?(fabrice)
Flags: needinfo?(jjong)
Flags: needinfo?(echen)
(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 :)
Flags: needinfo?(jjong)
Awesome!! It's really cool. :)
Flags: needinfo?(echen)
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
Attachment #8474351 - Flags: review?(mrbkap)
Attachment #8474351 - Flags: review?(fabrice)
Attachment #8474351 - Flags: review+
Comment on attachment 8474351 [details] [diff] [review]
Patch V4

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

r=me on the webidl changes.
Attachment #8474351 - Flags: review?(mrbkap) → review+
Thanks Fabrice and Blake!
Attached patch Patch V5 (r+'d) (obsolete) — Splinter Review
Waiting for full try run result: 

https://tbpl.mozilla.org/?tree=Try&rev=ef4db44a83a0
Keywords: checkin-needed
Attachment #8474351 - Attachment is obsolete: true
Attachment #8482178 - Attachment is obsolete: true
(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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/767e3d6ae319
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.