1337963 - Need to coalesce wheel events in the content process so that long wheel event handlers don't hang the content process indefinitely

Status: RESOLVED FIXED

(Core :: DOM: Events, defect, P1)

Description

[Markus Stange [:mstange]]

Attachment: testcase

Similar to bug 1330252, but for wheel events.

Especially on Mac, we fire wheel events in very small increments, and lots of them. Before e10s, the system would coalesce wheel events for us at the system level because the parent process was getting backed up. But now with e10s the parent process stays responsive and we just forward all wheel events to the content process as we get them, and call wheel handlers for each event. This means that the content event queue can grow without bounds as more wheel events come in.

We should coalesce adjacent wheel events on the content side and accumulate their delta, for some definition of "adjacent". Maybe if there are no other mouse or key events in between.

Comment 1

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

Even on Windows, some mice (I confirmed with Logitech's MX Master) generate a lot of wheel messages for smooth scrolling. So, it's nice if we could handle this at ESM or TabParent but I'm not sure where is the best due to APZ.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

It can't be in TabParent, since that doesn't know if child process is flooding. Need to happen in
child side, I think.

Comment 3

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

I'm thinking that after TabChild dispatches a wheel event in its process [1], it should notify its TabParent of "propagation finished". Until then, TabParent accumulates pending wheel events and send new merged wheel event to the remote.
https://dxr.mozilla.org/mozilla-central/rev/eea9995ed14c07675c972400e8ce36b3608c01b1/dom/ipc/TabChild.cpp#1640

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Child process could also accumulate deltas. Based on the timestamp of wheel event and its processing time TabChild would calculate how long wheel events from parent would be blocked and during that time it would just collect the deltas. Complication comes from ensuring that if there aren't new wheel events coming after the blocking time, we'd still need to dispatch an event. (though, in practice, perhaps we could just skip that).

If we always send notification back to parent (and then it sends another message event to child) I'm worried that we in worst case flood the main thread in parent process, which is way worse than it happening in child process.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

The child process could also use the MessageChannel::PeekMessages hook to look at the queued IPC messages and coalesce the deltas.

Comment 6

[Andrew Overholt [:overholt]]

Masayuki, can you take this?

Comment 7

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

Hmm, it's impossible to take this immediately, but could be later. If somebody can take this, please feel free to take this.

Comment 8

[Andrew Overholt [:overholt]]

Maybe Stone has time (I honestly don't know and feel free to say no, Stone!)?

Comment 9

[Ming-Chou Shih [:stone]]

Attachment: Coalesce wheel events in the content process so that long wheel event handlers don't hang the content process indefinitely

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8842281 [details] [diff] [review]
Coalesce wheel events in the content process so that long wheel event handlers don't hang the content process indefinitely

Review of attachment 8842281 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/TabChild.cpp
@@ +1666,5 @@
> +          PickleIterator iter(aMsg);
> +          WidgetWheelEvent wheelEvent(false, eVoidEvent, nullptr);
> +          if (!IPC::ReadParam(&aMsg, &iter, wheelEvent.AsInputEvent())) {
> +            MOZ_ASSERT(false);
> +            return false;

Please also read the ScrollableLayerGuid and input block id from the message and make sure they are the same as the wheel event you're coalescing into. If they are not the same and you coalesce them it will screw up APZ and possibly scroll the wrong thing.

Comment 11

[Ming-Chou Shih [:stone]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #10)
> Please also read the ScrollableLayerGuid and input block id from the message
> and make sure they are the same as the wheel event you're coalescing into.
> If they are not the same and you coalesce them it will screw up APZ and
> possibly scroll the wrong thing.

I found there are some sentinels between WidgetWheelEvent, ScrollableLayerGuid and InputBlockId when reading them from the message. I tried to find existed APIs to read them without reading sentinel with a hard code value. I'm wondering is it ok to read sentinels with some default value (e.g. 0) and ignore the result? (It's fragile to changes of PBrowser.ipdl). May I have your suggestions about it?

Comment 12

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

Comment on attachment 8842281 [details] [diff] [review]
Coalesce wheel events in the content process so that long wheel event handlers don't hang the content process indefinitely

>+bool
>+TabChild::MaybeCoalesceMouseWheelEvent(const WidgetWheelEvent& aEvent)
>+{
>+  if (!mRepeatedWheelEventTime.IsNull() && aEvent.mMessage == eWheel) {
>+    // We don't coalesce the first eWheel event, eWheelOperationStart, and
>+    // eWheelOperationEnd.
>+    bool isNextWheelEvent = false;
>+    Modifiers modifiers = 0;
>+    GetIPCChannel()->PeekMessages(
>+      [&isNextWheelEvent, &modifiers](const IPC::Message& aMsg) -> bool {
>+        if (aMsg.type() == mozilla::dom::PBrowser::Msg_MouseWheelEvent__ID) {
>+          PickleIterator iter(aMsg);
>+          WidgetWheelEvent wheelEvent(false, eVoidEvent, nullptr);
>+          if (!IPC::ReadParam(&aMsg, &iter, wheelEvent.AsInputEvent())) {
>+            MOZ_ASSERT(false);
>+            return false;
>+          }
>+          isNextWheelEvent = true;
>+          modifiers = wheelEvent.mModifiers;
>+        }
>+        return false; // Stop peeking.
>+      });
>+    mIsNextWheelEventWithSameModifiers = isNextWheelEvent &&
>+                                         aEvent.mModifiers == modifiers;
>+
>+    // Don't coalesce the last mouse wheel event or the wheel event with
>+    // different modifiers from the next wheel event.
>+    if (aEvent.mTimeStamp < mRepeatedWheelEventTime &&
>+        mIsNextWheelEventWithSameModifiers) {
>+      mAccuMouseWheelDelta.Accumulate(aEvent);
>+      return true;
>+    }

You should also compare mDeltaMode. This can be changed when system settings is changed in background or user changes to different pointing device (e.g., between old mouse and trackpad of Mac). When this is changed, mDelta* value meaning is changed, so, you shouldn't use old delta values as new delta mode's values.

>+    void MaybeCompensate(WidgetWheelEvent& aEvent)
>+    {
>+      if (mDirty) {
>+        aEvent.mDeltaX += mDeltaX;
>+        aEvent.mDeltaY += mDeltaY;
>+        aEvent.mDeltaZ += mDeltaZ;
>+        Reset();
>+      }
>+    }

I think that you also need to store mLineOrPageDeltaX and mLineOrPageDeltaY too. If you don't do so, the behavior could be changed on Windows because these values are set before small delta native wheel events are fired actually.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

(In reply to Ming-Chou Shih [:stone] from comment #11)
> I found there are some sentinels between WidgetWheelEvent,
> ScrollableLayerGuid and InputBlockId when reading them from the message. I
> tried to find existed APIs to read them without reading sentinel with a hard
> code value. I'm wondering is it ok to read sentinels with some default value
> (e.g. 0) and ignore the result? (It's fragile to changes of PBrowser.ipdl).
> May I have your suggestions about it?

Hm, that's a good point. I'm not sure what to do about that. The sentinels do appear to be hard-coded into the generated IPDL code (e.g. [1]) and it's probably not easy to extract them anywhere else. I agree that you could probably just read and throw away the sentinels but it would be fragile, and if the generated code for serializing/deserializing messages changed then this would silently break. Maybe the approach in comment 3 is better then? I don't have any good ideas here. One of the IPC peers might have a better idea about dealing with the sentinels.

[1] http://searchfox.org/mozilla-central/source/__GENERATED__/ipc/ipdl/PBrowserChild.cpp#3688

Comment 14

[Ming-Chou Shih [:stone]]

(In reply to Masayuki Nakano [:masayuki] from comment #12)
> I think that you also need to store mLineOrPageDeltaX and mLineOrPageDeltaY
> too. If you don't do so, the behavior could be changed on Windows because
> these values are set before small delta native wheel events are fired
> actually.
Thanks to masayuki and kats to provide the suggestions.

Comment 15

[Ming-Chou Shih [:stone]]

Attachment: Coalesce wheel events in the content process so that long wheel event handlers don't hang the content process indefinitely

The first version of patch tried to coalesce the current event when it's similar to the next wheel event. Because of sentinels, I got some problems to peek message and read message data. After discussing with kanru, one possible solution is to modify the ipdl code generator.

I found there is an another solution that always coalesces current message if the following conditions are meet.
1. The current event is fired before the last wheel event was processed.
2. The next event is wheel event
When receiving the next wheel event, we can compare it with the coalesced wheel event and fire both of them if they have different attributes.

Comment 16

[Ming-Chou Shih [:stone]]

Attachment: Coalesce wheel events in the content process so that long wheel event handlers don't hang the content process indefinitely

Revise comments

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8845253 [details] [diff] [review]
Coalesce wheel events in the content process so that long wheel event handlers don't hang the content process indefinitely

>+    // Don't coalesce current event when
>+    //   1. Event dispatched after the last wheel event processing was finished.
>+    //   2. It's the last wheel event of repeated wheel events.
>+    //   3. It has different attributes from the coalesced wheel which is not
>+    //      yet fired.
>+    if (aEvent.mTimeStamp < mRepeatedWheelEventTime && *aIsNextWheelEvent &&
>+        (mCoalescedWheelData.IsEmpty() ||
>+         mCoalescedWheelData.CanCoalesce(aEvent, aGuid, aInputBlockId))) {
>+      mCoalescedWheelData.Coalesce(aEvent, aGuid, aInputBlockId);
>+      return true;
>+    }
Why we peek even in case aEvent.mTimeStamp => mRepeatedWheelEventTime) ?

But anyhow, here we compare timestamp from parent process to a timestamp from child.
Could we possibly avoid that...


>+TabChild::UpdateRepeatedMouseWheelEventEndTime(bool aIsNextWheelEvent)
>+{
>+  // Set mRepeatedWheelEventTime to null moment when the next event is not wheel
>+  // event to avoid coalesce the first wheel event.
But you set mRepeatedWheelEventTime to Now() when next isn't wheel event, not to null.

>+  mRepeatedWheelEventTime =
>+    mRepeatedWheelEventTime.IsNull() || !aIsNextWheelEvent ? TimeStamp::Now() :
>+                                                             TimeStamp();
>+}
...so I'd prefer to make mRepeatedWheelEventTime to be event.mTimeStamp + duration (of the event handling).
That way comparing mRepeatedWheelEventTime and mTimeStamp would be a tad safer, since both use time from parent process.
So

>+    void Coalesce(const WidgetWheelEvent& aEvent,
>+                    const ScrollableLayerGuid& aGuid,
>+                    const uint64_t& aInputBlockId)
Fix indentation


>+    bool CanCoalesce(const WidgetWheelEvent& aEvent,
>+                     const ScrollableLayerGuid& aGuid,
>+                     const uint64_t& aInputBlockId)
>+    {
>+      MOZ_ASSERT(!IsEmpty());
>+      return !mCoalescedWheelEvent ||
>+             (mCoalescedWheelEvent->mModifiers == aEvent.mModifiers &&
>+              mCoalescedWheelEvent->mDeltaMode == aEvent.mDeltaMode &&
>+              mGuid == aGuid &&
>+              mInputBlockId == aInputBlockId);
Technically I don't think it is guaranteed that coordinates of the old and new events are the same, so could we check here that they are.

Comment 18

[Kartikaya Gupta (email:[e-mail])]

I didn't look at the patch closely but I would very much prefer if the helper class was extracted into a separate file rather than stuffing it in TabChild.h.

Comment 19

[Ming-Chou Shih [:stone]]

Attachment: Coalesce wheel events in the content process so that long wheel event handlers don't hang the content process

Comment 20

[Ming-Chou Shih [:stone]]

Attachment: Coalesce wheel events in the content process so that long wheel event handlers don't hang the content process

Extract the helper class CoalescedWheelData into a separate file.
Compare the wheel end time from the parent process.
Check mRefPoint to coalesce the wheel event.
Fix other problems addressed in review comments.

Comment 21

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8845866 [details] [diff] [review]
Coalesce wheel events in the content process so that long wheel event handlers don't hang the content process

Review of attachment 8845866 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/TabChild.cpp
@@ +1736,5 @@
>  {
> +  bool isNextWheelEvent = false;
> +  if (MaybeCoalesceMouseWheelEvent(aEvent, aGuid, aInputBlockId,
> +                                   &isNextWheelEvent)) {
> +    return IPC_OK();

It seems like |isNextWheelEvent| is redundant. Whenever it is true, MaybeCoalesceMouseWheelEvent returns true. And whenever it is false, MaybeCoalesceMouseWheelEvent returns false.

@@ +1744,5 @@
> +  // events. Otherwise, set it to the current event time, add it with the time
> +  // consumed by dispatching, then check the next event is fired before the
> +  // event is processed.
> +  bool updateLastWheelEndTime = mLastWheelProcessedTimeFromParent.IsNull() ||
> +                                isNextWheelEvent;

At this point, isNextWheelEvent will always be false, see my previous comment

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8845866 [details] [diff] [review]
Coalesce wheel events in the content process so that long wheel event handlers don't hang the content process

>+CoalescedWheelData::CanCoalesce(const WidgetWheelEvent& aEvent,
>+                                const ScrollableLayerGuid& aGuid,
>+                                const uint64_t& aInputBlockId)
>+{
>+  MOZ_ASSERT(!IsEmpty());
>+  return !mCoalescedWheelEvent ||
>+         (mCoalescedWheelEvent->mRefPoint == aEvent.mRefPoint &&
>+          mCoalescedWheelEvent->mModifiers == aEvent.mModifiers &&
>+          mCoalescedWheelEvent->mDeltaMode == aEvent.mDeltaMode &&
>+          mGuid == aGuid &&
>+          mInputBlockId == aInputBlockId);
Could we check still that both events have the same mCanTriggerSwipe value


>+  mLastWheelProcessedTimeFromParent =
>+    updateLastWheelEndTime ? aEvent.mTimeStamp : TimeStamp();
>+  mozilla::TimeStamp beforeDispatchingTime = TimeStamp::Now();
You could avoid Now() call if updateLastWheelEndTime is false.

>+  MaybeDispatchCoalescedMouseWheelEvent();
>+  DispatchMouseWheelEvent(aEvent, aGuid, aInputBlockId);
>+  if (updateLastWheelEndTime) {
>+    mLastWheelProcessedTimeFromParent +=
>+      (TimeStamp::Now() - beforeDispatchingTime);
>   }

Comment 23

[Ming-Chou Shih [:stone]]

Attachment: Coalesce wheel events in the content process so that long wheel event handlers don't hang the content process

When removing the redundant argument 'aIsNextWheelEvent', I notice that there is a problem when the current event isn't coalesced because of different causes. For the cause of different attributes or the current event timestamp before the end time of last wheel event, we still could coalesce the next wheel event. For the cause of the next event is not wheel, we couldn't coalesce the next wheel because it's the first wheel of the next repeated sequence of wheel events, we have to set mLastWheelProcessedTimeFromParent to null moment.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8846212 [details] [diff] [review]
Coalesce wheel events in the content process so that long wheel event handlers don't hang the content process

>+    // We only coalesce current event when
the current event

>+  if (isNextWheelEvent) {
>+    // Update mLastWheelProcessedTimeFromParent so that we can compare the end
>+    // time of current event with the dispatched time of the next event.
of the current event

>+    mLastWheelProcessedTimeFromParent = aEvent.mTimeStamp;
>+    mozilla::TimeStamp beforeDispatchingTime = TimeStamp::Now();
>+    MaybeDispatchCoalescedMouseWheelEvent();
>+    DispatchMouseWheelEvent(aEvent, aGuid, aInputBlockId);
>+    mLastWheelProcessedTimeFromParent +=
>+      (TimeStamp::Now() - beforeDispatchingTime);
>+  } else {
>+    // This is the last wheel event. Set mLastWheelProcessedTimeFromParent to
>+    // null moment to avoid coalesce the first wheel event.
maybe:               to avoid coalesce the next incoming wheel event.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8846212 [details] [diff] [review]
Coalesce wheel events in the content process so that long wheel event handlers don't hang the content process

> 
>+  bool MaybeCoalesceMouseWheelEvent(const WidgetWheelEvent& aEvent,
>+                                    const ScrollableLayerGuid& aGuid,
>+                                    const uint64_t& aInputBlockId,
>+                                    bool* aIsNextWheelEvent);
>+
>+  void MaybeDispatchCoalescedMouseWheelEvent();
>+
>+  void DispatchMouseWheelEvent(const WidgetWheelEvent& aEvent,
>+                               const ScrollableLayerGuid& aGuid,
>+                               const uint64_t& aInputBlockId);
>+
Ah, one thing. Could you drop "Mouse" from the names of these methods.
Just MaybeCoalesceWheelEvent etc.

Comment 26

[Ming-Chou Shih [:stone]]

Attachment: Coalesce wheel events in the content process so that long wheel event handlers don't hang the content process

Updated function name and comments.

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

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b47b363a447
Coalesce wheel events in the content process so that long wheel event handlers don't hang the content process. r=smaug

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1b47b363a447