Create and remove |UiDevToolsServer::server_| in the same thread.

components/ui_devtools/devtools_server.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/ui_devtools/devtools_server.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/format_macros.h"
@@ skipping 47 matching lines @@
     return;
   // If io_thread_task_runner not passed in, create an I/O thread
   thread_.reset(new base::Thread("UiDevToolsServerThread"));
   base::Thread::Options options;
   options.message_loop_type = base::MessageLoop::TYPE_IO;
   CHECK(thread_->StartWithOptions(options));
   io_thread_task_runner_ = thread_->task_runner();
 }
 
 UiDevToolsServer::~UiDevToolsServer() {
+  DCHECK(io_thread_task_runner_);
+  io_thread_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&UiDevToolsServer::StopServer, base::Unretained(this)));
+  if (thread_->IsRunning())

[sadrul, 2017/06/26 22:43:30]

This will not work when |io_thread_task_runner_| has been injected from outside
during creation. For that case, you would either need to use a WaitableEvent
(see examples of that in various places), or do something like this:

  io_thread_task_runner_->DeleteSoon(FROM_HERE, server_.release());

[thanhph, 2017/06/27 15:02:29]

To elaborate more, with current patch 1, does this mean we are afraid
|io_thread_task_runner_| gets cleaned up or gets blocked before
|io_thread_task_runner_| is used here? PostTask() uses PostDelayedTask() which
has default timedelta = 0, thus invoking StopSever() in future time with no
delay (but I guess still could get blocked with current running tasks) and
DeleteSoon() uses PostNonNestableTask() which provides this task will not run
nested within an already-running task.

[sadrul, 2017/06/28 04:49:10]

No. The problem with this change was, that if UiDevToolsServer() received a
custom io_thread_task_runner during creation, then it does not create |thread_|.
Which means this UiDevToolsServer object can still get destroyed before the task
you have just posted runs. This means (1) |server_| gets immediately destroyed,
still in the wrong thread, and on top of that (2) the posted task does run on
the io thread, but |this| has already been destroyed, leading to a
use-after-free.

The most important part of the approach with DeleteSoon() is that you give up
ownership of |server_|. Which means |this| can get destroyed, without destroying
the server. And the server will be correctly destroyed on the IO thread.

If the IO thread has already been torn down, that means |server_| will just
never be destroyed, and will leak instead. But I don't think there's a clean way
of fixing that.

[thanhph, 2017/06/28 14:38:22]

Acknowledged! Thanks for the explanation.

+    thread_->Stop();
   devtools_server_ = nullptr;
 }
 
 // static
 std::unique_ptr<UiDevToolsServer> UiDevToolsServer::Create(
     scoped_refptr<base::SingleThreadTaskRunner> io_thread_task_runner) {
   std::unique_ptr<UiDevToolsServer> server;
   if (IsUiDevToolsEnabled() && !devtools_server_) {
     // TODO(mhashmi): Change port if more than one inspectable clients
     server.reset(new UiDevToolsServer(io_thread_task_runner));
@@ skipping 42 matching lines @@
   DCHECK(!server_);
   std::unique_ptr<net::ServerSocket> socket(
       new net::TCPServerSocket(nullptr, net::NetLogSource()));
   constexpr int kBacklog = 1;
   if (socket->ListenWithAddressAndPort(address_string, port, kBacklog) !=
       net::OK)
     return;
   server_ = base::MakeUnique<net::HttpServer>(std::move(socket), this);
 }
 
+void UiDevToolsServer::StopServer() {
+  server_.reset();
+}
+
 // HttpServer::Delegate Implementation
 void UiDevToolsServer::OnConnect(int connection_id) {
   NOTIMPLEMENTED();
 }
 
 void UiDevToolsServer::OnHttpRequest(int connection_id,
                                      const net::HttpServerRequestInfo& info) {
   NOTIMPLEMENTED();
 }
 
@@ skipping 35 matching lines @@
     return;
   UiDevToolsClient* client = it->second;
   DCHECK(client);
   main_thread_task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&UiDevToolsClient::Disconnect, base::Unretained(client)));
   connections_.erase(it);
 }
 
 }  // namespace ui_devtools