Fix closing of handles in ChromePromptChannelProtobuf

Bug: 969139
Change-Id: I9bb7df38642244f3a0612d885d73caedc51a382b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1731293
Commit-Queue: Oliver Li <olivierli@chromium.org>
Reviewed-by: Joe Mason <joenotcharles@google.com>
Cr-Commit-Position: refs/heads/master@{#683262}

chrome/browser/safe_browsing/chrome_cleaner/chrome_prompt_channel_win.cc

@@ -592,6 +592,8 @@ void ChromePromptChannelProtobuf::CloseHandles() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   // This will cause the next ::ReadFile call in ServiceChromePromptRequests to
   // fail, triggering the error handler that kills the cleaner process.
+  ::CancelIoEx(request_read_handle_.Get(), nullptr);
+  ::CancelIoEx(response_write_handle_.Get(), nullptr);
   request_read_handle_.Close();
   response_write_handle_.Close();
 }
@@ -613,6 +615,8 @@ void ChromePromptChannelProtobuf::HandlePromptUserRequest(
   // an error, could just be a more recent cleaner version.)
   if (!request.unknown_fields().empty()) {
     LOG(ERROR) << "Discarding PromptUserRequest with unknown fields.";
+    WriteStatusErrorCodeToHistogram(ErrorCategory::kCustomError,
+                                    CustomErrors::kRequestUnknownField);
     return;
   }
 
@@ -623,6 +627,8 @@ void ChromePromptChannelProtobuf::HandlePromptUserRequest(
     if (!base::UTF8ToUTF16(file_path.c_str(), file_path.size(),
                            &file_path_utf16)) {
       LOG(ERROR) << "Undisplayable file path in PromptUserRequest.";
+      WriteStatusErrorCodeToHistogram(ErrorCategory::kCustomError,
+                                      CustomErrors::kUndisplayableFilePath);
       return;
     }
     files_to_delete.push_back(base::FilePath(file_path_utf16));
@@ -637,6 +643,9 @@ void ChromePromptChannelProtobuf::HandlePromptUserRequest(
       if (!base::UTF8ToUTF16(registry_key.c_str(), registry_key.size(),
                              &registry_key_utf16)) {
         LOG(ERROR) << "Undisplayable registry key in PromptUserRequest.";
+        WriteStatusErrorCodeToHistogram(
+            ErrorCategory::kCustomError,
+            CustomErrors::kUndisplayableRegistryKey);
         return;
       }
       registry_keys.push_back(registry_key_utf16);
@@ -655,6 +664,8 @@ void ChromePromptChannelProtobuf::HandlePromptUserRequest(
       if (!base::UTF8ToUTF16(extension_id.c_str(), extension_id.size(),
                              &extension_id_utf16)) {
         LOG(ERROR) << "Undisplayable extension id in PromptUserRequest.";
+        WriteStatusErrorCodeToHistogram(ErrorCategory::kCustomError,
+                                        CustomErrors::kUndisplayableExtension);
         return;
       }
       extension_ids.push_back(extension_id_utf16);

chrome/browser/safe_browsing/chrome_cleaner/chrome_prompt_channel_win.h

@@ -159,6 +159,10 @@ class ChromePromptChannelProtobuf : public ChromePromptChannel {
     kRequestInvalidSize = 4,
     kRequestContentInvalid = 5,
     kRequestUnknown = 6,
+    kRequestUnknownField = 7,
+    kUndisplayableFilePath = 8,
+    kUndisplayableRegistryKey = 9,
+    kUndisplayableExtension = 9,
   };
 
   static int32_t GetErrorCodeInt(ErrorCategory category,

chrome/browser/safe_browsing/chrome_cleaner/chrome_prompt_channel_win_unittest.cc

@@ -116,6 +116,15 @@ class ChromePromptChannelProtobufTest : public ::testing::Test {
             this, &ChromePromptChannelProtobufTest::CloseCleanerHandles));
   }
 
+  // Expect the histograms contains at least the specified sample.
+  template <typename T>
+  void ExpectSample(ErrorCategory category, T error, int count = 1) {
+    histogram_tester_.ExpectBucketCount(
+        ChromePromptChannelProtobuf::kErrorHistogramName,
+        ChromePromptChannelProtobuf::GetErrorCodeInt(category, error), count);
+  }
+
+  // Expect that the histogram contains only the specified sample.
   template <typename T>
   void ExpectUniqueSample(ErrorCategory category, T error, int count = 1) {
     histogram_tester_.ExpectUniqueSample(
@@ -123,19 +132,14 @@ class ChromePromptChannelProtobufTest : public ::testing::Test {
         ChromePromptChannelProtobuf::GetErrorCodeInt(category, error), count);
   }
 
-  void ExpectHistogramEmpty() {
+  void ExpectHistogramSize(uint32_t size) {
     histogram_tester_.ExpectTotalCount(
-        ChromePromptChannelProtobuf::kErrorHistogramName, 0);
+        ChromePromptChannelProtobuf::kErrorHistogramName, size);
   }
 
   // This is used when we want to validate that certain operations failed a
   // precise number of times without needing to know the specific error code.
   void ExpectCategoryErrorCount(const ErrorExpectationMap& expected_counts) {
-    // Not to be used with CustomErrors as those are well-known.
-    EXPECT_THAT(expected_counts,
-                Not(Contains(Key(ErrorCategory::kCustomError))))
-        << "For custom errors please use ExpectUniqueSample";
-
     const std::vector<base::Bucket> buckets = histogram_tester_.GetAllSamples(
         ChromePromptChannelProtobuf::kErrorHistogramName);
 
@@ -526,15 +530,15 @@ TEST_F(ChromePromptChannelProtobufTest, PostExtraPromptRequestField) {
 
   std::string request_content;
   PostRequestWrite(request, &request_content);
-  PostCloseCleanerHandles();
 
+  // Here we should not post a disconnect since the error will trigger one.
   WaitForDisconnect();
 
-  // Having extra fields in PromptUserRequest should not cause any problems.
-  // This means the handling of the first message does not generate errors. The
-  // only error we see is the reading of the next request length which cannot
-  // succeed because we closed the pipes.
-  ExpectCategoryErrorCount({{ErrorCategory::kReadRequestLengthWinError, 1}});
+  // The ReadRequestLengthWinError is because the pipe was closed by the unknown
+  // field error handler while waiting for the next request.
+  ExpectCategoryErrorCount({{ErrorCategory::kReadRequestLengthWinError, 1},
+                            {ErrorCategory::kCustomError, 1}});
+  ExpectSample(ErrorCategory::kCustomError, CustomErrors::kRequestUnknownField);
 
   ExpectReadFails();
 }