tracing: get rid of files in TracingController interface

content/browser/tracing/tracing_controller_impl.cc

Index: content/browser/tracing/tracing_controller_impl.cc
diff --git a/content/browser/tracing/tracing_controller_impl.cc b/content/browser/tracing/tracing_controller_impl.cc
index ca480c70638cf829cc292c7cb2a683d7bc5474cb..765ac2bee1b17335626e33f899fdc1c36c0af168 100644
--- a/content/browser/tracing/tracing_controller_impl.cc
+++ b/content/browser/tracing/tracing_controller_impl.cc
@@ -34,133 +34,134 @@ namespace {
 base::LazyInstance<TracingControllerImpl>::Leaky g_controller =
     LAZY_INSTANCE_INITIALIZER;
 
-}  // namespace
-
-TracingController* TracingController::GetInstance() {
-  return TracingControllerImpl::GetInstance();
-}
-
-class TracingControllerImpl::ResultFile {
+class FileTraceDataSink : public TracingController::TraceDataSink {
  public:
-  explicit ResultFile(const base::FilePath& path);
-  void Write(const scoped_refptr<base::RefCountedString>& events_str_ptr) {
+  explicit FileTraceDataSink(
+      const base::FilePath& trace_file_path, const base::Closure& callback)
+      : file_path_(trace_file_path),
+        completion_callback_(callback),
+        file_(NULL) {
+  }
+
+  virtual void AddTraceChunk(const std::string& chunk) OVERRIDE {
+    std::string tmp = chunk;
+    scoped_refptr<base::RefCountedString> chunk_ptr =
+        base::RefCountedString::TakeString(&tmp);
     BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
-        base::Bind(&TracingControllerImpl::ResultFile::WriteTask,
-                   base::Unretained(this), events_str_ptr));
+        base::Bind(&FileTraceDataSink::AddTraceChunkOnFileThread,
+                   base::Unretained(this), chunk_ptr));
   }
-  void Close(const base::Closure& callback) {
+  virtual void SetSystemTrace(const std::string& data) OVERRIDE {
+    system_trace_ = data;
+  }
+  virtual void Close() OVERRIDE {
     BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
-        base::Bind(&TracingControllerImpl::ResultFile::CloseTask,
-                   base::Unretained(this), callback));
-  }
-  void WriteSystemTrace(
-      const scoped_refptr<base::RefCountedString>& events_str_ptr) {
-    BrowserThread::PostTask(
-        BrowserThread::FILE,
-        FROM_HERE,
-        base::Bind(&TracingControllerImpl::ResultFile::WriteSystemTraceTask,
-                   base::Unretained(this), events_str_ptr));
+        base::Bind(&FileTraceDataSink::CloseOnFileThread,
+                   base::Unretained(this)));
   }
 
-  const base::FilePath& path() const { return path_; }
-
  private:
-  void OpenTask();
-  void WriteTask(const scoped_refptr<base::RefCountedString>& events_str_ptr);
-  void WriteSystemTraceTask(
-      const scoped_refptr<base::RefCountedString>& events_str_ptr);
-  void CloseTask(const base::Closure& callback);
+  virtual ~FileTraceDataSink() {}
+
+  void AddTraceChunkOnFileThread(
+      const scoped_refptr<base::RefCountedString> chunk) {
+    if (!OpenFileIfNeededOnFileThread())
+      return;
+    fwrite(chunk->data().c_str(), strlen(chunk->data().c_str()), 1, file_);
+  }
+
+  bool OpenFileIfNeededOnFileThread() {
+    if (file_)
+      return true;
+    file_ = base::OpenFile(file_path_, "w");
+    if (!file_) {
+      LOG(ERROR) << "Failed to open " << file_path_.value();
+      return false;
+    }
+    const char preamble[] = "{\"traceEvents\": [";
+    fwrite(preamble, strlen(preamble), 1, file_);
+    return true;
+  }
+
+  void CloseOnFileThread() {
+    if (OpenFileIfNeededOnFileThread()) {
+      fputc(']', file_);
+      if (!system_trace_.empty()) {
+        const char systemTraceEvents[] = ",\"systemTraceEvents\":[";
+        fwrite(systemTraceEvents, strlen(systemTraceEvents), 1, file_);
+        fwrite(system_trace_.c_str(), strlen(system_trace_.c_str()), 1, file_);
+        fputc(']', file_);
+      }
+      fputc('}', file_);
+      base::CloseFile(file_);
+    }
+    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+        base::Bind(&FileTraceDataSink::FinalizeOnUIThread,
+                   base::Unretained(this)));
+  }
 
-  FILE* file_;
-  base::FilePath path_;
-  bool has_at_least_one_result_;
-  scoped_refptr<base::RefCountedString> system_trace_;
+  void FinalizeOnUIThread() {
+    completion_callback_.Run();
+    delete this;
+  }
 
-  DISALLOW_COPY_AND_ASSIGN(ResultFile);
+  base::FilePath file_path_;
+  base::Closure completion_callback_;
+  FILE* file_;
+  std::string system_trace_;
 };
 
-TracingControllerImpl::ResultFile::ResultFile(const base::FilePath& path)
-    : file_(NULL),
-      path_(path),
-      has_at_least_one_result_(false) {
-  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
-      base::Bind(&TracingControllerImpl::ResultFile::OpenTask,
-                 base::Unretained(this)));
-}
+class StringTraceDataSink : public TracingController::TraceDataSink {
+ public:
+  typedef base::Callback<void(base::RefCountedMemory*)>
+      CompletionCallback;
 
-void TracingControllerImpl::ResultFile::OpenTask() {
-  if (path_.empty())
-    base::CreateTemporaryFile(&path_);
-  file_ = base::OpenFile(path_, "w");
-  if (!file_) {
-    LOG(ERROR) << "Failed to open " << path_.value();
-    return;
+  explicit StringTraceDataSink(CompletionCallback callback)
+      : completion_callback_(callback) {
   }
-  const char* preamble = "{\"traceEvents\": [";
-  size_t written = fwrite(preamble, strlen(preamble), 1, file_);
-  DCHECK(written == 1);
-}
 
-void TracingControllerImpl::ResultFile::WriteTask(
-    const scoped_refptr<base::RefCountedString>& events_str_ptr) {
-  if (!file_ || !events_str_ptr->data().size())
-    return;
-
-  // If there is already a result in the file, then put a comma
-  // before the next batch of results.
-  if (has_at_least_one_result_) {
-    size_t written = fwrite(",", 1, 1, file_);
-    DCHECK(written == 1);
+  // TracingController::TraceDataSink implementation
+  virtual void AddTraceChunk(const std::string& chunk) OVERRIDE {
+    if (!trace_.empty())
+      trace_ += ",";
+    trace_ += chunk;
   }
-  has_at_least_one_result_ = true;
-  size_t written = fwrite(events_str_ptr->data().c_str(),
-                          events_str_ptr->data().size(), 1,
-                          file_);
-  DCHECK(written == 1);
-}
-
-void TracingControllerImpl::ResultFile::WriteSystemTraceTask(
-    const scoped_refptr<base::RefCountedString>& events_str_ptr) {
-  system_trace_ = events_str_ptr;
-}
-
-void TracingControllerImpl::ResultFile::CloseTask(
-    const base::Closure& callback) {
-  if (!file_)
-    return;
-
-  const char* trailevents = "]";
-  size_t written = fwrite(trailevents, strlen(trailevents), 1, file_);
-  DCHECK(written == 1);
+  virtual void SetSystemTrace(const std::string& data) OVERRIDE {
+    system_trace_ = data;
+  }
+  virtual void Close() OVERRIDE {
+    std::string result = "{\"traceEvents\":[" + trace_ + "]";
+    if (!system_trace_.empty())
+      result += ",\"systemTraceEvents\":[" + system_trace_ + "]";
+    result += "}";
 
-  if (system_trace_.get()) {
-#if defined(OS_WIN)
-    // The Windows kernel events are kept into a JSon format stored as string
-    // and must not be escaped.
-    std::string json_string = system_trace_->data();
-#else
-    std::string json_string = base::GetQuotedJSONString(system_trace_->data());
-#endif
+    completion_callback_.Run(base::RefCountedString::TakeString(&result));
+    delete this;

[dsinclair, 2014/09/04 18:03:12]

I'm not a fan of the delete this on the Close() methods. It's going to lead to
leaks (the File sync above does not delete this).

I think the places where a raw TraceDataSink are pass should be changed to
scoped pointers. The Close() methods can go away and we move the contents into
the destructor.

Then, the tracing controller can free the object at the right time and we can
maintain clear ownership of the sink's as they're passed around.

+  }
 
-    const char* systemTraceHead = ",\n\"systemTraceEvents\": ";
-    written = fwrite(systemTraceHead, strlen(systemTraceHead), 1, file_);
-    DCHECK(written == 1);
+ private:
+  virtual ~StringTraceDataSink() {}
 
-    written = fwrite(json_string.data(), json_string.size(), 1, file_);
-    DCHECK(written == 1);
+  std::string trace_;
+  std::string system_trace_;
+  CompletionCallback completion_callback_;
+};
 
-    system_trace_ = NULL;
-  }
+}  // namespace
 
-  const char* trailout = "}";
-  written = fwrite(trailout, strlen(trailout), 1, file_);
-  DCHECK(written == 1);
-  base::CloseFile(file_);
-  file_ = NULL;
+TracingController* TracingController::GetInstance() {
+  return TracingControllerImpl::GetInstance();
+}
 
-  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, callback);
+TracingController::TraceDataSink* TracingControllerImpl::CreateStringSink(
+    const base::Callback<void(base::RefCountedMemory*)>& callback) {
+  return new StringTraceDataSink(callback);
 }
 
+TracingController::TraceDataSink* TracingControllerImpl::CreateFileSink(
+    const base::FilePath& file_path, const base::Closure& callback) {
+  return new FileTraceDataSink(file_path, callback);
+}
 
 TracingControllerImpl::TracingControllerImpl() :
     pending_disable_recording_ack_count_(0),
@@ -173,7 +174,9 @@ TracingControllerImpl::TracingControllerImpl() :
     is_system_tracing_(false),
 #endif
     is_recording_(TraceLog::GetInstance()->IsEnabled()),
-    is_monitoring_(false) {
+    is_monitoring_(false),
+    trace_data_sink_(NULL),
+    monitoring_data_sink_(NULL) {
 }
 
 TracingControllerImpl::~TracingControllerImpl() {
@@ -199,7 +202,7 @@ bool TracingControllerImpl::GetCategories(
     return false;
   }
 
-  bool ok = DisableRecording(base::FilePath(), TracingFileResultCallback());
+  bool ok = DisableRecording(NULL);
   DCHECK(ok);
   return true;
 }
@@ -285,9 +288,7 @@ void TracingControllerImpl::OnEnableRecordingDone(
     callback.Run();
 }
 
-bool TracingControllerImpl::DisableRecording(
-    const base::FilePath& result_file_path,
-    const TracingFileResultCallback& callback) {
+bool TracingControllerImpl::DisableRecording(TraceDataSink* trace_data_sink) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   if (!can_disable_recording())
@@ -298,8 +299,7 @@ bool TracingControllerImpl::DisableRecording(
   // interfering with the process.
   base::Closure on_disable_recording_done_callback =
       base::Bind(&TracingControllerImpl::OnDisableRecordingDone,
-                 base::Unretained(this),
-                 result_file_path, callback);
+                 base::Unretained(this), trace_data_sink);
   BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
       base::Bind(&TracingControllerImpl::SetDisabledOnFileThread,
                  base::Unretained(this),
@@ -308,19 +308,15 @@ bool TracingControllerImpl::DisableRecording(
 }
 
 void TracingControllerImpl::OnDisableRecordingDone(
-    const base::FilePath& result_file_path,
-    const TracingFileResultCallback& callback) {
+    TraceDataSink* trace_data_sink) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  pending_disable_recording_done_callback_ = callback;
-
 #if defined(OS_ANDROID)
   if (pending_get_categories_done_callback_.is_null())
     TraceLog::GetInstance()->AddClockSyncMetadataEvent();
 #endif
 
-  if (!callback.is_null() || !result_file_path.empty())
-    result_file_.reset(new ResultFile(result_file_path));
+  trace_data_sink_ = trace_data_sink;
 
   // Count myself (local trace) in pending_disable_recording_ack_count_,
   // acked below.
@@ -442,7 +438,6 @@ void TracingControllerImpl::OnDisableMonitoringDone(
       it != trace_message_filters_.end(); ++it) {
     it->get()->SendDisableMonitoring();
   }
-
   if (!callback.is_null())
     callback.Run();
 }
@@ -457,18 +452,16 @@ void TracingControllerImpl::GetMonitoringStatus(
 }
 
 bool TracingControllerImpl::CaptureMonitoringSnapshot(
-    const base::FilePath& result_file_path,
-    const TracingFileResultCallback& callback) {
+    TraceDataSink* monitoring_data_sink) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   if (!can_disable_monitoring())
     return false;
 
-  if (callback.is_null() && result_file_path.empty())
+  if (!monitoring_data_sink)
     return false;
 
-  pending_capture_monitoring_snapshot_done_callback_ = callback;
-  monitoring_snapshot_file_.reset(new ResultFile(result_file_path));
+  monitoring_data_sink_ = monitoring_data_sink;
 
   // Count myself in pending_capture_monitoring_snapshot_ack_count_,
   // acked below.
@@ -685,10 +678,6 @@ void TracingControllerImpl::OnDisableRecordingAcked(
   if (pending_disable_recording_ack_count_ != 0)
     return;
 
-  OnDisableRecordingComplete();
-}
-
-void TracingControllerImpl::OnDisableRecordingComplete() {
   // All acks (including from the subprocesses and the local trace) have been
   // received.
   is_recording_ = false;
@@ -697,34 +686,28 @@ void TracingControllerImpl::OnDisableRecordingComplete() {
   if (!pending_get_categories_done_callback_.is_null()) {
     pending_get_categories_done_callback_.Run(known_category_groups_);
     pending_get_categories_done_callback_.Reset();
-  } else if (result_file_) {
-    result_file_->Close(
-        base::Bind(&TracingControllerImpl::OnResultFileClosed,
-                   base::Unretained(this)));
+  } else if (trace_data_sink_) {
+    trace_data_sink_->Close();
+    trace_data_sink_ = 0;
   }
 }
 
-void TracingControllerImpl::OnResultFileClosed() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  if (!result_file_)
-    return;
-
-  if (!pending_disable_recording_done_callback_.is_null()) {
-    pending_disable_recording_done_callback_.Run(result_file_->path());
-    pending_disable_recording_done_callback_.Reset();
-  }
-  result_file_.reset();
-}
-
 #if defined(OS_CHROMEOS) || defined(OS_WIN)
 void TracingControllerImpl::OnEndSystemTracingAcked(
     const scoped_refptr<base::RefCountedString>& events_str_ptr) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  if (result_file_)
-    result_file_->WriteSystemTrace(events_str_ptr);
-
+  if (trace_data_sink_) {
+#if defined(OS_WIN)
+    // The Windows kernel events are kept into a JSon format stored as string
+    // and must not be escaped.
+    std::string json_string = events_str_ptr_->data();
+#else
+    std::string json_string =
+        base::GetQuotedJSONString(events_str_ptr_->data());
+#endif
+    trace_data_sink_->SetSystemTrace(json_string);
+  }
   DCHECK(!is_system_tracing_);
   std::vector<std::string> category_groups;
   OnDisableRecordingAcked(NULL, category_groups);
@@ -763,25 +746,10 @@ void TracingControllerImpl::OnCaptureMonitoringSnapshotAcked(
   if (pending_capture_monitoring_snapshot_ack_count_ != 0)
     return;
 
-  if (monitoring_snapshot_file_) {
-    monitoring_snapshot_file_->Close(
-        base::Bind(&TracingControllerImpl::OnMonitoringSnapshotFileClosed,
-                   base::Unretained(this)));
-  }
-}
-
-void TracingControllerImpl::OnMonitoringSnapshotFileClosed() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  if (!monitoring_snapshot_file_)
-    return;
-
-  if (!pending_capture_monitoring_snapshot_done_callback_.is_null()) {
-    pending_capture_monitoring_snapshot_done_callback_.Run(
-        monitoring_snapshot_file_->path());
-    pending_capture_monitoring_snapshot_done_callback_.Reset();
+  if (monitoring_data_sink_) {
+      monitoring_data_sink_->Close();
+      monitoring_data_sink_ =  NULL;
   }
-  monitoring_snapshot_file_.reset();
 }
 
 void TracingControllerImpl::OnTraceDataCollected(
@@ -795,8 +763,8 @@ void TracingControllerImpl::OnTraceDataCollected(
     return;
   }
 
-  if (result_file_)
-    result_file_->Write(events_str_ptr);
+  if (trace_data_sink_)
+    trace_data_sink_->AddTraceChunk(events_str_ptr->data());
 }
 
 void TracingControllerImpl::OnMonitoringTraceDataCollected(
@@ -808,8 +776,8 @@ void TracingControllerImpl::OnMonitoringTraceDataCollected(
     return;
   }
 
-  if (monitoring_snapshot_file_)
-    monitoring_snapshot_file_->Write(events_str_ptr);
+  if (monitoring_data_sink_)
+    monitoring_data_sink_->AddTraceChunk(events_str_ptr->data());
 }
 
 void TracingControllerImpl::OnLocalTraceDataCollected(