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..e2a4820a7dc07fd6dca9c07c45e7f6b85c911dc3 100644
--- a/content/browser/tracing/arc_tracing_agent.cc
+++ b/content/browser/tracing/arc_tracing_agent.cc
@@ -4,23 +4,48 @@
 
 #include "content/browser/tracing/arc_tracing_agent.h"
 
+#include <string.h>
+#include <sys/socket.h>
+
+#include <memory>
 #include <string>
+#include <utility>
 
 #include "base/bind.h"
+#include "base/files/file.h"
+#include "base/files/file_descriptor_watcher_posix.h"
 #include "base/logging.h"
 #include "base/memory/singleton.h"
-#include "base/threading/thread_checker.h"
+#include "base/memory/weak_ptr.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "base/time/time.h"
+#include "base/trace_event/trace_buffer.h"
+#include "content/public/browser/browser_thread.h"
+
+using base::trace_event::TraceEvent;
+using base::trace_event::TraceBuffer;
+using base::trace_event::TraceBufferChunk;
 
 namespace content {
 
 namespace {
 
+constexpr int kArcTraceMessageLength = 1024 + 512;

[Luis Héctor Chávez, 2017/03/22 17:04:58]

nit: size_t

[shunhsingou, 2017/03/28 13:54:15]

Done.

 constexpr char kArcTracingAgentName[] = "arc";
 constexpr char kArcTraceLabel[] = "ArcTraceEvents";
 
-void OnStopTracing(bool success) {
-  DLOG_IF(WARNING, !success) << "Failed to stop ARC tracing.";
+// Number of chunks for the ring buffer.
+constexpr int kTraceEventChunks =

[Luis Héctor Chávez, 2017/03/22 17:04:57]

nit: size_t

[shunhsingou, 2017/03/28 13:54:14]

Done.

+    64000 / TraceBufferChunk::kTraceBufferChunkSize;
+
+bool CreateSocketPair(base::ScopedFD* one, base::ScopedFD* two) {

[Luis Héctor Chávez, 2017/03/22 17:04:57]

I see that you copied this from base/posix/unix_domain_socket_linux.cc . Can you
try to declare it in the .h file (in another change) to avoid repetition?

[shunhsingou, 2017/03/28 13:54:14]

Done.

+  int raw_socks[2];
+  if (socketpair(AF_UNIX, SOCK_SEQPACKET, 0, raw_socks) == -1)
+    return false;
+  one->reset(raw_socks[0]);
+  two->reset(raw_socks[1]);
+  return true;
 }
 
 class ArcTracingAgentImpl : public ArcTracingAgent {
@@ -32,7 +57,7 @@ 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_) {
@@ -42,28 +67,36 @@ class ArcTracingAgentImpl : public ArcTracingAgent {
           FROM_HERE, base::Bind(callback, GetTracingAgentName(), false));
       return;
     }
-
-    delegate_->StartTracing(trace_config,
+    base::ScopedFD write_fd, read_fd;
+    CreateSocketPair(&read_fd, &write_fd);

[Luis Héctor Chávez, 2017/03/22 17:04:58]

Make this fail if CreateSocketPair() returns false.

[shunhsingou, 2017/03/28 13:54:15]

Done.

+    data_file_.reset(new base::File(read_fd.release()));

[Luis Héctor Chávez, 2017/03/22 17:04:57]

nit: data_file_ = base::MakeUnique<base::File>(read_fd.release());

[shunhsingou, 2017/03/28 13:54:14]

This data member is removed.

+    CreateBuffer();
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(&ArcTracingAgentImpl::InitFileDescriptorWatcher,
+                   weak_ptr_factory_.GetWeakPtr(),
+                   data_file_->GetPlatformFile()));
+    delegate_->StartTracing(trace_config, std::move(write_fd),
                             base::Bind(callback, GetTracingAgentName()));
   }
 
   void StopAgentTracing(const StopAgentTracingCallback& callback) override {
-    DCHECK(thread_checker_.CalledOnValidThread());
+    DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+    if (!callback_.is_null()) {
+      DLOG(WARNING) << "Already working on stopping ArcTracingAgent.";
+      return;
+    }
+    callback_ = callback;

[Luis Héctor Chávez, 2017/03/22 17:04:57]

You don't need to do this. You can instead do

delegate_->StopTracing(
    base::Bind(&ArcTracingAgentImpl::OnArcTracingStopped,
               weak_ptr_factory_.GetWeakPtr(),
               callback));

and add the |const StopAgentTracingCallback& callback| param to
OnArcTracingStopped() as the first parameter. Do the same thing for
StopTracingOnIOThread().

[shunhsingou, 2017/03/28 13:54:14]

Done. My original thought is that we still need another boolean flag, e.g.,
is_stopping, to check if we're currently stopping tracing, and it can be done by
callback_.is_null(). But good to remove it as we it's more clear.

     if (delegate_)

[Luis Héctor Chávez, 2017/03/22 17:04:58]

nit: you cannot elide braces since the body spans more than one line.

[shunhsingou, 2017/03/28 13:54:15]

Done.

-      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)));
+      delegate_->StopTracing(
+          base::Bind(&ArcTracingAgentImpl::OnArcTracingStopped,
+                     weak_ptr_factory_.GetWeakPtr()));
   }
 
   // ArcTracingAgent overrides:
   void SetDelegate(Delegate* delegate) override {
-    DCHECK(thread_checker_.CalledOnValidThread());
+    DCHECK_CURRENTLY_ON(BrowserThread::UI);
     delegate_ = delegate;
   }
 
@@ -76,11 +109,110 @@ class ArcTracingAgentImpl : public ArcTracingAgent {
   // by the Singleton class.
   friend struct base::DefaultSingletonTraits<ArcTracingAgentImpl>;
 
-  ArcTracingAgentImpl() = default;
+  ArcTracingAgentImpl() : weak_ptr_factory_(this) {}
+
   ~ArcTracingAgentImpl() override = default;
 
+  void CreateBuffer() {
+    trace_buffer_.reset(
+        TraceBuffer::CreateTraceBufferRingBuffer(kTraceEventChunks));
+    chunk_.reset(nullptr);

[Luis Héctor Chávez, 2017/03/22 17:04:58]

nit: chunk_.reset();

[shunhsingou, 2017/03/28 13:54:14]

Done.

+    chunk_index_ = 0;
+  }
+
+  void OnTraceDataAvailable() {

[Luis Héctor Chávez, 2017/03/22 17:04:58]

Can you move all the buffer-related stuff to another class? Make all the methods
of that class have DCHECK_CURRENTLY_ON(BrowserThread::IO) so that way it's
easier to reason about what data / methods must live in which threads.

[shunhsingou, 2017/03/28 13:54:14]

Done.

+    DCHECK_CURRENTLY_ON(BrowserThread::IO);
+    char buf[kArcTraceMessageLength + 1];
+    int n =
+        data_file_->ReadAtCurrentPosNoBestEffort(buf, kArcTraceMessageLength);

[Luis Héctor Chávez, 2017/03/22 17:04:58]

I'm a bit worried that we might need to use recv() instead of read() here in
case of zero-length datagrams. Can you use base::UnixDomainSocket::RecvMsg()
instead of this?

[shunhsingou, 2017/03/28 13:54:14]

Done.

+    if (n == 0)

[Luis Héctor Chávez, 2017/03/22 17:04:58]

What about errors?

[shunhsingou, 2017/03/28 13:54:15]

Done.

+      return;
+
+    buf[n] = 0;
+    char* data = new char[n + 1];

[Luis Héctor Chávez, 2017/03/22 17:04:57]

you're leaking this :(

[shunhsingou, 2017/03/28 13:54:14]

Done. It's now free in StopTracing.

+    memcpy(data, buf, n + 1);

[Luis Héctor Chávez, 2017/03/22 18:23:04]

You need to add

if (n > kArcTraceMessageLength) {
  LOG(ERROR) << "...";
  return;
}

prior to this memcpy.

[shunhsingou, 2017/03/28 13:54:14]

Done.

+
+    // Save data into trace buffer.
+    if (!chunk_)
+      chunk_ = trace_buffer_->GetChunk(&chunk_index_);
+    size_t event_index;
+    TraceEvent* event = chunk_->AddTraceEvent(&event_index);
+
+    // Here we don't actually parse the data, which should already be a valid

[Luis Héctor Chávez, 2017/03/22 17:04:57]

This is a dangerous assumption :( What happens if somebody manages to push
corrupt JSON data across this interface?

[shunhsingou, 2017/03/28 13:54:15]

Done add a validation check here.

+    // trace JSON object. We only use |TraceEvent| for saving data in
+    // |TraceBuffer| and |TraceBufferChunk|. So we can just put everything in
+    // |name|, and only fetch the |name| field after finishing tracing to
+    // avoid unnecessary deserialize and serialize from/to JSON.
+    event->Initialize(0, base::TimeTicks(), base::ThreadTicks(), 0, nullptr,
+                      data, nullptr, 0, 0, 0, nullptr, nullptr, nullptr,
+                      nullptr, 0);
+
+    if (chunk_->IsFull()) {
+      trace_buffer_->ReturnChunk(chunk_index_, std::move(chunk_));
+      chunk_ = trace_buffer_->GetChunk(&chunk_index_);
+    }
+  }
+
+  void InitFileDescriptorWatcher(int fd) {
+    DCHECK_CURRENTLY_ON(BrowserThread::IO);
+    fd_watcher_ = base::FileDescriptorWatcher::WatchReadable(
+        fd, base::Bind(&ArcTracingAgentImpl::OnTraceDataAvailable,
+                       weak_ptr_factory_.GetWeakPtr()));
+  }
+
+  void OnArcTracingStopped(bool success) {
+    if (!success) {
+      DLOG(WARNING) << "Failed to stop ARC tracing.";
+      std::string no_data;
+      base::ResetAndReturn(&callback_)
+          .Run(GetTracingAgentName(), GetTraceEventLabel(),
+               base::RefCountedString::TakeString(&no_data));
+      return;
+    }
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(&ArcTracingAgentImpl::StopTracingInIOThread,
+                   weak_ptr_factory_.GetWeakPtr()));
+  }
+
+  void StopTracingInIOThread() {
+    DCHECK_CURRENTLY_ON(BrowserThread::IO);
+    close(read_fd_.release());

[Luis Héctor Chávez, 2017/03/22 18:09:34]

nit: read_fd_.reset();

[shunhsingou, 2017/03/28 13:54:15]

Done.

+    // Stop fd_watcher_.
+    fd_watcher_.reset(nullptr);

[Luis Héctor Chávez, 2017/03/22 18:09:34]

nit: fd_watcher_.reset();

[shunhsingou, 2017/03/28 13:54:14]

Done.

+    if (chunk_)
+      trace_buffer_->ReturnChunk(chunk_index_, std::move(chunk_));
+
+    bool append_comma = false;
+    std::string data;
+    while (const TraceBufferChunk* chunk = trace_buffer_->NextChunk()) {
+      for (size_t i = 0; i < chunk->size(); ++i) {
+        const TraceEvent* event = chunk->GetEventAt(i);
+        if (append_comma)
+          data.append(",");
+        // See comment in |OnTraceDataAvailable|. We put the whole valid JSON
+        // object in |name|.
+        data.append(event->name());
+        append_comma = true;
+      }
+    }
+    base::ResetAndReturn(&callback_)
+        .Run(GetTracingAgentName(), GetTraceEventLabel(),
+             base::RefCountedString::TakeString(&data));
+  }
+
   Delegate* delegate_ = nullptr;  // Owned by ArcServiceLauncher.
-  base::ThreadChecker thread_checker_;
+  base::ScopedFD read_fd_;
+  std::unique_ptr<base::File> data_file_;
+  std::unique_ptr<base::FileDescriptorWatcher::Controller> fd_watcher_;
+  std::unique_ptr<TraceBuffer> trace_buffer_;
+  std::unique_ptr<TraceBufferChunk> chunk_;
+  size_t chunk_index_;
+  StopAgentTracingCallback callback_;
+
+  // 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);
 };