900773 - Application freezes (?) when Windows' keyboard finite-state automaton contains a loop

Status: NEW

(Core :: Widget: Win32, defect, P3)

Description

[Ilya Zakharevich]

This is a split of https://bugzilla.mozilla.org/show_bug.cgi?id=880971#c2

The second important component of the layout-scanner is KeyboardLayout::EnsureDeadKeyActive().
  http://mxr.mozilla.org/mozilla-central/source/widget/windows/KeyboardLayout.cpp#1747

Just look at it.  It is an infinite loop (at least potentially).  On Windows, “a dead key” is just a state in a finite automaton.  The only limitation on the automaton is the number of states (at most 2^16-1).  Infinite loops ARE possible.

And thinking of it more, making a keyboard layout which would make this code loop forever is a very nice keyboard’s UI!  [See below.]

The solution: first of all, it is possible to design a (malicious???) keyboard layout (a finite automaton) such that after pressing a certain key, one will not be able to return back to the “state 0” of the automaton.  So there is no way to code this function bullet-proof; it must be heuristic.  There should be a way to return failure.

Second, duplicating a dead key is not a very good heuristic to get out of the loop.  Think of a dead key meaning “an alternative variant”.  (Good for languages where MOST of the base characters take at most one accent/mogrifier; then there is little sense to use separate deadkeys for ´, `, ¨, ¸ etc. For example, see the aksent Polish keyboard.)  Then it makes sense to make double-press of the dead key to access the OTHER alternative variant (for the — presumably rare — keys which take two different accents).  Or look at how *I* use repeated Green/Ripe keys in 
 http://math.berkeley.edu/~ilya/keyboard/iz/windows/izKeys-visual-maps.html#bridges

#####  The solution I use in reset_ToUnicode() in
  http://cpansearch.perl.org/src/ILYAZ/UI-KeyboardLayout-0.60/examples/raw_keys_via_api.pl

Heuristic 0: from the point of view of UI design, the best candidate on exiting the loop (so producing a printable character) is SPACE.  So it is tried first (without modifiers), up to 5 times.
Heuristic 1: if (0) fails, try what the function ensureDeadKeyActive() does NOW: up to 5 keypresses of deadkey.
Heuristic 2: if (1) fails, try up to 5 keypresses of all 'a'-'z','0'-'1' VKs (without modifiers).
Heuristic 3: if (2) fails, just report the failure upstream.

   [OFF TOPIC: why making a looping-forever deadkey is a good UI.  Consider the situation of “satellite” layouts (as in Green/Ripe mentioned above).  It is very convenient to punch a key 1, 2, or maybe 3 times.  More than 3 is a problematic UI (I punch it as 2+2, or 3+2 if I need to repeat 4 or 5 times).  On the other hand, if a deadkey has repeat semantic: if all presses after the 3rd one produce “the same result”, then one gets 4 states which are all easy-to-type: just long-press the deadkey and let the typematic-repeat bring you into the 4th state.]

Comment 1

[Ilya Zakharevich]

Attachment: A very primitive fix. A better fix would follow what is outlined in the initial bug report.

Untested.  Very primitive.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 784699 [details] [diff] [review]
A very primitive fix.  A better fix would follow what is outlined in the initial bug report.

I don't have any objection for creating such safety lock.

However, in this case, most users don't have such trouble. On the other hands, some very rare users might complain about the limitation such as 12. As you said in the comment of this patch, 12 is just a magic number.

So, I think that the limitation should be customizable by users. You should do it with following code:

  static uint32_t sLimitOfSequence = 0;
  if (!sLimitOfSequence) {
    const char* kPrefName = "intl.keyboard.dead_key.limit_of_sequence";
    sLimitOfSequence = std::max(Preferences::GetUint(kPrefName), 12);
  }

And then, |for| loop is better to read:

  for (uint32_t i = 0; i < sLimitOfSequence; ++i) {
    ...
    if ((ret < 0) == aIsActive) {
      break;
    }
  }

Additionally, make debug build crash at reaching to the limit.

  MOZ_ASSERT((ret < 0) == aIsActive,
             "The dead key sequence is too deep!");
  return (ret < 0);

Please confirm that new patch doesn't break the dead key on some keyboard layouts like Spanish before requesting review.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.