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

There are four relevant workers here: * RIL worker (bug 864927) * Net worker (this bug) * wifi worker x2 (bug 864932)

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

Vincent, please do!

Dimi, are you actively working on that?

(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.

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

(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.

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

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

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

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 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

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)

(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

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

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

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).

(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.

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

We need this to shrink memory. 1.3+

(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.

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

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(); };

(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.

(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 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

(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 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 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 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 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.

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

(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 :(

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

(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.

Push my modified patch to try server. https://tbpl.mozilla.org/?tree=Try&rev=623eba08f5e9

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

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 on attachment 8365862 [details] [diff] [review] networker to c++ v7 Re-attach since some style error, please see Comment 46.

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

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.

I got conflict when pushing to inbound please rebase again.

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!

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

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

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

(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 :)

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