Fix race condition in net::SerialWorker.

net::SerialWorker is a RefCountedThreadSafe object that also uses
WeakPtrFactory to create WeakPtrs of itself. To run whatever work it's
supposed to run, it PostTaskWithTraitsAndReply's to another sequence to
do the work there, and then gets called back when the work is complete.
The work callback holds a (refcounted) reference to the SerialWorker,
and the reply callback is bound to a WeakPtr to ensure the SerialWorker
will still be deleted if posting the reply fails. WeakPtr enforces that
all dereferences to it, as well as its invalidation, must happen on the
same sequence to avoid races. When the reply callback is being called,
it dereferences the SerialWorker WeakPtr on the original sequence,
which binds that WeakPtr to that sequence. It's possible for the
SerialWorker’s owner to release it after the WeakPtr has been checked,
but before the worker function has returned, in which case the worker
function will have the last reference to the SerialWorker. In that case,
when the worker function finally does return, it will delete the
SerialWorker, which will delete its WeakPtrFactory, which will
invalidate existing WeakPtrs. However, the WeakPtr was previously
dereferenced on the SerialWorker sequence, and now it’s being
invalidated on the worker sequence, which is invalid.

To fix this, this CL makes SerialWorker a RefCountedDeleteOnSequence to
ensure the deletion always happens in the right sequence.

Bug: 882610
Change-Id: If738d3724384ffd5ac210130415bce7262932feb
Reviewed-on: https://chromium-review.googlesource.com/c/1336066
Commit-Queue: Robbie McElrath <rmcelrath@chromium.org>
Reviewed-by: Eric Roman <eroman@chromium.org>
Reviewed-by: Eric Orth <ericorth@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608223}

net/dns/dns_config_service_posix_unittest.cc

@@ -189,6 +189,7 @@ TEST(DnsConfigServicePosixTest, DestroyWhileJobsWorking) {
       new internal::DnsConfigServicePosix());
   service->ReadConfig(base::Bind(&DummyConfigCallback));
   service.reset();
+  scoped_task_environment.RunUntilIdle();
   base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(1000));
 }
 
@@ -215,6 +216,7 @@ class DnsConfigServicePosixTest : public testing::Test {
 
   void TearDown() override { ASSERT_TRUE(base::DeleteFile(temp_file_, false)); }
 
+  base::test::ScopedTaskEnvironment scoped_task_environment_;
   bool seen_config_;
   base::FilePath temp_file_;
   std::unique_ptr<DnsConfigServicePosix> service_;
@@ -223,11 +225,9 @@ class DnsConfigServicePosixTest : public testing::Test {
 
 // Regression test for https://crbug.com/704662.
 TEST_F(DnsConfigServicePosixTest, ChangeConfigMultipleTimes) {
-  base::test::ScopedTaskEnvironment scoped_task_environment;
-
   service_->WatchConfig(base::Bind(&DnsConfigServicePosixTest::OnConfigChanged,
                                    base::Unretained(this)));
-  scoped_task_environment.RunUntilIdle();
+  scoped_task_environment_.RunUntilIdle();
 
   for (int i = 0; i < 5; i++) {
     service_->OnConfigChanged(true);
@@ -235,7 +235,7 @@ TEST_F(DnsConfigServicePosixTest, ChangeConfigMultipleTimes) {
     // called if the new config is different from the old one, so this can't be
     // ExpectChange().
     base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(50));
-    scoped_task_environment.RunUntilIdle();
+    scoped_task_environment_.RunUntilIdle();
   }
 
   // There should never be more than 4 nameservers in a real config.

net/dns/serial_worker.cc

@@ -7,11 +7,16 @@
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/task/post_task.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 #include "base/threading/thread_task_runner_handle.h"
 
 namespace net {
 
-SerialWorker::SerialWorker() : state_(IDLE), weak_factory_(this) {}
+SerialWorker::SerialWorker()
+    : base::RefCountedDeleteOnSequence<SerialWorker>(
+          base::SequencedTaskRunnerHandle::Get()),
+      state_(IDLE),
+      weak_factory_(this) {}
 
 SerialWorker::~SerialWorker() = default;
 

net/dns/serial_worker.h

@@ -9,7 +9,7 @@
 
 #include "base/compiler_specific.h"
 #include "base/macros.h"
-#include "base/memory/ref_counted.h"
+#include "base/memory/ref_counted_delete_on_sequence.h"
 #include "base/memory/weak_ptr.h"
 #include "base/sequence_checker.h"
 #include "base/task/task_traits.h"
@@ -34,7 +34,7 @@ namespace net {
 // This implementation avoids locking by using the |state_| member to ensure
 // that |DoWork| and |OnWorkFinished| cannot execute in parallel.
 class NET_EXPORT_PRIVATE SerialWorker
-    : public base::RefCountedThreadSafe<SerialWorker> {
+    : public base::RefCountedDeleteOnSequence<SerialWorker> {
  public:
   SerialWorker();
 
@@ -48,7 +48,8 @@ class NET_EXPORT_PRIVATE SerialWorker
   bool IsCancelled() const { return state_ == CANCELLED; }
 
  protected:
-  friend class base::RefCountedThreadSafe<SerialWorker>;
+  friend class base::DeleteHelper<SerialWorker>;
+  friend class base::RefCountedDeleteOnSequence<SerialWorker>;
   // protected to allow sub-classing, but prevent deleting
   virtual ~SerialWorker();