[DeviceService] Use WeakPtrFactory instead of SupportsWeakPtr<> for SerialConnection

This CL changes device::SerialConnection to hold a base::WeakPtrFactory
member rather than inherit base::SupportsWeakPtr<> to avoid the subtle
use-after-destroy issue:
1.
SerialConnection dtor starts, destroyes its'
device::mojom::SerialIoHandlerPtr |io_handler_| member.
2.
The SerialIoHandlerPtr dtor triggers some one callback XXX wrapped by
ScopedCallbackRunner to run.
3.
The callback XXX was actually bound with a weak ptr of SerialConnection,
at this time point the parent class SupportsWeakPtr's dtor has not been
triggered yet so that the weak ptr is still in a valid state, although
we are already inside the execution context of SerialConnection dtor.

A base::WeakPtrFactory member at the end of member list will be
destroyed immediatelly once entering SerialConnection dtor, thus in the
above step 3 the bound weak ptr should be identified as invalid so that
callback XXX won't be called at all. This is the expected behavior.

BUG=757756

Change-Id: Ib73ac55dcfba250c4f8b96fb9e3d3e85490ea9a0
Reviewed-on: https://chromium-review.googlesource.com/626197Reviewed-by: Reilly Grant <reillyg@chromium.org>
Commit-Queue: Han Leon <leon.han@intel.com>
Cr-Commit-Position: refs/heads/master@{#496526}

extensions/browser/api/serial/serial_connection.cc

@@ -173,7 +173,8 @@ SerialConnection::SerialConnection(
       buffer_size_(kDefaultBufferSize),
       receive_timeout_(0),
       send_timeout_(0),
-      paused_(false) {
+      paused_(false),
+      weak_factory_(this) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(io_handler_info.is_valid());
   io_handler_.Bind(std::move(io_handler_info));
@@ -246,16 +247,16 @@ bool SerialConnection::Receive(ReceiveCompleteCallback callback) {
     return false;
   DCHECK(io_handler_);
   receive_complete_ = std::move(callback);
-  io_handler_->Read(
-      buffer_size_,
-      ScopedCallbackRunner(
-          base::BindOnce(&SerialConnection::OnAsyncReadComplete, AsWeakPtr()),
-          std::vector<uint8_t>(),
-          device::mojom::SerialReceiveError::DISCONNECTED));
+  io_handler_->Read(buffer_size_,
+                    ScopedCallbackRunner(
+                        base::BindOnce(&SerialConnection::OnAsyncReadComplete,
+                                       weak_factory_.GetWeakPtr()),
+                        std::vector<uint8_t>(),
+                        device::mojom::SerialReceiveError::DISCONNECTED));
   receive_timeout_task_.Cancel();
   if (receive_timeout_ > 0) {
-    receive_timeout_task_.Reset(
-        base::Bind(&SerialConnection::OnReceiveTimeout, AsWeakPtr()));
+    receive_timeout_task_.Reset(base::Bind(&SerialConnection::OnReceiveTimeout,
+                                           weak_factory_.GetWeakPtr()));
     base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
         FROM_HERE, receive_timeout_task_.callback(),
         base::TimeDelta::FromMilliseconds(receive_timeout_));
@@ -273,13 +274,14 @@ bool SerialConnection::Send(const std::vector<char>& data,
   io_handler_->Write(
       std::vector<uint8_t>(data.data(), data.data() + data.size()),
       ScopedCallbackRunner(
-          base::BindOnce(&SerialConnection::OnAsyncWriteComplete, AsWeakPtr()),
+          base::BindOnce(&SerialConnection::OnAsyncWriteComplete,
+                         weak_factory_.GetWeakPtr()),
           static_cast<uint32_t>(0),
           device::mojom::SerialSendError::DISCONNECTED));
   send_timeout_task_.Cancel();
   if (send_timeout_ > 0) {
-    send_timeout_task_.Reset(
-        base::Bind(&SerialConnection::OnSendTimeout, AsWeakPtr()));
+    send_timeout_task_.Reset(base::Bind(&SerialConnection::OnSendTimeout,
+                                        weak_factory_.GetWeakPtr()));
     base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
         FROM_HERE, send_timeout_task_.callback(),
         base::TimeDelta::FromMilliseconds(send_timeout_));

extensions/browser/api/serial/serial_connection.h

@@ -28,8 +28,7 @@ namespace extensions {
 // corresponds with an open serial port in remote side(Device Service). NOTE:
 // Instances of this object should only be constructed on the IO thread, and all
 // methods should only be called on the IO thread unless otherwise noted.
-class SerialConnection : public ApiResource,
-                         public base::SupportsWeakPtr<SerialConnection> {
+class SerialConnection : public ApiResource {
  public:
   using OpenCompleteCallback = device::mojom::SerialIoHandler::OpenCallback;
   using GetInfoCompleteCallback =
@@ -211,6 +210,8 @@ class SerialConnection : public ApiResource,
   // Closure which is set by client and will be called when |io_handler_|
   // connection encountered an error.
   base::OnceClosure connection_error_handler_;
+
+  base::WeakPtrFactory<SerialConnection> weak_factory_;
 };
 
 }  // namespace extensions