arc: enable Android tracing in verified-boot mode

content/browser/tracing/arc_tracing_agent.cc

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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:
   // base::trace_event::TracingAgent overrides:
   std::string GetTracingAgentName() override { return kArcTracingAgentName; }
 
   std::string GetTraceEventLabel() override { return kArcTraceLabel; }
 
   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(
           FROM_HERE, base::Bind(callback, GetTracingAgentName(), false));
       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));
+    DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-    // 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)));
+    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;
   }
 
   static ArcTracingAgentImpl* GetInstance() {
     return base::Singleton<ArcTracingAgentImpl>::get();
   }
 
  private:
   // This allows constructor and destructor to be private and usable only
   // 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);
 };
 
 }  // namespace
 
 // static
 ArcTracingAgent* ArcTracingAgent::GetInstance() {
   return ArcTracingAgentImpl::GetInstance();
 }
 
 ArcTracingAgent::ArcTracingAgent() = default;
 ArcTracingAgent::~ArcTracingAgent() = default;
 
 ArcTracingAgent::Delegate::~Delegate() = default;
 
 }  // namespace content