Fix MDnsClientImpl timer usage

Previously, cleanup timer (schedules cache cleanups for soonest TTL)
would keep running after MDnsClientImpl::Core destruction but it used
unretained references to the destroyed Core. Now it's always stopped on
Core destruction.

Also cleaned up MDnsClientImpl to ensure time is destroyed after Core to
ensure the new Stop() call is always safe. Reordered fields and added a
StopListening() call in the destructor.

Bug: 958307
Change-Id: I6d3063e7f6a6ad49991ae41cbcab4739d81c5da8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1593841
Auto-Submit: Eric Orth <ericorth@chromium.org>
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#656403}

net/dns/mdns_client_impl.cc

@@ -202,11 +202,17 @@ void MDnsConnection::OnDatagramReceived(
 MDnsClientImpl::Core::Core(base::Clock* clock, base::OneShotTimer* timer)
     : clock_(clock),
       cleanup_timer_(timer),
-      connection_(new MDnsConnection(this)) {}
+      connection_(new MDnsConnection(this)) {
+  DCHECK(cleanup_timer_);
+  DCHECK(!cleanup_timer_->IsRunning());
+}
 
-MDnsClientImpl::Core::~Core() = default;
+MDnsClientImpl::Core::~Core() {
+  cleanup_timer_->Stop();
+}
 
 int MDnsClientImpl::Core::Init(MDnsSocketFactory* socket_factory) {
+  CHECK(!cleanup_timer_->IsRunning());
   return connection_->Init(socket_factory);
 }
 
@@ -425,7 +431,9 @@ MDnsClientImpl::MDnsClientImpl(base::Clock* clock,
                                std::unique_ptr<base::OneShotTimer> timer)
     : clock_(clock), cleanup_timer_(std::move(timer)) {}
 
-MDnsClientImpl::~MDnsClientImpl() = default;
+MDnsClientImpl::~MDnsClientImpl() {
+  StopListening();
+}
 
 int MDnsClientImpl::StartListening(MDnsSocketFactory* socket_factory) {
   DCHECK(!core_.get());

net/dns/mdns_client_impl.h

@@ -218,10 +218,11 @@ class NET_EXPORT_PRIVATE MDnsClientImpl : public MDnsClient {
   Core* core() { return core_.get(); }
 
  private:
-  std::unique_ptr<Core> core_;
   base::Clock* clock_;
   std::unique_ptr<base::OneShotTimer> cleanup_timer_;
 
+  std::unique_ptr<Core> core_;
+
   DISALLOW_COPY_AND_ASSIGN(MDnsClientImpl);
 };
 

net/dns/mdns_client_unittest.cc

@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include <memory>
+#include <utility>
 #include <vector>
 
 #include "base/bind.h"
@@ -12,10 +13,12 @@
 #include "base/memory/ref_counted.h"
 #include "base/run_loop.h"
 #include "base/single_thread_task_runner.h"
+#include "base/test/simple_test_clock.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/clock.h"
 #include "base/time/default_clock.h"
 #include "base/timer/mock_timer.h"
+#include "base/timer/timer.h"
 #include "net/base/address_family.h"
 #include "net/base/completion_repeating_callback.h"
 #include "net/base/ip_address.h"
@@ -618,6 +621,36 @@ TEST_F(MDnsTest, CacheCleanupWithShortTTL) {
   timer->Fire();
 }
 
+TEST_F(MDnsTest, StopListening) {
+  ASSERT_TRUE(test_client_->IsListening());
+
+  test_client_->StopListening();
+  EXPECT_FALSE(test_client_->IsListening());
+}
+
+TEST_F(MDnsTest, StopListening_CacheCleanupScheduled) {
+  base::SimpleTestClock clock;
+  // Use a nonzero starting time as a base.
+  clock.SetNow(base::Time() + base::TimeDelta::FromSeconds(1));
+  auto cleanup_timer = std::make_unique<base::MockOneShotTimer>();
+  base::OneShotTimer* cleanup_timer_ptr = cleanup_timer.get();
+
+  test_client_ =
+      std::make_unique<MDnsClientImpl>(&clock, std::move(cleanup_timer));
+  ASSERT_THAT(test_client_->StartListening(&socket_factory_), test::IsOk());
+  ASSERT_TRUE(test_client_->IsListening());
+
+  // Receive one record (privet) with TTL=1s to schedule cleanup.
+  SimulatePacketReceive(kSamplePacket3, sizeof(kSamplePacket3));
+  ASSERT_TRUE(cleanup_timer_ptr->IsRunning());
+
+  test_client_->StopListening();
+  EXPECT_FALSE(test_client_->IsListening());
+
+  // Expect cleanup unscheduled.
+  EXPECT_FALSE(cleanup_timer_ptr->IsRunning());
+}
+
 TEST_F(MDnsTest, MalformedPacket) {
   StrictMock<MockListenerDelegate> delegate_printer;