864931 - Rewrite net worker in C++

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Justin Lebar (not reading bugmail)]

The net_worker JS worker thread uses a lot of memory in B2G.  I strongly suspect that if we rewrote it in C++, we'd be able to reduce the component's memory usage significantly.

Here's a dump of the worker thread's memory usage.  Most-to-all of unused-arenas will go away after bug 829482, but even after that this worker uses way too much memory.

> 2.31 MB (06.75%) -- worker(resource://gre/modules/net_worker.js, 0x44486800)
> ├──1.73 MB (05.06%) -- gc-heap
> │  ├──1.70 MB (04.97%) ── unused-arenas
> │  └──0.03 MB (00.09%) -- (2 tiny)
> │     ├──0.03 MB (00.09%) ── chunk-admin
> │     └──0.00 MB (00.00%) ── unused-chunks
> └──0.58 MB (01.69%) -- (3 tiny)
>    ├──0.26 MB (00.77%) -- compartment(web-worker)
>    │  ├──0.16 MB (00.47%) -- gc-heap
>    │  │  ├──0.07 MB (00.22%) ── unused-gc-things [2]
>    │  │  ├──0.04 MB (00.11%) -- objects
>    │  │  │  ├──0.02 MB (00.06%) ── function
>    │  │  │  └──0.02 MB (00.05%) ── non-function
>    │  │  ├──0.03 MB (00.10%) -- shapes
>    │  │  │  ├──0.02 MB (00.07%) ── tree
>    │  │  │  └──0.01 MB (00.03%) ── base
>    │  │  └──0.02 MB (00.05%) ── sundries [2]
>    │  ├──0.03 MB (00.09%) ── analysis-temporary
>    │  ├──0.02 MB (00.07%) ── other-sundries [2]
>    │  ├──0.02 MB (00.05%) ── script-data
>    │  ├──0.02 MB (00.05%) ── objects-extra/slots
>    │  └──0.01 MB (00.04%) ── shapes-extra/compartment-tables
>    ├──0.20 MB (00.60%) -- runtime
>    │  ├──0.13 MB (00.36%) ── gc-marker
>    │  ├──0.04 MB (00.10%) ── runtime-object
>    │  ├──0.02 MB (00.05%) ── script-sources
>    │  ├──0.02 MB (00.05%) ── atoms-table
>    │  ├──0.00 MB (00.01%) ── dtoa
>    │  ├──0.00 MB (00.01%) ── stack
>    │  ├──0.00 MB (00.01%) ── temporary
>    │  ├──0.00 MB (00.00%) ── contexts
>    │  ├──0.00 MB (00.00%) ── script-filenames
>    │  ├──0.00 MB (00.00%) ── ion-code
>    │  ├──0.00 MB (00.00%) ── jaeger-code
>    │  ├──0.00 MB (00.00%) ── math-cache
>    │  ├──0.00 MB (00.00%) ── regexp-code
>    │  └──0.00 MB (00.00%) ── unused-code
>    └──0.11 MB (00.33%) -- compartment(web-worker-atoms)
>       ├──0.10 MB (00.29%) -- gc-heap
>       │  ├──0.09 MB (00.27%) ── strings
>       │  └──0.01 MB (00.02%) ── sundries
>       ├──0.01 MB (00.04%) ── string-chars/non-huge
>       └──0.00 MB (00.01%) ── other-sundries

Comment 1

[Justin Lebar (not reading bugmail)]

There are four relevant workers here:

 * RIL worker (bug 864927)
 * Net worker (this bug)
 * wifi worker x2 (bug 864932)

Comment 2

[David Zbarsky (:dzbarsky)]

Attachment: WIP Part 1

Comment 3

[David Zbarsky (:dzbarsky)]

Attachment: WIP Part 2

Comment 4

[Vincent Chang[:vchang][changyihsin]]

Hi David, do you mind if I take this bug ?

Comment 5

[David Zbarsky (:dzbarsky)]

Vincent, please do!

Comment 6

[[:fabrice] Fabrice Desré]

Dimi, are you actively working on that?

Comment 7

[Dimi Lee [:dimi]]

(In reply to Fabrice Desré [:fabrice] from comment #6)
> Dimi, are you actively working on that?

Yes, I am working on it.
Current status is that net worker is changed to c++ and can communicate with netd.
And I am working on the command parsing and command chain now.

Comment 8

[Joe Cheng [:jcheng] (please needinfo)]

Hi Dimi, does networker still use a lot of memory after GC?

Comment 9

[Dimi Lee [:dimi]]

(In reply to Joe Cheng [:jcheng] from comment #8)
> Hi Dimi, does networker still use a lot of memory after GC?

Hi Joy, The networker is not yet finished, so memory measurement may not precise.
I will update it once the implementation is done.

Comment 10

[Joe Cheng [:jcheng] (please needinfo)]

Hi Dimi, do you mind providing updates on when you may have this done? 
and if you can please provide the memory saving when this is implemented, please update the bug. Thanks

Comment 11

[Dimi Lee [:dimi]]

Hi Joy,
  I will provide the patch and update the memory saving result before 1/10, is that ok for you ?

Comment 12

[[:fabrice] Fabrice Desré]

From the experience of the wifi worker, I expect a saving of 2 to 2.5MB

Comment 13

[Dimi Lee [:dimi]]

Attachment: WIP v1

Comment 14

[Mike Habicher [:mikeh] (high bugzilla latency)]

Comment on attachment 8357629 [details] [diff] [review]
WIP v1

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

::: dom/network/src/NetUtils.h
@@ +64,5 @@
> +// Set up a dlsymed function ready to use.
> +#define USE_DLFUNC(name)                                                      \
> +  FUNC##name name = (FUNC##name) dlsym(GetSharedLibrary(), #name);            \
> +  if (!name) {                                                                \
> +    MOZ_ASSUME_UNREACHABLE("Symbol not found in shared library : " #name);         \

drive-by nit: line up slashes.

::: dom/webidl/NetworkOptions.webidl
@@ +2,5 @@
> +* License, v. 2.0. If a copy of the MPL was not distributed with this file,
> +* You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/**
> +* This dictionnary holds the parameters sent to the network worker.

drive-by nit: 'dictionary'

Comment 15

[[:fabrice] Fabrice Desré]

Comment on attachment 8357629 [details] [diff] [review]
WIP v1

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

I didn't check the logic itself, but I'm super happy to see that coming up!

::: dom/system/gonk/NetworkService.js
@@ +49,5 @@
>   */
>  function NetworkService() {
>    if(DEBUG) debug("Starting net_worker.");
> +  let networkWorker = Cc["@mozilla.org/network/worker;1"];
> +  var self = this;

nit: s/var/let

::: dom/system/gonk/NetworkUtils.cpp
@@ +44,5 @@
> +static const char* DUMMY_COMMAND = "tether status";
> +
> +// Retry 20 times (2 seconds) for usb state transition.
> +static const uint32_t USB_FUNCTION_RETRY_TIMES = 20;
> +// Check "sys.usb.state" every 100ms. 

nit: trailing white space

@@ +133,5 @@
> +  if (!f) {
> +    delete aChain;
> +    return;
> +  }
> +  

nit: whitespace

@@ +148,5 @@
> +  }
> +  QueueData data = gCommandQueue[0];
> +  gCommandQueue.RemoveElementAt(0);
> +
> +  sprintf(gCurrentCommand, "%s", data.a->mData);

use snprintf(gCurrentCommand, MAX_COMMAND_SIZE -1, ...) instead, here and elsewhere too.

@@ +848,5 @@
> +/**
> + * Handle received data from netd.
> + */
> +void NetworkUtils::onNetdMessage(const NetdCommand& aCommand)
> +{  

nit: whitespace

@@ +859,5 @@
> +    nextNetdCommand();
> +    return;
> +  }
> +  uint32_t code = atoi(result);
> +  char* reason = NULL;

here and everywhere, s/NULL/nullptr

@@ +1102,5 @@
> +           GET_CHAR(mInternalIfname), GET_CHAR(mExternalIfname));
> +    RUN_CHAIN(aOptions, gWifiEnableChain, wifiTetheringFail)
> +  } else {
> +    DEBUG("Stopping Wifi Tethering on %s <-> %s",
> +           GET_CHAR(mInternalIfname), GET_CHAR(mExternalIfname));    

nit: whitespace

@@ +1152,5 @@
> +    result.mResult = true;
> +    postMessage(aOptions, result);
> +    retry = 0;
> +    return;
> +  } 

nit: whitespace

::: dom/system/gonk/NetworkUtils.h
@@ +71,5 @@
> +    mCurInternalIfname = aOther.mCurInternalIfname;
> +    mCurExternalIfname = aOther.mCurExternalIfname;
> +    mThreshold = aOther.mThreshold;
> +  }
> +  

nit: whitespace

@@ +210,5 @@
> +public:
> +  CommandChain(const NetworkParams& aParams,
> +               COMMAND aCmds[],
> +               uint32_t aLength,
> +               ERROR_CALLBACK aError) 

nit: whitespace

@@ +261,5 @@
> +  bool removeDefaultRoute(NetworkParams& aOptions);
> +  bool removeHostRoute(NetworkParams& aOptions);
> +  bool removeHostRoutes(NetworkParams& aOptions);
> +  bool removeNetworkRoute(NetworkParams& aOptions);
> +  bool getNetworkInterfaceStats(NetworkParams& aOptions);  

nit: whitespace

::: dom/system/gonk/NetworkWorker.cpp
@@ +23,5 @@
> +using namespace mozilla::ipc;
> +
> +namespace mozilla {
> +
> +// The singleton Wifi service, to be used on the main thread.

I see where you copy/pasted from... ;)

@@ +26,5 @@
> +
> +// The singleton Wifi service, to be used on the main thread.
> +StaticRefPtr<NetworkWorker> gNetworkWorker;
> +
> +// The singleton supplicant class, that can be used on any thread.

Here too!

@@ +222,5 @@
> +  mWorkerThread->Dispatch(runnable, nsIEventTarget::DISPATCH_NORMAL);
> +}
> +
> +// Callback function for worker thread to update result to main thread.
> +void 

nit: whitespace

Comment 16

[Dimi Lee [:dimi]]

Test on Nexus4 WITH net_worker.js

1.10 MB (100.0%) -- explicit
├──1.00 MB (91.54%) -- workers/workers()
│  ├──1.00 MB (91.54%) -- worker(resource://gre/modules/net_worker.js, 0xNNN)
│  │  ├──0.38 MB (34.44%) -- runtime
│  │  │  ├──0.10 MB (08.94%) ── script-sources [+]
│  │  │  ├──0.09 MB (08.00%) ── script-data [+]
│  │  │  ├──0.06 MB (05.70%) ── atoms-table [+]
│  │  │  ├──0.06 MB (05.70%) -- code
│  │  │  │  ├──0.04 MB (03.62%) ── other [+]
│  │  │  │  ├──0.01 MB (01.24%) ── baseline [+]
│  │  │  │  └──0.01 MB (00.84%) ── unused [+]
│  │  │  ├──0.04 MB (03.56%) ── runtime-object [+]
│  │  │  ├──0.02 MB (01.43%) ── gc-marker [+]
│  │  │  └──0.01 MB (01.11%) ++ (4 tiny)
│  │  ├──0.24 MB (22.11%) -- zone(0xb163c800)
│  │  │  ├──0.19 MB (17.14%) -- compartment(web-worker)
│  │  │  │  ├──0.09 MB (08.21%) -- objects
│  │  │  │  │  ├──0.05 MB (04.35%) -- malloc-heap
│  │  │  │  │  │  ├──0.03 MB (02.52%) ── ctypes-data [+]
│  │  │  │  │  │  └──0.02 MB (01.83%) ── slots [+]
│  │  │  │  │  └──0.04 MB (03.86%) -- gc-heap
│  │  │  │  │     ├──0.03 MB (02.31%) ── function [+]
│  │  │  │  │     └──0.02 MB (01.55%) ── ordinary [+]
│  │  │  │  ├──0.06 MB (05.78%) -- shapes
│  │  │  │  │  ├──0.04 MB (03.28%) -- gc-heap
│  │  │  │  │  │  ├──0.02 MB (01.94%) ── tree/global-parented
│  │  │  │  │  │  └──0.01 MB (01.34%) ── base [+]
│  │  │  │  │  └──0.03 MB (02.49%) ── malloc-heap/compartment-tables
│  │  │  │  ├──0.02 MB (02.13%) -- sundries
│  │  │  │  │  ├──0.01 MB (01.23%) ── gc-heap [+]
│  │  │  │  │  └──0.01 MB (00.90%) ── malloc-heap [+]
│  │  │  │  └──0.01 MB (01.02%) ── scripts/gc-heap
│  │  │  ├──0.05 MB (04.19%) ── unused-gc-things [+]
│  │  │  └──0.01 MB (00.79%) ++ sundries
│  │  ├──0.18 MB (16.01%) -- zone(0xb20c2800)
│  │  │  ├──0.13 MB (11.95%) -- compartment(web-worker)
│  │  │  │  ├──0.05 MB (04.50%) -- objects
│  │  │  │  │  ├──0.04 MB (03.29%) -- gc-heap
│  │  │  │  │  │  ├──0.02 MB (02.17%) ── function [+]
│  │  │  │  │  │  └──0.01 MB (01.12%) ── ordinary [+]
│  │  │  │  │  └──0.01 MB (01.21%) ── malloc-heap/slots
│  │  │  │  ├──0.04 MB (03.67%) -- shapes
│  │  │  │  │  ├──0.03 MB (02.78%) -- gc-heap
│  │  │  │  │  │  ├──0.02 MB (02.00%) ── tree/global-parented
│  │  │  │  │  │  └──0.01 MB (00.78%) ── dict [+]
│  │  │  │  │  └──0.01 MB (00.89%) ── malloc-heap/compartment-tables
│  │  │  │  ├──0.02 MB (01.94%) ── scripts/gc-heap
│  │  │  │  └──0.02 MB (01.83%) -- sundries
│  │  │  │     ├──0.01 MB (01.08%) ── malloc-heap [+]
│  │  │  │     └──0.01 MB (00.75%) ── gc-heap [+]
│  │  │  ├──0.04 MB (03.87%) ── unused-gc-things [+]
│  │  │  └──0.00 MB (00.19%) ── sundries/gc-heap
│  │  ├──0.17 MB (15.78%) -- zone(0xb20c5c00)
│  │  │  ├──0.15 MB (13.95%) -- strings
│  │  │  │  ├──0.12 MB (11.12%) -- normal
│  │  │  │  │  ├──0.09 MB (07.88%) ── gc-heap [+]
│  │  │  │  │  └──0.04 MB (03.24%) ── malloc-heap [+]
│  │  │  │  └──0.03 MB (02.84%) ── short-gc-heap [+]
│  │  │  └──0.02 MB (01.82%) ++ (4 tiny)
│  │  └──0.04 MB (03.21%) -- gc-heap
│  │     ├──0.03 MB (02.85%) ── chunk-admin [+]
│  │     └──0.00 MB (00.36%) ── unused-arenas [+]
│  └──0.00 MB (00.00%) ++ (2 tiny)
├──0.21 MB (19.19%) -- heap-overhead
│  ├──0.15 MB (13.85%) ── waste
│  └──0.06 MB (05.35%) ── page-cache
└──-0.12 MB (-10.73%) ++ (9 tiny)

Comment 17

[Dimi Lee [:dimi]]

(In reply to Dimi Lee[:dimi][:dlee] from comment #16)
> Test on Nexus4 WITH net_worker.js
> 
> 1.10 MB (100.0%) -- explicit
> ├──1.00 MB (91.54%) -- workers/workers()
> │  ├──1.00 MB (91.54%) -- worker(resource://gre/modules/net_worker.js, 0xNNN)
> │  │  ├──0.38 MB (34.44%) -- runtime
> │  │  │  ├──0.10 MB (08.94%) ── script-sources [+]
> │  │  │  ├──0.09 MB (08.00%) ── script-data [+]
> │  │  │  ├──0.06 MB (05.70%) ── atoms-table [+]
> │  │  │  ├──0.06 MB (05.70%) -- code
> │  │  │  │  ├──0.04 MB (03.62%) ── other [+]
> │  │  │  │  ├──0.01 MB (01.24%) ── baseline [+]
> │  │  │  │  └──0.01 MB (00.84%) ── unused [+]
> │  │  │  ├──0.04 MB (03.56%) ── runtime-object [+]
> │  │  │  ├──0.02 MB (01.43%) ── gc-marker [+]
> │  │  │  └──0.01 MB (01.11%) ++ (4 tiny)
> │  │  ├──0.24 MB (22.11%) -- zone(0xb163c800)
> │  │  │  ├──0.19 MB (17.14%) -- compartment(web-worker)
> │  │  │  │  ├──0.09 MB (08.21%) -- objects
> │  │  │  │  │  ├──0.05 MB (04.35%) -- malloc-heap
> │  │  │  │  │  │  ├──0.03 MB (02.52%) ── ctypes-data [+]
> │  │  │  │  │  │  └──0.02 MB (01.83%) ── slots [+]
> │  │  │  │  │  └──0.04 MB (03.86%) -- gc-heap
> │  │  │  │  │     ├──0.03 MB (02.31%) ── function [+]
> │  │  │  │  │     └──0.02 MB (01.55%) ── ordinary [+]
> │  │  │  │  ├──0.06 MB (05.78%) -- shapes
> │  │  │  │  │  ├──0.04 MB (03.28%) -- gc-heap
> │  │  │  │  │  │  ├──0.02 MB (01.94%) ── tree/global-parented
> │  │  │  │  │  │  └──0.01 MB (01.34%) ── base [+]
> │  │  │  │  │  └──0.03 MB (02.49%) ── malloc-heap/compartment-tables
> │  │  │  │  ├──0.02 MB (02.13%) -- sundries
> │  │  │  │  │  ├──0.01 MB (01.23%) ── gc-heap [+]
> │  │  │  │  │  └──0.01 MB (00.90%) ── malloc-heap [+]
> │  │  │  │  └──0.01 MB (01.02%) ── scripts/gc-heap
> │  │  │  ├──0.05 MB (04.19%) ── unused-gc-things [+]
> │  │  │  └──0.01 MB (00.79%) ++ sundries
> │  │  ├──0.18 MB (16.01%) -- zone(0xb20c2800)
> │  │  │  ├──0.13 MB (11.95%) -- compartment(web-worker)
> │  │  │  │  ├──0.05 MB (04.50%) -- objects
> │  │  │  │  │  ├──0.04 MB (03.29%) -- gc-heap
> │  │  │  │  │  │  ├──0.02 MB (02.17%) ── function [+]
> │  │  │  │  │  │  └──0.01 MB (01.12%) ── ordinary [+]
> │  │  │  │  │  └──0.01 MB (01.21%) ── malloc-heap/slots
> │  │  │  │  ├──0.04 MB (03.67%) -- shapes
> │  │  │  │  │  ├──0.03 MB (02.78%) -- gc-heap
> │  │  │  │  │  │  ├──0.02 MB (02.00%) ── tree/global-parented
> │  │  │  │  │  │  └──0.01 MB (00.78%) ── dict [+]
> │  │  │  │  │  └──0.01 MB (00.89%) ── malloc-heap/compartment-tables
> │  │  │  │  ├──0.02 MB (01.94%) ── scripts/gc-heap
> │  │  │  │  └──0.02 MB (01.83%) -- sundries
> │  │  │  │     ├──0.01 MB (01.08%) ── malloc-heap [+]
> │  │  │  │     └──0.01 MB (00.75%) ── gc-heap [+]
> │  │  │  ├──0.04 MB (03.87%) ── unused-gc-things [+]
> │  │  │  └──0.00 MB (00.19%) ── sundries/gc-heap
> │  │  ├──0.17 MB (15.78%) -- zone(0xb20c5c00)
> │  │  │  ├──0.15 MB (13.95%) -- strings
> │  │  │  │  ├──0.12 MB (11.12%) -- normal
> │  │  │  │  │  ├──0.09 MB (07.88%) ── gc-heap [+]
> │  │  │  │  │  └──0.04 MB (03.24%) ── malloc-heap [+]
> │  │  │  │  └──0.03 MB (02.84%) ── short-gc-heap [+]
> │  │  │  └──0.02 MB (01.82%) ++ (4 tiny)
> │  │  └──0.04 MB (03.21%) -- gc-heap
> │  │     ├──0.03 MB (02.85%) ── chunk-admin [+]
> │  │     └──0.00 MB (00.36%) ── unused-arenas [+]
> │  └──0.00 MB (00.00%) ++ (2 tiny)
> ├──0.21 MB (19.19%) -- heap-overhead
> │  ├──0.15 MB (13.85%) ── waste
> │  └──0.06 MB (05.35%) ── page-cache
> └──-0.12 MB (-10.73%) ++ (9 tiny)

Sorry wrong comment, this is diff result with/without net_worker.js 
The saving is about 1.1 MB

Comment 18

[Dimi Lee [:dimi]]

Attachment: baseline memory report on Nexus4

baseline memory report on Nexus4
Test with mozilla-central and gaia master

Comment 19

[Dimi Lee [:dimi]]

Attachment: change network from js to c++ memory-reports

change network from js to c++ memory-reports
Test with mozilla central with WIP V1 patch and GAIA master

Comment 20

[[:fabrice] Fabrice Desré]

That's great Dimi! I've seen it take much more in some memory reports (I have one laying around where the net worker eats up 2.5MB).

Comment 21

[Dimi Lee [:dimi]]

(In reply to Fabrice Desré [:fabrice] from comment #20)
> That's great Dimi! I've seen it take much more in some memory reports (I
> have one laying around where the net worker eats up 2.5MB).

Hi Fabrice,
 Yes, i did measure memory report several times, sometimes net worker may consume about 2.5Mb
 In that case it saves more memory.

Comment 22

[James Zhang (Spreadtrum)]

Great job, can this patch land on v1.3 ?

Comment 23

[Steven Yang [:styang]]

We need this to shrink memory. 1.3+

Comment 24

[Dimi Lee [:dimi]]

Attachment: WIP V2

Comment 25

[Jason Smith [:jsmith]]

(In reply to Steven Yang [:styang] from comment #23)
> We need this to shrink memory. 1.3+

This is a feature request with regression risk likely. I'd like to see more reasoning why this has to be included in 1.3 specifically. This should really be only included in tarako, not 1.3.

Comment 26

[[:fabrice] Fabrice Desré]

We'll take that on tarako, but not in 1.3 - too late & too risky for 1.3.

Comment 27

[[:fabrice] Fabrice Desré]

Dimi, is your patch up to date? I had to patch a bit to get it to build:

diff --git a/dom/webidl/DummyBinding.webidl b/dom/webidl/DummyBinding.webidl
--- a/dom/webidl/DummyBinding.webidl
+++ b/dom/webidl/DummyBinding.webidl
@@ -25,13 +25,15 @@ interface DummyInterface : EventTarget {
   void MmsAttachment(optional MmsAttachment arg);
   void AsyncScrollEventDetail(optional AsyncScrollEventDetail arg);
   void OpenWindowEventDetail(optional OpenWindowEventDetail arg);
   void DOMWindowResizeEventDetail(optional DOMWindowResizeEventDetail arg);
   void WifiOptions(optional WifiCommandOptions arg1,
                    optional WifiResultOptions arg2);
   void AppNotificationServiceOptions(optional AppNotificationServiceOptions arg);
   void AppInfo(optional AppInfo arg1);
+  void NetworkOptions(optional NetworkCommandOptions arg1,
+                      optional NetworkResultOptions arg2);
 };
 
 interface DummyInterfaceWorkers {
   BlobPropertyBag blobBag();
 };

Comment 28

[Dimi Lee [:dimi]]

Attachment: WIP networker to c++ v3

Comment 29

[Dimi Lee [:dimi]]

(In reply to Fabrice Desré [:fabrice] from comment #27)
> Dimi, is your patch up to date? I had to patch a bit to get it to build:
> 
> diff --git a/dom/webidl/DummyBinding.webidl b/dom/webidl/DummyBinding.webidl
> --- a/dom/webidl/DummyBinding.webidl
> +++ b/dom/webidl/DummyBinding.webidl
> @@ -25,13 +25,15 @@ interface DummyInterface : EventTarget {
>    void MmsAttachment(optional MmsAttachment arg);
>    void AsyncScrollEventDetail(optional AsyncScrollEventDetail arg);
>    void OpenWindowEventDetail(optional OpenWindowEventDetail arg);
>    void DOMWindowResizeEventDetail(optional DOMWindowResizeEventDetail arg);
>    void WifiOptions(optional WifiCommandOptions arg1,
>                     optional WifiResultOptions arg2);
>    void AppNotificationServiceOptions(optional AppNotificationServiceOptions
> arg);
>    void AppInfo(optional AppInfo arg1);
> +  void NetworkOptions(optional NetworkCommandOptions arg1,
> +                      optional NetworkResultOptions arg2);
>  };
>  
>  interface DummyInterfaceWorkers {
>    BlobPropertyBag blobBag();
>  };

Sorry i missed it. Attach patch v3 to fix this and also fix an compile error on desktop build.

Comment 30

[Dimi Lee [:dimi]]

(In reply to Dimi Lee[:dimi][:dlee] from comment #28)
> Created attachment 8359055 [details] [diff] [review]
> WIP networker to c++ v3

There are some comments i forgot to remove in NetworkUtils::ExecuteCommand.
Will remove it in next patch.

Comment 31

[[:fabrice] Fabrice Desré]

Comment on attachment 8359055 [details] [diff] [review]
WIP networker to c++ v3

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

Looks pretty good to me, and works well on device.

::: dom/system/gonk/NetworkUtils.cpp
@@ +243,5 @@
> +    strcpy(result, array[0].get());
> +    for (uint32_t i = 1; i < array.Length(); i++) {
> +      strcat(result, sep);
> +      strcat(result, array[i].get());
> +    }

We have no guarantee to not overrun result in this function. That's a bit scary.

@@ +267,5 @@
> + */
> +#define GET_CHAR(prop) NS_ConvertUTF16toUTF8(aChain->getParams().prop).get()
> +#define GET_FIELD(prop) aChain->getParams().prop
> +
> +void wifiFirmwareReload(CommandChain* aChain, CALLBACK aCallback, NetworkResultOptions& aResult)

aResult is unused in this function (and on many others). Am I missing something?

@@ +596,5 @@
> +
> +#define RUN_CHAIN(param, cmds, err)  uint32_t size = sizeof(cmds) / sizeof(COMMAND);  \
> +                                     CommandChain* chain = new CommandChain(param, cmds, size, err);  \
> +                                     NetworkResultOptions result;  \
> +                                     next(chain, false, result);

nit: align the \ in the macro

@@ +1241,5 @@
> +}
> +
> +void NetworkUtils::dumpParams(NetworkParams& aOptions, const char* aType)
> +{
> +#ifdef USE_DEBUG

why not just #ifdef DEBUG ?

::: dom/system/gonk/NetworkUtils.h
@@ +18,5 @@
> +
> +typedef void (*POSTMESSAGE)(NetworkResultOptions& aResult);
> +typedef void (*CALLBACK)(CommandChain*, bool, NetworkResultOptions& aResult);
> +typedef void (*ERROR_CALLBACK)(NetworkParams& aOptions, NetworkResultOptions& aResult);
> +typedef void (*COMMAND)(CommandChain*, CALLBACK, NetworkResultOptions& aResult);

nit: I'm not a big fan of ALLCAPS here. That make them look like constants.

::: dom/webidl/NetworkOptions.webidl
@@ +43,5 @@
> +  DOMString dns2;                     // for "setWifiTethering".
> +  float rxBytes;                      // for "getNetworkInterfaceStats".
> +  float txBytes;                      // for "getNetworkInterfaceStats".
> +  DOMString date;                     // for "getNetworkInterfaceStats".
> +  long threshold;                     // for "setNetworkInterfaceAlarm", 

nit: trailing whitespace

Comment 32

[Dimi Lee [:dimi]]

(In reply to Fabrice Desré [:fabrice] from comment #31)
> Comment on attachment 8359055 [details] [diff] [review]
> WIP networker to c++ v3
> 
> Review of attachment 8359055 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks pretty good to me, and works well on device.
> 
> ::: dom/system/gonk/NetworkUtils.cpp
> @@ +243,5 @@
> > +    strcpy(result, array[0].get());
> > +    for (uint32_t i = 1; i < array.Length(); i++) {
> > +      strcat(result, sep);
> > +      strcat(result, array[i].get());
> > +    }
> 
> We have no guarantee to not overrun result in this function. That's a bit
> scary.
> 
> @@ +267,5 @@
> > + */
> > +#define GET_CHAR(prop) NS_ConvertUTF16toUTF8(aChain->getParams().prop).get()
> > +#define GET_FIELD(prop) aChain->getParams().prop
> > +
> > +void wifiFirmwareReload(CommandChain* aChain, CALLBACK aCallback, NetworkResultOptions& aResult)
> 
> aResult is unused in this function (and on many others). Am I missing
> something?
> 

Take wifiFirmwareReload for example, it is store in the command chain gWifiEnableChain. And this chain also
contain function wifiTetheringSuccess.
The wifiTetheringSuccess will use aResult to post message to main thread.

And some functions like getTxBytes will also use return result from netd.

Comment 33

[Dimi Lee [:dimi]]

Attachment: WIP networker to c++ v4

Comment 34

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 8359583 [details] [diff] [review]
WIP networker to c++ v4

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

Thanks for your help on this bug. The overall patches looks good. Please see my commands below.

::: dom/system/gonk/NetworkUtils.cpp
@@ +98,5 @@
> +inline bool isProceeding(uint32_t code)
> +{
> +  uint32_t type = netdResponseType(code);
> +  return type == NETD_COMMAND_PROCEEDING;
> +}

Why these functions are declared as global scope ? Please declare they as private member functions if they are used in NetworkUtils class only.

@@ +117,5 @@
> +static CommandChain* gCurrentChain = nullptr;
> +static bool gPending = false;
> +static nsTArray<nsCString> gReason;
> +
> +static void next(CommandChain* aChain, bool aError, NetworkResultOptions& aResult)

Please add const if aResult is not modified in this function.

@@ +135,5 @@
> +    return;
> +  }
> +
> +  (*f)(aChain, next, aResult);
> +}

How do you think if we move this to a member function of CommandChain class ?

@@ +146,5 @@
> +  if (gCommandQueue.IsEmpty() || gPending) {
> +    return;
> +  }
> +  QueueData data = gCommandQueue[0];
> +  gCommandQueue.RemoveElementAt(0);

You remove the queue here and add the queue in doCommand() function. 
Please make sure these two functions are called in the same thread, or you should  
protect gCommandQueue. Add MOZ_ASSERT(is net worker thread) should be a good idea.

@@ +153,5 @@
> +  gCurrentCallback = data.b;
> +  gCurrentChain = data.c;
> +
> +  gPending = true;
> +

The command chain ownership is passed to gCurrentxxx so that you can pop out the command queue here. gCurrentxxx are global variables and protected by gPending flag. It would be nicer to minimize the use of global variables if we move these global to gCommandQueue.

@@ +173,5 @@
> +  }
> +  netdCommand->mSize = strlen((char*)netdCommand->mData) + 1;
> +
> +  gCommandQueue.AppendElement(QueueData(netdCommand, aCallback, aChain));
> +

Ditto, please make sure this function is called in the same thread as nextNedCommand(). 
Add MOZ_ASSERT(is net worker thread) should be a good idea.

@@ +179,5 @@
> +}
> +
> +static void postMessage(NetworkResultOptions& aResult)
> +{
> +  (*(gNetworkUtils->mMessageCallback))(aResult);

This is static function and can be misuse even if gNetworkUtils is not assigned a initial value. 
add MOZ_ASSERT(gNetworkUtils, "xxxx") and MOZ_ASSERT(gNetworkUtils->mPostCallback, "xxxx").

@@ +185,5 @@
> +
> +static void postMessage(NetworkParams& aOptions, NetworkResultOptions& aResult)
> +{
> +  aResult.mId = aOptions.mId;
> +  (*(gNetworkUtils->mMessageCallback))(aResult);

Ditto: Please add the asserts
MOZ_ASSERT(gNetworkUtils)
MOZ_ASSERT(gNetworkUtils->mPostCallback)

@@ +189,5 @@
> +  (*(gNetworkUtils->mMessageCallback))(aResult);
> +}
> +
> +static void sendBroadcastMessage(uint32_t code, char* reason)
> +{

Why do you declare this function static scope ? It seems only used in NetworkUtils class.

@@ +211,5 @@
> +/**
> + * Helper function to get the bit length from given mask.
> + */
> +static uint32_t getMaskLength(const uint32_t mask)
> +{

Ditto, can this be a member function of NetworkUtils class ?

@@ +236,5 @@
> +
> +/**
> + * Helper function that implement join function.
> + */
> +static void join(nsTArray<nsCString>& array, const char* sep, const uint32_t maxlen, char* result)

Ditto, can this be a member function of NetworkUtils class ?

@@ +281,5 @@
> + */
> +#define GET_CHAR(prop) NS_ConvertUTF16toUTF8(aChain->getParams().prop).get()
> +#define GET_FIELD(prop) aChain->getParams().prop
> +
> +void wifiFirmwareReload(CommandChain* aChain, CommandCallback aCallback, NetworkResultOptions& aResult)

Can you add the comments for this function ? Also these netd command functions are used in CommandChain, can we move them to CommandChain Class ?

@@ +290,5 @@
> +  doCommand(command, aChain, aCallback);
> +}
> +
> +void startAccessPointDriver(CommandChain* aChain, CommandCallback aCallback, NetworkResultOptions& aResult)
> +{

Ditto.

@@ +306,5 @@
> +  doCommand(command, aChain, aCallback);
> +}
> +
> +void stopAccessPointDriver(CommandChain* aChain, CommandCallback aCallback, NetworkResultOptions& aResult)
> +{

Ditto.

@@ +361,5 @@
> +  doCommand(command, aChain, aCallback);
> +}
> +
> +void cleanUpStream(CommandChain* aChain, CommandCallback aCallback, NetworkResultOptions& aResult)
> +{

Ditto.

@@ +369,5 @@
> +  doCommand(command, aChain, aCallback);
> +}
> +
> +void createUpStream(CommandChain* aChain, CommandCallback aCallback, NetworkResultOptions& aResult)
> +{

Ditto.

@@ +377,5 @@
> +  doCommand(command, aChain, aCallback);
> +}
> +
> +void startSoftAP(CommandChain* aChain, CommandCallback aCallback, NetworkResultOptions& aResult)
> +{

Ditto.

@@ +383,5 @@
> +  doCommand(command, aChain, aCallback);
> +}
> +
> +void stopSoftAP(CommandChain* aChain, CommandCallback aCallback, NetworkResultOptions& aResult)
> +{

Ditto.

@@ +389,5 @@
> +  doCommand(command, aChain, aCallback);
> +}
> +
> +void getRxBytes(CommandChain* aChain, CommandCallback aCallback, NetworkResultOptions& aResult)
> +{

Ditto.

@@ +397,5 @@
> +  doCommand(command, aChain, aCallback);
> +}
> +
> +void getTxBytes(CommandChain* aChain, CommandCallback aCallback, NetworkResultOptions& aResult)
> +{

Ditto.

@@ +408,5 @@
> +  doCommand(command, aChain, aCallback);
> +}
> +
> +void enableAlarm(CommandChain* aChain, CommandCallback aCallback, NetworkResultOptions& aResult)
> +{

Ditto.

@@ +414,5 @@
> +  doCommand(command, aChain, aCallback);
> +}
> +
> +void disableAlarm(CommandChain* aChain, CommandCallback aCallback, NetworkResultOptions& aResult)
> +{

Ditto.

@@ +420,5 @@
> +  doCommand(command, aChain, aCallback);
> +}
> +
> +void setQuota(CommandChain* aChain, CommandCallback aCallback, NetworkResultOptions& aResult)
> +{

Ditto.

@@ +428,5 @@
> +  doCommand(command, aChain, aCallback);
> +}
> +
> +void removeQuota(CommandChain* aChain, CommandCallback aCallback, NetworkResultOptions& aResult)
> +{

Ditto.

@@ +436,5 @@
> +  doCommand(command, aChain, aCallback);
> +}
> +
> +void setAlarm(CommandChain* aChain, CommandCallback aCallback, NetworkResultOptions& aResult)
> +{

Ditto.

@@ +444,5 @@
> +  doCommand(command, aChain, aCallback);
> +}
> +
> +void setInterfaceUp(CommandChain* aChain, CommandCallback aCallback, NetworkResultOptions& aResult)
> +{

Ditto.

@@ +463,5 @@
> +  doCommand(command, aChain, aCallback);
> +}
> +
> +void tetherInterface(CommandChain* aChain, CommandCallback aCallback, NetworkResultOptions& aResult)
> +{

Ditto.

@@ +471,5 @@
> +  doCommand(command, aChain, aCallback);
> +}
> +
> +void preTetherInterfaceList(CommandChain* aChain, CommandCallback aCallback, NetworkResultOptions& aResult)
> +{

Ditto.

@@ +483,5 @@
> +  doCommand(command, aChain, aCallback);
> +}
> +
> +void postTetherInterfaceList(CommandChain* aChain, CommandCallback aCallback, NetworkResultOptions& aResult)
> +{

Ditto.

@@ +497,5 @@
> +  doCommand(command, aChain, aCallback);
> +}
> +
> +void setIpForwardingEnabled(CommandChain* aChain, CommandCallback aCallback, NetworkResultOptions& aResult)
> +{

Ditto.

@@ +516,5 @@
> +  doCommand(command, aChain, aCallback);
> +}
> +
> +void tetheringStatus(CommandChain* aChain, CommandCallback aCallback, NetworkResultOptions& aResult)
> +{

Ditto.

@@ +522,5 @@
> +  doCommand(command, aChain, aCallback);
> +}
> +
> +void stopTethering(CommandChain* aChain, CommandCallback aCallback, NetworkResultOptions& aResult)
> +{

Ditto.

@@ +537,5 @@
> +  doCommand(command, aChain, aCallback);
> +}
> +
> +void startTethering(CommandChain* aChain, CommandCallback aCallback, NetworkResultOptions& aResult)
> +{

Ditto.

@@ +559,5 @@
> +  doCommand(command, aChain, aCallback);
> +}
> +
> +void untetherInterface(CommandChain* aChain, CommandCallback aCallback, NetworkResultOptions& aResult)
> +{

Ditto.

@@ +567,5 @@
> +  doCommand(command, aChain, aCallback);
> +}
> +
> +void setDnsForwarders(CommandChain* aChain, CommandCallback aCallback, NetworkResultOptions& aResult)
> +{

Ditto.

@@ +575,5 @@
> +  doCommand(command, aChain, aCallback);
> +}
> +
> +void enableNat(CommandChain* aChain, CommandCallback aCallback, NetworkResultOptions& aResult)
> +{

Ditto.

@@ +583,5 @@
> +  doCommand(command, aChain, aCallback);
> +}
> +
> +void disableNat(CommandChain* aChain, CommandCallback aCallback, NetworkResultOptions& aResult)
> +{

Ditto.

@@ +596,5 @@
> +
> +static CommandFunc gWifiFailChain[] = {stopSoftAP,
> +                                   setIpForwardingEnabled,
> +                                   stopTethering};
> +

Can you modify the coding style here and below ? 

static CommandFunc gWifiFailChain[] = {
  stopSoftAP,
  .....
}

@@ +600,5 @@
> +
> +static CommandFunc gUSBFailChain[] = {stopSoftAP,
> +                                  setIpForwardingEnabled,
> +                                  stopTethering};
> +

Ditto.

@@ +610,5 @@
> +
> +#define RUN_CHAIN(param, cmds, err)  uint32_t size = sizeof(cmds) / sizeof(CommandFunc);              \
> +                                     CommandChain* chain = new CommandChain(param, cmds, size, err);  \
> +                                     NetworkResultOptions result;                                     \
> +                                     next(chain, false, result);

Move to new line. 

#define RUN_CHAIN(param, cmds, err)                \
  uint32_t size = sizeof(cmds)/ sizeof(COMMAND);  \
  ....

If you move next to CommandChain, this will become chain->next(...)

@@ +612,5 @@
> +                                     CommandChain* chain = new CommandChain(param, cmds, size, err);  \
> +                                     NetworkResultOptions result;                                     \
> +                                     next(chain, false, result);
> +
> +void wifiTetheringFail(NetworkParams& aOptions, NetworkResultOptions& aResult)

Add the const to reference to object argument declaration here and below if they are not modified.  
void wifiTetheringFail(const NetworkParams& aOptions, const NetworkResultOptions& aResult)

@@ +624,5 @@
> +  RUN_CHAIN(aOptions, gWifiFailChain, nullptr)
> +}
> +
> +void wifiTetheringSuccess(CommandChain* aChain, CommandCallback aCallback, NetworkResultOptions& aResult)
> +{

Ditto.

@@ +630,5 @@
> +  postMessage(aChain->getParams(), aResult);
> +}
> +
> +void usbTetheringFail(NetworkParams& aOptions, NetworkResultOptions& aResult)
> +{

Ditto.

@@ +651,5 @@
> +  }
> +}
> +
> +void usbTetheringSuccess(CommandChain* aChain, CommandCallback aCallback, NetworkResultOptions& aResult)
> +{

Ditto.

@@ +657,5 @@
> +  postMessage(aChain->getParams(), aResult);
> +}
> +
> +void networkInterfaceStatsFail(NetworkParams& aOptions, NetworkResultOptions& aResult)
> +{

Ditto.

@@ +662,5 @@
> +  postMessage(aOptions, aResult);
> +}
> +
> +void networkInterfaceStatsSuccess(CommandChain* aChain, CommandCallback aCallback, NetworkResultOptions& aResult)
> +{

Ditto.

@@ +669,5 @@
> +  postMessage(aChain->getParams(), aResult);
> +}
> +
> +void networkInterfaceAlarmFail(NetworkParams& aOptions, NetworkResultOptions& aResult)
> +{

Ditto.

@@ +674,5 @@
> +  postMessage(aOptions, aResult);
> +}
> +
> +void networkInterfaceAlarmSuccess(CommandChain* aChain, CommandCallback aCallback, NetworkResultOptions& aResult)
> +{

Ditto.

@@ +680,5 @@
> +  // params.error = parseFloat(params.resultReason);
> +  postMessage(aChain->getParams(), aResult);
> +}
> +
> +void updateUpStreamFail(NetworkParams& aOptions, NetworkResultOptions& aResult)

Ditto.

@@ +686,5 @@
> +  postMessage(aOptions, aResult);
> +}
> +
> +void updateUpStreamSuccess(CommandChain* aChain, CommandCallback aCallback, NetworkResultOptions& aResult)
> +{

Ditto.

@@ +693,5 @@
> +  postMessage(aChain->getParams(), aResult);
> +}
> +
> +void setDhcpServerFail(NetworkParams& aOptions, NetworkResultOptions& aResult)
> +{

Ditto.

@@ +699,5 @@
> +  postMessage(aOptions, aResult);
> +}
> +
> +void setDhcpServerSuccess(CommandChain* aChain, CommandCallback aCallback, NetworkResultOptions& aResult)
> +{

Ditto.

@@ +705,5 @@
> +  postMessage(aChain->getParams(), aResult);
> +}
> +
> +void wifiOperationModeFail(NetworkParams& aOptions, NetworkResultOptions& aResult)
> +{

Ditto.

@@ +710,5 @@
> +  postMessage(aOptions, aResult);
> +}
> +
> +void wifiOperationModeSuccess(CommandChain* aChain, CommandCallback aCallback, NetworkResultOptions& aResult)
> +{

Ditto.

@@ +724,5 @@
> +                                        tetherInterface,
> +                                        tetheringStatus,
> +                                        startTethering,
> +                                        setDnsForwarders,
> +                                        usbTetheringSuccess};

Move to new line. 

static CommandFunc gUSBEnableChain[] = {
  setInterfaceUp,
  ...
};

@@ +733,5 @@
> +                                         disableNat,
> +                                         setIpForwardingEnabled,
> +                                         stopTethering,
> +                                         usbTetheringSuccess};
> +

Ditto.

@@ +746,5 @@
> +                                         startTethering,
> +                                         setDnsForwarders,
> +                                         enableNat,
> +                                         wifiTetheringSuccess};
> +

Ditto.

@@ +756,5 @@
> +                                          postTetherInterfaceList,
> +                                          disableNat,
> +                                          setIpForwardingEnabled,
> +                                          stopTethering,
> +                                          wifiTetheringSuccess};

Ditto.

@@ +760,5 @@
> +                                          wifiTetheringSuccess};
> +
> +static CommandFunc gStartDhcpServerChain[] = {setInterfaceUp,
> +                                              startTethering,
> +                                              setDhcpServerSuccess};

Ditto.

@@ +764,5 @@
> +                                              setDhcpServerSuccess};
> +
> +static CommandFunc gStopDhcpServerChain[] = {stopTethering,
> +                                             setDhcpServerSuccess};
> +

Ditto.

@@ +768,5 @@
> +
> +static CommandFunc gNetworkInterfaceStatsChain[] = {getRxBytes,
> +                                                    getTxBytes,
> +                                                    networkInterfaceStatsSuccess};
> +

Ditto.

@@ +773,5 @@
> +static CommandFunc gNetworkInterfaceEnableAlarmChain[] = {enableAlarm,
> +                                                          setQuota,
> +                                                          setAlarm,
> +                                                      networkInterfaceAlarmSuccess};
> +

Ditto.

@@ +776,5 @@
> +                                                      networkInterfaceAlarmSuccess};
> +
> +static CommandFunc gNetworkInterfaceDisableAlarmChain[] = {removeQuota,
> +                                                           disableAlarm,
> +                                                           networkInterfaceAlarmSuccess};

Ditto.

@@ +780,5 @@
> +                                                           networkInterfaceAlarmSuccess};
> +
> +static CommandFunc gNetworkInterfaceSetAlarmChain[] = {setAlarm,
> +                                                       networkInterfaceAlarmSuccess};
> +

Ditto.

@@ +782,5 @@
> +static CommandFunc gNetworkInterfaceSetAlarmChain[] = {setAlarm,
> +                                                       networkInterfaceAlarmSuccess};
> +
> +static CommandFunc gWifiOperationModeChain[] = {wifiFirmwareReload,
> +                                                wifiOperationModeSuccess};

Ditto.

@@ +787,5 @@
> +
> +static CommandFunc gUpdateUpStreamChain[] = {cleanUpStream,
> +                                             createUpStream,
> +                                             updateUpStreamSuccess};
> +

Ditto.

Comment 35

[Dimi Lee [:dimi]]

Attachment: WIP networker to c++ v5

Comment 36

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 8360239 [details] [diff] [review]
WIP networker to c++ v5

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

This looks pretty good overall. Just a couple of nits. I will try to do some test manually tomorrow. If everything goes well, I'll r+ the patch.

::: dom/system/gonk/NetworkUtils.cpp
@@ +19,5 @@
> +#include <cutils/properties.h>
> +#include "mozilla/dom/network/NetUtils.h"
> +
> +#define _DEBUG 1
> +

Nit: Please remember to set _DEBUG to zero when you commit the patches.

@@ +323,5 @@
> +  snprintf(gCurrentCommand.command, MAX_COMMAND_SIZE - 1, "%s", GET_CURRENT_COMMAND);
> +
> +  DEBUG("Sending \'%s\' command to netd.", gCurrentCommand.command);
> +  SendNetdCommand(GET_CURRENT_NETD_COMMAND);
> + 

Nit: extra space.

Comment 37

[[:fabrice] Fabrice Desré]

Comment on attachment 8360239 [details] [diff] [review]
WIP networker to c++ v5

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

I tested locally, and could get both wifi and data connections working.

Comment 38

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 8360239 [details] [diff] [review]
WIP networker to c++ v5

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

I have tested wifi/wifi hotspot/usb tethering/3G data connection/cost control, and these functions work pretty well. r=me with small nits addressed.

Comment 39

[Dimi Lee [:dimi]]

Attachment: networker to c++ v6

Comment 40

[Vincent Chang[:vchang][changyihsin]]

Hi Dimi, how about the try server result here ? If everything is fine, I would like this patch be landed soon.

Comment 41

[Dimi Lee [:dimi]]

(In reply to Vincent Chang[:vchang] from comment #40)
> Hi Dimi, how about the try server result here ? If everything is fine, I
> would like this patch be landed soon.

Hi Vincent,
  It seems there are some error on Mochitest 
  https://tbpl.mozilla.org/?tree=Try&rev=8b800382f044

  But because i am at NFC work week, i might not be able to check it soon :(

Comment 42

[James Ho]

Vincent, would you like to help here? We need this to land soon.

Comment 43

[Vincent Chang[:vchang][changyihsin]]

(In reply to James Ho from comment #42)
> Vincent, would you like to help here? We need this to land soon.

I am helping here and trying to fix the try server error right now.

Comment 44

[Vincent Chang[:vchang][changyihsin]]

Push my modified patch to try server. 

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

Comment 45

[Vincent Chang[:vchang][changyihsin]]

Fixed xpcshell errors with local test using below command. 
./test.sh xpcshell --test-path dom/network/tests/unit_stats/

push to try server again....

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

Comment 46

[Dimi Lee [:dimi]]

Attachment: networker to c++ v7

Hi Fabrice,
This patch fixs xpcshell test failure and all the fix are in NetworkService.js.
The problem is that asynchronous call is being called when xpcom is shutdown,
so the callback may cause crash when returned.

Comment 47

[Dimi Lee [:dimi]]

Attachment: networker to c++ v7

Comment 48

[Dimi Lee [:dimi]]

Comment on attachment 8365862 [details] [diff] [review]
networker to c++ v7

Re-attach since some style error, please see Comment 46.

Comment 49

[Dimi Lee [:dimi]]

Try server result : Passed
https://tbpl.mozilla.org/?tree=Try&rev=65fb7f6c934b

Comment 50

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8365862 [details] [diff] [review]
networker to c++ v7

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

::: dom/system/gonk/NetworkWorker.cpp
@@ +148,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(aListener);
> +
> +  WARN("[Dimi]NetworkWorker - Start");

I guess we don't need this.

Same for below.

Comment 51

[Dimi Lee [:dimi]]

Attachment: network to c++ v8

Comment 52

[Dimi Lee [:dimi]]

Attachment: networker to c++ v8

Comment 53

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

I got conflict when pushing to inbound
please rebase again.

Comment 54

[Dimi Lee [:dimi]]

Attachment: networker to c++ v9

Hi Yoshi,
  I try rebase on in-bound and there is no conflict, attach a new patch merged with in-bound.
  Could you help check if it is ok ? Thanks!

Comment 55

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/496458acf82a

Comment 56

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/496458acf82a

Comment 57

[Joe Cheng [:jcheng] (please needinfo)]

use 1.3T+ flag instead of [tarako][POVB] in whiteboard

Comment 58

[[:fabrice] Fabrice Desré]

Dimi, can you rebase this patch on 1.3t (and the couple of followups we had too)? thanks!

Comment 59

[Joe Cheng [:jcheng] (please needinfo)]

using status-b2g-v1.3T, remove [tarako]

Comment 60

[Dimi Lee [:dimi]]

(In reply to Fabrice Desré [:fabrice] from comment #58)
> Dimi, can you rebase this patch on 1.3t (and the couple of followups we had
> too)? thanks!

I am working on it :)

Comment 61

[Dimi Lee [:dimi]]

Attachment: networker to c++ fix for 1.3t

Comment 62

[[:fabrice] Fabrice Desré]

 https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/801d467d1b08