Browse Source

Fix possible repeat key infinite recursion (#25926)

Guard against infinite recursion when pressing this sequence after fresh boot: repeat key press -> another key press/release -> repeat key release.
windexlight 4 months ago
parent
commit
593cd168c6
3 changed files with 55 additions and 16 deletions
  1. 31 16
      quantum/repeat_key.c
  2. 1 0
      quantum/repeat_key.h
  3. 23 0
      tests/repeat_key/test_repeat_key.cpp

+ 31 - 16
quantum/repeat_key.c

@@ -25,6 +25,21 @@ static int8_t last_repeat_count = 0;
 // The repeat_count, but set to 0 outside of repeat_key_invoke() so that it is
 // nonzero only while a repeated key is being processed.
 static int8_t processing_repeat_count = 0;
+// It is possible (e.g. in rolled presses) that the last key changes while
+// the Repeat Key is pressed. To prevent stuck keys, it is important to
+// remember separately what key record was processed on press so that the
+// the corresponding record is generated on release.
+static keyrecord_t registered_record_repeat_key       = {0};
+static int8_t      registered_repeat_count_repeat_key = 0;
+
+void reset_repeat_key_state(void) {
+    last_record                        = (keyrecord_t){0};
+    last_mods                          = 0;
+    last_repeat_count                  = 0;
+    processing_repeat_count            = 0;
+    registered_record_repeat_key       = (keyrecord_t){0};
+    registered_repeat_count_repeat_key = 0;
+}
 
 uint16_t get_last_keycode(void) {
     return last_record.keycode;
@@ -67,12 +82,6 @@ int8_t get_repeat_key_count(void) {
 }
 
 void repeat_key_invoke(const keyevent_t* event) {
-    // It is possible (e.g. in rolled presses) that the last key changes while
-    // the Repeat Key is pressed. To prevent stuck keys, it is important to
-    // remember separately what key record was processed on press so that the
-    // the corresponding record is generated on release.
-    static keyrecord_t registered_record       = {0};
-    static int8_t      registered_repeat_count = 0;
     // Since this function calls process_record(), it may recursively call
     // itself. We return early if `processing_repeat_count` is nonzero to
     // prevent infinite recursion.
@@ -84,19 +93,25 @@ void repeat_key_invoke(const keyevent_t* event) {
         update_last_repeat_count(1);
         // On press, apply the last mods state, stacking on top of current mods.
         register_weak_mods(last_mods);
-        registered_record       = last_record;
-        registered_repeat_count = last_repeat_count;
+        registered_record_repeat_key       = last_record;
+        registered_repeat_count_repeat_key = last_repeat_count;
     }
 
-    // Generate a keyrecord and plumb it into the event pipeline.
-    registered_record.event = *event;
-    processing_repeat_count = registered_repeat_count;
-    process_record(&registered_record);
-    processing_repeat_count = 0;
+    // It is possible for this to be zero here in the case of pressing
+    // repeat key before any other after reset, pressing another key while
+    // holding repeat key, and then releasing repeat key. Must guard against
+    // this to prevent infinite recursion.
+    if (registered_repeat_count_repeat_key) {
+        // Generate a keyrecord and plumb it into the event pipeline.
+        registered_record_repeat_key.event = *event;
+        processing_repeat_count            = registered_repeat_count_repeat_key;
+        process_record(&registered_record_repeat_key);
+        processing_repeat_count = 0;
 
-    // On release, restore the mods state.
-    if (!event->pressed) {
-        unregister_weak_mods(last_mods);
+        // On release, restore the mods state.
+        if (!event->pressed) {
+            unregister_weak_mods(last_mods);
+        }
     }
 }
 

+ 1 - 0
quantum/repeat_key.h

@@ -19,6 +19,7 @@
 #include "action.h"
 #include "keyboard.h"
 
+void     reset_repeat_key_state(void);       /**< Resets repeat key state. */
 uint16_t get_last_keycode(void);             /**< Keycode of the last key. */
 uint8_t  get_last_mods(void);                /**< Mods active with the last key. */
 void     set_last_keycode(uint16_t keycode); /**< Sets the last key. */

+ 23 - 0
tests/repeat_key/test_repeat_key.cpp

@@ -54,6 +54,7 @@ class RepeatKey : public TestFixture {
     bool process_record_user_was_called_;
 
     void SetUp() override {
+        reset_repeat_key_state();
         autoshift_disable();
         process_record_user_fun    = process_record_user_default;
         remember_last_key_user_fun = remember_last_key_user_default;
@@ -784,4 +785,26 @@ TEST_F(RepeatKey, IgnoredKeys) {
     testing::Mock::VerifyAndClearExpectations(&driver);
 }
 
+// Check that on fresh boot, repeat press, another key, repeat release doesn't hang
+TEST_F(RepeatKey, RepeatKeyHeldAfterBoot) {
+    TestDriver driver;
+    KeymapKey  key_a(0, 1, 0, KC_A);
+    KeymapKey  key_repeat(0, 2, 0, QK_REP);
+    set_keymap({key_a, key_repeat});
+
+    EXPECT_EMPTY_REPORT(driver).Times(AnyNumber());
+    ExpectString(driver, "a");
+
+    // Press and hold repeat key, then press and release 'a' key, then release repeat key
+    key_repeat.press();
+    run_one_scan_loop();
+    key_a.press();
+    run_one_scan_loop();
+    key_a.release();
+    run_one_scan_loop();
+    key_repeat.release();
+    run_one_scan_loop();
+    testing::Mock::VerifyAndClearExpectations(&driver);
+}
+
 } // namespace