932630 - [NetworkStats API] Need to fix saveAppStats(...) in test_networkstats_service_proxy.js

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Gene Lian (I already quit Mozilla)]

Since we change saveAppStats(...) to eat nsINetworkInterface:

  void saveAppStats(in unsigned long aAppId,
                    in nsINetworkInterface aNetwork,
                    in unsigned long long aTimeStamp,
                    in unsigned long long aRxBytes,
                    in unsigned long long aTxBytes,
         [optional] in nsINetworkStatsServiceProxyCallback aCallback);

We need to fix the calls in test_networkstats_service_proxy.js. Like:

  nssProxy.saveAppStats(1, wifi, timestamp, 10, 20);
  nssProxy.saveAppStats(1, mobile, timestamp, 10, 20);

and so on.

How could the original test still work? John, could you please take a look?

Comment 1

[John Shih]

ok, I'll look at this

Comment 2

[John Shih]

I still curious about whether these test cases are executed on try server or not...
(including test_networkstats_db.js
           test_networkstats_service.js
           test_networkstats_service_proxy.js)

as I search the log in https://tbpl.mozilla.org/?tree=Try&rev=830274a9e4ce
I can only see the log with
test_networkstats_disabled.html
test_networkstats_enabled_no_perm.html
test_networkstats_enabled_perm.html
test_networkstats-manage.html

who can help checking on this? makefile seems like no problem

Comment 3

[John Shih]

Besides, for the code

  nssProxy.saveAppStats(1, wifi, timestamp, 10, 20);
  nssProxy.saveAppStats(1, mobile, timestamp, 10, 20);

since the nsINetworkInterface cannot be obtained in xpcshell,
the workaround here is to create the fake nsINetworkInterface 

  var wifi = {type: Ci.nsINetworkInterface.NETWORK_TYPE_WIFI, id: "0"};
  var mobile = {type: Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE, id: "1234"};

as what Albert did originally.
so looks like it is an ok solution.

Comment 4

[Gene Lian (I already quit Mozilla)]

I wonder you need the id.

Comment 5

[Albert [:albert]]

The id is used to insert the fake interface in networkStatsService. IF you want to keep code more clear we can define it outside of wifi and mobile vars. Is this your concern?

Comment 6

[Gene Lian (I already quit Mozilla)]

Because I saw we already had mokConvertNetworkInterface() to add them. Hmm... to have a more real test case, maybe keeping them is better.

Comment 7

[Gregor Wagner [:gwagner]]

Can we also get rid of all the assertions or should we file a separate bug?
09:51:20     INFO -  ************************************************************
09:51:20     INFO -  * Call to xpconnect wrapped JSObject produced this error:  *
09:51:21     INFO -  [Exception... "'[JavaScript Error: "aNetwork is null" {file: "resource://gre/modules/NetworkStatsService.jsm" line: 192}]' when calling method: [nsINetworkStatsServiceProxy::saveAppStats]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: yes]
09:51:21     INFO -  ************************************************************

Comment 8

[John Shih]

(In reply to Gregor Wagner [:gwagner] from comment #7)
> Can we also get rid of all the assertions or should we file a separate bug?
> 09:51:20     INFO - 
> ************************************************************
> 09:51:20     INFO -  * Call to xpconnect wrapped JSObject produced this
> error:  *
> 09:51:21     INFO -  [Exception... "'[JavaScript Error: "aNetwork is null"
> {file: "resource://gre/modules/NetworkStatsService.jsm" line: 192}]' when
> calling method: [nsINetworkStatsServiceProxy::saveAppStats]"  nsresult:
> "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "native
> frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: yes]
> 09:51:21     INFO - 
> ************************************************************

where did you get this message? was it in the try server or during the runtime?

Comment 9

[John Shih]

can you provide more detail on the error message you posted? I'd like to figure out what it is.
Thanks!

Comment 10

[Gregor Wagner [:gwagner]]

(In reply to John Shih from comment #9)
> can you provide more detail on the error message you posted? I'd like to
> figure out what it is.
> Thanks!

I see this very often running mochitests on a debug-enabled emulator. These builds are enabled on 'pine'
Fore example: https://tbpl.mozilla.org/php/getParsedLog.php?id=30242540&tree=Pine&full=1

Comment 11

[Gregor Wagner [:gwagner]]

Any update here?
You can see for example here: https://tbpl.mozilla.org/php/getParsedLog.php?id=30447610&tree=Pine&full=1#error0
and just grep for NetworkStatsService
Its all over the place.

Comment 12

[John Shih]

(In reply to Gregor Wagner [:gwagner] from comment #11)
> Any update here?
> You can see for example here:
> https://tbpl.mozilla.org/php/getParsedLog.
> php?id=30447610&tree=Pine&full=1#error0
> and just grep for NetworkStatsService
> Its all over the place.

I will try to reproduce this bug and figure out how to fix it.

Comment 13

[Albert [:albert]]

If it helps, I see same errors launching mochitest with a emulator build:

./config.sh emulator
./build.sh
./mach mochitest-remote

Comment 14

[John Shih]

(In reply to Albert [:albert] from comment #13)
> If it helps, I see same errors launching mochitest with a emulator build:
> 
> ./config.sh emulator
> ./build.sh
> ./mach mochitest-remote

Thanks for your information!
do you have any thought on this bug?
I think it is due to the nsINetworkManager is unavailable in the mochitest...

Comment 15

[Albert [:albert]]

As I see NetworkManager is available and has at least one network in my laptop for wlan in NetworkManager.networks but NetworkManager.active is null. It is due to wlan state is disconnected (don't know why) and it is not assigned as active.

From my point of view you have two options:
- Check if network is null in NetworkStatsServiceProxy and return if so.
- If active network is null, try to find some network from NetworkManager.networks

Comment 16

[John Shih]

(In reply to Albert [:albert] from comment #15)
> As I see NetworkManager is available and has at least one network in my
> laptop for wlan in NetworkManager.networks but NetworkManager.active is
> null. It is due to wlan state is disconnected (don't know why) and it is not
> assigned as active.
> 
> From my point of view you have two options:
> - Check if network is null in NetworkStatsServiceProxy and return if so.
> - If active network is null, try to find some network from
> NetworkManager.networks

has this issue to do with bug 908553?
wifi is actually not supported in emulator right now.
but yes, there is still some workaroud as you suggested.

Comment 17

[John Shih]

Attachment: Bug 932630 - Avoid to call in saveAppStats(...) if aNetwork is null. r=gene

This is to fix the error caused by calling saveAppStats(...) in NetworkStatsServiceProxy.js.

Although I'm not sure the root cause of this error (even I disable the
test_networkstats_service_proxy.js, I can still see this error message,
and I cannot find any other places that would call that function during
the mochitests-run).

However, add an additional check of aNetwork seems to be a reasonable way to avoid this problem.

Comment 18

[Gene Lian (I already quit Mozilla)]

Comment on attachment 832712 [details] [diff] [review]
Bug 932630 -  Avoid to call in saveAppStats(...) if aNetwork is null. r=gene

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

Honestly, I don't like this work-around but I don't have other better thoughts, though.

::: dom/network/src/NetworkStatsServiceProxy.js
@@ +35,5 @@
>        debug("saveAppStats: " + aAppId + " connectionType " + aNetwork.type +
>              " " + aTimeStamp + " " + aRxBytes + " " + aTxBytes);
>      }
>  
> +    if (!aNetwork) {

Please before move this check before the above |if (DEBUG) {...}| which also uses |aNetwork.type|.

Also, please add a debug message for it:

if (!aNetwork) {
  if (DEBUG) debug("|aNetwork| is not specified. Failed to save stats. Returning.");
  return;
}

Comment 19

[John Shih]

Attachment: Bug 932630 - Avoid to call in saveAppStats(...) if aNetwork is null. r=gene

patch update.

Comment 20

[Gene Lian (I already quit Mozilla)]

https://hg.mozilla.org/integration/b2g-inbound/rev/0713fab226df

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0713fab226df