Closed Bug 932630 Opened 11 years ago Closed 10 years ago

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

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: airpingu, Assigned: johnshih.bugs)

References

Details

Attachments

(1 file, 1 obsolete file)

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?
ok, I'll look at this
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
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.
I wonder you need the id.
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?
Because I saw we already had mokConvertNetworkInterface() to add them. Hmm... to have a more real test case, maybe keeping them is better.
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 -  ************************************************************
(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?
can you provide more detail on the error message you posted? I'd like to figure out what it is.
Thanks!
Flags: needinfo?(anygregor)
(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
Flags: needinfo?(anygregor)
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.
Flags: needinfo?(jshih)
(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.
Flags: needinfo?(jshih)
If it helps, I see same errors launching mochitest with a emulator build:

./config.sh emulator
./build.sh
./mach mochitest-remote
(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...
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
(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.
Depends on: 908553
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.
Attachment #832712 - Flags: review?(gene.lian)
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;
}
Attachment #832712 - Flags: review?(gene.lian) → review+
patch update.
Attachment #832712 - Attachment is obsolete: true
Attachment #832811 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0713fab226df
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.