Skip to content

Commit

Permalink
Revert "Added persistent setting for logtrace killswitch. When kPersi…
Browse files Browse the repository at this point in the history 
…stentSettingLogtraceEnable = false, then all logtrace methods are noop." (#3002)

Reverts #2900
  • Loading branch information
@oxve
oxve committed Apr 18, 2024
2 parents bbf5609 + 5b299c5 commit 46cf41b
Show file tree
Hide file tree
Showing 3 changed files with 15 additions and 148 deletions.
47 changes: 14 additions & 33 deletions cobalt/watchdog/watchdog.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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;

Expand Down Expand Up @@ -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<std::string> 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<base::Value>(enable_logtrace));
}
void Watchdog::ClearLog() { instrumentation_log_.ClearLog(); }

#if defined(_DEBUG)
// Sleeps threads for Watchdog debugging.
Expand Down
27 changes: 0 additions & 27 deletions cobalt/watchdog/watchdog.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -134,8 +112,6 @@ class Watchdog : public Singleton<Watchdog> {
bool LogEvent(const std::string& event);
std::vector<std::string> GetLogTrace();
void ClearLog();
bool GetPersistentSettingLogtraceEnable();
void SetPersistentSettingLogtraceEnable(bool enable_logtrace);

#if defined(_DEBUG)
// Sleeps threads based off of environment variables for Watchdog debugging.
Expand Down Expand Up @@ -205,9 +181,6 @@ class Watchdog : public Singleton<Watchdog> {
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_;
Expand Down
89 changes: 1 addition & 88 deletions cobalt/watchdog/watchdog_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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();
Expand All @@ -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) {
Expand Down Expand Up @@ -87,23 +80,7 @@ class WatchdogTest : public testing::Test {
return violation.Clone();
}

void DeletePersistentSettingsFile() {
std::vector<char> 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) {
Expand Down Expand Up @@ -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<PersistentSettings>(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<base::Value>(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<PersistentSettings>(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<base::Value>(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

0 comments on commit 46cf41b

Please sign in to comment.