Closed Bug 1330252 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: DOM: Events, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: ehsan, Assigned: smaug)

References

Details

Attachments

(2 files, 5 obsolete files)

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?
Flags: needinfo?(bugs)
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.
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.
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.
Attached patch repeated_keyevents.diff (obsolete) — Splinter Review
Testcase is coming. It needs to be some chrome test which loads a web page which does slow things in keydown listener.
Assignee: nobody → bugs
Attachment #8826073 - Attachment is obsolete: true
Flags: needinfo?(bugs)
Attachment #8826108 - Flags: review?(masayuki)
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.
Attachment #8826108 - Flags: review?(masayuki) → review-
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.
(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.
Attached patch fixing nits (v2) (obsolete) — Splinter Review
Am I still missing some changes you wanted?
Attachment #8826108 - Attachment is obsolete: true
Attachment #8826501 - Flags: review?(masayuki)
(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.
If you think that is safe enough, fine.
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.
Attached patch only mTimeStamp and mIsRepeat (obsolete) — Splinter Review
Attachment #8826504 - Flags: review?(masayuki)
Attachment #8826501 - Attachment is obsolete: true
Attachment #8826501 - Flags: review?(masayuki)
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;
};
I guess I could do that yes.
Attached patch only endtime (obsolete) — Splinter Review
So we need of course store then just the end time of the previous event handling.
Attachment #8826504 - Attachment is obsolete: true
Attachment #8826504 - Flags: review?(masayuki)
Attachment #8826518 - Flags: review?(masayuki)
Comment on attachment 8826518 [details] [diff] [review]
only endtime

Thanks!
Attachment #8826518 - Flags: review?(masayuki) → review+
Attached patch +testsSplinter Review
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
Attachment #8826518 - Attachment is obsolete: true
Attachment #8826783 - Flags: review?(masayuki)
crap, I pushed from a bad m-i. Need to rebase and re-push to try
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.
Attachment #8826783 - Flags: review?(masayuki) → review+
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
Attached patch +nitsSplinter Review
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/68aff122042f
try to avoid flooding child process with repeated key events, r=masayuki
https://hg.mozilla.org/mozilla-central/rev/68aff122042f
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
No longer depends on: 1441821
You need to log in before you can comment on or make changes to this bug.