1253041 - WheelPrefs::ApplyUserPrefsToDelta() gets called twice per event with e10s

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Andrew McCreight [:mccr8]]

A user can set preferences various like mousewheel.default.delta_multiplier_x to change how much the window scrolls when they use the mouse wheel. This actually happens in WheelPrefs::ApplyUserPrefsToDelta.

However, it appears that with e10s this preference gets multiplied into the scroll delta twice. For instance, I set mousewheel.default.delta_multiplier_y to 1000 (which multiplies the value by 10). Then, when I scroll once, I see the value for deltaY in one call for ApplyUserPrefsToDelta go from 1 to 10, then immediately in another call it goes from 10 to 100.

This is responsible for at least some of the failures in test_dom_wheel_event.html.

In a non-e10s build, the call stack for ApplyUsersPrefsToDelta() looks like:

#01: mozilla::EventStateManager::PreHandleEvent(nsPresContext*, mozilla::WidgetEvent*, nsIFrame*, nsIContent*, nsEventStatus*) (EventStateManager.cpp:790, in XUL)
#02: PresShell::HandleEventInternal(mozilla::WidgetEvent*, nsEventStatus*, bool) (nsPresShell.cpp:7811, in XUL)
#03: PresShell::HandlePositionedEvent(nsIFrame*, mozilla::WidgetGUIEvent*, nsEventStatus*) (nsPresShell.cpp:7662, in XUL)
#04: PresShell::HandleEvent(nsIFrame*, mozilla::WidgetGUIEvent*, bool, nsEventStatus*, nsIContent**) (nsPresShell.cpp:7447, in XUL)
#05: nsViewManager::DispatchEvent(mozilla::WidgetGUIEvent*, nsView*, nsEventStatus*) (nsCOMPtr.h:403, in XUL)
#06: nsView::HandleEvent(mozilla::WidgetGUIEvent*, bool) (RefPtr.h:39, in XUL)
#07: nsChildView::DispatchEvent(mozilla::WidgetGUIEvent*, nsEventStatus&) (nsChildView.mm:1469, in XUL)
#08: nsChildView::DispatchAPZWheelInputEvent(mozilla::InputData&, bool) (MouseEvents.h:69, in XUL)
#09: -[ChildView scrollWheel:] (nsChildView.mm:4993, in XUL)

With e10s, the first call happens in the parent with a stack like this:
#01: mozilla::EventStateManager::PreHandleEvent(nsPresContext*, mozilla::WidgetEvent*, nsIFrame*, nsIContent*, nsEventStatus*) (EventStateManager.cpp:790, in XUL)
#02: PresShell::HandleEventInternal(mozilla::WidgetEvent*, nsEventStatus*, bool) (nsPresShell.cpp:7811, in XUL)
#03: PresShell::HandlePositionedEvent(nsIFrame*, mozilla::WidgetGUIEvent*, nsEventStatus*) (nsPresShell.cpp:7662, in XUL)
#04: PresShell::HandleEvent(nsIFrame*, mozilla::WidgetGUIEvent*, bool, nsEventStatus*, nsIContent**) (nsPresShell.cpp:7449, in XUL)
#05: nsViewManager::DispatchEvent(mozilla::WidgetGUIEvent*, nsView*, nsEventStatus*) (nsCOMPtr.h:403, in XUL)
#06: nsView::HandleEvent(mozilla::WidgetGUIEvent*, bool) (RefPtr.h:39, in XUL)
#07: nsChildView::DispatchEvent(mozilla::WidgetGUIEvent*, nsEventStatus&) (nsChildView.mm:1469, in XUL)
#08: nsBaseWidget::ProcessUntransformedAPZEvent(mozilla::WidgetInputEvent*, mozilla::layers::ScrollableLayerGuid const&, unsigned long long, nsEventStatus) (RefPtr.h:289, in XUL)
#09: nsChildView::DispatchAPZWheelInputEvent(mozilla::InputData&, bool) (nsChildView.mm:2814, in XUL)
#10: -[ChildView scrollWheel:] (nsChildView.mm:4993, in XUL)

The second call happens in the child, with a stack like this:
#01: mozilla::EventStateManager::PreHandleEvent(nsPresContext*, mozilla::WidgetEvent*, nsIFrame*, nsIContent*, nsEventStatus*) (EventStateManager.cpp:790, in XUL)
#02: PresShell::HandleEventInternal(mozilla::WidgetEvent*, nsEventStatus*, bool) (nsPresShell.cpp:7811, in XUL)
#03: PresShell::HandlePositionedEvent(nsIFrame*, mozilla::WidgetGUIEvent*, nsEventStatus*) (nsPresShell.cpp:7662, in XUL)
#04: PresShell::HandleEvent(nsIFrame*, mozilla::WidgetGUIEvent*, bool, nsEventStatus*, nsIContent**) (nsPresShell.cpp:7449, in XUL)
#05: nsViewManager::DispatchEvent(mozilla::WidgetGUIEvent*, nsView*, nsEventStatus*) (nsCOMPtr.h:403, in XUL)
#06: nsView::HandleEvent(mozilla::WidgetGUIEvent*, bool) (RefPtr.h:39, in XUL)
#07: mozilla::widget::PuppetWidget::DispatchEvent(mozilla::WidgetGUIEvent*, nsEventStatus&) (PuppetWidget.cpp:348, in XUL)
#08: mozilla::layers::APZCCallbackHelper::DispatchWidgetEvent(mozilla::WidgetGUIEvent&) (APZCCallbackHelper.cpp:552, in XUL)
#09: mozilla::dom::TabChild::RecvMouseWheelEvent(mozilla::WidgetWheelEvent const&, mozilla::layers::ScrollableLayerGuid const&, unsigned long long const&) (TabChild.cpp:1901, in XUL)
#10: non-virtual thunk to mozilla::dom::TabChild::RecvMouseWheelEvent(mozilla::WidgetWheelEvent const&, mozilla::layers::ScrollableLayerGuid const&, unsigned long long const&) (TabChild.cpp:1887, in XUL)
#11: mozilla::dom::PBrowserChild::OnMessageReceived(IPC::Message const&) (PBrowserChild.cpp:3101, in XUL)
#12: mozilla::dom::PContentChild::OnMessageReceived(IPC::Message const&) (PContentChild.cpp:6175, in XUL)
#13: mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) (MessageChannel.h:553, in XUL)
#14: mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message const&) (MessageChannel.cpp:1384, in XUL)

It looks like what is happening is that the event is first thrown in the parent process (the stack looks different in the synthesizeMouseWheel test case, but it is the same basic idea), which causes it to accumulate this scaling during dispatch, then it sends the event to the child via SendMouseWheelEvent, which re-dispatches the same event, causing it to accumulate again.

I don't know enough about events to say if there are other non-idempotent properties of wheel events that might be thrown off by the double-dispatch.

Comment 1

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

Nice catch!

I guess that we should call CancelApplyingUserPrefsFromOverflowDelta() before dispatching to the remote content and call ApplyUserPrefsToDelta() after that.
http://mxr.mozilla.org/mozilla-central/source/dom/events/EventStateManager.cpp?rev=e7e8a495ded4&mark=1176-1176#1159

Otherwise, ApplyUserPrefsToDelta() should do nothing if aEvent->customizedByUserPrefs is already true.

Comment 2

[Andrew McCreight [:mccr8]]

CancelApplyingUserPrefsFromOverflowDelta() seems to apply only to overflowDeltaX and overflowDeltaY, whereas the fields in question are deltaX, deltaY and deltaZ. (And it doesn't seem to fix the test.) I'll try the other fix you suggested and see if that does the trick.

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: Don't apply user wheel prefs more than once.

This implements Masayuki's second suggestion and fixes the errors I was seeing.

Comment 4

[Andrew McCreight [:mccr8]]

I'll get Olli to review this when he's back.

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=704ae47f4e34

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: Don't apply user wheel prefs more than once.

This causes the preferences to be applied twice for events that are sent to the child process

Comment 6

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

Comment on attachment 8726704 [details] [diff] [review]
Don't apply user wheel prefs more than once.

Yeah, this should be enough, given that the flag is copied automatically when passing event from parent to child.

Comment 7

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

oh, hmm, is there still more to do... thinking

Comment 8

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

Yeah, ok, I think we can take the patch.
Handling of overflowDeltaX/Y isn't right in e10s, but that is a totally separate issue.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/827864e6dceb

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/827864e6dceb