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

NEW
Assigned to

Status

()

Core
WebRTC: Networking
P5
normal
Rank:
45
5 years ago
9 months ago

People

(Reporter: jolin, Assigned: jolin)

Tracking

Trunk
All
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

5 years ago
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
(Assignee)

Comment 1

5 years ago
Created attachment 763956 [details] [diff] [review]
A proposed fix: call original nr_stun_get_addrs() when gonk implementation fail
(Assignee)

Updated

5 years ago
Attachment #763956 - Flags: review?(swu)
Attachment #763956 - Flags: review?(pwang)
Attachment #763956 - Flags: review?(ekr)

Updated

5 years ago
Attachment #763956 - Flags: review?(ekr)
(Assignee)

Comment 2

5 years ago
Created attachment 765883 [details] [diff] [review]
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).
Attachment #763956 - Attachment is obsolete: true
Attachment #763956 - Flags: review?(swu)
Attachment #763956 - Flags: review?(pwang)
Attachment #765883 - Flags: review?(pwang)
Attachment #765883 - Flags: review?(ekr)

Comment 3

5 years ago
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.
Attachment #765883 - Flags: review?(ekr) → review?(rjesup)
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.
Attachment #765883 - Flags: review?(pwang)
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.
Attachment #765883 - Flags: review?(rjesup) → review?(benjamin)

Comment 6

5 years ago
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.
Attachment #765883 - Flags: review?(benjamin) → review-
(Assignee)

Comment 7

5 years ago
(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.
(Assignee)

Comment 8

5 years ago
(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.
(Assignee)

Comment 9

5 years ago
Created attachment 767102 [details] [diff] [review]
Proposed patch v3

Addresses review comments from bsmedberg & jesup.
Attachment #765883 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #767102 - Flags: review?(rjesup)
Attachment #767102 - Flags: review?(benjamin)

Comment 10

5 years ago
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?
(Assignee)

Comment 11

5 years ago
(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'?
(Assignee)

Updated

5 years ago
Blocks: 865956
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → All
Version: unspecified → Trunk

Comment 12

5 years ago
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.

Updated

5 years ago
Assignee: nobody → 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!)
> 
> 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.
(Assignee)

Comment 14

5 years ago
(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
(Assignee)

Comment 15

5 years ago
Created attachment 768782 [details] [diff] [review]
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.
Attachment #768782 - Flags: review?(ekr)
(Assignee)

Comment 16

5 years ago
Created attachment 769551 [details] [diff] [review]
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.
Attachment #768782 - Attachment is obsolete: true
Attachment #769551 - Flags: review?(ekr)
Attachment #768782 - Flags: review?(ekr)

Updated

5 years ago
Attachment #767102 - Flags: review?(benjamin)
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 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)
Attachment #767102 - Flags: review?(rjesup) → review+
(Assignee)

Comment 19

5 years ago
(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.
(Assignee)

Comment 20

5 years ago
Created attachment 772479 [details] [diff] [review]
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
Attachment #767102 - Attachment is obsolete: true
Attachment #769551 - Attachment is obsolete: true
Attachment #769551 - Flags: review?(ekr)
Attachment #772479 - Flags: review?(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).
Attachment #772479 - Flags: review?(ekr)

Comment 22

5 years ago
John, please keep working on this

Updated

3 years ago
backlog: --- → webRTC+
Rank: 45
Priority: -- → P4
Mass change P4->P5 to align with new Mozilla triage process.
Priority: P4 → P5
You need to log in before you can comment on or make changes to this bug.