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..7c36efa88db738647b369bf88232fd3a4c47a794 100644
--- a/content/browser/tracing/arc_tracing_agent.cc
+++ b/content/browser/tracing/arc_tracing_agent.cc
@@ -4,24 +4,116 @@
 
 #include "content/browser/tracing/arc_tracing_agent.h"
 
+#include <string.h>
+#include <sys/socket.h>
+
+#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 {
 
+constexpr size_t kArcTraceMessageLength = 1024 + 512;

[oystein (OOO til 10th of July), 2017/04/07 18:31:40]

Can you document why this particular value was chosen?

[Earl Ou, 2017/04/08 14:48:17]

Done.

 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;
+
+class ArcTracingReader {

[oystein (OOO til 10th of July), 2017/04/07 18:31:40]

It's not immediately obvious to me why this was factored out into its own class,
since the lifespan seems to be identical with that of the parent
ArcTracingAgentImpl. Can you put a comment explaining why?

[Earl Ou, 2017/04/08 14:48:17]

Done.

+ 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);
+    fd_watcher_ = base::FileDescriptorWatcher::WatchReadable(
+        read_fd_.get(), base::Bind(&ArcTracingReader::OnTraceDataAvailable,
+                                   base::Unretained(this)));

[Luis Héctor Chávez, 2017/04/07 16:17:46]

nit: s/base::Unretained(this)/GetWeakPtr())/

[dcheng, 2017/04/07 17:58:28]

I'm OK with unretained as long as it's explained why this is safe

(Using GetWeakPtr() here has the potential to make this class more complex, as
we call GetWeakPtr() on multiple threads and it will be a bit harder to
understand the thread safety)

[Earl Ou, 2017/04/08 14:48:17]

Yes, I think we should use |Unretained| here to avoid weak ptr in different
runner. Added a comment for this.  

+  }
+
+  void OnTraceDataAvailable() {
+    DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+    char buf[kArcTraceMessageLength + 1];
+    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) {
+      PLOG(WARNING) << "Unexpected error while reading trace from client.";

[dcheng, 2017/04/07 17:58:28]

DPLOG?

[Earl Ou, 2017/04/08 14:48:17]

Done.

+      // 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)) {
+      LOG(WARNING) << "Unexpected data size when reading trace from client.";

[dcheng, 2017/04/07 17:58:28]

DLOG?

[Earl Ou, 2017/04/08 14:48:17]

Done.

+      return;
+    }
+    buf[n] = 0;
+    ring_buffer_.SaveToBuffer(buf);

[Luis Héctor Chávez, 2017/04/07 16:17:46]

nit: you can still avoid the +1 above:

ring_buffer_.SaveToBuffer(std::string(buf, n));

[Earl Ou, 2017/04/08 14:48:17]

Done.

+  }
+
+  void StopTracing(const StopTracingCallback& callback) {
+    DCHECK_CURRENTLY_ON(BrowserThread::IO);
+    // Stop fd_watcher_.

[oystein (OOO til 10th of July), 2017/04/07 18:31:41]

nit: Not really a helpful comment, as it just states what the line below does. 

[Earl Ou, 2017/04/08 14:48:17]

Done.

+    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(",");

[oystein (OOO til 10th of July), 2017/04/07 18:31:41]

nit: maybe:

if (append_comma)
  data.append(",");
else
  append_comma = true;

instead to avoid assigning to append_comma in every loop iteration.

[Earl Ou, 2017/04/08 14:48:17]

Done.

+      data.append(**it);
+      append_comma = true;

[dcheng, 2017/04/07 17:58:28]

Nit: move inside the if at 91

[Earl Ou, 2017/04/08 14:48:17]

Done.

+    }
+    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 +124,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;

[oystein (OOO til 10th of July), 2017/04/07 18:31:40]

nit: (delegate_ != nullptr) for readability.

[Earl Ou, 2017/04/08 14:48:17]

Done.

+
+    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 +141,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 +180,43 @@ class ArcTracingAgentImpl : public ArcTracingAgent {
   // by the Singleton class.
   friend struct base::DefaultSingletonTraits<ArcTracingAgentImpl>;
 
-  ArcTracingAgentImpl() = default;
+  ArcTracingAgentImpl()
+      : reader_(base::MakeUnique<ArcTracingReader>()),
+        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);

[Luis Héctor Chávez, 2017/04/07 16:17:46]

nit: new base::RefCountedString()

(see
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Prefer...
, second note).

[Earl Ou, 2017/04/08 14:48:17]

Done.

+      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_;
+  std::unique_ptr<ArcTracingReader> reader_;

[oystein (OOO til 10th of July), 2017/04/07 18:31:41]

Why is this a unique_ptr rather than just an inlined member, given that the
whole definition of ArcTracingReader is available here?

[Earl Ou, 2017/04/08 14:48:17]

Done.

+  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);
 };