[crash_reports] Remove url from crash server config
The url portion of the crash server config is uncessary given that we
always use the same url.
Additionally, the CrashServer singleton is no longer conditionally
created and the Queue always receives a valid pointer.
Bug: 62362
TESTED=`fx test crash-reports-tests`
TESTED=`crasher` @ 370c47a2e6b01abd
Change-Id: I88c8f64b4be24c423e1b9c5ddc8fa25ebc3952f3
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/449173
Commit-Queue: Alex Pankhurst <pankhurst@google.com>
Testability-Review: Alex Pankhurst <pankhurst@google.com>
Testability-Review: Francois Rousseau <frousseau@google.com>
Reviewed-by: Francois Rousseau <frousseau@google.com>
diff --git a/src/developer/forensics/crash_reports/config.cc b/src/developer/forensics/crash_reports/config.cc
index a3df4bd..288e474 100644
--- a/src/developer/forensics/crash_reports/config.cc
+++ b/src/developer/forensics/crash_reports/config.cc
@@ -47,9 +47,6 @@
"enabled",
"read_from_privacy_settings"
]
- },
- "url": {
- "type": "string"
}
},
"required": [
@@ -113,25 +110,6 @@
return std::nullopt;
}
- config.url = nullptr;
- if (doc[kCrashServerKey].HasMember(kCrashServerUrlKey)) {
- config.url =
- std::make_unique<std::string>(doc[kCrashServerKey][kCrashServerUrlKey].GetString());
- }
-
- if ((config.upload_policy == CrashServerConfig::UploadPolicy::ENABLED ||
- config.upload_policy == CrashServerConfig::UploadPolicy::READ_FROM_PRIVACY_SETTINGS) &&
- !config.url) {
- FX_LOGS(ERROR) << "missing crash server URL in config with upload not disabled";
- return std::nullopt;
- }
-
- if (config.upload_policy == CrashServerConfig::UploadPolicy::DISABLED && config.url) {
- FX_LOGS(WARNING) << "crash server URL set in config with upload disabled, "
- "ignoring value";
- config.url = nullptr;
- }
-
return config;
}
diff --git a/src/developer/forensics/crash_reports/config.h b/src/developer/forensics/crash_reports/config.h
index dcfbf99..492fc9f 100644
--- a/src/developer/forensics/crash_reports/config.h
+++ b/src/developer/forensics/crash_reports/config.h
@@ -30,11 +30,6 @@
READ_FROM_PRIVACY_SETTINGS,
};
UploadPolicy upload_policy = UploadPolicy::DISABLED;
-
- // URL of the remote crash server.
- //
- // We use a std::unique_ptr to set it only when relevant, i.e. when the policy is not DISABLED.
- std::unique_ptr<std::string> url;
};
// Crash reporter static configuration.
diff --git a/src/developer/forensics/crash_reports/configs/upload_to_prod_server.json b/src/developer/forensics/crash_reports/configs/upload_to_prod_server.json
index 96c8b0b..a2ea2cd 100644
--- a/src/developer/forensics/crash_reports/configs/upload_to_prod_server.json
+++ b/src/developer/forensics/crash_reports/configs/upload_to_prod_server.json
@@ -1,6 +1,5 @@
{
"crash_server" : {
- "upload_policy": "enabled",
- "url": "https://clients2.google.com/cr/report"
+ "upload_policy": "enabled"
}
}
diff --git a/src/developer/forensics/crash_reports/configs/user.json b/src/developer/forensics/crash_reports/configs/user.json
index 5c281a6..05e06c5 100644
--- a/src/developer/forensics/crash_reports/configs/user.json
+++ b/src/developer/forensics/crash_reports/configs/user.json
@@ -3,7 +3,6 @@
"daily_per_product_quota": 100
},
"crash_server" : {
- "upload_policy": "read_from_privacy_settings",
- "url": "https://clients2.google.com/cr/report"
+ "upload_policy": "read_from_privacy_settings"
}
}
diff --git a/src/developer/forensics/crash_reports/configs/userdebug.json b/src/developer/forensics/crash_reports/configs/userdebug.json
index dbdcfa6d..e129584 100644
--- a/src/developer/forensics/crash_reports/configs/userdebug.json
+++ b/src/developer/forensics/crash_reports/configs/userdebug.json
@@ -1,6 +1,5 @@
{
"crash_server" : {
- "upload_policy": "read_from_privacy_settings",
- "url": "https://clients2.google.com/cr/report"
+ "upload_policy": "read_from_privacy_settings"
}
}
diff --git a/src/developer/forensics/crash_reports/constants.h b/src/developer/forensics/crash_reports/constants.h
index a8ce093..c2d00cc 100644
--- a/src/developer/forensics/crash_reports/constants.h
+++ b/src/developer/forensics/crash_reports/constants.h
@@ -15,7 +15,8 @@
constexpr char kCrashServerKey[] = "crash_server";
constexpr char kCrashServerUploadPolicyKey[] = "upload_policy";
-constexpr char kCrashServerUrlKey[] = "url";
+
+constexpr char kCrashServerUrl[] = "https://clients2.google.com/cr/report";
// Snapshots can occupy up to 10 MB of memory.
constexpr StorageSize kSnapshotAnnotationsMaxSize = StorageSize::Megabytes(5);
diff --git a/src/developer/forensics/crash_reports/crash_reporter.cc b/src/developer/forensics/crash_reports/crash_reporter.cc
index 7695e74..efd724d 100644
--- a/src/developer/forensics/crash_reports/crash_reporter.cc
+++ b/src/developer/forensics/crash_reports/crash_reporter.cc
@@ -77,11 +77,8 @@
auto tags = std::make_unique<LogTags>();
- std::unique_ptr<CrashServer> crash_server;
- if (config->crash_server.url) {
- crash_server = std::make_unique<CrashServer>(services, *(config->crash_server.url),
- snapshot_manager.get(), tags.get());
- }
+ auto crash_server =
+ std::make_unique<CrashServer>(services, kCrashServerUrl, snapshot_manager.get(), tags.get());
return std::unique_ptr<CrashReporter>(
new CrashReporter(dispatcher, std::move(services), clock, std::move(info_context),
@@ -115,9 +112,7 @@
FX_CHECK(dispatcher_);
FX_CHECK(services_);
FX_CHECK(crash_register_);
- if (config->crash_server.url) {
- FX_CHECK(crash_server_);
- }
+ FX_CHECK(crash_server_);
const auto& upload_policy = config->crash_server.upload_policy;
settings_.set_upload_policy(upload_policy);
diff --git a/src/developer/forensics/crash_reports/info/inspect_manager.cc b/src/developer/forensics/crash_reports/info/inspect_manager.cc
index 7d9f828..79114de 100644
--- a/src/developer/forensics/crash_reports/info/inspect_manager.cc
+++ b/src/developer/forensics/crash_reports/info/inspect_manager.cc
@@ -139,9 +139,6 @@
crash_server->upload_policy =
server.CreateString(kCrashServerUploadPolicyKey, ToString(config.crash_server.upload_policy));
- if (config.crash_server.url) {
- crash_server->url = server.CreateString(kCrashServerUrlKey, *config.crash_server.url.get());
- }
}
void InspectManager::ExposeSettings(crash_reports::Settings* settings) {
diff --git a/src/developer/forensics/crash_reports/info/inspect_manager.h b/src/developer/forensics/crash_reports/info/inspect_manager.h
index c336905..6ae8210 100644
--- a/src/developer/forensics/crash_reports/info/inspect_manager.h
+++ b/src/developer/forensics/crash_reports/info/inspect_manager.h
@@ -96,7 +96,6 @@
// Inspect node containing the crash server configuration.
struct CrashServerConfig {
inspect::StringProperty upload_policy;
- inspect::StringProperty url;
};
CrashServerConfig crash_server;
diff --git a/src/developer/forensics/crash_reports/queue.cc b/src/developer/forensics/crash_reports/queue.cc
index cfe1d77..67d9a647 100644
--- a/src/developer/forensics/crash_reports/queue.cc
+++ b/src/developer/forensics/crash_reports/queue.cc
@@ -30,6 +30,7 @@
snapshot_manager_(snapshot_manager),
info_(std::move(info_context)) {
FX_CHECK(dispatcher_);
+ FX_CHECK(crash_server_);
upload_all_every_fifteen_minutes_task_.set_handler([this]() { UploadAllEveryFifteenMinutes(); });
diff --git a/src/developer/forensics/crash_reports/tests/config_unittest.cc b/src/developer/forensics/crash_reports/tests/config_unittest.cc
index 35a3dc4..d660f75 100644
--- a/src/developer/forensics/crash_reports/tests/config_unittest.cc
+++ b/src/developer/forensics/crash_reports/tests/config_unittest.cc
@@ -49,41 +49,24 @@
}
})");
EXPECT_EQ(config.crash_server.upload_policy, kDisabled);
- EXPECT_EQ(config.crash_server.url, nullptr);
}
TEST_F(ConfigTest, ParseConfig_ValidConfig_UploadEnabled) {
PARSE_OR_ASSERT(config, R"({
"crash_server" : {
- "upload_policy": "enabled",
- "url": "http://localhost:1234"
+ "upload_policy": "enabled"
}
})");
EXPECT_EQ(config.crash_server.upload_policy, kEnabled);
- EXPECT_EQ(*config.crash_server.url, "http://localhost:1234");
}
TEST_F(ConfigTest, ParseConfig_ValidConfig_UploadReadFromPrivacySettings) {
PARSE_OR_ASSERT(config, R"({
"crash_server" : {
- "upload_policy": "read_from_privacy_settings",
- "url": "http://localhost:1234"
+ "upload_policy": "read_from_privacy_settings"
}
})");
EXPECT_EQ(config.crash_server.upload_policy, kReadFromPrivacySettings);
- EXPECT_EQ(*config.crash_server.url, "http://localhost:1234");
-}
-
-TEST_F(ConfigTest, ParseConfig_ValidConfig_UploadDisabledServerUrlIgnored) {
- PARSE_OR_ASSERT(config, R"({
- "crash_server" : {
- "upload_policy": "disabled",
- "url": "http://localhost:1234"
- }
-})");
- EXPECT_EQ(config.crash_server.upload_policy, kDisabled);
- // Even though a URL is set in the config file, we check that it is not set in the struct.
- EXPECT_EQ(config.crash_server.url, nullptr);
}
TEST_F(ConfigTest, ParseConfig_HasDailyPerProductQuota) {
@@ -92,8 +75,7 @@
"daily_per_product_quota": 100
},
"crash_server" : {
- "upload_policy": "enabled",
- "url": "http://localhost:1234"
+ "upload_policy": "enabled"
}
})");
ASSERT_TRUE(config.daily_per_product_quota.has_value());
@@ -103,8 +85,7 @@
TEST_F(ConfigTest, ParseConfig_MissingDailyPerProductQuota) {
PARSE_OR_ASSERT(config, R"({
"crash_server" : {
- "upload_policy": "enabled",
- "url": "http://localhost:1234"
+ "upload_policy": "enabled"
}
})");
EXPECT_FALSE(config.daily_per_product_quota.has_value());
@@ -128,22 +109,6 @@
})");
}
-TEST_F(ConfigTest, ParseConfig_BadConfig_MissingServerUrlWithUploadEnabled) {
- ASSERT_IS_BAD_CONFIG(R"({
- "crash_server" : {
- "upload_policy": "enabled"
- }
-})");
-}
-
-TEST_F(ConfigTest, ParseConfig_BadConfig_MissingServerUrlWithUploadReadFromPrivacySettings) {
- ASSERT_IS_BAD_CONFIG(R"({
- "crash_server" : {
- "upload_policy": "read_from_privacy_settings"
- }
-})");
-}
-
TEST_F(ConfigTest, ParseConfig_BadConfig_InvalidUploadPolicy) {
ASSERT_IS_BAD_CONFIG(R"({
"crash_server" : {
diff --git a/src/developer/forensics/crash_reports/tests/crash_reporter_unittest.cc b/src/developer/forensics/crash_reports/tests/crash_reporter_unittest.cc
index 2ea573a..38852c5 100644
--- a/src/developer/forensics/crash_reports/tests/crash_reporter_unittest.cc
+++ b/src/developer/forensics/crash_reports/tests/crash_reporter_unittest.cc
@@ -152,16 +152,16 @@
protected:
// Sets up the underlying crash reporter using the given |config| and |crash_server|.
- void SetUpCrashReporter(Config config, std::unique_ptr<StubCrashServer> crash_server) {
+ void SetUpCrashReporter(
+ Config config, const std::vector<CrashServer::UploadStatus>& upload_attempt_results = {}) {
auto snapshot_manager = std::make_unique<SnapshotManager>(
dispatcher(), services(), std::make_unique<timekeeper::TestClock>(), zx::sec(5),
StorageSize::Gigabytes(1u), StorageSize::Gigabytes(1u));
+ auto crash_server = std::make_unique<StubCrashServer>(upload_attempt_results);
config_ = std::move(config);
- FX_CHECK((config_.crash_server.url && crash_server) ||
- (!config_.crash_server.url && !crash_server));
- crash_server_ = crash_server.get();
+ crash_server_ = crash_server.get();
if (crash_server_) {
crash_server_->AddSnapshotManager(snapshot_manager.get());
}
@@ -173,12 +173,6 @@
FX_CHECK(crash_reporter_);
}
- // Sets up the underlying crash reporter using the given |config|.
- void SetUpCrashReporter(Config config) {
- FX_CHECK(!config.crash_server.url);
- return SetUpCrashReporter(std::move(config), /*crash_server=*/nullptr);
- }
-
// Sets up the underlying crash reporter using a default config.
void SetUpCrashReporterDefaultConfig(
const std::vector<CrashServer::UploadStatus>& upload_attempt_results = {}) {
@@ -186,10 +180,9 @@
Config{/*crash_server=*/
{
/*upload_policy=*/CrashServerConfig::UploadPolicy::ENABLED,
- /*url=*/std::make_unique<std::string>(kStubCrashServerUrl),
},
/*daily_per_product_quota=*/kDailyPerProductQuota},
- std::make_unique<StubCrashServer>(upload_attempt_results));
+ upload_attempt_results);
}
void SetUpChannelProviderServer(std::unique_ptr<stubs::ChannelProviderBase> server) {
@@ -482,11 +475,9 @@
Config{/*crash_server=*/
{
/*upload_policy=*/CrashServerConfig::UploadPolicy::ENABLED,
- /*url=*/std::make_unique<std::string>(kStubCrashServerUrl),
},
/*daily_per_product_quota=*/std::nullopt},
- std::make_unique<StubCrashServer>(
- std::vector<CrashServer::UploadStatus>(kDailyPerProductQuota, kUploadSuccessful)));
+ std::vector<CrashServer::UploadStatus>(kDailyPerProductQuota, kUploadSuccessful));
SetUpChannelProviderServer(std::make_unique<stubs::ChannelProvider>(kDefaultChannel));
SetUpDataProviderServer(
std::make_unique<stubs::DataProvider>(kDefaultAnnotations, kDefaultAttachmentBundleKey));
@@ -743,10 +734,9 @@
Config{/*crash_server=*/
{
/*upload_policy=*/CrashServerConfig::UploadPolicy::READ_FROM_PRIVACY_SETTINGS,
- /*url=*/std::make_unique<std::string>(kStubCrashServerUrl),
},
/*daily_per_product_quota=*/kDailyPerProductQuota},
- std::make_unique<StubCrashServer>(std::vector({kUploadSuccessful})));
+ std::vector({kUploadSuccessful}));
SetUpChannelProviderServer(std::make_unique<stubs::ChannelProvider>(kDefaultChannel));
SetUpDataProviderServer(
std::make_unique<stubs::DataProvider>(kDefaultAnnotations, kDefaultAttachmentBundleKey));
@@ -761,14 +751,12 @@
}
TEST_F(CrashReporterTest, Archive_OnUserAlreadyOptedOutDataSharing) {
- SetUpCrashReporter(
- Config{/*crash_server=*/
- {
- /*upload_policy=*/CrashServerConfig::UploadPolicy::READ_FROM_PRIVACY_SETTINGS,
- /*url=*/std::make_unique<std::string>(kStubCrashServerUrl),
- },
- /*daily_per_product_quota=*/kDailyPerProductQuota},
- std::make_unique<StubCrashServer>(std::vector<CrashServer::UploadStatus>({})));
+ SetUpCrashReporter(Config{
+ /*crash_server=*/
+ {
+ /*upload_policy=*/CrashServerConfig::UploadPolicy::READ_FROM_PRIVACY_SETTINGS,
+ },
+ /*daily_per_product_quota=*/kDailyPerProductQuota});
SetUpChannelProviderServer(std::make_unique<stubs::ChannelProvider>(kDefaultChannel));
SetUpDataProviderServer(
std::make_unique<stubs::DataProvider>(kEmptyAnnotations, kDefaultAttachmentBundleKey));
@@ -785,10 +773,9 @@
Config{/*crash_server=*/
{
/*upload_policy=*/CrashServerConfig::UploadPolicy::READ_FROM_PRIVACY_SETTINGS,
- /*url=*/std::make_unique<std::string>(kStubCrashServerUrl),
},
/*daily_per_product_quota=*/kDailyPerProductQuota},
- std::make_unique<StubCrashServer>(std::vector({kUploadSuccessful})));
+ std::vector({kUploadSuccessful}));
SetUpChannelProviderServer(std::make_unique<stubs::ChannelProvider>(kDefaultChannel));
SetUpDataProviderServer(
std::make_unique<stubs::DataProvider>(kDefaultAnnotations, kDefaultAttachmentBundleKey));
@@ -811,10 +798,9 @@
Config{/*crash_server=*/
{
/*upload_policy=*/CrashServerConfig::UploadPolicy::ENABLED,
- /*url=*/std::make_unique<std::string>(kStubCrashServerUrl),
},
/*daily_per_product_quota=*/kDailyPerProductQuota},
- std::make_unique<StubCrashServer>(std::vector({kUploadFailed})));
+ std::vector({kUploadFailed}));
SetUpChannelProviderServer(std::make_unique<stubs::ChannelProvider>(kDefaultChannel));
SetUpDataProviderServer(
std::make_unique<stubs::DataProvider>(kEmptyAnnotations, kEmptyAttachmentBundleKey));
@@ -829,10 +815,9 @@
Config{/*crash_server=*/
{
/*upload_policy=*/CrashServerConfig::UploadPolicy::ENABLED,
- /*url=*/std::make_unique<std::string>(kStubCrashServerUrl),
},
/*daily_per_product_quota=*/kDailyPerProductQuota},
- std::make_unique<StubCrashServer>(std::vector({kUploadThrottled})));
+ std::vector({kUploadThrottled}));
SetUpChannelProviderServer(std::make_unique<stubs::ChannelProvider>(kDefaultChannel));
SetUpDataProviderServer(
std::make_unique<stubs::DataProvider>(kEmptyAnnotations, kEmptyAttachmentBundleKey));
@@ -846,7 +831,6 @@
SetUpCrashReporter(Config{/*crash_server=*/
{
/*upload_policy=*/CrashServerConfig::UploadPolicy::DISABLED,
- /*url=*/nullptr,
},
/*daily_per_product_quota=*/kDailyPerProductQuota});
SetUpChannelProviderServer(std::make_unique<stubs::ChannelProvider>(kDefaultChannel));
@@ -932,14 +916,11 @@
}
TEST_F(CrashReporterTest, Check_CobaltAfterQuotaReached) {
- SetUpCrashReporter(
- Config{/*crash_server=*/
- {
- /*upload_policy=*/CrashServerConfig::UploadPolicy::ENABLED,
- /*url=*/std::make_unique<std::string>(kStubCrashServerUrl),
- },
- /*daily_per_product_quota=*/0u},
- std::make_unique<StubCrashServer>(std::vector<CrashServer::UploadStatus>({})));
+ SetUpCrashReporter(Config{/*crash_server=*/
+ {
+ /*upload_policy=*/CrashServerConfig::UploadPolicy::ENABLED,
+ },
+ /*daily_per_product_quota=*/0u});
SetUpChannelProviderServer(std::make_unique<stubs::ChannelProvider>(kDefaultChannel));
SetUpDataProviderServer(
std::make_unique<stubs::DataProvider>(kEmptyAnnotations, kEmptyAttachmentBundleKey));
diff --git a/src/developer/forensics/crash_reports/tests/inspect_manager_unittest.cc b/src/developer/forensics/crash_reports/tests/inspect_manager_unittest.cc
index 69634d1..604660b 100644
--- a/src/developer/forensics/crash_reports/tests/inspect_manager_unittest.cc
+++ b/src/developer/forensics/crash_reports/tests/inspect_manager_unittest.cc
@@ -307,13 +307,11 @@
}
TEST_F(InspectManagerTest, ExposeConfig_UploadEnabled) {
- inspect_manager_->ExposeConfig(Config{
- /*crash_server=*/
- {
- /*upload_policy=*/kConfigEnabled,
- /*url=*/std::make_unique<std::string>("http://localhost:1234"),
- },
- /*daily_per_product_quota=*/100u});
+ inspect_manager_->ExposeConfig(Config{/*crash_server=*/
+ {
+ /*upload_policy=*/kConfigEnabled,
+ },
+ /*daily_per_product_quota=*/100u});
EXPECT_THAT(InspectTree(),
ChildrenMatch(Contains(
AllOf(NodeMatches(NameMatches("config")),
@@ -321,7 +319,6 @@
NameMatches(kCrashServerKey),
PropertyList(UnorderedElementsAreArray({
StringIs(kCrashServerUploadPolicyKey, ToString(kConfigEnabled)),
- StringIs(kCrashServerUrlKey, "http://localhost:1234"),
}))))))))));
}
@@ -329,7 +326,6 @@
inspect_manager_->ExposeConfig(Config{/*crash_server=*/
{
/*upload_policy=*/kConfigDisabled,
- /*url=*/nullptr,
},
/*per_product_config=*/100});
EXPECT_THAT(InspectTree(),
@@ -345,7 +341,6 @@
inspect_manager_->ExposeConfig(Config{/*crash_server=*/
{
/*upload_policy=*/kConfigReadFromPrivacySettings,
- /*url=*/nullptr,
},
/*per_product_config=*/100u});
EXPECT_THAT(
diff --git a/src/developer/forensics/crash_reports/tests/main_service_unittest.cc b/src/developer/forensics/crash_reports/tests/main_service_unittest.cc
index ee55aa3..a82955c0 100644
--- a/src/developer/forensics/crash_reports/tests/main_service_unittest.cc
+++ b/src/developer/forensics/crash_reports/tests/main_service_unittest.cc
@@ -38,15 +38,12 @@
using testing::UnorderedElementsAre;
using testing::UnorderedElementsAreArray;
-constexpr char kCrashServerUrl[] = "localhost:1234";
-
class MainServiceTest : public UnitTestFixture {
public:
void SetUp() override {
auto config = std::make_unique<Config>();
config->crash_server = CrashServerConfig{
/*upload_policy=*/CrashServerConfig::UploadPolicy::ENABLED,
- /*url=*/std::make_unique<std::string>(kCrashServerUrl),
};
config->daily_per_product_quota = 100u;
@@ -105,7 +102,6 @@
PropertyList(UnorderedElementsAreArray({
StringIs(kCrashServerUploadPolicyKey,
ToString(CrashServerConfig::UploadPolicy::ENABLED)),
- StringIs(kCrashServerUrlKey, kCrashServerUrl),
}))))))),
AllOf(NodeMatches(NameMatches("crash_reporter")),
ChildrenMatch(UnorderedElementsAreArray({
