dbus: No infinite timeout calls on ChromeOS system daemon

ChromeOS system daemon will be configured to have a max reply
timeout (of 1 hour). Replace `TIMEOUT_INFINITE` in existing
calls with `TIMEOUT_MAX_CHROMEOS` for ChromeOS.

Bug: b:388332345
Change-Id: Ic664ab3047cd396911202e8da6cfd613ce4499a4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6261306
Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
Reviewed-by: David Padlipsky <dpad@google.com>
Commit-Queue: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Michael Sun <michaelfsun@google.com>
Reviewed-by: Nina Satragno <nsatragno@chromium.org>
Reviewed-by: Igor <igorcov@chromium.org>
Reviewed-by: Achuith Bhandarkar <achuith@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1422528}

chromeos/ash/components/dbus/fwupd/fwupd_client.cc

@@ -256,9 +256,10 @@ class FwupdClientImpl : public FwupdClient {
     writer.CloseContainer(&array_writer);
 
     // TODO(michaelcheco): Investigate whether or not the estimated install time
-    // multiplied by some factor can be used in place of |TIMEOUT_INFINITE|.
+    // multiplied by some factor can be used in place of
+    // `TIMEOUT_MAX`.
     proxy_->CallMethodWithErrorResponse(
-        &method_call, dbus::ObjectProxy::TIMEOUT_INFINITE,
+        &method_call, dbus::ObjectProxy::TIMEOUT_MAX,
         base::BindOnce(&FwupdClientImpl::InstallUpdateCallback,
                        weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
   }

chromeos/ash/components/dbus/session_manager/session_manager_client.cc

@@ -580,14 +580,10 @@ class SessionManagerClientImpl : public SessionManagerClient {
         login_manager::kSessionManagerInterface,
         login_manager::kSessionManagerGetServerBackedStateKeys);
 
-    // Infinite timeout needed because the state keys are not generated as long
+    // Long timeout needed because the state keys are not generated as long
     // as the time sync hasn't been done (which requires network).
-    // TODO(igorcov): Since this is a resource allocated that could last a long
-    // time, we will need to change the behavior to either listen to
-    // LastSyncInfo event from tlsdated or communicate through signals with
-    // session manager in this particular flow.
     session_manager_proxy_->CallMethodWithErrorResponse(
-        &method_call, dbus::ObjectProxy::TIMEOUT_INFINITE,
+        &method_call, dbus::ObjectProxy::TIMEOUT_MAX,
         base::BindOnce(&SessionManagerClientImpl::OnGetServerBackedStateKeys,
                        weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
   }

chromeos/dbus/u2f/u2f_client.cc

@@ -35,9 +35,16 @@ constexpr char kGetAssertionStatusHistogram[] =
 
 // Some methods trigger an OS modal dialog that needs to be completed or
 // dismissed before the method returns. These methods are cancelled explicitly
-// once original WebAuthn request times out. Hence, they are invoked with
-// infinite DBus timeouts.
-const int kU2FInfiniteTimeout = dbus::ObjectProxy::TIMEOUT_INFINITE;
+// once original WebAuthn request times out.
+//
+// W3C spec recommends 5 min timeout:
+// https://w3c.github.io/webauthn/#sctn-timeout-recommended-range
+//
+// Chrome since M120 can support up to 20 hours:
+// https://lists.w3.org/Archives/Public/public-webauthn/2023Oct/0151.html
+//
+// In practice, `TIMEOUT_MAX` (1 hour) should be long enough.
+const int kU2FMaxTimeout = dbus::ObjectProxy::TIMEOUT_MAX;
 
 // Timeout for methods which don't take time proportional to the number of total
 // credentials.
@@ -165,7 +172,7 @@ void U2FClientImpl::MakeCredential(
   dbus::MessageWriter writer(&method_call);
   writer.AppendProtoAsArrayOfBytes(request);
   proxy_->CallMethod(
-      &method_call, kU2FInfiniteTimeout,
+      &method_call, kU2FMaxTimeout,
       base::BindOnce(
           [](DBusMethodCallback<u2f::MakeCredentialResponse> callback,
              dbus::Response* dbus_response) {
@@ -203,7 +210,7 @@ void U2FClientImpl::GetAssertion(
       },
       std::move(callback));
   proxy_->CallMethod(
-      &method_call, kU2FInfiniteTimeout,
+      &method_call, kU2FMaxTimeout,
       base::BindOnce(&U2FClientImpl::HandleResponse<u2f::GetAssertionResponse>,
                      weak_factory_.GetWeakPtr(),
                      std::move(uma_callback_wrapper)));

dbus/object_proxy.cc

@@ -5,8 +5,10 @@
 #include "dbus/object_proxy.h"
 
 #include <stddef.h>
+
 #include <utility>
 
+#include "base/check.h"
 #include "base/containers/contains.h"
 #include "base/debug/alias.h"
 #include "base/debug/leak_annotations.h"
@@ -18,6 +20,7 @@
 #include "base/threading/scoped_blocking_call.h"
 #include "base/threading/thread.h"
 #include "base/threading/thread_restrictions.h"
+#include "build/build_config.h"
 #include "dbus/bus.h"
 #include "dbus/dbus_statistics.h"
 #include "dbus/error.h"
@@ -132,6 +135,11 @@ base::expected<std::unique_ptr<Response>, Error>
 ObjectProxy::CallMethodAndBlock(MethodCall* method_call, int timeout_ms) {
   bus_->AssertOnDBusThread();
 
+#if BUILDFLAG(IS_CHROMEOS)
+  // ChromeOS system daemon has `reply_timeout` configured.
+  CHECK_LE(timeout_ms, TIMEOUT_MAX);
+#endif  // BUILDFLAG(IS_CHROMEOS)
+
   if (!bus_->Connect() || !method_call->SetDestination(service_name_) ||
       !method_call->SetPath(object_path_)) {
     // Not an error from libdbus, so returns invalid error.
@@ -167,6 +175,11 @@ void ObjectProxy::CallMethodWithErrorResponse(
     ResponseOrErrorCallback callback) {
   bus_->AssertOnOriginThread();
 
+#if BUILDFLAG(IS_CHROMEOS)
+  // ChromeOS system daemon has `reply_timeout` configured.
+  CHECK_LE(timeout_ms, TIMEOUT_MAX);
+#endif  // BUILDFLAG(IS_CHROMEOS)
+
   ReplyCallbackHolder callback_holder(bus_->GetOriginTaskRunner(),
                                       std::move(callback));
 

dbus/object_proxy.h

@@ -62,13 +62,16 @@ class CHROME_DBUS_EXPORT ObjectProxy
   };
 
   // Special timeout constants.
-  //
-  // The constants correspond to DBUS_TIMEOUT_USE_DEFAULT and
-  // DBUS_TIMEOUT_INFINITE. Here we use literal numbers instead of these
-  // macros as these aren't defined with D-Bus earlier than 1.4.12.
   enum {
+    // The constant corresponds to DBUS_TIMEOUT_USE_DEFAULT. Here we use literal
+    // numbers instead of these macros as these aren't defined with D-Bus
+    // earlier than 1.4.12.
     TIMEOUT_USE_DEFAULT = -1,
-    TIMEOUT_INFINITE = 0x7fffffff,
+
+    // Max timeout for methods calls. Long method calls add to `pending_replies"
+    // in dbus_daemon and it has a limit. Once that limit is exceeded, no more
+    // calls could be made on the relevant DBusConnection.
+    TIMEOUT_MAX = base::Hours(1).InMilliseconds(),
   };
 
   // Called when an error response is returned or no response is returned.

device/bluetooth/dbus/bluetooth_device_client.cc

@@ -302,9 +302,9 @@ class BluetoothDeviceClientImpl : public BluetoothDeviceClient,
       return;
     }
 
-    // Connect may take an arbitrary length of time, so use no timeout.
+    // Connect may take an arbitrary length of time, so use the max timeout.
     object_proxy->CallMethodWithErrorCallback(
-        &method_call, dbus::ObjectProxy::TIMEOUT_INFINITE,
+        &method_call, dbus::ObjectProxy::TIMEOUT_MAX,
         base::BindOnce(&BluetoothDeviceClientImpl::OnSuccess,
                        weak_ptr_factory_.GetWeakPtr(), std::move(callback)),
         base::BindOnce(&BluetoothDeviceClientImpl::OnError,
@@ -326,9 +326,10 @@ class BluetoothDeviceClientImpl : public BluetoothDeviceClient,
       return;
     }
 
-    // ConnectClassic may take an arbitrary length of time, so use no timeout.
+    // ConnectClassic may take an arbitrary length of time, so use the max
+    // timeout.
     object_proxy->CallMethodWithErrorCallback(
-        &method_call, dbus::ObjectProxy::TIMEOUT_INFINITE,
+        &method_call, dbus::ObjectProxy::TIMEOUT_MAX,
         base::BindOnce(&BluetoothDeviceClientImpl::OnSuccess,
                        weak_ptr_factory_.GetWeakPtr(), std::move(callback)),
         base::BindOnce(&BluetoothDeviceClientImpl::OnError,
@@ -350,9 +351,9 @@ class BluetoothDeviceClientImpl : public BluetoothDeviceClient,
       return;
     }
 
-    // ConnectLE may take an arbitrary length of time, so use no timeout.
+    // ConnectLE may take an arbitrary length of time, so use the max timeout.
     object_proxy->CallMethodWithErrorCallback(
-        &method_call, dbus::ObjectProxy::TIMEOUT_INFINITE,
+        &method_call, dbus::ObjectProxy::TIMEOUT_MAX,
         base::BindOnce(&BluetoothDeviceClientImpl::OnSuccess,
                        weak_ptr_factory_.GetWeakPtr(), std::move(callback)),
         base::BindOnce(&BluetoothDeviceClientImpl::OnError,
@@ -424,9 +425,9 @@ class BluetoothDeviceClientImpl : public BluetoothDeviceClient,
       return;
     }
 
-    // Connect may take an arbitrary length of time, so use no timeout.
+    // Connect may take an arbitrary length of time, so use the max timeout.
     object_proxy->CallMethodWithErrorCallback(
-        &method_call, dbus::ObjectProxy::TIMEOUT_INFINITE,
+        &method_call, dbus::ObjectProxy::TIMEOUT_MAX,
         base::BindOnce(&BluetoothDeviceClientImpl::OnSuccess,
                        weak_ptr_factory_.GetWeakPtr(), std::move(callback)),
         base::BindOnce(&BluetoothDeviceClientImpl::OnError,
@@ -476,9 +477,9 @@ class BluetoothDeviceClientImpl : public BluetoothDeviceClient,
       return;
     }
 
-    // Pairing may take an arbitrary length of time, so use no timeout.
+    // Pairing may take an arbitrary length of time, so use the max timeout.
     object_proxy->CallMethodWithErrorCallback(
-        &method_call, dbus::ObjectProxy::TIMEOUT_INFINITE,
+        &method_call, dbus::ObjectProxy::TIMEOUT_MAX,
         base::BindOnce(&BluetoothDeviceClientImpl::OnSuccess,
                        weak_ptr_factory_.GetWeakPtr(), std::move(callback)),
         base::BindOnce(&BluetoothDeviceClientImpl::OnError,