Update UE to remove MessageLoop::current()->WatchFileDescriptor.

Tianjie · Tianjie Xu · commit 934b847620e6 · 2020-07-23T18:27:50.000Z
MessageLoop::current()-&gt;WatchFileDescriptor is deprecated. And UE should
remove usages of it.

Test: mma &amp;&amp; unittest
Change-Id: Ib1ef2e6b6a38ad2a8d07b78bcd72fdb3b7f82226
diff --git a/common/subprocess.cc b/common/subprocess.cc
@@ -129,12 +129,7 @@ void Subprocess::OnStdoutReady(SubprocessRecord* record) {
     if (!ok || eof) {
       // There was either an error or an EOF condition, so we are done watching
       // the file descriptor.
-#ifdef __ANDROID__
-      MessageLoop::current()->CancelTask(record->stdout_task_id);
-      record->stdout_task_id = MessageLoop::kTaskIdNull;
-#else
       record->stdout_controller.reset();
-#endif  // __ANDROID__
       return;
     }
   } while (bytes_read);
@@ -149,12 +144,7 @@ void Subprocess::ChildExitedCallback(const siginfo_t& info) {
   // Make sure we read any remaining process output and then close the pipe.
   OnStdoutReady(record);
 
-#ifdef __ANDROID__
-  MessageLoop::current()->CancelTask(record->stdout_task_id);
-  record->stdout_task_id = MessageLoop::kTaskIdNull;
-#else
   record->stdout_controller.reset();
-#endif  // __ANDROID__
 
   // Don't print any log if the subprocess exited with exit code 0.
   if (info.si_code != CLD_EXITED) {
@@ -209,18 +199,9 @@ pid_t Subprocess::ExecFlags(const vector<string>& cmd,
                << record->stdout_fd << ".";
   }
 
-#ifdef __ANDROID__
-  record->stdout_task_id = MessageLoop::current()->WatchFileDescriptor(
-      FROM_HERE,
-      record->stdout_fd,
-      MessageLoop::WatchMode::kWatchRead,
-      true,
-      base::Bind(&Subprocess::OnStdoutReady, record.get()));
-#else
   record->stdout_controller = base::FileDescriptorWatcher::WatchReadable(
       record->stdout_fd,
       base::BindRepeating(&Subprocess::OnStdoutReady, record.get()));
-#endif  // __ANDROID__
 
   subprocess_records_[pid] = std::move(record);
   return pid;
diff --git a/common/subprocess.h b/common/subprocess.h
@@ -123,12 +123,8 @@ class Subprocess {
 
     // These are used to monitor the stdout of the running process, including
     // the stderr if it was redirected.
-#ifdef __ANDROID__
-    brillo::MessageLoop::TaskId stdout_task_id{
-        brillo::MessageLoop::kTaskIdNull};
-#else
     std::unique_ptr<base::FileDescriptorWatcher::Controller> stdout_controller;
-#endif  // __ANDROID__
+
     int stdout_fd{-1};
     std::string stdout;
   };
diff --git a/common/subprocess_unittest.cc b/common/subprocess_unittest.cc
@@ -74,9 +74,7 @@ class SubprocessTest : public ::testing::Test {
   brillo::BaseMessageLoop loop_{&base_loop_};
   brillo::AsynchronousSignalHandler async_signal_handler_;
   Subprocess subprocess_;
-#ifndef __ANDROID__
   unique_ptr<base::FileDescriptorWatcher::Controller> watcher_;
-#endif  // __ANDROID__
 
 };
 
@@ -261,23 +259,6 @@ TEST_F(SubprocessTest, CancelTest) {
   int fifo_fd = HANDLE_EINTR(open(fifo_path.c_str(), O_RDONLY));
   EXPECT_GE(fifo_fd, 0);
 
-#ifdef __ANDROID__
-  loop_.WatchFileDescriptor(FROM_HERE,
-                            fifo_fd,
-                            MessageLoop::WatchMode::kWatchRead,
-                            false,
-                            base::Bind(
-                                [](int fifo_fd, uint32_t tag) {
-                                  char c;
-                                  EXPECT_EQ(1,
-                                            HANDLE_EINTR(read(fifo_fd, &c, 1)));
-                                  EXPECT_EQ('X', c);
-                                  LOG(INFO) << "Killing tag " << tag;
-                                  Subprocess::Get().KillExec(tag);
-                                },
-                                fifo_fd,
-                                tag));
-#else
   watcher_ = base::FileDescriptorWatcher::WatchReadable(
       fifo_fd,
       base::Bind(
@@ -295,7 +276,6 @@ TEST_F(SubprocessTest, CancelTest) {
           base::Unretained(&watcher_),
           fifo_fd,
           tag));
-#endif  // __ANDROID__
 
   // This test would leak a callback that runs when the child process exits
   // unless we wait for it to run.
diff --git a/libcurl_http_fetcher.cc b/libcurl_http_fetcher.cc
@@ -31,6 +31,8 @@
 #include <base/strings/string_split.h>
 #include <base/strings/string_util.h>
 #include <base/strings/stringprintf.h>
+#include <base/threading/thread_task_runner_handle.h>
+
 
 #ifdef __ANDROID__
 #include <cutils/qtaguid.h>
@@ -81,23 +83,9 @@ int LibcurlHttpFetcher::LibcurlCloseSocketCallback(void* clientp,
 
   LibcurlHttpFetcher* fetcher = static_cast<LibcurlHttpFetcher*>(clientp);
   // Stop watching the socket before closing it.
-#ifdef __ANDROID__
-  for (size_t t = 0; t < arraysize(fetcher->fd_task_maps_); ++t) {
-    const auto fd_task_pair = fetcher->fd_task_maps_[t].find(item);
-    if (fd_task_pair != fetcher->fd_task_maps_[t].end()) {
-      if (!MessageLoop::current()->CancelTask(fd_task_pair->second)) {
-        LOG(WARNING) << "Error canceling the watch task "
-                     << fd_task_pair->second << " for "
-                     << (t ? "writing" : "reading") << " the fd " << item;
-      }
-      fetcher->fd_task_maps_[t].erase(item);
-    }
-  }
-#else
   for (size_t t = 0; t < base::size(fetcher->fd_controller_maps_); ++t) {
     fetcher->fd_controller_maps_[t].erase(item);
   }
-#endif  // __ANDROID__
 
   // Documentation for this callback says to return 0 on success or 1 on error.
   if (!IGNORE_EINTR(close(item)))
@@ -471,6 +459,19 @@ void LibcurlHttpFetcher::CurlPerformOnce() {
     // There's either more work to do or we are paused, so we just keep the
     // file descriptors to watch up to date and exit, until we are done with the
     // work and we are not paused.
+#ifdef __ANDROID__
+    // When there's no base::SingleThreadTaskRunner on current thread, it's not
+    // possible to watch file descriptors. Just poll it later. This usually
+    // happens if brillo::FakeMessageLoop is used.
+    if (!base::ThreadTaskRunnerHandle::IsSet()) {
+        MessageLoop::current()->PostDelayedTask(
+            FROM_HERE,
+            base::Bind(&LibcurlHttpFetcher::CurlPerformOnce,
+                       base::Unretained(this)),
+            TimeDelta::FromSeconds(1));
+        return;
+    }
+#endif
     SetupMessageLoopSources();
     return;
   }
@@ -691,63 +692,6 @@ void LibcurlHttpFetcher::SetupMessageLoopSources() {
 
   // We should iterate through all file descriptors up to libcurl's fd_max or
   // the highest one we're tracking, whichever is larger.
-#ifdef __ANDROID__
-  for (size_t t = 0; t < arraysize(fd_task_maps_); ++t) {
-    if (!fd_task_maps_[t].empty())
-      fd_max = max(fd_max, fd_task_maps_[t].rbegin()->first);
-  }
-
-  // For each fd, if we're not tracking it, track it. If we are tracking it, but
-  // libcurl doesn't care about it anymore, stop tracking it. After this loop,
-  // there should be exactly as many tasks scheduled in fd_task_maps_[0|1] as
-  // there are read/write fds that we're tracking.
-  for (int fd = 0; fd <= fd_max; ++fd) {
-    // Note that fd_exc is unused in the current version of libcurl so is_exc
-    // should always be false.
-    bool is_exc = FD_ISSET(fd, &fd_exc) != 0;
-    bool must_track[2] = {
-        is_exc || (FD_ISSET(fd, &fd_read) != 0),  // track 0 -- read
-        is_exc || (FD_ISSET(fd, &fd_write) != 0)  // track 1 -- write
-    };
-    MessageLoop::WatchMode watch_modes[2] = {
-        MessageLoop::WatchMode::kWatchRead,
-        MessageLoop::WatchMode::kWatchWrite,
-    };
-
-    for (size_t t = 0; t < arraysize(fd_task_maps_); ++t) {
-      auto fd_task_it = fd_task_maps_[t].find(fd);
-      bool tracked = fd_task_it != fd_task_maps_[t].end();
-
-      if (!must_track[t]) {
-        // If we have an outstanding io_channel, remove it.
-        if (tracked) {
-          MessageLoop::current()->CancelTask(fd_task_it->second);
-          fd_task_maps_[t].erase(fd_task_it);
-        }
-        continue;
-      }
-
-      // If we are already tracking this fd, continue -- nothing to do.
-      if (tracked)
-        continue;
-
-      // Track a new fd.
-      fd_task_maps_[t][fd] = MessageLoop::current()->WatchFileDescriptor(
-          FROM_HERE,
-          fd,
-          watch_modes[t],
-          true,  // persistent
-          base::Bind(&LibcurlHttpFetcher::CurlPerformOnce,
-                     base::Unretained(this)));
-
-      static int io_counter = 0;
-      io_counter++;
-      if (io_counter % 50 == 0) {
-        LOG(INFO) << "io_counter = " << io_counter;
-      }
-    }
-  }
-#else
   for (size_t t = 0; t < base::size(fd_controller_maps_); ++t) {
     if (!fd_controller_maps_[t].empty())
       fd_max = max(fd_max, fd_controller_maps_[t].rbegin()->first);
@@ -803,7 +747,6 @@ void LibcurlHttpFetcher::SetupMessageLoopSources() {
       }
     }
   }
-#endif  // __ANDROID__
 
   // Set up a timeout callback for libcurl.
   if (timeout_id_ == MessageLoop::kTaskIdNull) {
@@ -848,22 +791,9 @@ void LibcurlHttpFetcher::CleanUp() {
   MessageLoop::current()->CancelTask(timeout_id_);
   timeout_id_ = MessageLoop::kTaskIdNull;
 
-#ifdef __ANDROID__
-  for (size_t t = 0; t < arraysize(fd_task_maps_); ++t) {
-    for (const auto& fd_taks_pair : fd_task_maps_[t]) {
-      if (!MessageLoop::current()->CancelTask(fd_taks_pair.second)) {
-        LOG(WARNING) << "Error canceling the watch task " << fd_taks_pair.second
-                     << " for " << (t ? "writing" : "reading") << " the fd "
-                     << fd_taks_pair.first;
-      }
-    }
-    fd_task_maps_[t].clear();
-  }
-#else
   for (size_t t = 0; t < base::size(fd_controller_maps_); ++t) {
     fd_controller_maps_[t].clear();
   }
-#endif  // __ANDROID__
 
   if (curl_http_headers_) {
     curl_slist_free_all(curl_http_headers_);
diff --git a/libcurl_http_fetcher.h b/libcurl_http_fetcher.h
@@ -255,12 +255,8 @@ class LibcurlHttpFetcher : public HttpFetcher {
   // the message loop. libcurl may open/close descriptors and switch their
   // directions so maintain two separate lists so that watch conditions can be
   // set appropriately.
-#ifdef __ANDROID__
-  std::map<int, brillo::MessageLoop::TaskId> fd_task_maps_[2];
-#else
   std::map<int, std::unique_ptr<base::FileDescriptorWatcher::Controller>>
       fd_controller_maps_[2];
-#endif  // __ANDROID__
 
   // The TaskId of the timer we're waiting on. kTaskIdNull if we are not waiting
   // on it.
diff --git a/libcurl_http_fetcher_unittest.cc b/libcurl_http_fetcher_unittest.cc
@@ -94,37 +94,24 @@ TEST_F(LibcurlHttpFetcherTest, InvalidURLTest) {
             no_network_max_retries);
 }
 
-#ifdef __ANDROID__
-TEST_F(LibcurlHttpFetcherTest, CouldntResolveHostTest) {
+TEST_F(LibcurlHttpFetcherTest, CouldNotResolveHostTest) {
   int no_network_max_retries = 1;
   libcurl_fetcher_.set_no_network_max_retries(no_network_max_retries);
 
-  // This test actually sends request to internet but according to
-  // https://tools.ietf.org/html/rfc2606#section-2, .invalid domain names are
-  // reserved and sure to be invalid. Ideally we should mock libcurl or
-  // reorganize LibcurlHttpFetcher so the part that sends request can be mocked
-  // easily.
-  // TODO(xiaochu) Refactor LibcurlHttpFetcher (and its relates) so it's
-  // easier to mock the part that depends on internet connectivity.
   libcurl_fetcher_.BeginTransfer("https://An-uNres0lvable-uRl.invalid");
-  while (loop_.PendingTasks()) {
+
+#ifdef __ANDROID__
+  // It's slower on Android that libcurl handle may not finish within 1 cycle.
+  // Will need to wait for more cycles until it finishes. Original test didn't
+  // correctly handle when we need to re-watch libcurl fds.
+  while (loop_.PendingTasks() &&
+         libcurl_fetcher_.GetAuxiliaryErrorCode() == ErrorCode::kSuccess) {
     loop_.RunOnce(true);
   }
-
-  // If libcurl fails to resolve the name, we call res_init() to reload
-  // resolv.conf and retry exactly once more. See crbug.com/982813 for details.
-  EXPECT_EQ(libcurl_fetcher_.get_no_network_max_retries(),
-            no_network_max_retries + 1);
-}
 #else
-TEST_F(LibcurlHttpFetcherTest, CouldNotResolveHostTest) {
-  int no_network_max_retries = 1;
-  libcurl_fetcher_.set_no_network_max_retries(no_network_max_retries);
-
-  libcurl_fetcher_.BeginTransfer("https://An-uNres0lvable-uRl.invalid");
-
   // The first time it can't resolve.
   loop_.RunOnce(true);
+#endif
   EXPECT_EQ(libcurl_fetcher_.GetAuxiliaryErrorCode(),
             ErrorCode::kUnresolvedHostError);
 
@@ -154,8 +141,18 @@ TEST_F(LibcurlHttpFetcherTest, HostResolvedTest) {
   // easier to mock the part that depends on internet connectivity.
   libcurl_fetcher_.BeginTransfer("https://An-uNres0lvable-uRl.invalid");
 
+#ifdef __ANDROID__
+  // It's slower on Android that libcurl handle may not finish within 1 cycle.
+  // Will need to wait for more cycles until it finishes. Original test didn't
+  // correctly handle when we need to re-watch libcurl fds.
+  while (loop_.PendingTasks() &&
+         libcurl_fetcher_.GetAuxiliaryErrorCode() == ErrorCode::kSuccess) {
+    loop_.RunOnce(true);
+  }
+#else
   // The first time it can't resolve.
   loop_.RunOnce(true);
+#endif
   EXPECT_EQ(libcurl_fetcher_.GetAuxiliaryErrorCode(),
             ErrorCode::kUnresolvedHostError);
 
@@ -168,9 +165,19 @@ TEST_F(LibcurlHttpFetcherTest, HostResolvedTest) {
           [this]() { libcurl_fetcher_.http_response_code_ = 0; }));
   libcurl_fetcher_.transfer_size_ = 10;
 
+#ifdef __ANDROID__
+  // It's slower on Android that libcurl handle may not finish within 1 cycle.
+  // Will need to wait for more cycles until it finishes. Original test didn't
+  // correctly handle when we need to re-watch libcurl fds.
+  while (loop_.PendingTasks() && libcurl_fetcher_.GetAuxiliaryErrorCode() ==
+                                     ErrorCode::kUnresolvedHostError) {
+    loop_.RunOnce(true);
+  }
+#else
   // This time the host is resolved. But after that again we can't resolve
   // anymore (See above).
   loop_.RunOnce(true);
+#endif
   EXPECT_EQ(libcurl_fetcher_.GetAuxiliaryErrorCode(),
             ErrorCode::kUnresolvedHostRecovered);
 
@@ -186,7 +193,6 @@ TEST_F(LibcurlHttpFetcherTest, HostResolvedTest) {
   EXPECT_EQ(libcurl_fetcher_.get_no_network_max_retries(),
             no_network_max_retries + 1);
 }
-#endif  // __ANDROID__
 
 TEST_F(LibcurlHttpFetcherTest, HttpFetcherStateMachineRetryFailedTest) {
   state_machine_.UpdateState(true);
diff --git a/payload_consumer/postinstall_runner_action.cc b/payload_consumer/postinstall_runner_action.cc
diff --git a/payload_consumer/postinstall_runner_action.h b/payload_consumer/postinstall_runner_action.h
