Commit 0f343f65 authored by Joshua LeVasseur's avatar Joshua LeVasseur Committed by Commit Bot

Cast: Fix bug introduced into the crash uploader.

Convert a OnceCallback to a RepeatingCallback.

Bug: b/148234741
Change-Id: I32bcf351f2888925fb5c92b6cea0fd7d04b2f11f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2019505
Commit-Queue: Luke Halliwell (slow) <halliwell@chromium.org>
Reviewed-by: default avatarLuke Halliwell (slow) <halliwell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#735050}
parent 27fd43da
...@@ -94,7 +94,7 @@ MinidumpUploader::MinidumpUploader(CastSysInfo* sys_info, ...@@ -94,7 +94,7 @@ MinidumpUploader::MinidumpUploader(CastSysInfo* sys_info,
: MinidumpUploader(sys_info, : MinidumpUploader(sys_info,
server_url, server_url,
nullptr, nullptr,
base::BindOnce(&CreatePrefService)) {} base::BindRepeating(&CreatePrefService)) {}
MinidumpUploader::~MinidumpUploader() {} MinidumpUploader::~MinidumpUploader() {}
...@@ -126,8 +126,7 @@ bool MinidumpUploader::DoWork() { ...@@ -126,8 +126,7 @@ bool MinidumpUploader::DoWork() {
int num_uploaded = 0; int num_uploaded = 0;
std::unique_ptr<PrefService> pref_service = std::unique_ptr<PrefService> pref_service = pref_service_generator_.Run();
std::move(pref_service_generator_).Run();
const std::string& client_id( const std::string& client_id(
pref_service->GetString(::metrics::prefs::kMetricsClientID)); pref_service->GetString(::metrics::prefs::kMetricsClientID));
std::string virtual_channel(pref_service->GetString(kVirtualChannel)); std::string virtual_channel(pref_service->GetString(kVirtualChannel));
......
...@@ -23,7 +23,7 @@ class CastSysInfo; ...@@ -23,7 +23,7 @@ class CastSysInfo;
class MinidumpUploader : public SynchronizedMinidumpManager { class MinidumpUploader : public SynchronizedMinidumpManager {
public: public:
using PrefServiceGeneratorCallback = using PrefServiceGeneratorCallback =
base::OnceCallback<std::unique_ptr<PrefService>()>; base::RepeatingCallback<std::unique_ptr<PrefService>()>;
// If |server_url| is empty, a default server url will be chosen. // If |server_url| is empty, a default server url will be chosen.
MinidumpUploader(CastSysInfo* sys_info, const std::string& server_url); MinidumpUploader(CastSysInfo* sys_info, const std::string& server_url);
......
...@@ -147,7 +147,7 @@ TEST_F(MinidumpUploaderTest, AvoidsLockingWithoutDumps) { ...@@ -147,7 +147,7 @@ TEST_F(MinidumpUploaderTest, AvoidsLockingWithoutDumps) {
MinidumpUploader* const minidump_uploader_; MinidumpUploader* const minidump_uploader_;
}; };
MinidumpUploader uploader(&sys_info_dummy(), "", &mock_crash_uploader(), MinidumpUploader uploader(&sys_info_dummy(), "", &mock_crash_uploader(),
base::BindOnce(&CreateFakePrefService, true)); base::BindRepeating(&CreateFakePrefService, true));
// Will lock for the first run to initialize file state. // Will lock for the first run to initialize file state.
ASSERT_TRUE(uploader.UploadAllMinidumps()); ASSERT_TRUE(uploader.UploadAllMinidumps());
...@@ -162,7 +162,7 @@ TEST_F(MinidumpUploaderTest, RemovesDumpsWithoutOptIn) { ...@@ -162,7 +162,7 @@ TEST_F(MinidumpUploaderTest, RemovesDumpsWithoutOptIn) {
// Write a dump info entry. // Write a dump info entry.
GenerateDumpWithFiles(minidump_path, logfile_path); GenerateDumpWithFiles(minidump_path, logfile_path);
MinidumpUploader uploader(&sys_info_dummy(), "", &mock_crash_uploader(), MinidumpUploader uploader(&sys_info_dummy(), "", &mock_crash_uploader(),
base::BindOnce(&CreateFakePrefService, false)); base::BindRepeating(&CreateFakePrefService, false));
// MinidumpUploader should not call upon CastCrashdumpUploader. // MinidumpUploader should not call upon CastCrashdumpUploader.
ASSERT_TRUE(uploader.UploadAllMinidumps()); ASSERT_TRUE(uploader.UploadAllMinidumps());
...@@ -184,7 +184,7 @@ TEST_F(MinidumpUploaderTest, SavesDumpInfoWithUploadFailure) { ...@@ -184,7 +184,7 @@ TEST_F(MinidumpUploaderTest, SavesDumpInfoWithUploadFailure) {
std::unique_ptr<DumpInfo> dump( std::unique_ptr<DumpInfo> dump(
GenerateDumpWithFiles(minidump_path, logfile_path)); GenerateDumpWithFiles(minidump_path, logfile_path));
MinidumpUploader uploader(&sys_info_dummy(), "", &mock_crash_uploader(), MinidumpUploader uploader(&sys_info_dummy(), "", &mock_crash_uploader(),
base::BindOnce(&CreateFakePrefService, true)); base::BindRepeating(&CreateFakePrefService, true));
// Induce an upload failure. // Induce an upload failure.
EXPECT_CALL(mock_crash_uploader(), EXPECT_CALL(mock_crash_uploader(),
...@@ -214,8 +214,9 @@ TEST_F(MinidumpUploaderTest, SavesRemainingDumpInfoWithMidwayUploadFailure) { ...@@ -214,8 +214,9 @@ TEST_F(MinidumpUploaderTest, SavesRemainingDumpInfoWithMidwayUploadFailure) {
std::unique_ptr<DumpInfo> dump2( std::unique_ptr<DumpInfo> dump2(
GenerateDumpWithFiles(minidump_path2, logfile_path2)); GenerateDumpWithFiles(minidump_path2, logfile_path2));
{ {
MinidumpUploader uploader(&sys_info_dummy(), "", &mock_crash_uploader(), MinidumpUploader uploader(
base::BindOnce(&CreateFakePrefService, true)); &sys_info_dummy(), "", &mock_crash_uploader(),
base::BindRepeating(&CreateFakePrefService, true));
// First allow a successful upload, then induce failure. // First allow a successful upload, then induce failure.
EXPECT_CALL(mock_crash_uploader(), EXPECT_CALL(mock_crash_uploader(),
...@@ -243,8 +244,9 @@ TEST_F(MinidumpUploaderTest, SavesRemainingDumpInfoWithMidwayUploadFailure) { ...@@ -243,8 +244,9 @@ TEST_F(MinidumpUploaderTest, SavesRemainingDumpInfoWithMidwayUploadFailure) {
ASSERT_TRUE(base::PathExists(logfile_path2)); ASSERT_TRUE(base::PathExists(logfile_path2));
{ {
MinidumpUploader uploader(&sys_info_dummy(), "", &mock_crash_uploader(), MinidumpUploader uploader(
base::BindOnce(&CreateFakePrefService, true)); &sys_info_dummy(), "", &mock_crash_uploader(),
base::BindRepeating(&CreateFakePrefService, true));
// Finally, upload successfully. // Finally, upload successfully.
EXPECT_CALL(mock_crash_uploader(), EXPECT_CALL(mock_crash_uploader(),
...@@ -272,7 +274,7 @@ TEST_F(MinidumpUploaderTest, FailsUploadWithMissingMinidumpFile) { ...@@ -272,7 +274,7 @@ TEST_F(MinidumpUploaderTest, FailsUploadWithMissingMinidumpFile) {
// Write one entry with appropriate files. // Write one entry with appropriate files.
GenerateDumpWithFiles(minidump_path, logfile_path); GenerateDumpWithFiles(minidump_path, logfile_path);
MinidumpUploader uploader(&sys_info_dummy(), "", &mock_crash_uploader(), MinidumpUploader uploader(&sys_info_dummy(), "", &mock_crash_uploader(),
base::BindOnce(&CreateFakePrefService, true)); base::BindRepeating(&CreateFakePrefService, true));
// No CastCrashdumpUploader methods should be called. // No CastCrashdumpUploader methods should be called.
ASSERT_TRUE(base::DeleteFile(minidump_path, false)); ASSERT_TRUE(base::DeleteFile(minidump_path, false));
...@@ -294,7 +296,7 @@ TEST_F(MinidumpUploaderTest, UploadsWithoutMissingLogFile) { ...@@ -294,7 +296,7 @@ TEST_F(MinidumpUploaderTest, UploadsWithoutMissingLogFile) {
// Write one entry with appropriate files. // Write one entry with appropriate files.
GenerateDumpWithFiles(minidump_path, logfile_path); GenerateDumpWithFiles(minidump_path, logfile_path);
MinidumpUploader uploader(&sys_info_dummy(), "", &mock_crash_uploader(), MinidumpUploader uploader(&sys_info_dummy(), "", &mock_crash_uploader(),
base::BindOnce(&CreateFakePrefService, true)); base::BindRepeating(&CreateFakePrefService, true));
// Delete logfile, crash uploader should still work as intended. // Delete logfile, crash uploader should still work as intended.
ASSERT_TRUE(base::DeleteFile(logfile_path, false)); ASSERT_TRUE(base::DeleteFile(logfile_path, false));
...@@ -330,7 +332,7 @@ TEST_F(MinidumpUploaderTest, DeletesLingeringFiles) { ...@@ -330,7 +332,7 @@ TEST_F(MinidumpUploaderTest, DeletesLingeringFiles) {
// Write a real entry. // Write a real entry.
GenerateDumpWithFiles(minidump_path, logfile_path); GenerateDumpWithFiles(minidump_path, logfile_path);
MinidumpUploader uploader(&sys_info_dummy(), "", &mock_crash_uploader(), MinidumpUploader uploader(&sys_info_dummy(), "", &mock_crash_uploader(),
base::BindOnce(&CreateFakePrefService, true)); base::BindRepeating(&CreateFakePrefService, true));
EXPECT_CALL(mock_crash_uploader(), EXPECT_CALL(mock_crash_uploader(),
AddAttachment("log_file", logfile_path.value())) AddAttachment("log_file", logfile_path.value()))
...@@ -355,7 +357,7 @@ TEST_F(MinidumpUploaderTest, SchedulesRebootWhenRatelimited) { ...@@ -355,7 +357,7 @@ TEST_F(MinidumpUploaderTest, SchedulesRebootWhenRatelimited) {
const base::FilePath& logfile_path = minidump_dir_.Append("lmao"); const base::FilePath& logfile_path = minidump_dir_.Append("lmao");
MinidumpUploader uploader(&sys_info_dummy(), "", &mock_crash_uploader(), MinidumpUploader uploader(&sys_info_dummy(), "", &mock_crash_uploader(),
base::BindOnce(&CreateFakePrefService, true)); base::BindRepeating(&CreateFakePrefService, true));
// Generate max dumps. // Generate max dumps.
for (int i = 0; i < SynchronizedMinidumpManager::kRatelimitPeriodMaxDumps + 1; for (int i = 0; i < SynchronizedMinidumpManager::kRatelimitPeriodMaxDumps + 1;
i++) i++)
...@@ -383,7 +385,7 @@ TEST_F(MinidumpUploaderTest, SchedulesRebootWhenRatelimited) { ...@@ -383,7 +385,7 @@ TEST_F(MinidumpUploaderTest, SchedulesRebootWhenRatelimited) {
// Generate one dump for a second pass. // Generate one dump for a second pass.
GenerateDumpWithFiles(minidump_path, logfile_path); GenerateDumpWithFiles(minidump_path, logfile_path);
MinidumpUploader uploader2(&sys_info_dummy(), "", &mock_crash_uploader(), MinidumpUploader uploader2(&sys_info_dummy(), "", &mock_crash_uploader(),
base::BindOnce(&CreateFakePrefService, true)); base::BindRepeating(&CreateFakePrefService, true));
// MinidumpUploader should not call CastCrashdumpUploader (due to ratelimit). // MinidumpUploader should not call CastCrashdumpUploader (due to ratelimit).
// Reboot should NOT be scheduled, as this is second ratelimit. // Reboot should NOT be scheduled, as this is second ratelimit.
...@@ -400,7 +402,7 @@ TEST_F(MinidumpUploaderTest, SchedulesRebootWhenRatelimited) { ...@@ -400,7 +402,7 @@ TEST_F(MinidumpUploaderTest, SchedulesRebootWhenRatelimited) {
TEST_F(MinidumpUploaderTest, UploadInitializesFileState) { TEST_F(MinidumpUploaderTest, UploadInitializesFileState) {
MinidumpUploader uploader(&sys_info_dummy(), "", &mock_crash_uploader(), MinidumpUploader uploader(&sys_info_dummy(), "", &mock_crash_uploader(),
base::BindOnce(&CreateFakePrefService, true)); base::BindRepeating(&CreateFakePrefService, true));
ASSERT_TRUE(base::IsDirectoryEmpty(minidump_dir_)); ASSERT_TRUE(base::IsDirectoryEmpty(minidump_dir_));
ASSERT_TRUE(uploader.UploadAllMinidumps()); ASSERT_TRUE(uploader.UploadAllMinidumps());
base::File lockfile(lockfile_, base::File::FLAG_OPEN | base::File::FLAG_READ); base::File lockfile(lockfile_, base::File::FLAG_OPEN | base::File::FLAG_READ);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment