[CrOS Tether] DisconnectTetheringOperation: Delay device unregistration.

Previously, DisconnectTetheringOperation would call
SendMessageToDevice(), then immediately call UnregisterDevice().
However, this created a race condition where if the device was
unregistered before the message was successfully sent, we crashed the
browser.

Bug: 672263, 731181
Change-Id: I0da8b47b2129c7bf68f693d15c821e67415737b1
Reviewed-on: https://chromium-review.googlesource.com/590810
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#490605}

chromeos/components/tether/disconnect_tethering_operation.cc

@@ -48,7 +48,7 @@ DisconnectTetheringOperation::DisconnectTetheringOperation(
     : MessageTransferOperation(
           std::vector<cryptauth::RemoteDevice>{device_to_connect},
           connection_manager),
-      device_id_(device_to_connect.GetDeviceId()),
+      remote_device_(device_to_connect),
       has_authenticated_(false) {}
 
 DisconnectTetheringOperation::~DisconnectTetheringOperation() {}
@@ -64,7 +64,7 @@ void DisconnectTetheringOperation::RemoveObserver(Observer* observer) {
 void DisconnectTetheringOperation::NotifyObserversOperationFinished(
     bool success) {
   for (auto& observer : observer_list_) {
-    observer.OnOperationFinished(device_id_, success);
+    observer.OnOperationFinished(remote_device_.GetDeviceId(), success);
   }
 }
 
@@ -73,9 +73,9 @@ void DisconnectTetheringOperation::OnDeviceAuthenticated(
   DCHECK(remote_devices().size() == 1u && remote_devices()[0] == remote_device);
   has_authenticated_ = true;
 
-  SendMessageToDevice(remote_device, base::MakeUnique<MessageWrapper>(
-                                         DisconnectTetheringRequest()));
-  UnregisterDevice(remote_device);
+  disconnect_message_sequence_number_ = SendMessageToDevice(
+      remote_device,
+      base::MakeUnique<MessageWrapper>(DisconnectTetheringRequest()));
 }
 
 void DisconnectTetheringOperation::OnOperationFinished() {
@@ -90,6 +90,13 @@ bool DisconnectTetheringOperation::ShouldWaitForResponse() {
   return false;
 }
 
+void DisconnectTetheringOperation::OnMessageSent(int sequence_number) {
+  if (sequence_number != disconnect_message_sequence_number_)
+    return;
+
+  UnregisterDevice(remote_device_);
+}
+
 }  // namespace tether
 
 }  // namespace chromeos

chromeos/components/tether/disconnect_tethering_operation.h

@@ -59,12 +59,14 @@ class DisconnectTetheringOperation : public MessageTransferOperation {
   void OnOperationFinished() override;
   MessageType GetMessageTypeForConnection() override;
   bool ShouldWaitForResponse() override;
+  void OnMessageSent(int sequence_number) override;
 
  private:
   friend class DisconnectTetheringOperationTest;
 
   base::ObserverList<Observer> observer_list_;
-  std::string device_id_;
+  cryptauth::RemoteDevice remote_device_;
+  int disconnect_message_sequence_number_ = -1;
   bool has_authenticated_;
 
   DISALLOW_COPY_AND_ASSIGN(DisconnectTetheringOperation);

chromeos/components/tether/disconnect_tethering_operation_unittest.cc

@@ -72,12 +72,19 @@ class DisconnectTetheringOperationTest : public testing::Test {
   void SimulateDeviceAuthenticationAndVerifyMessageSent() {
     operation_->OnDeviceAuthenticated(test_device_);
 
-    // Verify that the message was sent successfully.
+    // Verify that the message was passed to |fake_ble_connection_manager_|
+    // correctly.
     std::vector<FakeBleConnectionManager::SentMessage>& sent_messages =
         fake_ble_connection_manager_->sent_messages();
     ASSERT_EQ(1u, sent_messages.size());
     EXPECT_EQ(test_device_, sent_messages[0].remote_device);
     EXPECT_EQ(disconnect_tethering_request_string_, sent_messages[0].message);
+
+    // Now, simulate the message being sent.
+    int last_sequence_number =
+        fake_ble_connection_manager_->last_sequence_number();
+    EXPECT_NE(last_sequence_number, -1);
+    fake_ble_connection_manager_->SetMessageSent(last_sequence_number);
   }
 
   void SimulateConnectionTimeout() {

chromeos/components/tether/fake_ble_connection_manager.h

@@ -32,7 +32,10 @@ class FakeBleConnectionManager : public BleConnectionManager {
   void ReceiveMessage(const cryptauth::RemoteDevice& remote_device,
                       const std::string& payload);
   void SetMessageSent(int sequence_number);
+
   std::vector<SentMessage>& sent_messages() { return sent_messages_; }
+  // Returns -1 if no sequence numbers have been used yet.
+  int last_sequence_number() { return next_sequence_number_ - 1; }
 
   // BleConnectionManager:
   void RegisterRemoteDevice(const cryptauth::RemoteDevice& remote_device,