884144 - gonk version of nr_stun_get_addrs() doesn't work in WebRTC C++ unit tests

Status: RESOLVED WONTFIX

(Core :: WebRTC: Networking, defect, P5)

Description

[John Lin [:jhlin][:jolin]]

Gonk implementation from the patch for bug 867933 uses NetworkInterfaceListService & NetworkManager to get the addresses, and failed in unit tests because:
1. their components were not registered during XPCOM initialization
2. even registered, the test executable has to create the services explicitly
3. the network interfaces have to be registered to NetworkManager explicitly

b2g+content processes won't have this problem, only standalone executables do.

failed tests:
mtransport/transport_unittests
- TransportTest.TestConnectIce
- TransportTest.TestTransferIce
webrtc/signaling_unittests
- all

Comment 1

[John Lin [:jhlin][:jolin]]

Attachment: A proposed fix: call original nr_stun_get_addrs() when gonk implementation fail

Comment 2

[John Lin [:jhlin][:jolin]]

Attachment: Proposed fix v2: use private nsINetworkInterfaceListService when running tests on B2G.

This proposal injects a private nsINetworkInterfaceListService implmentation into XPCOM for gonk_addrs.c:GetInterfaces() to call when running tests on B2G. This way nothing outside test code need to be tweaked (as in last prososal).

Comment 3

[Eric Rescorla (:ekr)]

Comment on attachment 765883 [details] [diff] [review]
Proposed fix v2: use private nsINetworkInterfaceListService when running tests on B2G.

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

This looks conceptually good to me, but I'm not the man to review the
service code. Re-assigning to jesup.

Comment 4

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Comment on attachment 765883 [details] [diff] [review]
Proposed fix v2: use private nsINetworkInterfaceListService when running tests on B2G.

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

I am not familiar with testing enough to review this patch, sorry.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 765883 [details] [diff] [review]
Proposed fix v2: use private nsINetworkInterfaceListService when running tests on B2G.

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

::: media/mtransport/test/private_nils.h
@@ +221,5 @@
> +  *_retval = interface;
> +  return NS_OK;
> +}
> +
> +// Same as in addrs.c

Mostly the same as stun_get_siocgifconf_addrs() in addrs.c (it's not exactly the same)

@@ +266,5 @@
> +      }
> +
> +      struct sockaddr_in* addr_in = (sockaddr_in*)&ifr2.ifr_addr;
> +      char addr_str[INET_ADDRSTRLEN + 1];
> +      inet_ntop(AF_INET, &(addr_in->sin_addr), addr_str, INET_ADDRSTRLEN + 1);

Minor - I'd prefer "sizeof(addr_str)" - that way it can't ever be wrong if someone changes addr_str's defintion.
Unlikely to ever happen here in practice, but a good habit.

@@ +274,5 @@
> +
> +   close(s);
> +
> +    _status = 0;
> +    return _status;

indent is wrong

@@ +315,5 @@
> +  rv = reg->RegisterFactory(kListIID,
> +    "Mock NetworkInterfaceList implementation",
> +    LIST_CONTRACTID,
> +    new mozilla::GenericFactory(PrivateNetworkInterfaceListConstructor));
> +  MOZ_ASSERT(NS_SUCCEED(rv));

While I'm not saying this won't work, I'm surprised by the code pattern - direct use of RegisterFactory() here.  Nothing else in the mozilla codebase uses RegisterFactory directly that I can find.
Almost all (all really that I can find) services are instantiated via XPCOM modules.  Moving r? to bsmedberg, or just take it as an r- to add these to a Module.

Comment 6

[Benjamin Smedberg]

Comment on attachment 765883 [details] [diff] [review]
Proposed fix v2: use private nsINetworkInterfaceListService when running tests on B2G.

I'm not qualified to review networking changes here. But as a build-config peer I'm sure that the definition of MOZ_GONK here isn't right... Unless I'm mistaken about the defines, MOZ_B2G already exists for this purpose.

Comment 7

[John Lin [:jhlin][:jolin]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #6)
> Comment on attachment 765883 [details] [diff] [review]
> Proposed fix v2: use private nsINetworkInterfaceListService when running
> tests on B2G.
> 
> I'm not qualified to review networking changes here. But as a build-config
> peer I'm sure that the definition of MOZ_GONK here isn't right... Unless I'm
> mistaken about the defines, MOZ_B2G already exists for this purpose.

Got it. Will change that to MOZ_B2G. Thanks a lot.

Comment 8

[John Lin [:jhlin][:jolin]]

(In reply to Randell Jesup [:jesup] from comment #5)
> Comment on attachment 765883 [details] [diff] [review]
> Proposed fix v2: use private nsINetworkInterfaceListService when running
> tests on B2G.
> 
> Review of attachment 765883 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/mtransport/test/private_nils.h
> @@ +221,5 @@
> > +  *_retval = interface;
> > +  return NS_OK;
> > +}
> > +
> > +// Same as in addrs.c
> 
> Mostly the same as stun_get_siocgifconf_addrs() in addrs.c (it's not exactly
> the same)
 Actually what I meant here was that MAXADDRS has the same value as in addrs.c. :)
 Will move the comment to the same line to make it more clear.

> 
> @@ +266,5 @@
> > +      }
> > +
> > +      struct sockaddr_in* addr_in = (sockaddr_in*)&ifr2.ifr_addr;
> > +      char addr_str[INET_ADDRSTRLEN + 1];
> > +      inet_ntop(AF_INET, &(addr_in->sin_addr), addr_str, INET_ADDRSTRLEN + 1);
> 
> Minor - I'd prefer "sizeof(addr_str)" - that way it can't ever be wrong if
> someone changes addr_str's defintion.
> Unlikely to ever happen here in practice, but a good habit.
 Good point. Will do.

> 
> @@ +274,5 @@
> > +
> > +   close(s);
> > +
> > +    _status = 0;
> > +    return _status;
> 
> indent is wrong
 Thanks for pointing that out! In fact the whole function body didn't conform to Mozilla coding style. Will reformat the code.

> 
> @@ +315,5 @@
> > +  rv = reg->RegisterFactory(kListIID,
> > +    "Mock NetworkInterfaceList implementation",
> > +    LIST_CONTRACTID,
> > +    new mozilla::GenericFactory(PrivateNetworkInterfaceListConstructor));
> > +  MOZ_ASSERT(NS_SUCCEED(rv));
> 
> While I'm not saying this won't work, I'm surprised by the code pattern -
> direct use of RegisterFactory() here.  Nothing else in the mozilla codebase
> uses RegisterFactory directly that I can find.
> Almost all (all really that I can find) services are instantiated via XPCOM
> modules.  Moving r? to bsmedberg, or just take it as an r- to add these to a
> Module.
 When reading document I got the wrong impression that a module need to be put inside a shared library and building one just for testing purpose sounds like an overkill to me, hence the dirty work there.
 Turns out there is a function XRE_AddStaticComponent() which makes it easy. Will use that in patch v3.

Comment 9

[John Lin [:jhlin][:jolin]]

Attachment: Proposed patch v3

Addresses review comments from bsmedberg & jesup.

Comment 10

[Benjamin Smedberg]

I don't understand what's going on here enough to review this yet.

* Why aren't the components of the original bug available/working?
* What test executable are we talking about? Is this using the gmock testing framework or something else?
* Is this thing you're registering basically mocking some interface? Are there comments in the code which explain any of this?
* Does this test binary run as part of the automated testsuite, and if so which one/where does it report?

Comment 11

[John Lin [:jhlin][:jolin]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #10)
> I don't understand what's going on here enough to review this yet.
> 
> * Why aren't the components of the original bug available/working?
 The original implementation of nsINetworkInterfaceListService is not available because it's registered through chrome.manifest and the JS file is packed inside omni.ja. Test executable has neither.
 I tried MOZ_XRE_DIR=/system/b2g to make it available to the test executable, but it still didn't work (an empty list will be returned) because the IPs were only registered to the NetworkManager running in b2g/chrome process. (I think Patrick can explain this much more clearly)
 After discussing with Patrick, we both agreed that creating a mock service here is good enough for testing purpose.

> * What test executable are we talking about? Is this using the gmock testing
> framework or something else?
 These tests use gtest. AFAICT, gmock is not used.

> * Is this thing you're registering basically mocking some interface? Are
> there comments in the code which explain any of this?
 I prefer to call it an alternative implementation. :)
 And you're right about comments. I will add some more and link to this bug.

> * Does this test binary run as part of the automated testsuite, and if so
> which one/where does it report?
 AFAIK, the tests are enabled for desktop builds and I'm currently working on enabling them for B2G builds too.
 What do you mean by '... which one/where does it report'?

Comment 12

[Benjamin Smedberg]

The gtest framework doesn't start XPCOM itself (yet, bug 855462). If you are starting XPCOM, there is no reason why you couldn't make it use omni.jar (IMO it should!)

If these tests are enabled for desktop builds, what letter on TBPL turns orange when the tests fail? I want to see the log for them.

Comment 13

[Eric Rescorla (:ekr)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #12)
> The gtest framework doesn't start XPCOM itself (yet, bug 855462). If you are
> starting XPCOM, there is no reason why you couldn't make it use omni.jar
> (IMO it should!)
> 
> If these tests are enabled for desktop builds, what letter on TBPL turns
> orange when the tests fail? I want to see the log for them.

These tests are enabled for desktop builds (though some of them are
conditional on an environment variable). They are standalone
binaries.

They run as part of "make check" though Ted has been working on
splitting them out to run as part of the test harness instead as
part of build.

Comment 14

[John Lin [:jhlin][:jolin]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #12)
> The gtest framework doesn't start XPCOM itself (yet, bug 855462). If you are
> starting XPCOM, there is no reason why you couldn't make it use omni.jar
> (IMO it should!)
 A ScopedXPCOM was set up for the WebRTC tests already (bug 855462, comment 6) and I did tried using omni.jar by setting MOZ_XRE_DIR=/system/b2g (is there other way to do it?)
 Unfortunately, even though the test executable can load & create required components, our nsINetowrkManager instance still won't return any network interfaces, unless we explicitly register currently active interface to it first.

> 
> If these tests are enabled for desktop builds, what letter on TBPL turns
> orange when the tests fail? I want to see the log for them.
 As ekr said, the tests are part of 'make check' and I think it's the 'B' letter on TBPL. You should be able to find them by searching 'make -C media/mtransport/test check' and 'make -C media/webrtc/signaling/test check' in the log.
 More info about these tests can be found on wiki: https://wiki.mozilla.org/Media/WebRTC/Testing#C.2B.2B_Unit_Tests

Comment 15

[John Lin [:jhlin][:jolin]]

Attachment: A proposed trick to make sure events dispatched to the main thread will be executed in time.

When running tests, all events dispatched to the main thread won't be executed until all tests are complete. Therefore it's not possible for the runnable used to get network interfaces(gonk_addrs.cpp:92) to do its job in time. Even worse, peer connection initialization never gets completed since this is a sync dispatch.
 By explicitly flushing events while test is waiting for state change in the main thread, the runnable gets a chance to be executed.
 This trick can also address a similar situation found in bug 865956, comment 53 which makes some time-out failures in full runs because of thread leakage.

Comment 16

[John Lin [:jhlin][:jolin]]

Attachment: main thread flushing trick v2

Fix a bug in previous patch(attachment 768782 [details] [diff] [review]) by adding curly brackets to make it flush after each sleep, not just at the end of waiting.

Comment 17

[(not currently active) Ted Mielczarek]

These are CPP_UNIT_TESTS that use a separate copy of gtest right now, FYI. They run in "make check" on desktop, but we're working on splitting them out (so we can run them on Android/B2G as well).

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 767102 [details] [diff] [review]
Proposed patch v3

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

r+ with nits addressed

::: media/mtransport/test/private_nils.h
@@ +218,5 @@
> +  return NS_OK;
> +}
> +
> +/* nsINetworkInterface getInterface (in long interfaceIndex); */
> +NS_IMETHODIMP PrivateNetworkInterfaceList::GetInterface(int32_t interfaceIndex,

It there a reason this is int32_t instead of uint32_t?  (perhaps an existing IDL signature?)
If it can be converted, you can get rid of 'index'

@@ +221,5 @@
> +/* nsINetworkInterface getInterface (in long interfaceIndex); */
> +NS_IMETHODIMP PrivateNetworkInterfaceList::GetInterface(int32_t interfaceIndex,
> +  nsINetworkInterface * *_retval)
> +{
> +  PRUint32 index = interfaceIndex;

Ancient type.  uint32_t

@@ +222,5 @@
> +NS_IMETHODIMP PrivateNetworkInterfaceList::GetInterface(int32_t interfaceIndex,
> +  nsINetworkInterface * *_retval)
> +{
> +  PRUint32 index = interfaceIndex;
> +  if (!_retval || index >= mInterfaces.Length()) {

Even better: get rid of index, and change to:
 if (!_retval || interfaceIndex < 0 || interfaceIndex >= mInterfaces.Length()) {

(Assuming you can't change the type of interfaceIndex)

Comment 19

[John Lin [:jhlin][:jolin]]

(In reply to Randell Jesup [:jesup] from comment #18)
> Comment on attachment 767102 [details] [diff] [review]
> Proposed patch v3
> 
> Review of attachment 767102 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with nits addressed
> 
> ::: media/mtransport/test/private_nils.h
> @@ +218,5 @@
> > +  return NS_OK;
> > +}
> > +
> > +/* nsINetworkInterface getInterface (in long interfaceIndex); */
> > +NS_IMETHODIMP PrivateNetworkInterfaceList::GetInterface(int32_t interfaceIndex,
> 
> It there a reason this is int32_t instead of uint32_t?  (perhaps an existing
> IDL signature?)
 Yes. This is the signature defined for the original implementation.

> If it can be converted, you can get rid of 'index'
> 
> @@ +221,5 @@
> > +/* nsINetworkInterface getInterface (in long interfaceIndex); */
> > +NS_IMETHODIMP PrivateNetworkInterfaceList::GetInterface(int32_t interfaceIndex,
> > +  nsINetworkInterface * *_retval)
> > +{
> > +  PRUint32 index = interfaceIndex;
> 
> Ancient type.  uint32_t
Thanks! it's good to learn something new. :)

> 
> @@ +222,5 @@
> > +NS_IMETHODIMP PrivateNetworkInterfaceList::GetInterface(int32_t interfaceIndex,
> > +  nsINetworkInterface * *_retval)
> > +{
> > +  PRUint32 index = interfaceIndex;
> > +  if (!_retval || index >= mInterfaces.Length()) {
> 
> Even better: get rid of index, and change to:
>  if (!_retval || interfaceIndex < 0 || interfaceIndex >=
> mInterfaces.Length()) {
> 
> (Assuming you can't change the type of interfaceIndex)

Great idea! Will do.

Comment 20

[John Lin [:jhlin][:jolin]]

Attachment: patch v4

- address nits in rjesup's r+
- use MOZ_WIDGET_GONK instead of MOZ_B2G. Turns out MOZ_B2G is also defined in B2G Gecko builds for desktop and causes 'Bg' burn on tryserver. This fix should only be enabled for Gonk.
- include patch in attachment 769551 [details] [diff] [review] which flushes main thread events during testing
- some more comments

Comment 21

[Eric Rescorla (:ekr)]

Comment on attachment 772479 [details] [diff] [review]
patch v4

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

I don't think this is quite what we want, since now we're running the nils on
the thread that came from main() rather than on the pseudo-main thread we
usually use.

Suggest that instead you have mtransport_test_utils spin up a new pseudo-main
thread that you can then access from users. Then you can remove the pseudo-main
setup from signaling_unittests.cpp (and perhaps others).

Comment 22

[u459114]

John, please keep working on this

Comment 23

[Bulk Bug Changes for mreavy's org]

Mass change P4->P5 to align with new Mozilla triage process.

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Closing as we are not working on Firefox OS anymore.