Merge "Fix Reset() calls from crashing." into sc-dev am: ad02ac98de

Original change: https://googleplex-android-review.googlesource.com/c/platform/external/icing/+/15225909

Change-Id: Ie9232d8cc1c5a2ff6beff8426e539a891b7c7ee8

2 files changed, 22 insertions, 58 deletions

diff --git a/icing/icing-search-engine.cc b/icing/icing-search-engine.cc
index cf998bf..20a6bb9 100644
--- a/icing/icing-search-engine.cc
+++ b/icing/icing-search-engine.cc
@@ -1662,24 +1662,22 @@ ResetResultProto IcingSearchEngine::Reset() {
   ResetResultProto result_proto;
   StatusProto* result_status = result_proto.mutable_status();
 
-  int64_t before_size = filesystem_->GetDiskUsage(options_.base_dir().c_str());
+  absl_ports::unique_lock l(&mutex_);
 
-  if (!filesystem_->DeleteDirectoryRecursively(options_.base_dir().c_str())) {
-    int64_t after_size = filesystem_->GetDiskUsage(options_.base_dir().c_str());
-    if (after_size != before_size) {
-      // Our filesystem doesn't atomically delete. If we have a discrepancy in
-      // size, then that means we may have deleted some files, but not others.
-      // So our data is in an invalid state now.
-      result_status->set_code(StatusProto::INTERNAL);
-      return result_proto;
-    }
+  initialized_ = false;
 
-    result_status->set_code(StatusProto::ABORTED);
+  // Resets members variables
+  schema_store_.reset();
+  document_store_.reset();
+  language_segmenter_.reset();
+  normalizer_.reset();
+  index_.reset();
+
+  if (!filesystem_->DeleteDirectoryRecursively(options_.base_dir().c_str())) {
+    result_status->set_code(StatusProto::INTERNAL);
     return result_proto;
   }
 
-  absl_ports::unique_lock l(&mutex_);
-  initialized_ = false;
   if (InternalInitialize().status().code() != StatusProto::OK) {
     // We shouldn't hit the following Initialize errors:
     //   NOT_FOUND: all data was cleared, we aren't expecting anything
diff --git a/icing/icing-search-engine_test.cc b/icing/icing-search-engine_test.cc
index 5760614..4c15827 100644
--- a/icing/icing-search-engine_test.cc
+++ b/icing/icing-search-engine_test.cc
@@ -5532,11 +5532,11 @@ TEST_F(IcingSearchEngineTest, ResetOk) {
   EXPECT_THAT(icing.SetSchema(empty_schema).status(), ProtoIsOk());
 }
 
-TEST_F(IcingSearchEngineTest, ResetDeleteFailureCausesAbortedError) {
+TEST_F(IcingSearchEngineTest, ResetDeleteFailureCausesInternalError) {
   auto mock_filesystem = std::make_unique<MockFilesystem>();
 
-  // This fails IcingSearchEngine::Reset(). But since we didn't actually delete
-  // anything, we'll be able to consider this just an ABORTED call.
+  // This fails IcingSearchEngine::Reset() with status code INTERNAL and leaves
+  // the IcingSearchEngine instance in an uninitialized state.
   ON_CALL(*mock_filesystem,
           DeleteDirectoryRecursively(StrEq(GetTestBaseDir().c_str())))
       .WillByDefault(Return(false));
@@ -5550,51 +5550,17 @@ TEST_F(IcingSearchEngineTest, ResetDeleteFailureCausesAbortedError) {
 
   DocumentProto document = CreateMessageDocument("namespace", "uri");
   ASSERT_THAT(icing.Put(document).status(), ProtoIsOk());
-  EXPECT_THAT(icing.Reset().status(), ProtoStatusIs(StatusProto::ABORTED));
+  EXPECT_THAT(icing.Reset().status(), ProtoStatusIs(StatusProto::INTERNAL));
 
-  // Everything is still intact.
-  // Can get old data.
   GetResultProto expected_get_result_proto;
-  expected_get_result_proto.mutable_status()->set_code(StatusProto::OK);
+  expected_get_result_proto.mutable_status()->set_code(
+      StatusProto::FAILED_PRECONDITION);
   *expected_get_result_proto.mutable_document() = document;
-  EXPECT_THAT(icing.Get(document.namespace_(), document.uri(),
-                        GetResultSpecProto::default_instance()),
-              EqualsProto(expected_get_result_proto));
-
-  // Can add new data.
-  EXPECT_THAT(icing.Put(CreateMessageDocument("namespace", "uri")).status(),
-              ProtoIsOk());
-}
-
-TEST_F(IcingSearchEngineTest, ResetCreateFailureCausesInternalError) {
-  auto mock_filesystem = std::make_unique<MockFilesystem>();
-
-  // Let all other delete directory calls succeed.
-  EXPECT_CALL(*mock_filesystem,
-              DeleteDirectoryRecursively(Matcher<const char*>(_)))
-      .WillRepeatedly(Return(true));
-
-  // This prevents IcingSearchEngine from deleting our base dir when resetting
-  EXPECT_CALL(*mock_filesystem, DeleteDirectoryRecursively(Matcher<const char*>(
-                                    StrEq(GetTestBaseDir().c_str()))))
-      .WillOnce(Return(false));
-
-  // The first call will show our base directory had 100 bytes, but after we
-  // falied to delete, we lost those 100 bytes. So this will be reported as an
-  // INTERNAL error since data was lost.
-  EXPECT_CALL(
-      *mock_filesystem,
-      GetDiskUsage(Matcher<const char*>(StrEq(GetTestBaseDir().c_str()))))
-      .WillOnce(Return(100))
-      .WillOnce(Return(0));
-
-  TestIcingSearchEngine icing(GetDefaultIcingOptions(),
-                              std::move(mock_filesystem),
-                              std::make_unique<IcingFilesystem>(),
-                              std::make_unique<FakeClock>(), GetTestJniCache());
-  ASSERT_THAT(icing.Initialize().status(), ProtoIsOk());
-  ASSERT_THAT(icing.SetSchema(CreateMessageSchema()).status(), ProtoIsOk());
-  EXPECT_THAT(icing.Reset().status(), ProtoStatusIs(StatusProto::INTERNAL));
+  EXPECT_THAT(icing
+                  .Get(document.namespace_(), document.uri(),
+                       GetResultSpecProto::default_instance())
+                  .status(),
+              ProtoStatusIs(StatusProto::FAILED_PRECONDITION));
 }
 
 TEST_F(IcingSearchEngineTest, SnippetNormalization) {