From 5b299c5ca664fd0301a6868f67b83b74585f06b2 Mon Sep 17 00:00:00 2001 From: Anton Date: Thu, 18 Apr 2024 14:33:24 -0700 Subject: [PATCH] =?UTF-8?q?Revert=20"Added=20persistent=20setting=20for=20?= =?UTF-8?q?logtrace=20killswitch.=20When=20kPersistentSet=E2=80=A6"?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 941a8424d235d407faf4920e4bf614237ff11a98. --- cobalt/watchdog/watchdog.cc | 47 +++++------------ cobalt/watchdog/watchdog.h | 27 ---------- cobalt/watchdog/watchdog_test.cc | 89 +------------------------------- 3 files changed, 15 insertions(+), 148 deletions(-) diff --git a/cobalt/watchdog/watchdog.cc b/cobalt/watchdog/watchdog.cc index 526b92be4034..7c58c6670eb0 100644 --- a/cobalt/watchdog/watchdog.cc +++ b/cobalt/watchdog/watchdog.cc @@ -51,6 +51,19 @@ const int kWatchdogMaxPingInfoLength = 1024; // The maximum number of milliseconds old of an unfetched Watchdog violation. const int64_t kWatchdogMaxViolationsAge = 86400000; +// Persistent setting name and default setting for the boolean that controls +// whether or not Watchdog is enabled. When disabled, Watchdog behaves like a +// stub except that persistent settings can still be get/set. Requires a +// restart to take effect. +const char kPersistentSettingWatchdogEnable[] = + "kPersistentSettingWatchdogEnable"; +const bool kDefaultSettingWatchdogEnable = true; +// Persistent setting name and default setting for the boolean that controls +// whether or not a Watchdog violation will trigger a crash. +const char kPersistentSettingWatchdogCrash[] = + "kPersistentSettingWatchdogCrash"; +const bool kDefaultSettingWatchdogCrash = false; + } // namespace bool Watchdog::Initialize( @@ -65,7 +78,6 @@ bool Watchdog::InitializeCustom( std::string watchdog_file_name, int64_t watchdog_monitor_frequency) { persistent_settings_ = persistent_settings; is_disabled_ = !GetPersistentSettingWatchdogEnable(); - is_logtrace_disabled_ = !GetPersistentSettingLogtraceEnable(); if (is_disabled_) return true; @@ -791,45 +803,14 @@ void Watchdog::SetPersistentSettingWatchdogCrash(bool can_trigger_crash) { } bool Watchdog::LogEvent(const std::string& event) { - if (is_logtrace_disabled_) { - return true; - } - return instrumentation_log_.LogEvent(event); } std::vector Watchdog::GetLogTrace() { - if (is_logtrace_disabled_) { - return {}; - } - return instrumentation_log_.GetLogTrace(); } -void Watchdog::ClearLog() { - if (is_logtrace_disabled_) { - return; - } - - instrumentation_log_.ClearLog(); -} - -bool Watchdog::GetPersistentSettingLogtraceEnable() { - if (!persistent_settings_) return kDefaultSettingLogtraceEnable; - - // Gets the boolean that controls whether or not LogTrace is enabled. - return persistent_settings_->GetPersistentSettingAsBool( - kPersistentSettingLogtraceEnable, kDefaultSettingLogtraceEnable); -} - -void Watchdog::SetPersistentSettingLogtraceEnable(bool enable_logtrace) { - if (!persistent_settings_) return; - - // Sets the boolean that controls whether or not LogTrace is enabled. - persistent_settings_->SetPersistentSetting( - kPersistentSettingLogtraceEnable, - std::make_unique(enable_logtrace)); -} +void Watchdog::ClearLog() { instrumentation_log_.ClearLog(); } #if defined(_DEBUG) // Sleeps threads for Watchdog debugging. diff --git a/cobalt/watchdog/watchdog.h b/cobalt/watchdog/watchdog.h index 412afef74841..f967957f793d 100644 --- a/cobalt/watchdog/watchdog.h +++ b/cobalt/watchdog/watchdog.h @@ -33,28 +33,6 @@ namespace cobalt { namespace watchdog { -// Persistent setting name and default setting for the boolean that controls -// whether or not Watchdog is enabled. When disabled, Watchdog behaves like a -// stub except that persistent settings can still be get/set. Requires a -// restart to take effect. -constexpr char kPersistentSettingWatchdogEnable[] = - "kPersistentSettingWatchdogEnable"; -constexpr bool kDefaultSettingWatchdogEnable = true; - -// Persistent setting name and default setting for the boolean that controls -// whether or not a Watchdog violation will trigger a crash. -constexpr char kPersistentSettingWatchdogCrash[] = - "kPersistentSettingWatchdogCrash"; -constexpr bool kDefaultSettingWatchdogCrash = false; - -// Persistent setting name and default setting for the boolean that controls -// whether or not LogTrace API is enabled. When disabled, all LogTrace methods -// behave like a stub except for persistent settings itself. Requires a -// restart to take effect. -constexpr char kPersistentSettingLogtraceEnable[] = - "kPersistentSettingLogtraceEnable"; -constexpr bool kDefaultSettingLogtraceEnable = true; - // Client to monitor typedef struct Client { std::string name; @@ -134,8 +112,6 @@ class Watchdog : public Singleton { bool LogEvent(const std::string& event); std::vector GetLogTrace(); void ClearLog(); - bool GetPersistentSettingLogtraceEnable(); - void SetPersistentSettingLogtraceEnable(bool enable_logtrace); #if defined(_DEBUG) // Sleeps threads based off of environment variables for Watchdog debugging. @@ -205,9 +181,6 @@ class Watchdog : public Singleton { int64_t watchdog_monitor_frequency_; // Captures string events emitted from Kabuki via logEvent() h5vcc API. InstrumentationLog instrumentation_log_; - // Flag to disable LogTrace API. When disabled, all LogTrace methods behave - // like a stub except that the flag itself can still be get/set. - bool is_logtrace_disabled_; #if defined(_DEBUG) starboard::Mutex delay_mutex_; diff --git a/cobalt/watchdog/watchdog_test.cc b/cobalt/watchdog/watchdog_test.cc index 05f5fc030f50..024be9ee4dd6 100644 --- a/cobalt/watchdog/watchdog_test.cc +++ b/cobalt/watchdog/watchdog_test.cc @@ -20,15 +20,12 @@ #include "base/json/json_reader.h" #include "base/json/json_writer.h" -#include "base/test/scoped_task_environment.h" #include "starboard/common/file.h" #include "testing/gtest/include/gtest/gtest.h" namespace cobalt { namespace watchdog { -using persistent_storage::PersistentSettings; - namespace { const char kWatchdogViolationsJson[] = "watchdog_test.json"; @@ -39,9 +36,7 @@ const int64_t kWatchdogSleepDuration = kWatchdogMonitorFrequency * 4; class WatchdogTest : public testing::Test { protected: - WatchdogTest() - : scoped_task_environment_( - base::test::ScopedTaskEnvironment::MainThreadType::DEFAULT) {} + WatchdogTest() {} void SetUp() final { watchdog_ = new watchdog::Watchdog(); @@ -54,8 +49,6 @@ class WatchdogTest : public testing::Test { watchdog_->Uninitialize(); delete watchdog_; watchdog_ = nullptr; - - DeletePersistentSettingsFile(); } base::Value CreateDummyViolationDict(std::string desc, int begin, int end) { @@ -87,23 +80,7 @@ class WatchdogTest : public testing::Test { return violation.Clone(); } - void DeletePersistentSettingsFile() { - std::vector storage_dir(kSbFileMaxPath + 1, 0); - SbSystemGetPath(kSbSystemPathCacheDirectory, storage_dir.data(), - kSbFileMaxPath); - std::string path = - std::string(storage_dir.data()) + kSbFileSepString + kSettingsFileName; - - starboard::SbFileDeleteRecursive(path.c_str(), true); - } - - const std::string kSettingsFileName = "test-settings.json"; - watchdog::Watchdog* watchdog_; - base::test::ScopedTaskEnvironment scoped_task_environment_; - base::WaitableEvent task_done_ = { - base::WaitableEvent::ResetPolicy::MANUAL, - base::WaitableEvent::InitialState::NOT_SIGNALED}; }; TEST_F(WatchdogTest, RedundantRegistersShouldFail) { @@ -740,69 +717,5 @@ TEST_F(WatchdogTest, ViolationContainsEmptyLogTrace) { ASSERT_EQ(logTrace->size(), 0); } -TEST_F(WatchdogTest, WatchdogMethodsAreNoopWhenWatchdogIsDisabled) { - // init and destroy existing watchdog to re-initialize it later - watchdog_->Register("test-name", "test-desc", base::kApplicationStateStarted, - kWatchdogMonitorFrequency); - TearDown(); - - // PersistentSettings doesn't have interface so it's not mockable - auto persistent_settings = - std::make_unique(kSettingsFileName); - persistent_settings->ValidatePersistentSettings(); - - base::OnceClosure closure = base::BindOnce( - [](base::WaitableEvent* task_done) { task_done->Signal(); }, &task_done_); - persistent_settings->SetPersistentSetting( - kPersistentSettingWatchdogEnable, std::make_unique(false), - std::move(closure), true); - task_done_.Wait(); - - watchdog_ = new watchdog::Watchdog(); - watchdog_->InitializeCustom(persistent_settings.get(), - std::string(kWatchdogViolationsJson), - kWatchdogMonitorFrequency); - - ASSERT_TRUE(watchdog_->Register("test-name", "test-desc", - base::kApplicationStateStarted, - kWatchdogMonitorFrequency)); - ASSERT_TRUE(watchdog_->Ping("test-name")); - ASSERT_TRUE(watchdog_->PingByClient(nullptr)); - - SbThreadSleep(kWatchdogSleepDuration); - - ASSERT_EQ(watchdog_->GetWatchdogViolations(), ""); - ASSERT_TRUE(watchdog_->Unregister("test-name")); - ASSERT_TRUE(watchdog_->Unregister("")); -} - -TEST_F(WatchdogTest, LogtraceMethodsAreNoopWhenLogtraceIsDisabled) { - // init and destroy existing watchdog to re-initialize it later - watchdog_->Register("test-name", "test-desc", base::kApplicationStateStarted, - kWatchdogMonitorFrequency); - TearDown(); - - // PersistentSettings doesn't have interface so it's not mockable - auto persistent_settings = - std::make_unique(kSettingsFileName); - persistent_settings->ValidatePersistentSettings(); - - base::OnceClosure closure = base::BindOnce( - [](base::WaitableEvent* task_done) { task_done->Signal(); }, &task_done_); - persistent_settings->SetPersistentSetting( - kPersistentSettingLogtraceEnable, std::make_unique(false), - std::move(closure), true); - task_done_.Wait(); - - watchdog_ = new watchdog::Watchdog(); - watchdog_->InitializeCustom(persistent_settings.get(), - std::string(kWatchdogViolationsJson), - kWatchdogMonitorFrequency); - - ASSERT_TRUE(watchdog_->LogEvent("foo")); - ASSERT_EQ(watchdog_->GetLogTrace().size(), 0); - ASSERT_NO_FATAL_FAILURE(watchdog_->ClearLog()); -} - } // namespace watchdog } // namespace cobalt