update_engine: Make SystemState pointer static only
The style guide does not allow a global object with non-trivial dtor. It
can cause hidden problems and it has caused issue this CL is
fixing (look at the attached bug). Instead of keeping the ownership of
the SystemState in global, we can keep the ownership in the high level
object DaemonChromeOS and keep a global static pointer to it so it can
easily be accessed by SystemState::Get().
BUG=b:174212887
TEST=cros_workon_make --board reef --test update_engine
TEST=cros deploy + stop update-engine -> The update_engine did not crash anymore.
Change-Id: I442f4220bfd8586c59fcdfd7d699776362143467
Reviewed-on: https://chromium-review.googlesource.com/c/aosp/platform/system/update_engine/+/2566875
Tested-by: Amin Hassani <ahassani@chromium.org>
Reviewed-by: Jae Hoon Kim <kimjae@chromium.org>
Commit-Queue: Amin Hassani <ahassani@chromium.org>
diff --git a/cros/daemon_chromeos.cc b/cros/daemon_chromeos.cc
index 5fa24ea..366fb9a 100644
--- a/cros/daemon_chromeos.cc
+++ b/cros/daemon_chromeos.cc
@@ -45,7 +45,7 @@
// TODO(deymo): Move the initialization to a factory method avoiding the
// explicit re-usage of the |bus| instance, shared between D-Bus service and
// D-Bus client calls.
- RealSystemState::CreateInstance();
+ RealSystemState::SetInstance(&system_state_);
// Create the DBus service.
dbus_adaptor_.reset(new UpdateEngineAdaptor());
diff --git a/cros/daemon_chromeos.h b/cros/daemon_chromeos.h
index ab9d4b2..b23c2a6 100644
--- a/cros/daemon_chromeos.h
+++ b/cros/daemon_chromeos.h
@@ -23,6 +23,7 @@
#include "update_engine/common/daemon_state_interface.h"
#include "update_engine/common/subprocess.h"
#include "update_engine/cros/dbus_service.h"
+#include "update_engine/cros/real_system_state.h"
namespace chromeos_update_engine {
@@ -39,6 +40,13 @@
// initialization.
void OnDBusRegistered(bool succeeded);
+ // |SystemState| is a global context, but we can't have a static singleton of
+ // its object because the style guide does not allow that (it has non-trivial
+ // dtor). We need an instance of |SystemState| in this class instead and have
+ // a global pointer to it. This is better to be defined as the first variable
+ // of this class so it is initialized first and destructed last.
+ RealSystemState system_state_;
+
// Main D-Bus service adaptor.
std::unique_ptr<UpdateEngineAdaptor> dbus_adaptor_;
diff --git a/cros/fake_system_state.h b/cros/fake_system_state.h
index 7eb830c..da36306 100644
--- a/cros/fake_system_state.h
+++ b/cros/fake_system_state.h
@@ -17,6 +17,8 @@
#ifndef UPDATE_ENGINE_CROS_FAKE_SYSTEM_STATE_H_
#define UPDATE_ENGINE_CROS_FAKE_SYSTEM_STATE_H_
+#include <memory>
+
#include <base/logging.h>
#include <gmock/gmock.h>
#include <policy/mock_device_policy.h>
@@ -43,10 +45,14 @@
// OOBE is completed even when there's no such marker file, etc.
class FakeSystemState : public SystemState {
public:
- static void CreateInstance() { g_instance_.reset(new FakeSystemState()); }
+ static void CreateInstance() {
+ static std::unique_ptr<FakeSystemState> system_state;
+ system_state.reset(new FakeSystemState());
+ g_pointer_ = system_state.get();
+ }
static FakeSystemState* Get() {
- return reinterpret_cast<FakeSystemState*>(g_instance_.get());
+ return reinterpret_cast<FakeSystemState*>(g_pointer_);
}
// Base class overrides. All getters return the current implementation of
diff --git a/cros/real_system_state.h b/cros/real_system_state.h
index 348c31b..81a5e0e 100644
--- a/cros/real_system_state.h
+++ b/cros/real_system_state.h
@@ -47,13 +47,16 @@
// used by the actual product code.
class RealSystemState : public SystemState {
public:
+ // Constructs all system objects that do not require separate initialization;
+ // see Initialize() below for the remaining ones.
+ RealSystemState() = default;
~RealSystemState() = default;
- static void CreateInstance() {
- CHECK(!g_instance_) << "SystemState has been previously created.";
- RealSystemState* rss = new RealSystemState();
- g_instance_.reset(rss);
- LOG_IF(FATAL, !rss->Initialize()) << "Failed to initialize system state.";
+ static void SetInstance(RealSystemState* system_state) {
+ CHECK(g_pointer_ == nullptr) << "SystemState has been previously set.";
+ g_pointer_ = system_state;
+ LOG_IF(FATAL, !system_state->Initialize())
+ << "Failed to initialize system state.";
}
// SystemState overrides.
@@ -108,10 +111,6 @@
DlcServiceInterface* dlcservice() override { return dlcservice_.get(); }
private:
- // Constructs all system objects that do not require separate initialization;
- // see Initialize() below for the remaining ones.
- RealSystemState() = default;
-
// Initializes and sets systems objects that require an initialization
// separately from construction. Returns |true| on success.
bool Initialize();