1330252 - A flood of incoming keyboard events can cause us to skip a lot of frames when going down on GSlides

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[(no longer active)]

STR:

1. Open the PLR slide deck (or any other big one, this one has 47 slides in it)
2. Wait for the load to finish, and scroll down through the thumbnails waiting for all of them to be generated.
3. Go back to the first slide, focus the thumbnail and hold down the arrow key.

On OSX and Windows, we paint a few frames only (4-5 frames for example), but on Linux things are *much* faster.  If instead of holding down the key, you press it as fast as you can, we can successfully paint all frames and keep up with the events.

smaug, can you please take a look?

Comment 1

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

This is actually not clear to me yet at all. Since vsync is prioritized, flooding keyevents doesn't prevent painting.
Is GSlides possibly handling several key events before explicitly calling requestAnimationFrame to get something painted.

Comment 2

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

And on linux at least refresh driver is ticking in child process even while we're waiting for slides to change.
So, still very much unclear what is happening.

Comment 3

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Attachment: WIP, doesn't work if key is changed during the repeat loop

Comment 4

[(no longer active)]

This improves things *a lot* on my machine.  Now we can paint pretty much every single slide going down the list.  Much better than Chrome too on my machine.

Comment 5

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Attachment: repeated_keyevents.diff

Testcase is coming. It needs to be some chrome test which loads a web page which does slow things in keydown listener.

Comment 6

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d289cf92c9328c1b42adf4f0e22e9eb0109bbf8c

Comment 7

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

Comment on attachment 8826108 [details] [diff] [review]
repeated_keyevents.diff

>+// In case handling repeated keys takes much time, we skip firing new ones.
>+bool
>+TabChild::SkipRepeatedKeyEvent(const WidgetKeyboardEvent& aEvent)
>+{
>+  if (aEvent.mMessage == eKeyUp) {
>+    mRepeatedKeyEvents.Clear();
>+    return false;
>+  }
>+
>+  if (!aEvent.mIsRepeat ||
>+      (aEvent.mMessage != eKeyDown && aEvent.mMessage != eKeyPress)) {
>+    return false;
>+  }
>+
>+  uint32_t i = aEvent.mMessage == eKeyDown ? eKeyDownIndex : eKeyPressIndex;
>+  if (!mRepeatedKeyEvents.Length()) {

nit: IsEmpty()?

>+    Unused << mRepeatedKeyEvents.AppendElements(2, mozilla::fallible);

I'm not sure why you use "fallible" but you don't check its result.

>+    mRepeatedKeyEvents[i].mEvent = aEvent.Duplicate()->AsKeyboardEvent();
>+    MOZ_ASSERT(mRepeatedKeyEvents.Length() == 2);

Hmm, but you check the result only on debug build... Why?

>+    return false;
>+  }
>+
>+  WidgetKeyboardEvent* oldEvent = mRepeatedKeyEvents[i].mEvent;
>+  if (oldEvent &&
>+      !mRepeatedKeyEvents[i].mEndTime.IsNull() &&
>+      mRepeatedKeyEvents[i].mEndTime > aEvent.mTimeStamp &&

mEndTime is TimeStamp::Now() of previous keyboard event is dispatched, but mTimeStamp of WidgetKeyboardEvent is initialized with native event's time stamp. If the system or the parent process is busy, native events are delivered with some delay. So, this is a hack for reducing number of repeated keyboard events but I feel this logic is tricky. Although, I don't have better idea. I hope that this won't cause dropping necessary keyboard events for the user.

>+      oldEvent->mKeyCode == aEvent.mKeyCode &&
>+      oldEvent->mCharCode == aEvent.mCharCode &&
>+      oldEvent->mLocation == aEvent.mLocation &&
>+      oldEvent->mKeyNameIndex == aEvent.mKeyNameIndex &&
>+      oldEvent->mCodeNameIndex == aEvent.mCodeNameIndex) {

At least both Windows and macOS, only the last pressed key is repeated, so, this must be safe at least for eKeyDown. On Android, I don't see auto repeated keyboard events with both USB keyboard and Bluetooth keyboard (tested on Nexus 7 (2013)), so, we cannot skip any keyboard events even if we implement e10s on Android. I'm not sure about Linux because I need to reboot my machine to test this, but I cannot do it right now. I'll check it later.

Anyway, there is a problem. This is not work as you expected when the event is eKeyPress and the key causes 2 or more characters. When a key press causes 2 or more characters, following events are fired:

eKeyDown  = { key: "c1c2c3...", charCode: 0 }
eKeyPress = { key: "c1",        charCode: "c1".charCodeAt(0) }
eKeyPress = { key: "c2",        charCode: "c2".charCodeAt(0) }
eKeyPress = { key: "c3",        charCode: "c3".charCodeAt(0) }
eKeyPress = ...
...
eKeyUp    = { key: "c1c2c3...", charCode: 0 }

If you'd like to check if received keyboard event is an expected event, you need to check if .key value is included in .key of preceding keydown event.

However, I wonder, we could skip any repeated keyboard events simply because even if two or more keys are repeated on Linux, I don't see any difference between skipping only one key and skipping 2 or more keys.

>+    return true;
>+  }
>+
>+  mRepeatedKeyEvents[i].mEvent = aEvent.Duplicate()->AsKeyboardEvent();
>+  if (aEvent.mMessage == eKeyDown) {
>+    // If keydown wasn't skipped, nor should the possible following keypress.
>+    mRepeatedKeyEvents[eKeyPressIndex].mEndTime = TimeStamp();
>+  }
>+  return false;
>+}
>+
>+void
>+TabChild::UpdateRepeatedKeyEventEndTimes(const WidgetKeyboardEvent& aEvent)
>+{
>+  if (aEvent.mIsRepeat &&
>+      (aEvent.mMessage == eKeyDown || aEvent.mMessage == eKeyPress)) {
>+    uint32_t i = aEvent.mMessage == eKeyDown ? 0 : 1;
>+    mRepeatedKeyEvents[i].mEndTime = TimeStamp::Now();

If this is called before SkipRepeatedKeyEvent() accidentally, this might cause crash (I guess that it's safe because you use AutoTArray, so, the address is allocated even if SkipRepeatedKeyEvent() hasn't been called yet). But I hope that you should check mRepeatedKeyEvents.IsEmpty() at the if condition with NS_WARN_IF() or MOZ_ASSERT().

>+  }
>+}
>+
> mozilla::ipc::IPCResult
> TabChild::RecvRealKeyEvent(const WidgetKeyboardEvent& event,
>                            const MaybeNativeKeyBinding& aBindings)
> {
>+  if (SkipRepeatedKeyEvent(event)) {
>+    return IPC_OK();
>+  }
>+
>   AutoCacheNativeKeyCommands autoCache(mPuppetWidget);
> 
>   if (event.mMessage == eKeyPress) {
>     // If content code called preventDefault() on a keydown event, then we don't
>     // want to process any following keypress events.
>     if (mIgnoreKeyPressEvent) {
>       return IPC_OK();
>     }

>+  bool SkipRepeatedKeyEvent(const WidgetKeyboardEvent& aEvent);
>+
>+  void UpdateRepeatedKeyEventEndTimes(const WidgetKeyboardEvent& aEvent);

It's nice if you explain what these method does.

>+  struct KeyboardEventAndEndTime
>+  {
>+    nsAutoPtr<WidgetKeyboardEvent> mEvent;
>+    mozilla::TimeStamp mEndTime;
>+  };
>+
>+  enum {

nir: |{| should be at next line.

>+    eKeyDownIndex = 0,
>+    eKeyPressIndex = 1
>+  };
>+  AutoTArray<KeyboardEventAndEndTime, 2> mRepeatedKeyEvents;

I think that you should explain why mRepeatedKeyEvents is necessary and how to use it as comment.

Comment 8

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

I confirmed that also Linux performs only the last key's auto repeat. So, all platforms we're supporting don't fire different key's events during performing auto repeat.

Comment 9

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

(In reply to Masayuki Nakano [:masayuki] from comment #7)
> 
> nit: IsEmpty()?
Find

> 
> >+    Unused << mRepeatedKeyEvents.AppendElements(2, mozilla::fallible);
> 
> I'm not sure why you use "fallible" but you don't check its result.
I guess I could check oom here.

> 
> >+    mRepeatedKeyEvents[i].mEvent = aEvent.Duplicate()->AsKeyboardEvent();
> >+    MOZ_ASSERT(mRepeatedKeyEvents.Length() == 2);
> 
> Hmm, but you check the result only on debug build... Why?
And change this to mRepeatedKeyEvents.Length() == eKeyPress + 1;



> >+  WidgetKeyboardEvent* oldEvent = mRepeatedKeyEvents[i].mEvent;
> >+  if (oldEvent &&
> >+      !mRepeatedKeyEvents[i].mEndTime.IsNull() &&
> >+      mRepeatedKeyEvents[i].mEndTime > aEvent.mTimeStamp &&
> 
> mEndTime is TimeStamp::Now() of previous keyboard event is dispatched, but
> mTimeStamp of WidgetKeyboardEvent is initialized with native event's time
> stamp. 
Exactly

> If the system or the parent process is busy, native events are
> delivered with some delay. 
I'm not even trying to handle that case, since if parent process is busy, we have bad bug there.
This bug is about the case when child process (web pages) have slow listeners for key events

> So, this is a hack for reducing number of
> repeated keyboard events but I feel this logic is tricky. Although, I don't
> have better idea. I hope that this won't cause dropping necessary keyboard
> events for the user.
All this code kicks in only when event listeners in child process are way to slow to process all the incoming events.



> 
> >+      oldEvent->mKeyCode == aEvent.mKeyCode &&
> >+      oldEvent->mCharCode == aEvent.mCharCode &&
> >+      oldEvent->mLocation == aEvent.mLocation &&
> >+      oldEvent->mKeyNameIndex == aEvent.mKeyNameIndex &&
> >+      oldEvent->mCodeNameIndex == aEvent.mCodeNameIndex) {
> 
> At least both Windows and macOS, only the last pressed key is repeated, so,
> this must be safe at least for eKeyDown.
Right, and on linux eKeyPress too

> On Android, I don't see auto
> repeated keyboard events with both USB keyboard and Bluetooth keyboard
> (tested on Nexus 7 (2013)), so, we cannot skip any keyboard events even if
> we implement e10s on Android.
Which is totally fine. This is just an optimization for the user experience

> I'm not sure about Linux because I need to
> reboot my machine to test this, but I cannot do it right now. I'll check it
> later.
linux has eKeyDown/Press repeated


> Anyway, there is a problem. This is not work as you expected when the event
> is eKeyPress and the key causes 2 or more characters. When a key press
> causes 2 or more characters, following events are fired:
> 
> eKeyDown  = { key: "c1c2c3...", charCode: 0 }
> eKeyPress = { key: "c1",        charCode: "c1".charCodeAt(0) }
> eKeyPress = { key: "c2",        charCode: "c2".charCodeAt(0) }
> eKeyPress = { key: "c3",        charCode: "c3".charCodeAt(0) }
> eKeyPress = ...
> ...
> eKeyUp    = { key: "c1c2c3...", charCode: 0 }
Do you have some testcase for this? Which key should I enter?


> However, I wonder, we could skip any repeated keyboard events simply because
> even if two or more keys are repeated on Linux, I don't see any difference
> between skipping only one key and skipping 2 or more keys.
Trying to understand this. Do you mean that we could just skip any slow repeated keypress?




> >+TabChild::UpdateRepeatedKeyEventEndTimes(const WidgetKeyboardEvent& aEvent)
> >+{
> >+  if (aEvent.mIsRepeat &&
> >+      (aEvent.mMessage == eKeyDown || aEvent.mMessage == eKeyPress)) {
> >+    uint32_t i = aEvent.mMessage == eKeyDown ? 0 : 1;
> >+    mRepeatedKeyEvents[i].mEndTime = TimeStamp::Now();
> 
> If this is called before SkipRepeatedKeyEvent() accidentally, this might
> cause crash (I guess that it's safe because you use AutoTArray, so, the
> address is allocated even if SkipRepeatedKeyEvent() hasn't been called yet).
> But I hope that you should check mRepeatedKeyEvents.IsEmpty() at the if
> condition with NS_WARN_IF() or MOZ_ASSERT().
Well, this would crash in debug builds anyhow in that case, so MOZ_ASSERT doesn't really give any new 
useful information.

Comment 10

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Attachment: fixing nits (v2)

Am I still missing some changes you wanted?

Comment 11

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

(In reply to Olli Pettay [:smaug] from comment #9)
> (In reply to Masayuki Nakano [:masayuki] from comment #7)
> > Anyway, there is a problem. This is not work as you expected when the event
> > is eKeyPress and the key causes 2 or more characters. When a key press
> > causes 2 or more characters, following events are fired:
> > 
> > eKeyDown  = { key: "c1c2c3...", charCode: 0 }
> > eKeyPress = { key: "c1",        charCode: "c1".charCodeAt(0) }
> > eKeyPress = { key: "c2",        charCode: "c2".charCodeAt(0) }
> > eKeyPress = { key: "c3",        charCode: "c3".charCodeAt(0) }
> > eKeyPress = ...
> > ...
> > eKeyUp    = { key: "c1c2c3...", charCode: 0 }
> Do you have some testcase for this? Which key should I enter?

On Windows, you can test this with Shift+T of Arabic keyboard layout (I confirmed with Arabic keyboard layout of United Arab Emirates):

We don't test this yet, but there are some tests which check dead key's event sequence.

On Mac, you can test this with Shift+T of Arabic - PC keyboard layout:

https://dxr.mozilla.org/mozilla-central/rev/b1c31c4a0a678194931779e0f13fba7b508eb109/widget/tests/test_keycodes.xul#1910-1922

> > However, I wonder, we could skip any repeated keyboard events simply because
> > even if two or more keys are repeated on Linux, I don't see any difference
> > between skipping only one key and skipping 2 or more keys.
> Trying to understand this. Do you mean that we could just skip any slow
> repeated keypress?

Yes. And keydown too. So, just checking mTimeStamp and mIsRepeat seems enough (see also comment 8).

Then, the array becomes really simple.

Comment 12

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

If you think that is safe enough, fine.

Comment 13

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

I think so at least current running on current OSes.

And I think that even if we need to limit to a key, checking mCodeNameIndex should be enough because changing modifier flag causes firing non-repeated keydown event first.

Comment 14

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Attachment: only mTimeStamp and mIsRepeat

Comment 15

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

Comment on attachment 8826504 [details] [diff] [review]
only mTimeStamp and mIsRepeat

>+  uint32_t i = aEvent.mMessage == eKeyDown ? eKeyDownIndex : eKeyPressIndex;
>+  if (mRepeatedKeyEvents.IsEmpty()) {
>+    if (!mRepeatedKeyEvents.AppendElements(2, mozilla::fallible)) {
>+      return false;
>+    }
>+    mRepeatedKeyEvents[i].mEvent = aEvent.Duplicate()->AsKeyboardEvent();

Then, why do you need to store all WidgetKeyboardEvent members? I think that just storing the time stamp is enough.

>+  struct KeyboardEventAndEndTime
>+  {
>+    nsAutoPtr<WidgetKeyboardEvent> mEvent;
>+    mozilla::TimeStamp mEndTime;
>+  };

So, cannot this be as following struct?

struct KeyboardEventAndEndTime
{
  mozilla::TimeStamp mEventTime;
  mozilla::TimeStamp mEndTime;
};

Comment 16

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

I guess I could do that yes.

Comment 17

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Attachment: only endtime

So we need of course store then just the end time of the previous event handling.

Comment 18

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

Comment on attachment 8826518 [details] [diff] [review]
only endtime

Thanks!

Comment 19

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Attachment: +tests

While writing tests I realized this needs to be still a bit different to handle slow keypress listeners (on linux at least).
So, only keydown compares the timestamps, but endtime is recorded from the last keydown or keypress.

The test is rather weird, but I don't think we have really other ways to test repeated keys, and it anyhow works.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5350955281cd2afbed08f3678f4e551ab07f7636

Comment 20

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

crap, I pushed from a bad m-i. Need to rebase and re-push to try

Comment 21

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Comment on attachment 8826783 [details] [diff] [review]
+tests

https://treeherder.mozilla.org/#/jobs?repo=try&revision=39b827f5710865e494919153ffd2420b604dc885&selectedJob=69009561 is better looking

Comment 22

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

Comment on attachment 8826783 [details] [diff] [review]
+tests

>diff --git a/dom/tests/browser/browser_bug1316330.js b/dom/tests/browser/browser_bug1316330.js
>new file mode 100644
>--- /dev/null
>+++ b/dom/tests/browser/browser_bug1316330.js
>@@ -0,0 +1,50 @@
>+const URL =
>+  "data:text/html,<script>" +
>+  "window.focus();" + 
>+  "var down = 0; var press = 0;" +
>+  "onkeydown = function(e) {" +
>+  "  document.body.setAttribute('down', ++down);" +

nit: shouldn't be "data-down"?

>+  "  if (e.keyCode == 'D') while (Date.now() - startTime < 500) {}" +
>+  "};" +
>+  "onkeypress = function(e) {" +
>+  "  document.body.setAttribute('press', ++press);" +

nit: shouldn't be "data-press"?

>+  "  if (e.charCode == 'P') while (Date.now() - startTime < 500) {}" +
>+  "};" +
>+  "</script>";
>+
>+function createKeyEvent(type, id) {
>+  var code = id.charCodeAt(0);
>+  return new KeyboardEvent(type, { keyCode: code, charCode: code, bubbles: true, repeat: true });
>+}
>+
>+add_task(function* () {
>+  let tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, URL);
>+  let browser = tab.linkedBrowser;
>+
>+  // Need to dispatch a DOM Event explicitly via PresShell to get KeyEvent with .repeat = true to 
>+  // be handled by EventStateManager, which then forwards the event to the child process.
>+  var utils = EventUtils._getDOMWindowUtils(window);
>+  utils.dispatchDOMEventViaPresShell(browser, createKeyEvent("keydown", "D"), true);
>+  utils.dispatchDOMEventViaPresShell(browser, createKeyEvent("keypress", "D"), true);

Hmm, first keydown/keypress are also |.repeat == true|. It may be better you to emulate actual event order.

>+  utils.dispatchDOMEventViaPresShell(browser, createKeyEvent("keydown", "D"), true);
>+  utils.dispatchDOMEventViaPresShell(browser, createKeyEvent("keypress", "D"), true);
>+  utils.dispatchDOMEventViaPresShell(browser, createKeyEvent("keyup", "D"), true);
>+
>+  yield ContentTask.spawn(browser, null, function* () {
>+    is(content.document.body.getAttribute("down"), "1", "Correct number of events");
>+    is(content.document.body.getAttribute("press"), "1", "Correct number of events");
>+  });
>+
>+  utils.dispatchDOMEventViaPresShell(browser, createKeyEvent("keydown", "P"), true);
>+  utils.dispatchDOMEventViaPresShell(browser, createKeyEvent("keypress", "P"), true);
>+  utils.dispatchDOMEventViaPresShell(browser, createKeyEvent("keydown", "P"), true);
>+  utils.dispatchDOMEventViaPresShell(browser, createKeyEvent("keypress", "P"), true);
>+  utils.dispatchDOMEventViaPresShell(browser, createKeyEvent("keyup", "P"), true);
>+
>+  yield ContentTask.spawn(browser, null, function* () {
>+    is(content.document.body.getAttribute("down"), "2", "Correct number of events");
>+    is(content.document.body.getAttribute("press"), "2", "Correct number of events");
>+  });
>+
>+  gBrowser.removeCurrentTab();
>+});

Otherwise, looks good.

Comment 23

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

data- or not doesn't really matter in tests IMO.

But ok, I can change, and make first event without repeat and then add third event which is repeat

Comment 24

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Attachment: +nits

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/68aff122042f
try to avoid flooding child process with repeated key events, r=masayuki

Comment 26

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/68aff122042f