1223946 - In Fennec with C++APZ enabled, wheel events dispatched from nsDOWWindowUtil cause assert

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Randall Barker [:rbarker]]

The base widget will assert that controller thread != main when wheel events are dispatched fro from nsDOMWindowUtil when C++APZ is enabled in Fennec. Wheel events need to be dispatched from the proper thread in Fennec to prevent the assert.

Comment 1

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1223946-In-Fennec-with-C-APZ-enabled-wheel-events-dispatched-from-nsDOWWindowUtil-cause-assert-15111112-9c97593.patch

Comment 2

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

Ideally I'd like to see this patch split in two - the first does the justification change and other cleanup, and the second introduces the new code to dispatch on the new thread.

Comment 3

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1223946-Part-1-Clean-up-and-supported-need-for-converting-WidgetWheelEvent-to-ScrollWheelInput-15111212-accdf21.patch

Comment 4

[Randall Barker [:rbarker]]

Attachment: 0002-Bug-1223946-Part-2-Ensure-wheel-event-from-nsDOMWindowUtil-is-dispatched-on-correct-thread-15111212-2fc58b5.patch

Comment 5

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1223946-Part-1-Clean-up-and-support-needed-for-converting-WidgetWheelEvent-to-ScrollWheelInput-15111212-d1350e3.patch

Fix description typo

Comment 6

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

Comment on attachment 8686749 [details] [diff] [review]
0002-Bug-1223946-Part-2-Ensure-wheel-event-from-nsDOMWindowUtil-is-dispatched-on-correct-thread-15111212-2fc58b5.patch

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

::: widget/nsBaseWidget.cpp
@@ +1052,5 @@
> +                                 mWheelInput(aWheelInput),
> +                                 mWidget(aWidget),
> +                                 mAPZResult(aAPZResult),
> +                                 mInputBlockId(aInputBlockId),
> +                                 mGuid(aGuid)

Move the initializers over, so this is formatted like:

  ConstructorName(Arg1 arg1,
                  Arg2 arg2)
    : m1(arg1)
    , m2(arg2)
  {
  }

Ditto for the other helper class below

@@ +1106,5 @@
>  nsEventStatus
>  nsBaseWidget::DispatchAPZAwareEvent(WidgetInputEvent* aEvent)
>  {
>    if (mAPZC) {
> +    if (APZThreadUtils::IsControllerThreadOnMainThread()) {

I think this should just be APZThreadUtils::IsControllerThread(). (I'll put a comment on the other patch that explains more)

@@ +1118,5 @@
> +      return ProcessUntransformedAPZEvent(aEvent, guid, inputBlockId, result);
> +    } else {
> +      WidgetWheelEvent* wheelEvent = aEvent->AsWheelEvent();
> +      if (wheelEvent) {
> +        MOZ_ASSERT(NS_IsMainThread());

Move this assert to the start of the function. This function must run on the main thread because it's dealing with a WidgetInputEvent which can only be used on the main thread.

Comment 7

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

Comment on attachment 8686753 [details] [diff] [review]
0001-Bug-1223946-Part-1-Clean-up-and-support-needed-for-converting-WidgetWheelEvent-to-ScrollWheelInput-15111212-d1350e3.patch

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

::: gfx/layers/apz/util/APZThreadUtils.cpp
@@ +100,5 @@
>  
> +/*static*/ bool
> +APZThreadUtils::IsControllerThreadOnMainThread()
> +{
> +  return (sControllerThread == MessageLoop::current()) && NS_IsMainThread();

This function name implies that you can call it from any thread and it will return true iff the main thread is the same as the controller thread on the current platform. However, that's not what this is doing, since it's also checking that you're calling it from the main thread.

I think this function can be renamed to just be IsControllerThread(), and only do the first check (sControllerThread == MessageLoop::current()). I believe it will need to be ifdef'd for MOZ_ANDROID_APZ as well though, since the controller thread there doesn't have a MessageLoop:current(), we'll need to call AndroidBridge::IsJavaUiThread() instead.

Comment 8

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

Also for posterity I want to say that ideally we should update the tests that are relying on this to use synthesizeNativeWheel instead (and support that function on all the relevant platforms).

Back in https://bugzilla.mozilla.org/show_bug.cgi?id=1126090#c42 dvander wanted to do basically this exact same thing for B2G and I asked him to disable the tests on B2G instead because B2G didn't support wheel events. However on Android the JPZC does support wheel events, so we'll need to add support for that here, and so we should probably keep these tests. There's definitely a better way to do all this though in the APZ world. I'll file a follow-up for that.

Comment 9

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1223946-Part-1-Clean-up-and-support-needed-for-converting-WidgetWheelEvent-to-ScrollWheelInput-15111819-6f229d3.patch

Comment 10

[Randall Barker [:rbarker]]

Attachment: 0002-Bug-1223946-Part-2-Ensure-wheel-event-from-nsDOMWindowUtil-is-dispatched-on-correct-thread-15111819-8c41aa5.patch

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ffa10317f183
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca5fae7924d2

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ffa10317f183
https://hg.mozilla.org/mozilla-central/rev/ca5fae7924d2