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

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

(Blocks 1 bug)

Trunk
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox47 affected, firefox48 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

3 years ago
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.
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.
Assignee

Comment 2

3 years ago
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.
Assignee

Updated

3 years ago
Blocks: 1252251
Assignee

Comment 3

3 years ago
This implements Masayuki's second suggestion and fixes the errors I was seeing.
Assignee

Updated

3 years ago
Assignee: nobody → continuation
Whiteboard: btpp-active
Assignee

Comment 4

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

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

Comment 5

3 years ago
This causes the preferences to be applied twice for events that are sent to the child process
Assignee

Updated

3 years ago
Attachment #8725985 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8726704 - Flags: review?(bugs)
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.
Attachment #8726704 - Flags: review?(bugs) → review+
oh, hmm, is there still more to do... thinking
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 10

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/827864e6dceb
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.