Stop using raw pointers to manage shared ConverterInterface.

Background:
The converter is now shared across different modules. Engine::GetConverter() returns the raw pointer of Converter. When the Engine is updated/reloaded, all clients that hold the previous Converter will access the dangling pointer. Since it is not possible to detect if any client is holding the previous pointer, Converter has virtually shared ownership, so we prefer to use std::shared_ptr. We can safely update Engine.

Ideally, EngineConverter doesn't want to hold the converter_ but call Engine::GetConverter() on-the-fly, assuming that the actual Converter is dynamically and asynchronously updated. However, such a change may cause another problem because EngineConverter is not designed or at least tested with the situation where the underlying converter is updated every method call or key event.

PiperOrigin-RevId: 730738794

src/converter/converter_main.cc

@@ -469,7 +469,7 @@ bool IsConsistentEngineNameAndType(const std::string &engine_name,
 
 void RunLoop(std::unique_ptr<Engine> engine, commands::Request &&request,
              config::Config &&config) {
-  ConverterInterface *converter = engine->GetConverter();
+  std::shared_ptr<const ConverterInterface> converter = engine->GetConverter();
   CHECK(converter);
 
   Segments segments;

src/converter/converter_regression_test.cc

@@ -54,7 +54,7 @@ class ConverterRegressionTest : public testing::TestWithTempUserProfile {};
 
 TEST_F(ConverterRegressionTest, QueryOfDeathTest) {
   std::unique_ptr<Engine> engine = EngineFactory::Create().value();
-  ConverterInterface *converter = engine->GetConverter();
+  std::shared_ptr<const ConverterInterface> converter = engine->GetConverter();
 
   CHECK(converter);
   {
@@ -81,7 +81,7 @@ TEST_F(ConverterRegressionTest, QueryOfDeathTest) {
 
 TEST_F(ConverterRegressionTest, Regression3323108) {
   std::unique_ptr<Engine> engine = EngineFactory::Create().value();
-  ConverterInterface *converter = engine->GetConverter();
+  std::shared_ptr<const ConverterInterface> converter = engine->GetConverter();
   Segments segments;
 
   EXPECT_TRUE(

src/converter/converter_test.cc

@@ -407,7 +407,7 @@ class ConverterTest : public testing::TestWithTempUserProfile {
 // TODO(toshiyuki): make dictionary mock and test strictly.
 TEST_F(ConverterTest, CanConvertTest) {
   std::unique_ptr<Engine> engine = MockDataEngineFactory::Create().value();
-  ConverterInterface *converter = engine->GetConverter();
+  std::shared_ptr<const ConverterInterface> converter = engine->GetConverter();
   CHECK(converter);
   {
     Segments segments;
@@ -426,7 +426,7 @@ std::string ContextAwareConvert(const std::string &first_key,
                                 const std::string &first_value,
                                 const std::string &second_key) {
   std::unique_ptr<Engine> engine = MockDataEngineFactory::Create().value();
-  ConverterInterface *converter = engine->GetConverter();
+  std::shared_ptr<const ConverterInterface> converter = engine->GetConverter();
   CHECK(converter);
 
   Segments segments;
@@ -494,7 +494,7 @@ TEST_F(ConverterTest, ContextAwareConversionTest) {
 
 TEST_F(ConverterTest, CommitSegmentValue) {
   std::unique_ptr<Engine> engine = MockDataEngineFactory::Create().value();
-  ConverterInterface *converter = engine->GetConverter();
+  std::shared_ptr<const ConverterInterface> converter = engine->GetConverter();
   CHECK(converter);
   Segments segments;
 
@@ -546,7 +546,7 @@ TEST_F(ConverterTest, CommitSegmentValue) {
 
 TEST_F(ConverterTest, CommitSegments) {
   std::unique_ptr<Engine> engine = MockDataEngineFactory::Create().value();
-  ConverterInterface *converter = engine->GetConverter();
+  std::shared_ptr<const ConverterInterface> converter = engine->GetConverter();
   CHECK(converter);
   Segments segments;
 
@@ -610,7 +610,7 @@ TEST_F(ConverterTest, CommitSegments) {
 
 TEST_F(ConverterTest, CommitPartialSuggestionSegmentValue) {
   std::unique_ptr<Engine> engine = MockDataEngineFactory::Create().value();
-  ConverterInterface *converter = engine->GetConverter();
+  std::shared_ptr<const ConverterInterface> converter = engine->GetConverter();
   CHECK(converter);
   Segments segments;
 
@@ -656,7 +656,7 @@ TEST_F(ConverterTest, CommitPartialSuggestionSegmentValue) {
 
 TEST_F(ConverterTest, CandidateKeyTest) {
   std::unique_ptr<Engine> engine = MockDataEngineFactory::Create().value();
-  ConverterInterface *converter = engine->GetConverter();
+  std::shared_ptr<const ConverterInterface> converter = engine->GetConverter();
   CHECK(converter);
   Segments segments;
   EXPECT_TRUE(converter->StartConversion(
@@ -668,7 +668,7 @@ TEST_F(ConverterTest, CandidateKeyTest) {
 
 TEST_F(ConverterTest, Regression3437022) {
   std::unique_ptr<Engine> engine = MockDataEngineFactory::Create().value();
-  ConverterInterface *converter = engine->GetConverter();
+  std::shared_ptr<const ConverterInterface> converter = engine->GetConverter();
   Segments segments;
 
   const std::string kKey1 = "けいたい";
@@ -785,7 +785,7 @@ TEST_F(ConverterTest, CompletePosIds) {
 TEST_F(ConverterTest, Regression3046266) {
   // Shouldn't correct nodes at the beginning of a sentence.
   std::unique_ptr<Engine> engine = MockDataEngineFactory::Create().value();
-  ConverterInterface *converter = engine->GetConverter();
+  std::shared_ptr<const ConverterInterface> converter = engine->GetConverter();
   Segments segments;
 
   // Can be any string that has "ん" at the end
@@ -817,7 +817,7 @@ TEST_F(ConverterTest, Regression3046266) {
 TEST_F(ConverterTest, Regression5502496) {
   // Make sure key correction works for the first word of a sentence.
   std::unique_ptr<Engine> engine = MockDataEngineFactory::Create().value();
-  ConverterInterface *converter = engine->GetConverter();
+  std::shared_ptr<const ConverterInterface> converter = engine->GetConverter();
   Segments segments;
 
   constexpr char kKey[] = "みんあ";
@@ -841,7 +841,7 @@ TEST_F(ConverterTest, StartSuggestion) {
   client_request.set_mixed_conversion(true);
 
   std::unique_ptr<Engine> engine = MockDataEngineFactory::Create().value();
-  ConverterInterface *converter = engine->GetConverter();
+  std::shared_ptr<const ConverterInterface> converter = engine->GetConverter();
   CHECK(converter);
 
   const std::string kShi = "し";
@@ -896,7 +896,7 @@ TEST_F(ConverterTest, StartSuggestion) {
 
 TEST_F(ConverterTest, StartPartialPrediction) {
   std::unique_ptr<Engine> engine = MockDataEngineFactory::Create().value();
-  ConverterInterface *converter = engine->GetConverter();
+  std::shared_ptr<const ConverterInterface> converter = engine->GetConverter();
   CHECK(converter);
   Segments segments;
   EXPECT_TRUE(converter->StartPrediction(
@@ -908,7 +908,7 @@ TEST_F(ConverterTest, StartPartialPrediction) {
 
 TEST_F(ConverterTest, StartPartialSuggestion) {
   std::unique_ptr<Engine> engine = MockDataEngineFactory::Create().value();
-  ConverterInterface *converter = engine->GetConverter();
+  std::shared_ptr<const ConverterInterface> converter = engine->GetConverter();
   CHECK(converter);
   Segments segments;
   EXPECT_TRUE(converter->StartPrediction(
@@ -920,7 +920,7 @@ TEST_F(ConverterTest, StartPartialSuggestion) {
 
 TEST_F(ConverterTest, StartPartialPredictionMobile) {
   std::unique_ptr<Engine> engine = CreateEngineWithMobilePredictor();
-  ConverterInterface *converter = engine->GetConverter();
+  std::shared_ptr<const ConverterInterface> converter = engine->GetConverter();
   CHECK(converter);
   Segments segments;
   EXPECT_TRUE(converter->StartPrediction(
@@ -932,7 +932,7 @@ TEST_F(ConverterTest, StartPartialPredictionMobile) {
 
 TEST_F(ConverterTest, StartPartialSuggestionMobile) {
   std::unique_ptr<Engine> engine = CreateEngineWithMobilePredictor();
-  ConverterInterface *converter = engine->GetConverter();
+  std::shared_ptr<const ConverterInterface> converter = engine->GetConverter();
   CHECK(converter);
   Segments segments;
   EXPECT_TRUE(converter->StartPrediction(
@@ -1108,7 +1108,7 @@ TEST_F(ConverterTest, VariantExpansionForSuggestion) {
 
 TEST_F(ConverterTest, ComposerKeySelection) {
   std::unique_ptr<Engine> engine = MockDataEngineFactory::Create().value();
-  ConverterInterface *converter = engine->GetConverter();
+  std::shared_ptr<const ConverterInterface> converter = engine->GetConverter();
   auto table = std::make_shared<composer::Table>();
   config::Config config;
   {
@@ -1181,7 +1181,7 @@ TEST_F(ConverterTest, SuppressionDictionaryForRewriter) {
 TEST_F(ConverterTest, EmptyConvertReverseIssue8661091) {
   // This is a test case against b/8661091.
   std::unique_ptr<Engine> engine = MockDataEngineFactory::Create().value();
-  ConverterInterface *converter = engine->GetConverter();
+  std::shared_ptr<const ConverterInterface> converter = engine->GetConverter();
 
   Segments segments;
   EXPECT_FALSE(converter->StartReverseConversion(&segments, ""));
@@ -1189,7 +1189,8 @@ TEST_F(ConverterTest, EmptyConvertReverseIssue8661091) {
 
 TEST_F(ConverterTest, StartReverseConversion) {
   std::unique_ptr<Engine> engine = MockDataEngineFactory::Create().value();
-  const ConverterInterface *converter = engine->GetConverter();
+  const std::shared_ptr<const ConverterInterface> converter =
+      engine->GetConverter();
 
   const std::string kHonKanji = "本";
   const std::string kHonHiragana = "ほん";
@@ -1334,7 +1335,7 @@ TEST_F(ConverterTest, StartReverseConversion) {
 
 TEST_F(ConverterTest, ReconstructHistory) {
   std::unique_ptr<Engine> engine = MockDataEngineFactory::Create().value();
-  ConverterInterface *converter = engine->GetConverter();
+  std::shared_ptr<const ConverterInterface> converter = engine->GetConverter();
 
   constexpr char kTen[] = "１０";
 
@@ -1357,7 +1358,7 @@ TEST_F(ConverterTest, ReconstructHistory) {
 
 TEST_F(ConverterTest, LimitCandidatesSize) {
   std::unique_ptr<Engine> engine = MockDataEngineFactory::Create().value();
-  ConverterInterface *converter = engine->GetConverter();
+  std::shared_ptr<const ConverterInterface> converter = engine->GetConverter();
 
   auto table = std::make_shared<composer::Table>();
   const config::Config &config = config::ConfigHandler::DefaultConfig();
@@ -1620,7 +1621,7 @@ TEST_F(ConverterTest, SuggestionOnlyShouldBeIndependentPrediction) {
 
 TEST_F(ConverterTest, RewriterShouldRespectDefaultCandidates) {
   std::unique_ptr<Engine> engine = CreateEngineWithMobilePredictor();
-  ConverterInterface *converter = engine->GetConverter();
+  std::shared_ptr<const ConverterInterface> converter = engine->GetConverter();
   CHECK(converter);
 
   config::Config config;
@@ -1679,7 +1680,7 @@ TEST_F(ConverterTest, RewriterShouldRespectDefaultCandidates) {
 TEST_F(ConverterTest,
        DoNotPromotePrefixOfSingleEntryForEnrichPartialCandidates) {
   std::unique_ptr<Engine> engine = CreateEngineWithMobilePredictor();
-  ConverterInterface *converter = engine->GetConverter();
+  std::shared_ptr<const ConverterInterface> converter = engine->GetConverter();
   CHECK(converter);
 
   commands::Request request;
@@ -1708,7 +1709,7 @@ TEST_F(ConverterTest,
 
 TEST_F(ConverterTest, DoNotAddOverlappingNodesForPrediction) {
   std::unique_ptr<Engine> engine = CreateEngineWithMobilePredictor();
-  ConverterInterface *converter = engine->GetConverter();
+  std::shared_ptr<const ConverterInterface> converter = engine->GetConverter();
   CHECK(converter);
   commands::Request request;
   config::Config config;
@@ -1782,7 +1783,7 @@ TEST_F(ConverterTest, ResizeSegmentWithOffset) {
   constexpr Segment::SegmentType kFree = Segment::FREE;
 
   std::unique_ptr<Engine> engine = MockDataEngineFactory::Create().value();
-  ConverterInterface *converter = engine->GetConverter();
+  std::shared_ptr<const ConverterInterface> converter = engine->GetConverter();
 
   {
     // Resize {"あいうえ"} to {"あいう", "え"}
@@ -1885,7 +1886,7 @@ TEST_F(ConverterTest, ResizeSegmentsWithArray) {
   constexpr Segment::SegmentType kFree = Segment::FREE;
 
   std::unique_ptr<Engine> engine = MockDataEngineFactory::Create().value();
-  ConverterInterface *converter = engine->GetConverter();
+  std::shared_ptr<const ConverterInterface> converter = engine->GetConverter();
 
   {
     // Resize {"あいうえ"} to {"あいう", "え"}
@@ -2121,7 +2122,7 @@ TEST_F(ConverterTest, ResizeSegmentsRequest) {
 
 TEST_F(ConverterTest, IntegrationWithCalculatorRewriter) {
   std::unique_ptr<Engine> engine = MockDataEngineFactory::Create().value();
-  ConverterInterface *converter = engine->GetConverter();
+  std::shared_ptr<const ConverterInterface> converter = engine->GetConverter();
 
   {
     Segments segments;
@@ -2162,7 +2163,7 @@ TEST_F(ConverterTest, IntegrationWithDateRewriter) {
 
 TEST_F(ConverterTest, IntegrationWithSymbolRewriter) {
   std::unique_ptr<Engine> engine = MockDataEngineFactory::Create().value();
-  ConverterInterface *converter = engine->GetConverter();
+  std::shared_ptr<const ConverterInterface> converter = engine->GetConverter();
 
   {
     Segments segments;
@@ -2176,7 +2177,7 @@ TEST_F(ConverterTest, IntegrationWithSymbolRewriter) {
 
 TEST_F(ConverterTest, IntegrationWithUnicodeRewriter) {
   std::unique_ptr<Engine> engine = MockDataEngineFactory::Create().value();
-  ConverterInterface *converter = engine->GetConverter();
+  std::shared_ptr<const ConverterInterface> converter = engine->GetConverter();
 
   {
     Segments segments;
@@ -2190,7 +2191,7 @@ TEST_F(ConverterTest, IntegrationWithUnicodeRewriter) {
 
 TEST_F(ConverterTest, IntegrationWithSmallLetterRewriter) {
   std::unique_ptr<Engine> engine = MockDataEngineFactory::Create().value();
-  ConverterInterface *converter = engine->GetConverter();
+  std::shared_ptr<const ConverterInterface> converter = engine->GetConverter();
 
   {
     Segments segments;

src/converter/quality_regression_util.cc

@@ -185,7 +185,8 @@ absl::Status QualityRegressionUtil::TestItem::ParseFromTSV(
   return absl::OkStatus();
 }
 
-QualityRegressionUtil::QualityRegressionUtil(ConverterInterface *converter)
+QualityRegressionUtil::QualityRegressionUtil(
+    std::shared_ptr<const ConverterInterface> converter)
     : converter_(converter) {}
 
 namespace {

src/converter/quality_regression_util.h

@@ -73,7 +73,8 @@ class QualityRegressionUtil {
     absl::Status ParseFromTSV(const std::string &tsv_line);
   };
 
-  explicit QualityRegressionUtil(ConverterInterface *converter);
+  explicit QualityRegressionUtil(
+      std::shared_ptr<const ConverterInterface> converter);
   QualityRegressionUtil(const QualityRegressionUtil &) = delete;
   QualityRegressionUtil &operator=(const QualityRegressionUtil &) = delete;
   virtual ~QualityRegressionUtil() = default;
@@ -92,7 +93,7 @@ class QualityRegressionUtil {
   static std::string GetPlatformString(uint32_t platform_bitfiled);
 
  private:
-  ConverterInterface *converter_;
+  std::shared_ptr<const ConverterInterface> converter_;
   commands::Request request_;
   config::Config config_;
   Segments segments_;

src/engine/engine.cc

@@ -139,7 +139,7 @@ absl::Status Engine::Init(std::unique_ptr<engine::Modules> modules,
     return std::make_unique<Rewriter>(modules);
   };
 
-  auto converter = std::make_unique<Converter>(
+  auto converter = std::make_shared<Converter>(
       std::move(modules), immutable_converter_factory, predictor_factory,
       rewriter_factory);
 

src/engine/engine.h

@@ -87,15 +87,13 @@ class Engine : public EngineInterface {
   Engine(const Engine &) = delete;
   Engine &operator=(const Engine &) = delete;
 
-  // TODO(taku): Avoid returning pointer, as converter_ may be updated
-  // dynamically and return value will become a dangling pointer.
-  ConverterInterface *GetConverter() const {
-    return converter_ ? converter_.get() : minimal_converter_.get();
+  std::shared_ptr<const ConverterInterface> GetConverter() const {
+    return converter_ ? converter_ : minimal_converter_;
   }
 
   std::unique_ptr<engine::EngineConverterInterface> CreateEngineConverter()
       const override {
-    return std::make_unique<engine::EngineConverter>(*GetConverter());
+    return std::make_unique<engine::EngineConverter>(GetConverter());
   }
 
   // Functions for Reload, Sync, Wait return true if successfully operated
@@ -156,8 +154,8 @@ class Engine : public EngineInterface {
   DataLoader loader_;
 
   std::unique_ptr<engine::SupplementalModelInterface> supplemental_model_;
-  std::unique_ptr<Converter> converter_;
-  std::unique_ptr<ConverterInterface> minimal_converter_;
+  std::shared_ptr<Converter> converter_;
+  std::shared_ptr<ConverterInterface> minimal_converter_;
   std::unique_ptr<DataLoader::Response> loader_response_;
   // Do not initialized with Init() because the cost of initialization is
   // negligible.

src/engine/engine_converter.cc

@@ -106,16 +106,17 @@ int32_t CalculateCursorOffset(absl::string_view committed_text) {
 }
 }  // namespace
 
-EngineConverter::EngineConverter(const ConverterInterface &converter)
-    : EngineConverter(converter, composer::GetSharedDefaultRequest(),
+EngineConverter::EngineConverter(
+    std::shared_ptr<const ConverterInterface> converter)
+    : EngineConverter(std::move(converter), composer::GetSharedDefaultRequest(),
                       config::ConfigHandler::GetSharedDefaultConfig()) {}
 
 EngineConverter::EngineConverter(
-    const ConverterInterface &converter,
+    std::shared_ptr<const ConverterInterface> converter,
     std::shared_ptr<const commands::Request> request,
     std::shared_ptr<const Config> config)
     : EngineConverterInterface(),
-      converter_(&converter),
+      converter_(std::move(converter)),
       segments_(),
       incognito_segments_(),
       segment_index_(0),
@@ -127,6 +128,7 @@ EngineConverter::EngineConverter(
       client_revision_(0),
       candidate_list_visible_(false) {
   DCHECK(request_);
+  DCHECK(converter_);
   DCHECK(config);
   conversion_preferences_.use_history = true;
   conversion_preferences_.request_suggestion = true;
@@ -1148,7 +1150,7 @@ void EngineConverter::FillOutput(const composer::Composer &composer,
 
 EngineConverter *EngineConverter::Clone() const {
   EngineConverter *engine_converter =
-      new EngineConverter(*converter_, request_, config_);
+      new EngineConverter(converter_, request_, config_);
 
   // Copy the members in order of their declarations.
   engine_converter->state_ = state_;

src/engine/engine_converter.h

@@ -56,10 +56,10 @@ namespace engine {
 // support stateful operations related with the converter.
 class EngineConverter : public EngineConverterInterface {
  public:
-  EngineConverter(const ConverterInterface &converter,
+  EngineConverter(std::shared_ptr<const ConverterInterface> converter,
                   std::shared_ptr<const commands::Request> request,
                   std::shared_ptr<const config::Config> config);
-  explicit EngineConverter(const ConverterInterface &converter);
+  explicit EngineConverter(std::shared_ptr<const ConverterInterface> converter);
   EngineConverter(const EngineConverter &) = delete;
   EngineConverter &operator=(const EngineConverter &) = delete;
 
@@ -358,7 +358,8 @@ class EngineConverter : public EngineConverterInterface {
   void SetRequestType(ConversionRequest::RequestType request_type,
                       ConversionRequest::Options &options);
 
-  const ConverterInterface *converter_ = nullptr;
+  std::shared_ptr<const ConverterInterface> converter_;
+
   // Conversion stats used by converter_.
   Segments segments_;
 

src/engine/engine_converter_stress_test.cc

@@ -94,7 +94,7 @@ TEST_F(EngineConverterStressTest, ConvertToHalfWidthForRandomAsciiInput) {
   auto config = std::make_shared<config::Config>();
 
   std::unique_ptr<Engine> engine = MockDataEngineFactory::Create().value();
-  EngineConverter sconverter(*engine->GetConverter(), request, config);
+  EngineConverter sconverter(engine->GetConverter(), request, config);
   auto table = std::make_shared<composer::Table>();
   table->LoadFromFile(kRomajiHiraganaTable.c_str());
   composer::Composer composer(table, request, config);

src/session/ime_context_test.cc

@@ -73,7 +73,7 @@ TEST(ImeContextTest, BasicTest) {
   auto config = std::make_shared<const config::Config>();
   auto keymap = std::make_shared<const keymap::KeyMapManager>();
 
-  MockConverter converter;
+  auto converter = std::make_shared<MockConverter>();
   ImeContext context(std::make_unique<EngineConverter>(converter));
 
   context.set_create_time(absl::FromUnixSeconds(100));
@@ -115,14 +115,14 @@ TEST(ImeContextTest, CopyContext) {
 
   config->set_session_keymap(config::Config::CHROMEOS);
 
-  MockConverter converter;
+  auto converter = std::make_shared<MockConverter>();
 
   Segments segments;
   Segment *segment = segments.add_segment();
   segment->set_key("あん");
   Segment::Candidate *candidate = segment->add_candidate();
   candidate->value = "庵";
-  EXPECT_CALL(converter, StartConversion(_, _))
+  EXPECT_CALL(*converter, StartConversion(_, _))
       .WillOnce(DoAll(SetArgPointee<1>(segments), Return(true)));
 
   {