arc: enable Android tracing in verified-boot mode

content/browser/tracing/arc_tracing_agent.cc

Index: content/browser/tracing/arc_tracing_agent.cc
diff --git a/content/browser/tracing/arc_tracing_agent.cc b/content/browser/tracing/arc_tracing_agent.cc
index d71cd647ae870308c2d5c68e3f600c2345115814..93a258db32975f8efe9561d05ff1fee84e15f611 100644
--- a/content/browser/tracing/arc_tracing_agent.cc
+++ b/content/browser/tracing/arc_tracing_agent.cc
@@ -4,24 +4,126 @@
 
 #include "content/browser/tracing/arc_tracing_agent.h"
 
+#include <string.h>

[Luis Héctor Chávez, 2017/04/13 21:37:52]

do you still need this?

[Earl Ou, 2017/04/15 20:05:51]

Done.

+#include <sys/socket.h>

[Luis Héctor Chávez, 2017/04/13 21:37:52]

Do you still need this?

[Earl Ou, 2017/04/15 20:05:51]

Done.

+
+#include <memory>
 #include <string>
+#include <utility>
+#include <vector>
 
 #include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/files/file.h"
+#include "base/files/file_descriptor_watcher_posix.h"
 #include "base/logging.h"
+#include "base/memory/ptr_util.h"
 #include "base/memory/singleton.h"
-#include "base/threading/thread_checker.h"
+#include "base/memory/weak_ptr.h"
+#include "base/posix/unix_domain_socket_linux.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "base/time/time.h"
+#include "cc/base/ring_buffer.h"
+#include "content/public/browser/browser_thread.h"
 
 namespace content {
 
 namespace {
 
+// The maximum size used to store one trace event. The ad hoc trace format for
+// atrace is 1024 bytes. Here we add additional size as we're using JSON and
+// have additional data field.
+constexpr size_t kArcTraceMessageLength = 1024 + 512;
+
 constexpr char kArcTracingAgentName[] = "arc";
 constexpr char kArcTraceLabel[] = "ArcTraceEvents";
 
-void OnStopTracing(bool success) {
-  DLOG_IF(WARNING, !success) << "Failed to stop ARC tracing.";
-}
+// Number of events for the ring buffer.
+constexpr size_t kTraceEventBufferSize = 64000;
+
+// A helper class for reading trace data from the client side. We separate this
+// from |ArcTracingAgentImpl| to isolate logics that runs on browser's IO

[Luis Héctor Chávez, 2017/04/13 21:37:52]

nit: s/logics/the logic/

[Earl Ou, 2017/04/15 20:05:51]

Done.

+// thread. All the functions in this class except for constructor, destructor,
+// and |GetWeakPtr| are expected to be run on browser's IO thread.
+class ArcTracingReader {
+ public:
+  using StopTracingCallback =
+      base::Callback<void(const scoped_refptr<base::RefCountedString>&)>;
+
+  ArcTracingReader() : weak_ptr_factory_(this) {}
+
+  void StartTracing(base::ScopedFD read_fd) {
+    DCHECK_CURRENTLY_ON(BrowserThread::IO);
+    read_fd_ = std::move(read_fd);
+    // We don't use the weak pointer returned by |GetWeakPtr| to avoid using it
+    // on different task runner. Instead, we use |base::Unretained| here as
+    // fd_watcher_ is always destructed before |this| got destructed.

[Luis Héctor Chávez, 2017/04/13 21:37:52]

nit: |fd_watcher_|. s/got destructed/is destroyed/.

Also s/destructed/destroyed/ in all the change since that spelling is way more
common:

chromium/src$ ag '//.*destructed' chrome | wc -l
50
chromium/src$ ag '//.*destroyed' chrome | wc -l
972

+    fd_watcher_ = base::FileDescriptorWatcher::WatchReadable(
+        read_fd_.get(), base::Bind(&ArcTracingReader::OnTraceDataAvailable,
+                                   base::Unretained(this)));
+  }
+
+  void OnTraceDataAvailable() {
+    DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+    char buf[kArcTraceMessageLength];
+    std::vector<base::ScopedFD> unused_fds;
+    ssize_t n = base::UnixDomainSocket::RecvMsg(
+        read_fd_.get(), buf, kArcTraceMessageLength, &unused_fds);
+    // When EOF, return and do nothing. The clean up is done in StopTracing.
+    if (n == 0)
+      return;
+
+    if (n < 0) {
+      DPLOG(WARNING) << "Unexpected error while reading trace from client.";
+      // Do nothing here as StopTracing will do the clean up and the existing
+      // trace logs will be returned.
+      return;
+    }
+
+    if (n > static_cast<ssize_t>(kArcTraceMessageLength)) {
+      DLOG(WARNING) << "Unexpected data size when reading trace from client.";
+      return;
+    }
+    ring_buffer_.SaveToBuffer(std::string(buf, n));
+  }
+
+  void StopTracing(const StopTracingCallback& callback) {
+    DCHECK_CURRENTLY_ON(BrowserThread::IO);
+    fd_watcher_.reset();
+    read_fd_.reset();
+
+    bool append_comma = false;
+    std::string data;
+    for (auto it = ring_buffer_.Begin(); it; ++it) {
+      if (append_comma)
+        data.append(",");
+      else
+        append_comma = true;
+      data.append(**it);
+    }
+    ring_buffer_.Clear();
+
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(callback, base::RefCountedString::TakeString(&data)));
+  }
+
+  base::WeakPtr<ArcTracingReader> GetWeakPtr() {
+    return weak_ptr_factory_.GetWeakPtr();
+  }
+
+ private:
+  base::ScopedFD read_fd_;
+  std::unique_ptr<base::FileDescriptorWatcher::Controller> fd_watcher_;
+  cc::RingBuffer<std::string, kTraceEventBufferSize> ring_buffer_;
+  // NOTE: Weak pointers must be invalidated before all other member variables
+  // so it must be the last member.
+  base::WeakPtrFactory<ArcTracingReader> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(ArcTracingReader);
+};
 
 class ArcTracingAgentImpl : public ArcTracingAgent {
  public:
@@ -32,10 +134,16 @@ class ArcTracingAgentImpl : public ArcTracingAgent {
 
   void StartAgentTracing(const base::trace_event::TraceConfig& trace_config,
                          const StartAgentTracingCallback& callback) override {
-    DCHECK(thread_checker_.CalledOnValidThread());
+    DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
     // delegate_ may be nullptr if ARC is not enabled on the system. In such
     // case, simply do nothing.
-    if (!delegate_) {
+    bool success = (delegate_ != nullptr);
+
+    base::ScopedFD write_fd, read_fd;
+    success = success && CreateSocketPair(&read_fd, &write_fd);
+
+    if (!success) {
       // Use PostTask as the convention of TracingAgent. The caller expects
       // callback to be called after this function returns.
       base::ThreadTaskRunnerHandle::Get()->PostTask(
@@ -43,27 +151,33 @@ class ArcTracingAgentImpl : public ArcTracingAgent {
       return;
     }
 
-    delegate_->StartTracing(trace_config,
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(&ArcTracingReader::StartTracing, reader_.GetWeakPtr(),
+                   base::Passed(&read_fd)));
+
+    delegate_->StartTracing(trace_config, std::move(write_fd),
                             base::Bind(callback, GetTracingAgentName()));
   }
 
   void StopAgentTracing(const StopAgentTracingCallback& callback) override {
-    DCHECK(thread_checker_.CalledOnValidThread());
-    if (delegate_)
-      delegate_->StopTracing(base::Bind(OnStopTracing));
-
-    // Trace data is collect via systrace (debugd) in dev-mode. Simply
-    // return empty data here.
-    std::string no_data;
-    base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE,
-        base::Bind(callback, GetTracingAgentName(), GetTraceEventLabel(),
-                   base::RefCountedString::TakeString(&no_data)));
+    DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+    if (is_stopping_) {
+      DLOG(WARNING) << "Already working on stopping ArcTracingAgent.";
+      return;
+    }
+    is_stopping_ = true;
+    if (delegate_) {
+      delegate_->StopTracing(
+          base::Bind(&ArcTracingAgentImpl::OnArcTracingStopped,
+                     weak_ptr_factory_.GetWeakPtr(), callback));
+    }
   }
 
   // ArcTracingAgent overrides:
   void SetDelegate(Delegate* delegate) override {
-    DCHECK(thread_checker_.CalledOnValidThread());
+    DCHECK_CURRENTLY_ON(BrowserThread::UI);
     delegate_ = delegate;
   }
 
@@ -76,11 +190,48 @@ class ArcTracingAgentImpl : public ArcTracingAgent {
   // by the Singleton class.
   friend struct base::DefaultSingletonTraits<ArcTracingAgentImpl>;
 
-  ArcTracingAgentImpl() = default;
+  ArcTracingAgentImpl() : weak_ptr_factory_(this) {}
+
   ~ArcTracingAgentImpl() override = default;
 
+  void OnArcTracingStopped(const StopAgentTracingCallback& callback,
+                           bool success) {
+    DCHECK_CURRENTLY_ON(BrowserThread::UI);
+    if (!success) {
+      DLOG(WARNING) << "Failed to stop ARC tracing.";
+      callback.Run(GetTracingAgentName(), GetTraceEventLabel(),
+                   new base::RefCountedString());
+      is_stopping_ = false;
+      return;
+    }
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(&ArcTracingReader::StopTracing, reader_.GetWeakPtr(),
+                   base::Bind(&ArcTracingAgentImpl::OnTracingReaderStopped,
+                              weak_ptr_factory_.GetWeakPtr(), callback)));
+  }
+
+  void OnTracingReaderStopped(
+      const StopAgentTracingCallback& callback,
+      const scoped_refptr<base::RefCountedString>& data) {
+    DCHECK_CURRENTLY_ON(BrowserThread::UI);
+    callback.Run(GetTracingAgentName(), GetTraceEventLabel(), data);
+    is_stopping_ = false;
+  }
+
   Delegate* delegate_ = nullptr;  // Owned by ArcServiceLauncher.
-  base::ThreadChecker thread_checker_;
+  // We use |reader_.GetWeakPtr()| when binding callbacks with its functions.
+  // Notes that the weak pointer returned by it can only be deferenced or
+  // invalided in the same task runner to avoid racing condition. The
+  // destruction of |reader_| is also a source of invalidation. However, we're
+  // lucky as we're using |ArcTracingAgentImpl| as a singleton, the
+  // destruction is always performed after all messageloop destructed, and thus

[Luis Héctor Chávez, 2017/04/13 21:37:52]

nit: all MessageLoops are destructed.

[Earl Ou, 2017/04/15 20:05:51]

Done.

+  // no racing condition in such situation.

[Luis Héctor Chávez, 2017/04/13 21:37:52]

nit: "and thus there are no race conditions in such situation".

[Earl Ou, 2017/04/15 20:05:51]

Done.

+  ArcTracingReader reader_;
+  bool is_stopping_ = false;
+  // NOTE: Weak pointers must be invalidated before all other member variables
+  // so it must be the last member.
+  base::WeakPtrFactory<ArcTracingAgentImpl> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ArcTracingAgentImpl);
 };