NetworkService leaks in unit tests

[Anatoly Scheglov]

Hi,

currently RenderViewHostTestHarness indirectly creates NetworkService and leaks it. This can break certain tests.

NetworkService also creates and owns NetworkChangeNotifier, so it gets leaked as well.

NetworkChangeNotifier is supposed to clear g_network_change_notifier in its destructor, but it doesn't happen.

Thus, the next test creating a NetworkChangeNotifier hits "Check failed: !g_network_change_notifier.".

For example:

./unit_tests  --gtest_filter=BackgroundSyncPermissionContextTest.TestSecureRequestingUrl:PrerenderTest.LinkRelAllowedOnCellular

BackgroundSyncPermissionContextTest uses ChromeRenderViewHostTestHarness and creates NetworkService and NetworkChangeNotifier.

PrerenderTest doesn't use the harness, but it creates NetworkChangeNotifier on its own and hits the DCHECK(!g_network_change_notifier)

[Helen Li]

Thanks for reporting this bug. 

I think I know a way to fix the NetworkChangeNotifier leak in unit tests. I will take this one.

--
You received this message because you are subscribed to the Google Groups "network-service-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to network-service...@chromium.org.
To post to this group, send email to network-s...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/network-service-dev/3a2805dd-4077-4ac9-be21-d3477095f114%40chromium.org.

[Helen Li]

This should be fixed by https://chromium-review.googlesource.com/c/chromium/src/+/1114924. Let me know if you run into any issues related to this.

[Anatoly Scheglov]

Hi, unfortunately there are many other tests which indirectly create NetworkService and leak it.

I've added this check

diff --git a/chrome/test/base/chrome_unit_test_suite.cc b/chrome/test/base/chrome_unit_test_suite.cc
index e45783fcb538..42e9a15971af 100644
--- a/chrome/test/base/chrome_unit_test_suite.cc
+++ b/chrome/test/base/chrome_unit_test_suite.cc
@@ -88,6 +88,15 @@ class ChromeUnitTestSuiteInitializer : public testing::EmptyTestEventListener {
   }

   void OnTestEnd(const testing::TestInfo& test_info) override {
+    if (net::NetworkChangeNotifier::HasNetworkChangeNotifier()) {
+      DCHECK(false) << "NetworkChangeNotifier leak: "
+                    << test_info.test_case_name() << "." << test_info.name();
+
+      // Leak it for good, so it won't break next tests.
+      (void)new net::NetworkChangeNotifier::DisableForTest();
+    }
+
     browser_content_client_.reset();

and there were some 2000+ tests which don't clear the g_network_change_notifier variable.

Many of those tests just create the TestingProfile.

So changing RenderViewHostTestHarness didn't cover all the cases.

On Wednesday, June 27, 2018 at 1:09:32 AM UTC+3, Helen Li wrote:

This should be fixed by https://chromium-review.googlesource.com/c/chromium/src/+/1114924. Let me know if you run into any issues related to this.

On Wed, Jun 20, 2018 at 11:52 AM Helen Li <xunj...@chromium.org> wrote:

Thanks for reporting this bug. 

I think I know a way to fix the NetworkChangeNotifier leak in unit tests. I will take this one.

On Wed, Jun 20, 2018 at 6:36 AM Anatoly Scheglov <asch...@yandex-team.ru> wrote:

Hi,

currently RenderViewHostTestHarness indirectly creates NetworkService and leaks it. This can break certain tests.

NetworkService also creates and owns NetworkChangeNotifier, so it gets leaked as well.

NetworkChangeNotifier is supposed to clear g_network_change_notifier in its destructor, but it doesn't happen.

Thus, the next test creating a NetworkChangeNotifier hits "Check failed: !g_network_change_notifier.".

For example:

./unit_tests  --gtest_filter=BackgroundSyncPermissionContextTest.TestSecureRequestingUrl:PrerenderTest.LinkRelAllowedOnCellular

BackgroundSyncPermissionContextTest uses ChromeRenderViewHostTestHarness and creates NetworkService and NetworkChangeNotifier.

PrerenderTest doesn't use the harness, but it creates NetworkChangeNotifier on its own and hits the DCHECK(!g_network_change_notifier)

--
You received this message because you are subscribed to the Google Groups "network-service-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to network-service-dev+unsub...@chromium.org.

[Helen Li]

Hi Anatoly,

Please file a bug on crbug.com. 

As you said, the underlying issue is that the global singleton NetworkService leaks in unit tests. 

I think we should fix this, but I am not sure how. Filing a crbug should be a good start. 

To unsubscribe from this group and stop receiving emails from it, send an email to network-service...@chromium.org.

To post to this group, send email to network-s...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/network-service-dev/3a2805dd-4077-4ac9-be21-d3477095f114%40chromium.org.

--

You received this message because you are subscribed to the Google Groups "network-service-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to network-service...@chromium.org.

To post to this group, send email to network-s...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/network-service-dev/a2fef97f-3fc2-4b77-8232-d800f55e1b87%40chromium.org.

[Anatoly Scheglov]

Created crbug.com/867414

To unsubscribe from this group and stop receiving emails from it, send an email to network-service-dev+unsub...@chromium.org.

To post to this group, send email to network-s...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/network-service-dev/3a2805dd-4077-4ac9-be21-d3477095f114%40chromium.org.

--
You received this message because you are subscribed to the Google Groups "network-service-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to network-service-dev+unsub...@chromium.org.