From 98ee14e936f39daa1bd2d8de2f570ce2198305cd Mon Sep 17 00:00:00 2001 From: Maks Orlovich Date: Tue, 1 Nov 2016 14:23:39 -0400 Subject: [PATCH] Avoid warnings in DownstreamCache tests about an uncleaned up rewrite_driver by making sure that other_rewrite_driver gets cleaned up: the test sets RewriteTestBase into managed mode, which basically means it does rewrite_driver = NewRewriteDriver other_rewrite_driver = NewRewriterDriver ... but then nothing actually does anything with other_rewrite_driver, making it stick-around until factory shutdown does emergency cleanup. (see https://github.com/pagespeed/mod_pagespeed/issues/1421) --- .../rewriter/public/rewrite_test_base.h | 4 +++- net/instaweb/rewriter/rewrite_driver_test.cc | 20 +++++++++++++++++-- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/net/instaweb/rewriter/public/rewrite_test_base.h b/net/instaweb/rewriter/public/rewrite_test_base.h index 40c063285..f2a5d2ace 100644 --- a/net/instaweb/rewriter/public/rewrite_test_base.h +++ b/net/instaweb/rewriter/public/rewrite_test_base.h @@ -241,7 +241,9 @@ class RewriteTestBase : public RewriteOptionsTestBase { // Use managed rewrite drivers for the test so that we see the same behavior // in tests that we see in real servers. By default, tests use unmanaged // drivers so that _test.cc files can add options after the driver was created - // and before the filters are added. + // and before the filters are added. Note that this will only clean them up + // via shutdown codepath if you don't actually use them, unless an explicit + // Cleanup() call is made. void SetUseManagedRewriteDrivers(bool use_managed_rewrite_drivers); GoogleString CssLinkHref(const StringPiece& url) { diff --git a/net/instaweb/rewriter/rewrite_driver_test.cc b/net/instaweb/rewriter/rewrite_driver_test.cc index 1960be850..7167fecdd 100644 --- a/net/instaweb/rewriter/rewrite_driver_test.cc +++ b/net/instaweb/rewriter/rewrite_driver_test.cc @@ -2146,11 +2146,19 @@ TEST_F(RewriteDriverTest, SetRequestHeadersPopulatesWebpNoAccept) { // rewritten in the very first go. class DownstreamCacheWithPossiblePurgeTest : public RewriteDriverTest { protected: - void SetUp() { + void SetUp() override { options()->EnableFilter(RewriteOptions::kExtendCacheCss); SetUseManagedRewriteDrivers(true); RewriteDriverTest::SetUp(); } + + void TearDown() override { + // We need to clean up the other rewrite driver manually since we don't + // parse anything through it --- NewRewriteDriver is called, but nothing + // else is done otherwise. + other_rewrite_driver()->Cleanup(); + RewriteDriverTest::TearDown(); + } }; // This class has CollapseWhitespace filter enabled and has no possibility of @@ -2158,11 +2166,19 @@ class DownstreamCacheWithPossiblePurgeTest : public RewriteDriverTest { // in the very first go. class DownstreamCacheWithNoPossiblePurgeTest : public RewriteDriverTest { protected: - void SetUp() { + void SetUp() override { options()->EnableFilter(RewriteOptions::kCollapseWhitespace); SetUseManagedRewriteDrivers(true); RewriteDriverTest::SetUp(); } + + void TearDown() override { + // We need to clean up the other rewrite driver manually since we don't + // parse anything through it --- NewRewriteDriver is called, but nothing + // else is done otherwise. + other_rewrite_driver()->Cleanup(); + RewriteDriverTest::TearDown(); + } }; TEST_F(DownstreamCacheWithPossiblePurgeTest, DownstreamCacheEnabled) {