Avoid manual lifetime management of std::string

This change touches several places in `Source` and `Storage` where
string was being passed as a pointer, with expectations for `delete` to
be called once done with the value. There may even be a case in Chromium
at the moment where one of these calls may be leaking memory due to how
the memory is expected to managed.

This change passes the string by value, and in cases where the value may
be omitted the string is wrapped on a `std::optional`.

Bug: 423542167

Author: cdesouza-chromium

cpp/include/libaddressinput/null_storage.h

@@ -35,7 +35,7 @@ class NullStorage : public Storage {
   ~NullStorage() override;
 
   // No-op.
-  void Put(const std::string& key, std::string* data) override;
+  void Put(const std::string& key, std::string data) override;
 
   // Always calls the |data_ready| callback function signaling failure.
   void Get(const std::string& key, const Callback& data_ready) const override;

cpp/include/libaddressinput/source.h

@@ -21,6 +21,7 @@
 
 #include <libaddressinput/callback.h>
 
+#include <optional>
 #include <string>
 
 namespace i18n {
@@ -41,7 +42,7 @@ namespace addressinput {
 class Source {
  public:
   using Callback =
-      i18n::addressinput::Callback<const std::string&, std::string*>;
+      i18n::addressinput::Callback<const std::string&, std::optional<std::string>>;
 
   virtual ~Source() = default;
 

cpp/include/libaddressinput/storage.h

@@ -21,6 +21,7 @@
 #include <libaddressinput/callback.h>
 
 #include <string>
+#include <optional>
 
 namespace i18n {
 namespace addressinput {
@@ -45,13 +46,13 @@ namespace addressinput {
 class Storage {
  public:
   using Callback =
-      i18n::addressinput::Callback<const std::string&, std::string*>;
+      i18n::addressinput::Callback<const std::string&, std::optional<std::string>>;
 
   virtual ~Storage() = default;
 
   // Stores |data| for |key|, where |data| is an object allocated on the heap,
   // which Storage takes ownership of.
-  virtual void Put(const std::string& key, std::string* data) = 0;
+  virtual void Put(const std::string& key, std::string data) = 0;
 
   // Retrieves the data for |key| and invokes the |data_ready| callback.
   virtual void Get(const std::string& key,

cpp/src/null_storage.cc

@@ -24,14 +24,11 @@ namespace addressinput {
 NullStorage::NullStorage() = default;
 NullStorage::~NullStorage() = default;
 
-void NullStorage::Put(const std::string& key, std::string* data) {
-  assert(data != nullptr);  // Sanity check.
-  delete data;
-}
+void NullStorage::Put(const std::string& key, std::string data) {}
 
 void NullStorage::Get(const std::string& key,
                       const Callback& data_ready) const {
-  data_ready(false, key, nullptr);
+  data_ready(false, key, std::nullopt);
 }
 
 }  // namespace addressinput

cpp/src/retriever.cc

@@ -56,38 +56,35 @@ class Helper {
 
   void OnValidatedDataReady(bool success,
                             const std::string& key,
-                            std::string* data) {
+                            std::optional<std::string> data) {
     if (success) {
-      assert(data != nullptr);
+      assert(data != std::nullopt);
       retrieved_(success, key, *data);
       delete this;
     } else {
       // Validating storage returns (false, key, stale-data) for valid but stale
       // data. If |data| is empty, however, then it's either missing or invalid.
-      if (data != nullptr && !data->empty()) {
-        stale_data_ = *data;
+      if (data.has_value() && !data->empty()) {
+        stale_data_ = std::move(data).value();
       }
       source_.Get(key, *fresh_data_ready_);
     }
-    delete data;
   }
 
   void OnFreshDataReady(bool success,
                         const std::string& key,
-                        std::string* data) {
+                        std::optional<std::string> data) {
     if (success) {
-      assert(data != nullptr);
+      assert(data.has_value());
       retrieved_(true, key, *data);
-      storage_->Put(key, data);
-      data = nullptr;  // Deleted by Storage::Put().
+      storage_->Put(key, std::move(data).value());
     } else if (!stale_data_.empty()) {
       // Reuse the stale data if a download fails. It's better to have slightly
       // outdated validation rules than to suddenly lose validation ability.
       retrieved_(true, key, stale_data_);
     } else {
       retrieved_(false, key, std::string());
     }
-    delete data;
     delete this;
   }
 

cpp/src/validating_storage.cc

@@ -51,20 +51,18 @@ class Helper {
 
   void OnWrappedDataReady(bool success,
                           const std::string& key,
-                          std::string* data) {
+                          std::optional<std::string> data) {
     if (success) {
-      assert(data != nullptr);
+      assert(data != std::nullopt);
       bool is_stale =
-          !ValidatingUtil::UnwrapTimestamp(data, std::time(nullptr));
-      bool is_corrupted = !ValidatingUtil::UnwrapChecksum(data);
+          !ValidatingUtil::UnwrapTimestamp(&*data, std::time(nullptr));
+      bool is_corrupted = !ValidatingUtil::UnwrapChecksum(&*data);
       success = !is_corrupted && !is_stale;
       if (is_corrupted) {
-        delete data;
-        data = nullptr;
+        data = std::nullopt;
       }
     } else {
-      delete data;
-      data = nullptr;
+      data = std::nullopt;
     }
     data_ready_(success, key, data);
     delete this;
@@ -83,10 +81,9 @@ ValidatingStorage::ValidatingStorage(Storage* storage)
 
 ValidatingStorage::~ValidatingStorage() = default;
 
-void ValidatingStorage::Put(const std::string& key, std::string* data) {
-  assert(data != nullptr);
-  ValidatingUtil::Wrap(std::time(nullptr), data);
-  wrapped_storage_->Put(key, data);
+void ValidatingStorage::Put(const std::string& key, std::string data) {
+  ValidatingUtil::Wrap(std::time(nullptr), &data);
+  wrapped_storage_->Put(key, std::move(data));
 }
 
 void ValidatingStorage::Get(const std::string& key,

cpp/src/validating_storage.h

@@ -44,7 +44,7 @@ class ValidatingStorage : public Storage {
   ~ValidatingStorage() override;
 
   // Storage implementation.
-  void Put(const std::string& key, std::string* data) override;
+  void Put(const std::string& key, std::string data) override;
 
   // Storage implementation.
   // If the data is invalid, then |data_ready| will be called with (false, key,

cpp/test/fake_storage.cc

@@ -23,19 +23,13 @@ namespace addressinput {
 
 FakeStorage::FakeStorage() = default;
 
-FakeStorage::~FakeStorage() {
-  for (const auto& pair : data_) {
-    delete pair.second;
-  }
-}
+FakeStorage::~FakeStorage() = default;
 
-void FakeStorage::Put(const std::string& key, std::string* data) {
-  assert(data != nullptr);
-  auto result = data_.emplace(key, data);
+void FakeStorage::Put(const std::string& key, std::string data) {
+  auto result = data_.emplace(key, std::move(data));
   if (!result.second) {
     // Replace data in existing entry for this key.
-    delete result.first->second;
-    result.first->second = data;
+    result.first->second = std::move(data);
   }
 }
 
@@ -44,7 +38,7 @@ void FakeStorage::Get(const std::string& key,
   auto data_it = data_.find(key);
   bool success = data_it != data_.end();
   data_ready(success, key,
-             success ? new std::string(*data_it->second) : nullptr);
+             success ? std::make_optional<std::string>(data_it->second) : std::nullopt);
 }
 
 }  // namespace addressinput

cpp/test/fake_storage.h

@@ -65,11 +65,11 @@ class FakeStorage : public Storage {
   ~FakeStorage() override;
 
   // Storage implementation.
-  void Put(const std::string& key, std::string* data) override;
+  void Put(const std::string& key, std::string data) override;
   void Get(const std::string& key, const Callback& data_ready) const override;
 
  private:
-  std::map<std::string, std::string*> data_;
+  std::map<std::string, std::string> data_;
 };
 
 }  // namespace addressinput

cpp/test/fake_storage_test.cc

@@ -50,13 +50,12 @@ class FakeStorageTest : public testing::Test {
   const std::unique_ptr<const Storage::Callback> data_ready_;
 
  private:
-  void OnDataReady(bool success, const std::string& key, std::string* data) {
-    ASSERT_FALSE(success && data == nullptr);
+  void OnDataReady(bool success, const std::string& key, std::optional<std::string> data) {
+    ASSERT_FALSE(success && !data.has_value());
     success_ = success;
     key_ = key;
-    if (data != nullptr) {
-      data_ = *data;
-      delete data;
+    if (data.has_value()) {
+      data_ = std::move(data).value();
     }
   }
 };
@@ -70,7 +69,7 @@ TEST_F(FakeStorageTest, GetWithoutPutReturnsEmptyData) {
 }
 
 TEST_F(FakeStorageTest, GetReturnsWhatWasPut) {
-  storage_.Put("key", new std::string("value"));
+  storage_.Put("key", std::string("value"));
   storage_.Get("key", *data_ready_);
 
   EXPECT_TRUE(success_);
@@ -79,8 +78,8 @@ TEST_F(FakeStorageTest, GetReturnsWhatWasPut) {
 }
 
 TEST_F(FakeStorageTest, SecondPutOverwritesData) {
-  storage_.Put("key", new std::string("bad-value"));
-  storage_.Put("key", new std::string("good-value"));
+  storage_.Put("key", std::string("bad-value"));
+  storage_.Put("key", std::string("good-value"));
   storage_.Get("key", *data_ready_);
 
   EXPECT_TRUE(success_);