Avoid blocking in NearbyConnectionsManagerImpl::ClearIncomingPayloads

- Used NearbyFileHandler to properly delete PayloadPtr
- Added base::ScopedDisallowBlocking to the test to ensure the class
  won't use blocking code except in thread pool
- Moved creation of input payload file into NearbyFileHandler
- Added various variable clearign to NearbyConnectionsManagerImpl::Reset

Bug: 1076008
Change-Id: I2c35ddb3d01ae23186416b0175fbdb48d295251e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2367682
Commit-Queue: Alex Chau <alexchau@chromium.org>
Reviewed-by: Richard Knoll <knollr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#800710}

chrome/browser/nearby_sharing/nearby_connections_manager_impl.cc

@@ -46,17 +46,6 @@ bool ShouldEnableWebRtc(bool is_advertising,
   return true;
 }
 
-InitializeFileResult CreateAndOpenFile(base::FilePath file_path) {
-  base::FilePath unique_path = base::GetUniquePath(file_path);
-  InitializeFileResult result;
-  result.output_file.Initialize(
-      unique_path,
-      base::File::Flags::FLAG_CREATE_ALWAYS | base::File::Flags::FLAG_WRITE);
-  result.input_file.Initialize(
-      unique_path, base::File::Flags::FLAG_OPEN | base::File::Flags::FLAG_READ);
-  return result;
-}
-
 }  // namespace
 
 NearbyConnectionsManagerImpl::NearbyConnectionsManagerImpl(
@@ -68,11 +57,11 @@ NearbyConnectionsManagerImpl::NearbyConnectionsManagerImpl(
   nearby_process_observer_.Add(process_manager_);
 }
 
-NearbyConnectionsManagerImpl::~NearbyConnectionsManagerImpl() = default;
+NearbyConnectionsManagerImpl::~NearbyConnectionsManagerImpl() {
+  ClearIncomingPayloads();
+}
 
 void NearbyConnectionsManagerImpl::Shutdown() {
-  // TOOD(crbug/1076008): Implement.
-  // Disconnects from all endpoints and shut down Nearby Connections.
   Reset();
 }
 
@@ -260,18 +249,17 @@ void NearbyConnectionsManagerImpl::RegisterPayloadPath(
     return;
 
   DCHECK(!file_path.empty());
-  base::ThreadPool::PostTaskAndReplyWithResult(
-      FROM_HERE, {base::MayBlock()},
-      base::BindOnce(&CreateAndOpenFile, file_path),
-      base::BindOnce(&NearbyConnectionsManagerImpl::OnFileInitialized,
-                     weak_ptr_factory_.GetWeakPtr(), payload_id,
-                     std::move(callback)));
+
+  file_handler_.CreateFile(
+      file_path, base::BindOnce(&NearbyConnectionsManagerImpl::OnFileCreated,
+                                weak_ptr_factory_.GetWeakPtr(), payload_id,
+                                std::move(callback)));
 }
 
-void NearbyConnectionsManagerImpl::OnFileInitialized(
+void NearbyConnectionsManagerImpl::OnFileCreated(
     int64_t payload_id,
     ConnectionsCallback callback,
-    InitializeFileResult result) {
+    NearbyFileHandler::CreateFileResult result) {
   nearby_connections_->RegisterPayloadFile(
       payload_id, std::move(result.input_file), std::move(result.output_file),
       std::move(callback));
@@ -312,6 +300,11 @@ void NearbyConnectionsManagerImpl::Cancel(int64_t payload_id) {
 }
 
 void NearbyConnectionsManagerImpl::ClearIncomingPayloads() {
+  std::vector<PayloadPtr> payloads;
+  for (auto& it : incoming_payloads_)
+    payloads.push_back(std::move(it.second));
+
+  file_handler_.ReleaseFilePayloads(std::move(payloads));
   incoming_payloads_.clear();
 }
 
@@ -569,6 +562,11 @@ void NearbyConnectionsManagerImpl::Reset() {
   }
   nearby_connections_ = nullptr;
   discovered_endpoints_.clear();
+  pending_outgoing_connections_.clear();
+  payload_status_listeners_.clear();
+  ClearIncomingPayloads();
+  connections_.clear();
+  connection_info_map_.clear();
   discovery_listener_ = nullptr;
   incoming_connection_listener_ = nullptr;
   endpoint_discovery_listener_.reset();

chrome/browser/nearby_sharing/nearby_connections_manager_impl.h

@@ -13,6 +13,7 @@
 #include "base/gtest_prod_util.h"
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/nearby_sharing/nearby_connection_impl.h"
+#include "chrome/browser/nearby_sharing/nearby_file_handler.h"
 #include "chrome/browser/nearby_sharing/nearby_process_manager.h"
 #include "chrome/services/sharing/public/mojom/nearby_connections.mojom.h"
 #include "mojo/public/cpp/bindings/receiver.h"
@@ -20,11 +21,6 @@
 
 class Profile;
 
-struct InitializeFileResult {
-  base::File input_file;
-  base::File output_file;
-};
-
 // Concrete NearbyConnectionsManager implementation.
 class NearbyConnectionsManagerImpl
     : public NearbyConnectionsManager,
@@ -132,12 +128,13 @@ class NearbyConnectionsManagerImpl
   bool BindNearbyConnections();
   void Reset();
 
-  void OnFileInitialized(int64_t payload_id,
-                         ConnectionsCallback callback,
-                         InitializeFileResult result);
+  void OnFileCreated(int64_t payload_id,
+                     ConnectionsCallback callback,
+                     NearbyFileHandler::CreateFileResult result);
 
   NearbyProcessManager* process_manager_;
   Profile* profile_;
+  NearbyFileHandler file_handler_;
   IncomingConnectionListener* incoming_connection_listener_ = nullptr;
   DiscoveryListener* discovery_listener_ = nullptr;
   base::flat_set<std::string> discovered_endpoints_;

chrome/browser/nearby_sharing/nearby_connections_manager_impl_unittest.cc

@@ -43,6 +43,7 @@ const uint64_t kBytesTransferred = 721831;
 const uint8_t kPayload[] = {0x0f, 0x0a, 0x0c, 0x0e};
 
 void VerifyFileReadWrite(base::File& input_file, base::File& output_file) {
+  base::ScopedAllowBlockingForTesting allow_blocking;
   const std::vector<uint8_t> expected_bytes(std::begin(kPayload),
                                             std::end(kPayload));
   EXPECT_TRUE(output_file.WriteAndCheck(
@@ -57,6 +58,21 @@ void VerifyFileReadWrite(base::File& input_file, base::File& output_file) {
   input_file.Close();
 }
 
+base::FilePath InitializeTemporaryFile(base::File& file) {
+  base::ScopedAllowBlockingForTesting allow_blocking;
+  base::FilePath path;
+  if (!base::CreateTemporaryFile(&path))
+    return path;
+
+  file.Initialize(path, base::File::Flags::FLAG_CREATE_ALWAYS |
+                            base::File::Flags::FLAG_READ |
+                            base::File::Flags::FLAG_WRITE);
+  EXPECT_TRUE(file.WriteAndCheck(
+      /*offset=*/0, base::make_span(kPayload, sizeof(kPayload))));
+  EXPECT_TRUE(file.Flush());
+  return path;
+}
+
 }  // namespace
 
 using Status = location::nearby::connections::mojom::Status;
@@ -291,13 +307,8 @@ class NearbyConnectionsManagerImplTest : public testing::Test {
     const std::vector<uint8_t> expected_payload(std::begin(kPayload),
                                                 std::end(kPayload));
 
-    base::FilePath path;
-    ASSERT_TRUE(base::CreateTemporaryFile(&path));
-    base::File file(path, base::File::Flags::FLAG_CREATE_ALWAYS |
-                              base::File::Flags::FLAG_READ |
-                              base::File::Flags::FLAG_WRITE);
-    EXPECT_TRUE(file.WriteAndCheck(
-        /*offset=*/0, base::make_span(expected_payload)));
+    base::File file;
+    InitializeTemporaryFile(file);
 
     base::RunLoop run_loop;
     EXPECT_CALL(nearby_connections_, SendPayload)
@@ -309,9 +320,10 @@ class NearbyConnectionsManagerImplTest : public testing::Test {
           ASSERT_TRUE(payload);
           ASSERT_EQ(PayloadContent::Tag::FILE, payload->content->which());
 
-          std::vector<uint8_t> payload_bytes(
-              payload->content->get_file()->file.GetLength());
-          EXPECT_TRUE(payload->content->get_file()->file.ReadAndCheck(
+          base::ScopedAllowBlockingForTesting allow_blocking;
+          base::File file = std::move(payload->content->get_file()->file);
+          std::vector<uint8_t> payload_bytes(file.GetLength());
+          EXPECT_TRUE(file.ReadAndCheck(
               /*offset=*/0, base::make_span(payload_bytes)));
           EXPECT_EQ(expected_payload, payload_bytes);
 
@@ -331,6 +343,7 @@ class NearbyConnectionsManagerImplTest : public testing::Test {
   TestingProfile profile_;
   std::unique_ptr<net::test::MockNetworkChangeNotifier> network_notifier_ =
       net::test::MockNetworkChangeNotifier::Create();
+  base::ScopedDisallowBlocking disallow_blocking_;
   testing::NiceMock<MockNearbyConnections> nearby_connections_;
   testing::NiceMock<MockNearbyProcessManager> nearby_process_manager_;
   NearbyConnectionsManagerImpl nearby_connections_manager_{
@@ -885,7 +898,11 @@ TEST_F(NearbyConnectionsManagerImplTest, IncomingRegisterPayloadPath) {
           });
 
   base::FilePath path;
-  ASSERT_TRUE(base::CreateTemporaryFile(&path));
+  {
+    base::ScopedAllowBlockingForTesting allow_blocking;
+    ASSERT_TRUE(base::CreateTemporaryFile(&path));
+  }
+
   base::MockCallback<NearbyConnectionsManager::ConnectionsCallback> callback;
   EXPECT_CALL(callback, Run(testing::Eq(Status::kSuccess)));
   nearby_connections_manager_.RegisterPayloadPath(kPayloadId, path,
@@ -906,15 +923,12 @@ TEST_F(NearbyConnectionsManagerImplTest,
       payload_listener_remote);
   EXPECT_TRUE(connection);
 
-  base::FilePath path;
-  ASSERT_TRUE(base::CreateTemporaryFile(&path));
-  base::File file(path, base::File::Flags::FLAG_CREATE_ALWAYS |
-                            base::File::Flags::FLAG_READ |
-                            base::File::Flags::FLAG_WRITE);
-  EXPECT_TRUE(file.WriteAndCheck(
-      /*offset=*/0, base::make_span(kPayload, sizeof(kPayload))));
-  file.Flush();
-  file.Close();
+  base::File file;
+  base::FilePath path = InitializeTemporaryFile(file);
+  {
+    base::ScopedAllowBlockingForTesting allow_blocking;
+    file.Close();
+  }
 
   base::RunLoop register_payload_run_loop;
   EXPECT_CALL(nearby_connections_, RegisterPayloadFile)
@@ -997,13 +1011,8 @@ TEST_F(NearbyConnectionsManagerImplTest, IncomingFilePayload) {
   const std::vector<uint8_t> expected_payload(std::begin(kPayload),
                                               std::end(kPayload));
 
-  base::FilePath path;
-  ASSERT_TRUE(base::CreateTemporaryFile(&path));
-  base::File file(path, base::File::Flags::FLAG_CREATE_ALWAYS |
-                            base::File::Flags::FLAG_READ |
-                            base::File::Flags::FLAG_WRITE);
-  EXPECT_TRUE(file.WriteAndCheck(
-      /*offset=*/0, base::make_span(expected_payload)));
+  base::File file;
+  InitializeTemporaryFile(file);
 
   payload_listener_remote->OnPayloadReceived(
       kRemoteEndpointId,
@@ -1020,14 +1029,18 @@ TEST_F(NearbyConnectionsManagerImplTest, IncomingFilePayload) {
                                  kTotalSize, /*bytes_transferred=*/kTotalSize));
   payload_run_loop.Run();
 
-  Payload* payload = nearby_connections_manager_.GetIncomingPayload(kPayloadId);
-  ASSERT_TRUE(payload);
-  ASSERT_TRUE(payload->content->is_file());
-  std::vector<uint8_t> payload_bytes(
-      payload->content->get_file()->file.GetLength());
-  EXPECT_TRUE(payload->content->get_file()->file.ReadAndCheck(
-      /*offset=*/0, base::make_span(payload_bytes)));
-  EXPECT_EQ(expected_payload, payload_bytes);
+  {
+    base::ScopedAllowBlockingForTesting allow_blocking;
+    Payload* payload =
+        nearby_connections_manager_.GetIncomingPayload(kPayloadId);
+    ASSERT_TRUE(payload);
+    ASSERT_TRUE(payload->content->is_file());
+    std::vector<uint8_t> payload_bytes(
+        payload->content->get_file()->file.GetLength());
+    EXPECT_TRUE(payload->content->get_file()->file.ReadAndCheck(
+        /*offset=*/0, base::make_span(payload_bytes)));
+    EXPECT_EQ(expected_payload, payload_bytes);
+  }
 }
 
 TEST_F(NearbyConnectionsManagerImplTest, ClearIncomingPayloads) {
@@ -1045,9 +1058,13 @@ TEST_F(NearbyConnectionsManagerImplTest, ClearIncomingPayloads) {
   testing::NiceMock<MockPayloadStatusListener> payload_listener;
   nearby_connections_manager_.RegisterPayloadStatusListener(kPayloadId,
                                                             &payload_listener);
+
+  base::File file;
+  InitializeTemporaryFile(file);
   payload_listener_remote->OnPayloadReceived(
       kRemoteEndpointId,
-      Payload::New(kPayloadId, PayloadContent::NewBytes(BytesPayload::New())));
+      Payload::New(kPayloadId,
+                   PayloadContent::NewFile(FilePayload::New(std::move(file)))));
 
   base::RunLoop payload_run_loop;
   EXPECT_CALL(payload_listener, OnStatusUpdate(testing::_))

chrome/browser/nearby_sharing/nearby_file_handler.cc

@@ -36,6 +36,17 @@ std::vector<NearbyFileHandler::FileInfo> DoOpenFiles(
   return files;
 }
 
+NearbyFileHandler::CreateFileResult DoCreateFile(base::FilePath file_path) {
+  base::FilePath unique_path = base::GetUniquePath(file_path);
+  NearbyFileHandler::CreateFileResult result;
+  result.output_file.Initialize(
+      unique_path,
+      base::File::Flags::FLAG_CREATE_ALWAYS | base::File::Flags::FLAG_WRITE);
+  result.input_file.Initialize(
+      unique_path, base::File::Flags::FLAG_OPEN | base::File::Flags::FLAG_READ);
+  return result;
+}
+
 }  // namespace
 
 NearbyFileHandler::NearbyFileHandler() : task_runner_(CreateFileTaskRunner()) {}
@@ -58,3 +69,9 @@ void NearbyFileHandler::ReleaseFilePayloads(std::vector<PayloadPtr> payloads) {
   if (!files->empty())
     task_runner_->DeleteSoon(FROM_HERE, std::move(files));
 }
+
+void NearbyFileHandler::CreateFile(const base::FilePath& file_path,
+                                   CreateFileCallback callback) {
+  task_runner_->PostTaskAndReplyWithResult(
+      FROM_HERE, base::BindOnce(&DoCreateFile, file_path), std::move(callback));
+}

chrome/browser/nearby_sharing/nearby_file_handler.h

@@ -23,8 +23,13 @@ class NearbyFileHandler {
     int64_t size;
     base::File file;
   };
+  struct CreateFileResult {
+    base::File input_file;
+    base::File output_file;
+  };
   using PayloadPtr = location::nearby::connections::mojom::PayloadPtr;
   using OpenFilesCallback = base::OnceCallback<void(std::vector<FileInfo>)>;
+  using CreateFileCallback = base::OnceCallback<void(CreateFileResult)>;
 
   NearbyFileHandler();
   ~NearbyFileHandler();
@@ -39,6 +44,10 @@ class NearbyFileHandler {
   // might block.
   void ReleaseFilePayloads(std::vector<PayloadPtr> payloads);
 
+  // Create and open the file given in |file_path| and returns the opened files
+  // via |callback|.
+  void CreateFile(const base::FilePath& file_path, CreateFileCallback callback);
+
  private:
   // Task runner for doing file operations.
   const scoped_refptr<base::SequencedTaskRunner> task_runner_;