perfetto_cmd: don't delete not empty `write_into_file` trace on error. (#4005)
This bug happens because the `bytes_written_` field was not updated
when the `perfetto_cmd` exits with an error.
To prevent such bugs in the future, we extract the logic to a separate
method that should be called to get the the count of bytes written.
This function is called only at the end, when all the data is written so
there is no need to do something smarter than `fseek`+`ftell`.
diff --git a/src/perfetto_cmd/perfetto_cmd.cc b/src/perfetto_cmd/perfetto_cmd.cc
index 44fdc00..94eef06 100644
--- a/src/perfetto_cmd/perfetto_cmd.cc
+++ b/src/perfetto_cmd/perfetto_cmd.cc
@@ -1190,7 +1190,8 @@
// In case of errors don't leave a partial file around. This happens
// frequently in the case of --save-for-bugreport if there is no eligible
// trace. See also b/279753347 .
- if (bytes_written_ == 0 && !trace_out_path_.empty() &&
+ uint64_t bytes_written = GetBytesWritten();
+ if (bytes_written == 0 && !trace_out_path_.empty() &&
trace_out_path_ != "-") {
remove(trace_out_path_.c_str());
}
@@ -1228,13 +1229,6 @@
LogUploadEvent(PerfettoStatsdAtom::kFinalizeTraceAndExit);
packet_writer_.reset();
- if (trace_out_stream_) {
- fseek(*trace_out_stream_, 0, SEEK_END);
- off_t sz = ftell(*trace_out_stream_);
- if (sz > 0)
- bytes_written_ = static_cast<size_t>(sz);
- }
-
if (save_to_incidentd_) {
#if PERFETTO_BUILDFLAG(PERFETTO_OS_ANDROID)
SaveTraceIntoIncidentOrCrash();
@@ -1244,12 +1238,13 @@
ReportTraceToAndroidFrameworkOrCrash();
#endif
} else {
+ uint64_t bytes_written = GetBytesWritten();
trace_out_stream_.reset();
if (trace_config_->write_into_file()) {
// trace_out_path_ might be empty in the case of --attach.
PERFETTO_LOG("Trace written into the output file");
} else {
- PERFETTO_LOG("Wrote %" PRIu64 " bytes into %s", bytes_written_,
+ PERFETTO_LOG("Wrote %" PRIu64 " bytes into %s", bytes_written,
trace_out_path_ == "-" ? "stdout" : trace_out_path_.c_str());
}
}
@@ -1282,6 +1277,20 @@
return true;
}
+uint64_t PerfettoCmd::GetBytesWritten() {
+ if (!trace_out_stream_)
+ return 0;
+ // Seek to the end of the file in case |trace_out_stream_| points to the file
+ // written by traced.
+ if (fseek(*trace_out_stream_, 0, SEEK_END) < 0) {
+ return 0;
+ }
+ off_t bytes_written = ftell(*trace_out_stream_);
+ if (bytes_written < 0)
+ return 0;
+ return static_cast<uint64_t>(bytes_written);
+}
+
void PerfettoCmd::SetupCtrlCSignalHandler() {
// Only the main thread instance should handle CTRL+C.
if (g_perfetto_cmd != this)
diff --git a/src/perfetto_cmd/perfetto_cmd.h b/src/perfetto_cmd/perfetto_cmd.h
index 6681c59..3d8993f 100644
--- a/src/perfetto_cmd/perfetto_cmd.h
+++ b/src/perfetto_cmd/perfetto_cmd.h
@@ -85,6 +85,7 @@
enum CloneThreadMode { kSingleExtraThread, kNewThreadPerRequest };
bool OpenOutputFile();
+ uint64_t GetBytesWritten();
void SetupCtrlCSignalHandler();
void FinalizeTraceAndExit();
void PrintUsage(const char* argv0);
@@ -187,7 +188,6 @@
bool report_to_android_framework_ = false;
bool statsd_logging_ = false;
bool tracing_succeeded_ = false;
- uint64_t bytes_written_ = 0;
std::string detach_key_;
std::string attach_key_;
bool stop_trace_once_attached_ = false;
diff --git a/src/perfetto_cmd/perfetto_cmd_android.cc b/src/perfetto_cmd/perfetto_cmd_android.cc
index 154ae36..433f0f4 100644
--- a/src/perfetto_cmd/perfetto_cmd_android.cc
+++ b/src/perfetto_cmd/perfetto_cmd_android.cc
@@ -61,7 +61,8 @@
PERFETTO_CHECK(!cfg.destination_package().empty());
PERFETTO_CHECK(!cfg.skip_incidentd());
- if (bytes_written_ == 0) {
+ uint64_t bytes_written = GetBytesWritten();
+ if (bytes_written == 0) {
LogUploadEvent(PerfettoStatsdAtom::kNotUploadingEmptyTrace);
PERFETTO_LOG("Skipping write to incident. Empty trace.");
return;
@@ -73,11 +74,11 @@
// Skip the trace-uuid link for traces that are too small. Realistically those
// traces contain only a marker (e.g. seized_for_bugreport, or the trace
// expired without triggers). Those are useless and introduce only noise.
- if (bytes_written_ > 4096) {
+ if (bytes_written > 4096) {
base::Uuid uuid(uuid_);
PERFETTO_LOG("go/trace-uuid/%s name=\"%s\" size=%" PRIu64,
uuid.ToPrettyString().c_str(),
- trace_config_->unique_session_name().c_str(), bytes_written_);
+ trace_config_->unique_session_name().c_str(), bytes_written);
}
// Ask incidentd to create a report, which will read the file we just
@@ -149,10 +150,11 @@
void PerfettoCmd::ReportTraceToAndroidFrameworkOrCrash() {
PERFETTO_CHECK(trace_out_stream_);
+ uint64_t bytes_written = GetBytesWritten();
int trace_fd = fileno(*trace_out_stream_);
base::Uuid uuid(uuid_);
base::Status status = ReportTraceToAndroidFramework(
- trace_fd, bytes_written_, uuid, trace_config_->unique_session_name(),
+ trace_fd, bytes_written, uuid, trace_config_->unique_session_name(),
trace_config_->android_report_config(), statsd_logging_);
if (!status.ok()) {
PERFETTO_FATAL("ReportTraceToAndroidFramework: %s", status.c_message());
@@ -185,20 +187,21 @@
O_CREAT | O_EXCL | O_RDWR, 0666);
PERFETTO_CHECK(staging_fd);
+ uint64_t bytes_written = GetBytesWritten();
int fd = fileno(*trace_out_stream_);
off_t offset = 0;
- size_t remaining = static_cast<size_t>(bytes_written_);
+ size_t remaining = static_cast<size_t>(bytes_written);
// Count time in terms of CPU to avoid timeouts due to suspend:
base::TimeNanos start = base::GetThreadCPUTimeNs();
for (;;) {
errno = 0;
- PERFETTO_DCHECK(static_cast<size_t>(offset) + remaining == bytes_written_);
+ PERFETTO_DCHECK(static_cast<size_t>(offset) + remaining == bytes_written);
auto wsize = PERFETTO_EINTR(sendfile(*staging_fd, fd, &offset, remaining));
if (wsize < 0) {
PERFETTO_FATAL("sendfile() failed wsize=%zd, off=%" PRId64
", initial=%" PRIu64 ", remaining=%zu",
- wsize, static_cast<int64_t>(offset), bytes_written_,
+ wsize, static_cast<int64_t>(offset), bytes_written,
remaining);
}
remaining -= static_cast<size_t>(wsize);
@@ -210,7 +213,7 @@
PERFETTO_FATAL("sendfile() timed out wsize=%zd, off=%" PRId64
", initial=%" PRIu64
", remaining=%zu, start=%lld, now=%lld",
- wsize, static_cast<int64_t>(offset), bytes_written_,
+ wsize, static_cast<int64_t>(offset), bytes_written,
remaining, static_cast<long long int>(start.count()),
static_cast<long long int>(now.count()));
}
diff --git a/test/cmdline_integrationtest.cc b/test/cmdline_integrationtest.cc
index cce59d2..4d4f49c 100644
--- a/test/cmdline_integrationtest.cc
+++ b/test/cmdline_integrationtest.cc
@@ -1232,6 +1232,67 @@
EXPECT_NE(base::GetFileSize(GetBugreportTracePath()), 0);
}
+#if PERFETTO_BUILDFLAG(PERFETTO_START_DAEMONS)
+TEST_F(PerfettoCmdlineTest, DoNotDeleteNotEmptyWriteIntoFileTraceOnError) {
+ // We call `test_helper().RestartService()` to simulate `traced` dropping
+ // the connection to the `perfetto_cmd`, so we run this test only in
+ // `PERFETTO_START_DAEMONS` mode.
+ // FakeProducer crashes is this case, so we just don't start it. Even without
+ // a data source, the tracing session writes some data on disk, that is enough
+ // for us.
+ TraceConfig trace_config;
+ trace_config.set_unique_session_name("my_write_into_file_session");
+ trace_config.add_buffers()->set_size_kb(1024);
+ trace_config.set_write_into_file(true);
+ trace_config.set_file_write_period_ms(10);
+
+ const std::string write_into_file_path = RandomTraceFileName();
+ ScopedFileRemove remove_on_test_exit(write_into_file_path);
+ auto perfetto_proc = ExecPerfetto(
+ {
+ "-o",
+ write_into_file_path,
+ "-c",
+ "-",
+ },
+ trace_config.SerializeAsString());
+
+ StartServiceIfRequiredNoNewExecsAfterThis();
+
+ std::string perfetto_cmd_stderr;
+ std::thread background_trace([&perfetto_proc, &perfetto_cmd_stderr]() {
+ EXPECT_EQ(0, perfetto_proc.Run(&perfetto_cmd_stderr))
+ << perfetto_cmd_stderr;
+ });
+
+ // Wait until some data is written into the trace file.
+ bool write_into_file_data_ready = false;
+ for (int i = 0; i < 100; i++) {
+ std::this_thread::sleep_for(std::chrono::milliseconds(10));
+ protos::gen::Trace trace;
+ if (ParseNotEmptyTraceFromFile(write_into_file_path, trace)) {
+ write_into_file_data_ready = true;
+ break;
+ }
+ }
+ ASSERT_TRUE(write_into_file_data_ready);
+
+ // Tracing session is still running, now simulate `traced` dropping the
+ // connection to the `perfetto_cmd`
+ test_helper().RestartService();
+
+ background_trace.join();
+ // Assert perfetto_cmd disconnected with an error.
+ EXPECT_THAT(
+ perfetto_cmd_stderr,
+ HasSubstr("Service error: EnableTracing IPC request rejected. This is "
+ "likely due to a loss of the traced connection"));
+ // Assert trace file exists and not empty.
+ protos::gen::Trace trace;
+ EXPECT_TRUE(ParseNotEmptyTraceFromFile(write_into_file_path, trace));
+}
+#endif
+
// Tests that SaveTraceForBugreport() works also if the trace has triggers
// defined and those triggers have not been hit. This is a regression test for
// b/188008375 .
