[gamepad] Add null PadState check in NintendoDataFetcher

In NintendoDataFetcher::GetGamepadData we call GetPadState to
get a pointer to an unused PadState slot. This pointer may be
null if there are no more unused slots (Chrome supports at most
four connected gamepads).

This CL adds a null check so we don't try to dereference the
null PadState and crash.

BUG=996052

Change-Id: I48cb1611b1c557e5eaa1a5791bf68d5acf8841ad
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1762603
Commit-Queue: Matt Reynolds <mattreynolds@chromium.org>
Reviewed-by: Ovidio de Jesús Ruiz-Henríquez <odejesush@chromium.org>
Cr-Commit-Position: refs/heads/master@{#689709}

device/gamepad/gamepad_provider.cc

@@ -41,12 +41,7 @@ GamepadProvider::ClosureAndThread::~ClosureAndThread() = default;
 
 GamepadProvider::GamepadProvider(
     GamepadConnectionChangeClient* connection_change_client)
-    : is_paused_(true),
-      have_scheduled_do_poll_(false),
-      devices_changed_(true),
-      ever_had_user_gesture_(false),
-      sanitize_(true),
-      gamepad_shared_buffer_(std::make_unique<GamepadSharedBuffer>()),
+    : gamepad_shared_buffer_(std::make_unique<GamepadSharedBuffer>()),
       connection_change_client_(connection_change_client) {
   Initialize(std::unique_ptr<GamepadDataFetcher>());
 }
@@ -55,12 +50,7 @@ GamepadProvider::GamepadProvider(
     GamepadConnectionChangeClient* connection_change_client,
     std::unique_ptr<GamepadDataFetcher> fetcher,
     std::unique_ptr<base::Thread> polling_thread)
-    : is_paused_(true),
-      have_scheduled_do_poll_(false),
-      devices_changed_(true),
-      ever_had_user_gesture_(false),
-      sanitize_(true),
-      gamepad_shared_buffer_(std::make_unique<GamepadSharedBuffer>()),
+    : gamepad_shared_buffer_(std::make_unique<GamepadSharedBuffer>()),
       polling_thread_(std::move(polling_thread)),
       connection_change_client_(connection_change_client) {
   Initialize(std::move(fetcher));
@@ -146,6 +136,18 @@ void GamepadProvider::Resume() {
       base::BindOnce(&GamepadProvider::ScheduleDoPoll, Unretained(this)));
 }
 
+void GamepadProvider::PollOnceForTesting() {
+  poll_once_ = true;
+
+  // In tests it can be difficult to simulate a user gesture from a gamepad.
+  // Pretend that a user gesture was already received.
+  OnUserGesture();
+
+  polling_thread_->task_runner()->PostTask(
+      FROM_HERE,
+      base::BindOnce(&GamepadProvider::DoPollOnce, Unretained(this)));
+}
+
 void GamepadProvider::RegisterForUserGesture(const base::Closure& closure) {
   base::AutoLock lock(user_gesture_lock_);
   user_gesture_observers_.push_back(
@@ -297,11 +299,25 @@ void GamepadProvider::SendPauseHint(bool paused) {
   }
 }
 
-void GamepadProvider::DoPoll() {
+void GamepadProvider::DoPollRepeating() {
   DCHECK(polling_thread_->task_runner()->BelongsToCurrentThread());
   DCHECK(have_scheduled_do_poll_);
   have_scheduled_do_poll_ = false;
+  DoPoll();
 
+  // Schedule our next interval of polling.
+  ScheduleDoPoll();
+}
+
+void GamepadProvider::DoPollOnce() {
+  DCHECK(polling_thread_->task_runner()->BelongsToCurrentThread());
+  DCHECK(poll_once_);
+  poll_once_ = false;
+  DoPoll();
+}
+
+void GamepadProvider::DoPoll() {
+  DCHECK(polling_thread_->task_runner()->BelongsToCurrentThread());
   bool changed;
 
   ANNOTATE_BENIGN_RACE_SIZED(gamepad_shared_buffer_->buffer(), sizeof(Gamepads),
@@ -380,9 +396,6 @@ void GamepadProvider::DoPoll() {
     for (size_t i = 0; i < Gamepads::kItemsLengthCap; ++i)
       pad_states_.get()[i].is_newly_active = false;
   }
-
-  // Schedule our next interval of polling.
-  ScheduleDoPoll();
 }
 
 void GamepadProvider::ScheduleDoPoll() {
@@ -397,7 +410,8 @@ void GamepadProvider::ScheduleDoPoll() {
   }
 
   base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE, base::BindOnce(&GamepadProvider::DoPoll, Unretained(this)),
+      FROM_HERE,
+      base::BindOnce(&GamepadProvider::DoPollRepeating, Unretained(this)),
       sampling_interval_delta_);
   have_scheduled_do_poll_ = true;
 }
@@ -416,15 +430,22 @@ bool GamepadProvider::CheckForUserGesture() {
 
   const Gamepads* pads = gamepad_shared_buffer_->buffer();
   if (GamepadsHaveUserGesture(*pads)) {
-    ever_had_user_gesture_ = true;
-    for (size_t i = 0; i < user_gesture_observers_.size(); i++) {
-      user_gesture_observers_[i].task_runner->PostTask(
-          FROM_HERE, user_gesture_observers_[i].closure);
-    }
-    user_gesture_observers_.clear();
+    OnUserGesture();
     return true;
   }
   return false;
 }
 
+void GamepadProvider::OnUserGesture() {
+  ever_had_user_gesture_ = true;
+  for (auto& observer : user_gesture_observers_)
+    observer.task_runner->PostTask(FROM_HERE, observer.closure);
+  user_gesture_observers_.clear();
+}
+
+scoped_refptr<base::SingleThreadTaskRunner>
+GamepadProvider::GetPollingThreadRunnerForTesting() const {
+  return polling_thread_->task_runner();
+}
+
 }  // namespace device

device/gamepad/gamepad_provider.h

@@ -89,6 +89,13 @@ class DEVICE_GAMEPAD_EXPORT GamepadProvider
 
   void SetSanitizationEnabled(bool sanitize) { sanitize_ = sanitize; }
 
+  // Returns a task runner for the polling thread, for use in tests.
+  scoped_refptr<base::SingleThreadTaskRunner> GetPollingThreadRunnerForTesting()
+      const;
+
+  // Calls DoPoll once on the polling thread, for use in tests.
+  void PollOnceForTesting();
+
  private:
   void Initialize(std::unique_ptr<GamepadDataFetcher> fetcher);
 
@@ -104,6 +111,8 @@ class DEVICE_GAMEPAD_EXPORT GamepadProvider
   void SendPauseHint(bool paused);
 
   // Method for polling a GamepadDataFetcher. Runs on the polling_thread_.
+  void DoPollOnce();
+  void DoPollRepeating();
   void DoPoll();
   void ScheduleDoPoll();
 
@@ -115,6 +124,10 @@ class DEVICE_GAMEPAD_EXPORT GamepadProvider
   // true if any user gesture observers were notified.
   bool CheckForUserGesture();
 
+  // Called the first time a gamepad user gesture is received. Records that the
+  // gesture was seen and notifies observers.
+  void OnUserGesture();
+
   void PlayEffectOnPollingThread(
       uint32_t pad_index,
       mojom::GamepadHapticEffectType,
@@ -133,12 +146,15 @@ class DEVICE_GAMEPAD_EXPORT GamepadProvider
   // Keeps track of when the background thread is paused. Access to is_paused_
   // must be guarded by is_paused_lock_.
   base::Lock is_paused_lock_;
-  bool is_paused_;
+  bool is_paused_ = true;
 
-  // Keep track of when a polling task is schedlued, so as to prevent us from
+  // Keep track of when a polling task is scheduled, so as to prevent us from
   // accidentally scheduling more than one at any time, when rapidly toggling
   // |is_paused_|.
-  bool have_scheduled_do_poll_;
+  bool have_scheduled_do_poll_ = false;
+
+  // True if a single (non-repeating) polling task is scheduled. Used in tests.
+  bool poll_once_ = false;
 
   // Lists all observers registered for user gestures, and the thread which
   // to issue the callbacks on. Since we always issue the callback on the
@@ -164,10 +180,10 @@ class DEVICE_GAMEPAD_EXPORT GamepadProvider
   // tests. Access to devices_changed_ must be guarded by
   // devices_changed_lock_.
   base::Lock devices_changed_lock_;
-  bool devices_changed_;
+  bool devices_changed_ = true;
 
-  bool ever_had_user_gesture_;
-  bool sanitize_;
+  bool ever_had_user_gesture_ = false;
+  bool sanitize_ = true;
 
   // Only used on the polling thread.
   using GamepadFetcherVector = std::vector<std::unique_ptr<GamepadDataFetcher>>;

device/gamepad/nintendo_controller.cc

@@ -8,7 +8,6 @@
 #include <utility>
 
 #include "base/bind.h"
-#include "base/strings/stringprintf.h"
 #include "device/gamepad/gamepad_data_fetcher.h"
 #include "device/gamepad/gamepad_id_list.h"
 
@@ -1170,6 +1169,9 @@ void NintendoController::FinishInitSequence() {
 }
 
 void NintendoController::FailInitSequence() {
+  // Ignore initialization failures in tests.
+  if (initialized_for_testing_)
+    return;
   state_ = kUninitialized;
   UpdatePadConnected();
 }
@@ -1733,6 +1735,12 @@ void NintendoController::OnTimeout() {
   }
 }
 
+void NintendoController::FinishInitSequenceForTesting() {
+  initialized_for_testing_ = true;
+  CancelTimeout();
+  FinishInitSequence();
+}
+
 base::WeakPtr<AbstractHapticGamepad> NintendoController::GetWeakPtr() {
   return weak_factory_.GetWeakPtr();
 }

device/gamepad/nintendo_controller.h

@@ -12,6 +12,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/optional.h"
 #include "device/gamepad/abstract_haptic_gamepad.h"
+#include "device/gamepad/gamepad_export.h"
 #include "device/gamepad/gamepad_id_list.h"
 #include "device/gamepad/gamepad_standard_mappings.h"
 #include "device/gamepad/public/cpp/gamepad.h"
@@ -57,7 +58,8 @@ namespace device {
 // strong and weak effect magnitudes. When a vibration effect is played on a
 // composite device, the effect is split so that each component receives one
 // channel of the dual-rumble effect.
-class NintendoController final : public AbstractHapticGamepad {
+class DEVICE_GAMEPAD_EXPORT NintendoController final
+    : public AbstractHapticGamepad {
  public:
   struct SwitchCalibrationData {
     SwitchCalibrationData();
@@ -184,6 +186,9 @@ class NintendoController final : public AbstractHapticGamepad {
   double GetMaxEffectDurationMillis() override;
   base::WeakPtr<AbstractHapticGamepad> GetWeakPtr() override;
 
+  // Simulate successful initialization, for use in tests.
+  void FinishInitSequenceForTesting();
+
   NintendoController(int source_id,
                      mojom::HidDeviceInfoPtr device_info,
                      mojom::HidManager* hid_manager);
@@ -404,6 +409,9 @@ class NintendoController final : public AbstractHapticGamepad {
   // becomes ready.
   base::OnceClosure device_ready_closure_;
 
+  // True if the device is simulating an initialized state, used in tests.
+  bool initialized_for_testing_ = false;
+
   base::WeakPtrFactory<NintendoController> weak_factory_{this};
 };
 

device/gamepad/nintendo_data_fetcher.cc

@@ -181,6 +181,9 @@ void NintendoDataFetcher::GetGamepadData(bool) {
     auto& device = entry.second;
     if (device->IsOpen() && device->IsUsable()) {
       PadState* state = GetPadState(device->GetSourceId());
+      if (!state)
+        continue;
+
       if (!state->is_initialized) {
         state->mapper = device->GetMappingFunction();
         device->InitializeGamepadState(state->mapper != nullptr, state->data);

device/gamepad/nintendo_data_fetcher_unittest.cc

@@ -6,13 +6,20 @@
 
 #include <utility>
 
+#include "base/format_macros.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted_memory.h"
 #include "base/run_loop.h"
+#include "base/strings/stringprintf.h"
+#include "base/test/bind_test_util.h"
 #include "base/threading/thread.h"
 #include "build/build_config.h"
+#include "device/gamepad/gamepad_pad_state_provider.h"
+#include "device/gamepad/gamepad_provider.h"
 #include "device/gamepad/gamepad_service.h"
+#include "device/gamepad/nintendo_controller.h"
 #include "services/device/device_service_test_base.h"
+#include "services/device/hid/hid_device_info.h"
 #include "services/device/hid/hid_manager_impl.h"
 #include "services/device/hid/mock_hid_service.h"
 #include "services/device/public/mojom/hid.mojom.h"
@@ -23,21 +30,21 @@
 #ifdef LEAK_SANITIZER
 #define MAYBE_AddAndRemoveSwitchPro DISABLED_AddAndRemoveSwitchPro
 #define MAYBE_UnsupportedDeviceIsIgnored DISABLED_UnsupportedDeviceIsIgnored
+#define MAYBE_ExceedGamepadLimit DISABLED_ExceedGamepadLimit
 #else
 #define MAYBE_AddAndRemoveSwitchPro AddAndRemoveSwitchPro
 #define MAYBE_UnsupportedDeviceIsIgnored UnsupportedDeviceIsIgnored
+#define MAYBE_ExceedGamepadLimit ExceedGamepadLimit
 #endif
 
 namespace device {
 
 namespace {
-
-#if defined(OS_MACOSX)
-const uint64_t kTestDeviceId = 123;
-#else
-const char* kTestDeviceId = "123";
-#endif
-
+constexpr uint16_t kVendorNintendo = 0x57e;
+constexpr uint16_t kProductSwitchPro = 0x2009;
+constexpr size_t kMaxInputReportSizeBytes = 63;
+constexpr size_t kMaxOutputReportSizeBytes = 63;
+constexpr size_t kMaxFeatureReportSizeBytes = 0;
 }  // namespace
 
 // Main test fixture
@@ -81,21 +88,47 @@ class NintendoDataFetcherTest : public DeviceServiceTestBase {
     polling_thread_->FlushForTesting();
   }
 
+  // HidPlatformDeviceId is uint64_t on macOS, std::string elsewhere.
+  HidPlatformDeviceId GetUniqueDeviceId() {
+#if defined(OS_MACOSX)
+    return next_device_id_++;
+#else
+    return base::StringPrintf("%" PRIu64, next_device_id_++);
+#endif
+  }
+
+  scoped_refptr<HidDeviceInfo> CreateSwitchProUsb() {
+    auto collection = mojom::HidCollectionInfo::New();
+    collection->usage = mojom::HidUsageAndPage::New(
+        mojom::kGenericDesktopJoystick, mojom::kPageGenericDesktop);
+    return new HidDeviceInfo(GetUniqueDeviceId(), kVendorNintendo,
+                             kProductSwitchPro, "Switch Pro Controller",
+                             "test-serial", mojom::HidBusType::kHIDBusTypeUSB,
+                             std::move(collection), kMaxInputReportSizeBytes,
+                             kMaxOutputReportSizeBytes,
+                             kMaxFeatureReportSizeBytes);
+  }
+
   MockHidService* mock_hid_service_;
   std::unique_ptr<GamepadProvider> provider_;
   NintendoDataFetcher* fetcher_;
   base::Thread* polling_thread_;
 
+  uint64_t next_device_id_ = 0;
+
   DISALLOW_COPY_AND_ASSIGN(NintendoDataFetcherTest);
 };
 
 TEST_F(NintendoDataFetcherTest, MAYBE_UnsupportedDeviceIsIgnored) {
   // Simulate an unsupported, non-Nintendo HID device.
   auto collection = mojom::HidCollectionInfo::New();
-  collection->usage = mojom::HidUsageAndPage::New(0, 0);
+  collection->usage = mojom::HidUsageAndPage::New(
+      mojom::kGenericDesktopJoystick, mojom::kPageGenericDesktop);
   scoped_refptr<HidDeviceInfo> device_info(new HidDeviceInfo(
-      kTestDeviceId, 0x1234, 0xabcd, "Invalipad", "",
-      mojom::HidBusType::kHIDBusTypeUSB, std::move(collection), 0, 0, 0));
+      GetUniqueDeviceId(), /*vendor_id=*/0x1234, /*product_id=*/0xabcd,
+      "NonTendo", "test-serial", mojom::HidBusType::kHIDBusTypeUSB,
+      std::move(collection), kMaxInputReportSizeBytes,
+      kMaxOutputReportSizeBytes, kMaxFeatureReportSizeBytes));
 
   // Add the device to the mock HID service. The HID service should notify the
   // data fetcher.
@@ -106,17 +139,13 @@ TEST_F(NintendoDataFetcherTest, MAYBE_UnsupportedDeviceIsIgnored) {
   EXPECT_TRUE(fetcher_->GetControllersForTesting().empty());
 
   // Remove the device.
-  mock_hid_service_->RemoveDevice(kTestDeviceId);
+  mock_hid_service_->RemoveDevice(device_info->platform_device_id());
   RunUntilIdle();
 }
 
 TEST_F(NintendoDataFetcherTest, MAYBE_AddAndRemoveSwitchPro) {
   // Simulate a Switch Pro over USB.
-  auto collection = mojom::HidCollectionInfo::New();
-  collection->usage = mojom::HidUsageAndPage::New(0, 0);
-  scoped_refptr<HidDeviceInfo> device_info(new HidDeviceInfo(
-      kTestDeviceId, 0x057e, 0x2009, "Switch Pro Controller", "",
-      mojom::HidBusType::kHIDBusTypeUSB, std::move(collection), 0, 63, 0));
+  scoped_refptr<HidDeviceInfo> device_info = CreateSwitchProUsb();
 
   // Add the device to the mock HID service. The HID service should notify the
   // data fetcher.
@@ -124,10 +153,10 @@ TEST_F(NintendoDataFetcherTest, MAYBE_AddAndRemoveSwitchPro) {
   RunUntilIdle();
 
   // The fetcher should have added the device to its internal device map.
-  EXPECT_EQ(fetcher_->GetControllersForTesting().size(), 1U);
+  EXPECT_EQ(1U, fetcher_->GetControllersForTesting().size());
 
   // Remove the device.
-  mock_hid_service_->RemoveDevice(kTestDeviceId);
+  mock_hid_service_->RemoveDevice(device_info->platform_device_id());
 
   RunUntilIdle();
 
@@ -135,4 +164,59 @@ TEST_F(NintendoDataFetcherTest, MAYBE_AddAndRemoveSwitchPro) {
   EXPECT_TRUE(fetcher_->GetControllersForTesting().empty());
 }
 
+TEST_F(NintendoDataFetcherTest, MAYBE_ExceedGamepadLimit) {
+  // Simulate connecting Switch Pro gamepads until the limit is exceeded.
+  const size_t kNumGamepads = Gamepads::kItemsLengthCap + 1;
+  std::vector<HidPlatformDeviceId> device_ids;
+  for (size_t i = 0; i < kNumGamepads; ++i) {
+    auto device_info = CreateSwitchProUsb();
+    device_ids.push_back(device_info->platform_device_id());
+    mock_hid_service_->AddDevice(device_info);
+
+    RunUntilIdle();
+
+    // Simulate successful initialization. This must occur on the polling
+    // thread.
+    provider_->GetPollingThreadRunnerForTesting()->PostTask(
+        FROM_HERE, base::BindLambdaForTesting([&]() {
+          auto& controller_map = fetcher_->GetControllersForTesting();
+          for (auto& entry : controller_map)
+            entry.second->FinishInitSequenceForTesting();
+        }));
+
+    RunUntilIdle();
+
+    // Check that the gamepad was added.
+    EXPECT_EQ(i + 1, fetcher_->GetControllersForTesting().size());
+  }
+
+  // Poll once. This allows the data fetcher to acquire PadState slots for
+  // connected gamepads.
+  provider_->PollOnceForTesting();
+  RunUntilIdle();
+
+  // Check that all PadState slots are assigned.
+  EXPECT_EQ(nullptr,
+            provider_->GetPadState(GAMEPAD_SOURCE_TEST, /*source_id=*/100));
+
+  // Remove all gamepads.
+  for (size_t i = 0; i < kNumGamepads; ++i) {
+    mock_hid_service_->RemoveDevice(device_ids[i]);
+    RunUntilIdle();
+
+    // Check that the gamepad was removed.
+    EXPECT_EQ(kNumGamepads - i - 1,
+              fetcher_->GetControllersForTesting().size());
+  }
+
+  // Poll once. This allows the provider to detect that the gamepads have been
+  // disconnected.
+  provider_->PollOnceForTesting();
+  RunUntilIdle();
+
+  // After disconnecting the devices there should be unassigned PadState slots.
+  EXPECT_NE(nullptr,
+            provider_->GetPadState(GAMEPAD_SOURCE_TEST, /*source_id=*/100));
+}
+
 }  // namespace device