[ipc] Add feature to report possible skipped message in ChannelAssoci…

…atedGroupController.

Enabling the MojoChannelAssociatedSendUsesRunOrPostTask feature
resulted in crash reports indicating that messages were not received
in the expected order (could be skipped or reordered). This CL adds
a feature to crash when sending a message fails but more messages could
potentially be sent, in order to better understand these crash reports.

Bug: 1503967
Change-Id: I89d49f3b36269359f01af5861154619665706991
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5521936
Reviewed-by: Ken Rockot <[email protected]>
Commit-Queue: Francois Pierre Doray <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1301609}

ipc/ipc_mojo_bootstrap.cc

@@ -38,6 +38,7 @@
 #include "base/trace_event/typed_macros.h"
 #include "ipc/ipc_channel.h"
 #include "ipc/urgent_message_observer.h"
+#include "mojo/public/c/system/types.h"
 #include "mojo/public/cpp/bindings/associated_group.h"
 #include "mojo/public/cpp/bindings/associated_group_controller.h"
 #include "mojo/public/cpp/bindings/connector.h"
@@ -51,6 +52,7 @@
 #include "mojo/public/cpp/bindings/pipe_control_message_handler.h"
 #include "mojo/public/cpp/bindings/pipe_control_message_handler_delegate.h"
 #include "mojo/public/cpp/bindings/pipe_control_message_proxy.h"
+#include "mojo/public/cpp/bindings/scoped_message_error_crash_key.h"
 #include "mojo/public/cpp/bindings/sequence_local_sync_event_watcher.h"
 #include "mojo/public/cpp/bindings/tracing_helpers.h"
 #include "third_party/abseil-cpp/absl/base/attributes.h"
@@ -67,6 +69,10 @@ BASE_FEATURE(kMojoChannelAssociatedSendUsesRunOrPostTask,
              "MojoChannelAssociatedSendUsesRunOrPostTask",
              base::FEATURE_DISABLED_BY_DEFAULT);
 
+BASE_FEATURE(kMojoChannelAssociatedCrashesOnSendError,
+             "MojoChannelAssociatedCrashesOnSendError",
+             base::FEATURE_DISABLED_BY_DEFAULT);
+
 // Used to track some internal Channel state in pursuit of message leaks.
 //
 // TODO(crbug.com/40563310): Remove this.
@@ -918,7 +924,24 @@ class ChannelAssociatedGroupController
       }
       return true;
     }
-    return connector_->Accept(message);
+    MojoResult result = connector_->AcceptAndGetResult(message);
+
+    // TODO(crbug.com/40944462): Remove this code when the cause of skipped
+    // messages with MojoChannelAssociatedSendUsesRunOrPostTask is understood,
+    // or no later than November 2024.
+    if (result != MOJO_RESULT_OK && !connector_->encountered_error() &&
+        base::FeatureList::IsEnabled(
+            kMojoChannelAssociatedCrashesOnSendError)) {
+      // Crash when sending a message fails and `connector_` can send more
+      // messages, as that breaks the assumption that messages are received in
+      // the order they were sent. Note: `connector_` cannot send more messages
+      // when `encountered_error()` is true.
+      mojo::debug::ScopedMessageErrorCrashKey crash_key(
+          base::StringPrintf("SendMessage failed with error %d", result));
+      CHECK(false);
+    }
+
+    return result == MOJO_RESULT_OK;
   }
 
   void SendMessageOnSequenceViaTask(mojo::Message message) {

mojo/public/cpp/bindings/connector.h

@@ -19,6 +19,7 @@
 #include "base/sequence_checker.h"
 #include "base/task/sequenced_task_runner.h"
 #include "base/thread_annotations.h"
+#include "mojo/public/c/system/types.h"
 #include "mojo/public/cpp/bindings/connection_group.h"
 #include "mojo/public/cpp/bindings/message.h"
 #include "mojo/public/cpp/bindings/message_header_validator.h"
@@ -203,6 +204,8 @@ class COMPONENT_EXPORT(MOJO_CPP_BINDINGS) Connector : public MessageReceiver {
   bool PrefersSerializedMessages() override;
   bool Accept(Message* message) override;
 
+  MojoResult AcceptAndGetResult(Message* message);
+
   MessagePipeHandle handle() const {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
     return message_pipe_.get();

mojo/public/cpp/bindings/lib/connector.cc

@@ -314,17 +314,22 @@ bool Connector::PrefersSerializedMessages() {
 }
 
 bool Connector::Accept(Message* message) {
+  MojoResult result = AcceptAndGetResult(message);
+  return result == MOJO_RESULT_OK;
+}
+
+MojoResult Connector::AcceptAndGetResult(Message* message) {
   if (!lock_ && task_runner_)
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   if (TS_UNCHECKED_READ(error_)) {
-    return false;
+    return MOJO_RESULT_UNKNOWN;
   }
 
   internal::MayAutoLock locker(&lock_);
 
   if (!message_pipe_.is_valid() || drop_writes_)
-    return true;
+    return MOJO_RESULT_OK;
 
 #if defined(ENABLE_IPC_FUZZER)
   if (message_dumper_ && message->is_serialized()) {
@@ -359,6 +364,7 @@ bool Connector::Accept(Message* message) {
       // from the caller since we'd like them to continue consuming any backlog
       // of incoming messages before regarding the message pipe as closed.
       drop_writes_ = true;
+      rv = MOJO_RESULT_OK;
       break;
     case MOJO_RESULT_BUSY:
       // We'd get a "busy" result if one of the message's handles is:
@@ -372,13 +378,13 @@ bool Connector::Accept(Message* message) {
       // crbug.com/389666, etc. are resolved, this will make tests fail quickly
       // rather than hanging.)
       CHECK(false) << "Race condition or other bug detected";
-      return false;
+      break;
     default:
       // This particular write was rejected, presumably because of bad input.
       // The pipe is not necessarily in a bad state.
-      return false;
+      break;
   }
-  return true;
+  return rv;
 }
 
 void Connector::AllowWokenUpBySyncWatchOnSameThread() {